0

The code below is part of my code to read CAN ID in Rx callback:

tmpp = (((0x07 << 0x1D)|(0x368 << 0x12)) & 0x1FFC0000); //unsigned long long int tmpp - equal to 0xDA00000
if (CAN0_IF2ARB0_ID == tmpp) {
    //do some action        
}

The problem is that while the 29 bit CAN ID is 0xDA00000, the condition is not true. But when I directly set tmpp as tmpp = 0xDA00000, the program successfully enters the loop. In fact, the calculation tmpp = (((0x07 << 0x1D)|(0x368 << 0x12)) & 0x1FFC0000); seems to have some problem (the value is 0xDA00000, but in Softune, it is not calculated correctly). I would be grateful if you could help me to find the problem. Thanks.

Lundin
  • 195,001
  • 40
  • 254
  • 396
mr. noone
  • 1
  • 3
  • 3
    I suggest using the right type: `0x07ull << 0x1D` – Weather Vane May 30 '22 at 09:29
  • What is the size of `int` on you machine? – Gerhardh May 30 '22 at 09:34
  • 1
    *but in Softune, it is not calculated correctly)* can you tell us what is calculated instead? – Gerhardh May 30 '22 at 09:40
  • `(0x07 << 0x1D)` What is this supposed to be? You shift left by 29 bits (which shifts the whole value out of the 29 bit CAN id range) and then clear all these bits afterwards using `& 0x1FFC0000`. That does not seem to be very useful. – Gerhardh May 30 '22 at 09:44
  • @Gerhardh Unfortunately I could not debug to see the actual value. But, for example, in an online C compiler, you can check the result which is equal to 0xDA00000. – mr. noone May 30 '22 at 09:46
  • Don't you have any spare UART to add debug prints? Or any other mechanism to make some values visible outside of your MCU? – Gerhardh May 30 '22 at 09:49
  • @Gerhardh It needs serial to usb converter which I have not currently! – mr. noone May 30 '22 at 09:50
  • 1
    You did not yet tell us what size an `int` have on your MCU – Gerhardh May 30 '22 at 09:51
  • 1
    I bet you totally have a CAN bus where you can send messages... but also, it is impossible to write (quality) firmware without an in-circuit debugger. Carpenters don't make furniture while wearing a blindfold. Embedded systems programmers don't make programs without an in-circuit debugger. – Lundin May 30 '22 at 09:52
  • You should think about refilling your toolbox for embedded development. Having a few FTDI USB2serial adapters at hand is never wrong. – Gerhardh May 30 '22 at 09:56
  • @Gerhardh It is 4 bytes. – mr. noone May 30 '22 at 10:01
  • @Lundin You are right. Debuggers are a MUST for embedded applications. – mr. noone May 30 '22 at 10:03

2 Answers2

0

0x07 is an int - perhaps even a 16-bit int. Use at least unsigned long constants for values to be shifted into a 32-bit value.

// tmpp = (((0x07 << 0x1D)|(0x368 << 0x12)) & 0x1FFC0000);
tmpp = ((0x07ul << 0x1D) | (0x368ul << 0x12)) & 0x1FFC0000u;
chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
  • I believe Softune is some tool for Fujitsu 32 bit MCUs so `int` should be 32 bits in the OP's case. – Lundin May 30 '22 at 09:39
  • Thanks for the answer. It seems that it is working with your answer. Could you please provide more detail? Thanks. – mr. noone May 30 '22 at 09:54
  • @mr.noone all your bit shifting is done using `int`. Only afterwards the result is assigned to an `unsigned long long` variable. Whatever was lost before will never come back. If you really need type `unsigned long long` to hold the 32 bits, then your integers are too short. – Gerhardh May 30 '22 at 09:58
  • @mr.noone Your calculation has 2 problems: Lundin well describes the problem of shifting onto the sign bit and here: using `l` to insure at least as 32-bit calculation. Still remains a mystery why you assert "it is not calculated correctly", yet "could not debug to see the actual value". – chux - Reinstate Monica May 30 '22 at 09:58
  • @chux-ReinstateMonica "it is not calculated correctly" should be changed to "I did not correctly shifted values"! I inserted a breakpoint at that line and it did not reached. – mr. noone May 30 '22 at 10:08
0

Left-shifting an integer constant such as 1 is almost always a bug, because integer constants in C have a type just like variables and in most cases it is int. Now since int is a signed type, we cannot left shift data into the sign bit or we invoke undefined behavior. 0x07 << 0x1D does exactly that, it shifts data into bits 31 (sign bit), 30 and 29.

Solve this by always adding an u suffix to all your integer constants.

Furthermore, you shouldn't use "magic numbers" but named constants. And in case you mean to shift something 29 bits, use decimal notation 29 since that's self-documenting code.

Your fixed code should look something like this (replace "MASKn" with something meaningful):

#define MASK1 (0x07u << 29)
#define MASK2 (0x368u << 18)
#define MASK3 (MASK1 | MASK2)
#define MASK4 0x1FFC0000u

if (CAN0_IF2ARB0_ID == (MASK3 & MASK4))

Also an extended CAN identifier doesn't use those bits 31,30,29... so I have no idea what you are even doing here. If you seek to calculate some value for CAN acceptance filtering etc, then it would seem you seem to have managed to confuse yourself by the original use of hex constants for shifting.

Lundin
  • 195,001
  • 40
  • 254
  • 396