Filter for multiple CAN frame ids/masks

[M4GNV5]

Hey,

Im not sure about the SJA1000 but the MCP2515 supports multiple filters, however the api of this library does not support multiple filters. Maybe a macro constant like MAX_FILTERS and a function multiFilter(int *ids, int *masks, size_t count) would be possible?

I'm using this library together with an Arduino MKR WAN and a MKR CAN shield to read data from a Renault Twizy. Bitwise anding together all the CAN IDs I need results in 0, so using a single filter is not possible.

[aviatorhh]

I can confirm on the MCP2515. If you look at the code, the MCP routine just loops 6 times and writes to all registers. I have changed the (my) function to support multiple IDs and masks. There are only two mask filters on the MCP which must be set. Maybe we will see that implementation in future versions.

EDIT: I have just seen the implementation has a bug. It is iterating over the 6 registers by incrementing an array counter by four. But this is only half the truth :-). If you look at the datasheet, we have the registers 0x00 0x04 0x08 0x10 0x14 0x18 for the filters (hi). After 0x08 you have to add 8 to get to 0x10. So I did this:

#define REG_RXFnSIDH1(n)            (0x00 + (n * 4))
#define REG_RXFnSIDL1(n)            (0x01 + (n * 4))
#define REG_RXFnSIDH2(n)            (0x10 + (n * 4))
#define REG_RXFnSIDL2(n)            (0x11 + (n * 4))
#define REG_RXFnEID81(n)            (0x02 + (n * 4))
#define REG_RXFnEID01(n)            (0x03 + (n * 4))
#define REG_RXFnEID82(n)            (0x12 + (n * 4))
#define REG_RXFnEID02(n)            (0x13 + (n * 4))

int MCP2515Class::set_mask_and_filter(int* mask, int* filter, byte f_len) {
  // config mode
  writeRegister(REG_CANCTRL, 0x80);
  if (readRegister(REG_CANCTRL) != 0x80) {
    return 0;
  }

  writeRegister(REG_RXBnCTRL(0), FLAG_RXM0);
  writeRegister(REG_RXBnCTRL(0), FLAG_RXM0);

  writeRegister(REG_RXMnSIDH(0), mask[0] >> 3);
  writeRegister(REG_RXMnSIDL(0), mask[0] << 5);
  writeRegister(REG_RXMnEID8(0), 0);
  writeRegister(REG_RXMnEID0(0), 0);

  writeRegister(REG_RXBnCTRL(0), FLAG_RXM1);
  writeRegister(REG_RXBnCTRL(0), FLAG_RXM1);

  writeRegister(REG_RXMnSIDH(1), mask[1] >> 3);
  writeRegister(REG_RXMnSIDL(1), mask[1] << 5);
  writeRegister(REG_RXMnEID8(1), 0);
  writeRegister(REG_RXMnEID0(1), 0);

  byte a = 3;
  if (f_len < 3) a = f_len;

  for (byte i = 0; i < a; i++) {
    writeRegister(REG_RXFnSIDH1(i), filter[i] >> 3);
    writeRegister(REG_RXFnSIDL1(i), filter[i] << 5);
    writeRegister(REG_RXFnEID81(i), 0);
    writeRegister(REG_RXFnEID01(i), 0);
  }
  for (byte i = 3; i < f_len; i++) {
    writeRegister(REG_RXFnSIDH2(i - 3), filter[i] >> 3);
    writeRegister(REG_RXFnSIDL2(i - 3), filter[i] << 5);
    writeRegister(REG_RXFnEID82(i - 3), 0);
    writeRegister(REG_RXFnEID02(i - 3), 0);
  }

  // normal mode
  writeRegister(REG_CANCTRL, 0x00);
  if (readRegister(REG_CANCTRL) != 0x00) {
    return 0;
  }

  return 1;
}

to make it work.

[M4GNV5]

looking at the MCP2515 SPI manuel, you are absolutely right. But maybe a macro like this allows to use the old code without changes?

#define REG_RXFnSIDH(n)            ((n) < 3 ? (0x00 + (n) * 4) : (0x10 + ((n) - 3) * 4)
#define REG_RXFnSIDL(n)            ((n) < 3 ? (0x01 + (n) * 4) : (0x11 + ((n) - 3) * 4)
#define REG_RXFnEID8(n)            ((n) < 3 ? (0x02 + (n) * 4) : (0x12 + ((n) - 3) * 4)
#define REG_RXFnEID0(n)            ((n) < 3 ? (0x03 + (n) * 4) : (0x13 + ((n) - 3) * 4)

