drivers: can: mcan: Add CAN statistics

[gramsay0]

I had a go at adding MCAN statistics, although I ran into a couple of issues:

• I tested a TX with nothing connected to the CAN transceiver and got endless AckError ISRs (>100k in 30 seconds)
  • i.e. shouldn't this stop when the controller enters an error state?
• The PSR Protocol Exception Event (PXE) bit never seems to get set - I misunderstood what this is used for

I am testing with this MCAN version:

can_mcan: IP rel: 3.2.3 08.6.8

Any thoughts/ideas on these?

@henrikbrixandersen

[gramsay0]

I tested this with Linux MCAN and got the same result, hundreds of thousands of bus errors:

Interestingly, if I disable the transceiver, lower the bitrate (testing with 250k) or sneeze then the behavior is different (usually the ISR is called just once and the controller goes to bus-off).

The front end of the CAN transceiver is terminated:

Does the ISR maybe need a hold-off time to prevent chewing up CPU cycles under certain circumstances?

[henrikbrixandersen]

• I tested a TX with nothing connected to the CAN transceiver and got endless AckError ISRs (>100k in 30 seconds)
  • i.e. shouldn't this stop when the controller enters an error state?

No, that is expected. The Bosch M_CAN will attempt retransmission when it sees a missing ACK (and trigger the AckError interrupt). The retransmission also sees a missing ACK and so it continues.

[henrikbrixandersen]

I wonder how you verified this? Did you use an out-of-tree driver front-end? You neglected to update all the in-tree driver front-ends from using DEVICE_DT_INST_DEFINE() to using CAN_DEVICE_DT_INST_DEFINE().

[gramsay0]

• I tested a TX with nothing connected to the CAN transceiver and got endless AckError ISRs (>100k in 30 seconds)

  • i.e. shouldn't this stop when the controller enters an error state?

No, that is expected. The Bosch M_CAN will attempt retransmission when it sees a missing ACK (and trigger the AckError interrupt). The retransmission also sees a missing ACK and so it continues.

Yes, although I thought the controller would go to bus-off after enough failures.

I tested this with a zero byte CAN payload at 1Mbps, it generated an ISR every 54us until the controller state went to error-passive, and then every 62us indefinitely.

Do you think this could cause loading/performance issues on some CPUs (especially with external controllers like tcan4x5x over SPI)?

On second thought, this driver is only really an interface to the hardware, and that's just what the hardware does 🤷
Maybe a warning in the CAN_STATS kconfig would suffice, and let the user decide if it meets their requirements

[henrikbrixandersen]

Yes, although I thought the controller would go to bus-off after enough failures.

Do you have CAN_AUTO_BUS_OFF_RECOVERY=y?

[gramsay0]

Yes, although I thought the controller would go to bus-off after enough failures.

Do you have CAN_AUTO_BUS_OFF_RECOVERY=y?

I did try toggling that, same result.

Is auto-recovery implemented for MCAN, it looked like the config only optionally compiled the recover function?

[henrikbrixandersen]

Looks good! One thing missing is a call to CAN_STATS_RESET() in can_mcan_start(). See

zephyr/drivers/can/can_mcux_flexcan.c

Line 314 in 8e7c7c6

CAN_STATS_RESET(dev);

for inspiration.

[henrikbrixandersen]

Is auto-recovery implemented for MCAN, it looked like the config only optionally compiled the recover function?

That implementation does look a bit fishy. We will need to look into that. From a quick test here, it looks like the controller is going into bus-off, but auto-recovers. I think this explains the jump from 54 usec to 62 usec.

[gramsay0]

That implementation does look a bit fishy. We will need to look into that.

👍

I think this explains the jump from 54 usec to 62 usec.

I think it's the "8 bit suspend transmission time" when in error-passive state (1Mbps == 1bit/us):

Bus-off recovery should be 128*11 = 1408us

[henrikbrixandersen]

I think it's the "8 bit suspend transmission time" when in error-passive state (1Mbps == 1bit/us):

Ahh, right. Good find. I still think it is okay behaviour for now. If you can add the call to reset the statistics on start, I think it's ready to go in.