Incorrect data type used in MCP2515Class::setFilterRegisters() ?

[timurrrr]

Originally reported by @RudolfAtHome on https://github.com/sandeepmistry/arduino-CAN/issues/42#issuecomment-892905787

Here we're implicitly casting a bunch of uint16_t variables to uint8_t when we put them into an array: https://github.com/timurrrr/arduino-CAN/blob/9823a1143c2bfc55ce97326b7e31e0e425ffdaf6/src/MCP2515.cpp#L420-L421

This was not intended, and probably causes bugs on certain compilers when using filters with any bits set in the more significant byte.

[timurrrr]

Can someone try replacing uint8_t on line 420 with uint16_t and test with some non-trivial filter?

It will probably take me a few weeks before I can find time to try to test it myself.

[Jojo-A]

Hi, first of all thank you for your amazing effort on this library! I wonder, why the "original" CAN library by @sandeepmistry is so popular although it still has not merged these many fundamental improvements and bugfixes you applied here!

In addition to the variable mentioned above I think that also the variable for the filter mask should be 16 bit. Like this:

for (int n = 0; n < 2; n++) {
    uint16_t mask = (n == 0) ? mask0 : mask1;
    writeRegister(REG_RXMnSIDH(n), mask >> 3);
    writeRegister(REG_RXMnSIDL(n), mask << 5);
    writeRegister(REG_RXMnEID8(n), 0);
    writeRegister(REG_RXMnEID0(n), 0);
  }

  uint16_t filter_array[6] =
      {filter0, filter1, filter2, filter3, filter4, filter5};
  for (int n = 0; n < 6; n++) {
    uint16_t id = filter_array[n];
    writeRegister(REG_RXFnSIDH(n), id >> 3);
    writeRegister(REG_RXFnSIDL(n), id << 5);
    writeRegister(REG_RXFnEID8(n), 0);
    writeRegister(REG_RXFnEID0(n), 0);
  }

I have not tested it, yet. But I think it makes sense

[timurrrr]

Thanks for the positive feedback, and for reporting this bug!

I agree that the bit math for SIDH is wrong:

boolean MCP2515Class::setFilterRegisters(
    uint16_t mask0, uint16_t filter0, uint16_t filter1,
    uint16_t mask1, uint16_t filter2, uint16_t filter3, uint16_t filter4, uint16_t filter5,
...
    uint8_t mask = (n == 0) ? mask0 : mask1;
    writeRegister(REG_RXMnSIDH(n), mask >> 3);
...
  uint8_t filter_array[6] =
      {filter0, filter1, filter2, filter3, filter4, filter5};
  ...
    uint8_t id = filter_array[n];
    writeRegister(REG_RXFnSIDH(n), id >> 3);
...

I wonder how this ever worked in my project, given that half of the CAN IDs my project is listening to are >255.... I'll try to find time to fix this and verify the fix.

(I think the SIDL values are correct though)

I wonder, why the "original" CAN library by @sandeepmistry is so popular although it still has not merged these many fundamental improvements and bugfixes you applied here!

I wonder the same haha

[timurrrr]

I wonder how this ever worked in my project, given that half of the CAN IDs my project is listening to are >255....

Oh, I guess it's the classic case of "even number of mistakes in bit math gives a correct(ish) result"! 😂

Since the first conversion turns the higher bits of RXMnSIDH to 0, the incorrect higher bits of RXFnSIDH get ignored.

[Jojo-A]

Yeah, that sounds reasonable. Although not intuitive ;) .

But in the same step you should also correct the local variables to be all 16-bit types because you always assign the 16-bit parameters to them. Like here:

uint8_t mask = (n == 0) ? mask0 : mask1;
writeRegister(REG_RXMnSIDH(n), mask >> 3);

Sure, the writeRegister function expects an 8-bit parameter. But from my point of view it looks a little bit unintended to do this by assigning a 16-bit variable to a 8-bit (mask = mask0). Same applies to the filter stuff. So I (!) personally would do it like

uint16_t mask = (n == 0) ? mask0 : mask1;
writeRegister(REG_RXMnSIDH(n), mask >> 3);
// or of the compiler complains:
writeRegister(REG_RXMnSIDH(n), (uint8_t)(mask >> 3));

What do you think?