of course you need to make sure, that n does not have side effects.

[aviatorhh]

Perfect :-)

I have not testet yet but this looks like the way to go. I will implement and let you know.

[aviatorhh]

#define REG_RXFnSIDH(n)            n < 3 ? 0x00 + n * 4 : 0x10 + (n - 3) * 4
#define REG_RXFnSIDL(n)            n < 3 ? 0x01 + n * 4 : 0x11 + (n - 3) * 4
#define REG_RXFnEID8(n)            n < 3 ? 0x02 + n * 4 : 0x12 + (n - 3) * 4
#define REG_RXFnEID0(n)            n < 3 ? 0x03 + n * 4 : 0x13 + (n - 3) * 4

int MCP2515Class::set_mask_and_filter(int* mask, int* filter, byte f_len) {
  // config mode
  writeRegister(REG_CANCTRL, 0x80);
  if (readRegister(REG_CANCTRL) != 0x80) {
    return 0;
  }

  writeRegister(REG_RXBnCTRL(0), FLAG_RXM0);
  writeRegister(REG_RXBnCTRL(0), FLAG_RXM0);

  writeRegister(REG_RXMnSIDH(0), mask[0] >> 3);
  writeRegister(REG_RXMnSIDL(0), mask[0] << 5);
  writeRegister(REG_RXMnEID8(0), 0);
  writeRegister(REG_RXMnEID0(0), 0);

  writeRegister(REG_RXBnCTRL(0), FLAG_RXM1);
  writeRegister(REG_RXBnCTRL(0), FLAG_RXM1);

  writeRegister(REG_RXMnSIDH(1), mask[1] >> 3);
  writeRegister(REG_RXMnSIDL(1), mask[1] << 5);
  writeRegister(REG_RXMnEID8(1), 0);
  writeRegister(REG_RXMnEID0(1), 0);


  for (byte i = 0; i < f_len; i++) {
    writeRegister(REG_RXFnSIDH(i), filter[i] >> 3);
    writeRegister(REG_RXFnSIDL(i), filter[i] << 5);
    writeRegister(REG_RXFnEID8(i), 0);
    writeRegister(REG_RXFnEID0(i), 0);
  }



  // normal mode
  writeRegister(REG_CANCTRL, 0x00);
  if (readRegister(REG_CANCTRL) != 0x00) {
    return 0;
  }


  return 1;
}

That runs on my side.

But you have to change the old code in any case, because it does not allow to set individual masks or filters. The old code just fills both masks withe one value and also fills all six filters with only one value.

[M4GNV5]

I've sent a PR a week ago which implements mostly the same but also handles the case when the amount of ids > 6 (in your code when f_len > 6).

see #20

I still have to add the updated register macros and test it with my hardware.

[aviatorhh]

There is more. My above code changes has only effect if the control bits within some registers are set correctly. The above code has a sequence take from the origin:

  writeRegister(REG_RXBnCTRL(0), FLAG_RXM0);
  writeRegister(REG_RXBnCTRL(0), FLAG_RXM0);

In fact, this is nonsense here. Replace it with the following code:

  // Enable filtering for buffer 0
  uint8_t val = readRegister(REG_RXBnCTRL(0));
  val &= B10011111;
  writeRegister(REG_RXBnCTRL(0), val);
  // Enable filtering for buffer 1
  val = readRegister(REG_RXBnCTRL(1));
  val &= B10011111;
  writeRegister(REG_RXBnCTRL(1), val);
  // And allow interrupt therefore
  val = readRegister(REG_CANINTE);
  val |= B00000011;
  CAN.writeRegister(REG_CANINTE, val); 

What does it do? At first it clears the bits within the control registers for the RX buffers 0 and 1 to allow the usage of filtering. Without setting any filters or masks it does not make any sense. Setting the flags within the control register CANINTE enables the external interrupt to shoot if one of the RX buffers has been filled (after the filtering process)! Why this all? Well I was trying to wake up my MCU by receiving only special CAN ids. It did not work but after altering my code as above it works like charm :-). Hope this helps someone.

[timurrrr]

Sent a PR https://github.com/sandeepmistry/arduino-CAN/pull/47 that allows specifying registers directly.