ChibiOS Free Embedded RTOS

Would be possible to try without DMA? that would rule out a bit of things.

Giovanni

Giovanni wrote:Would be possible to try without DMA? that would rule out a bit of things.

we'd need to develop a non-DMA driver for I2Cv1 first, unless you already have one?

Nope, anyway, it is strange it takes so many interrupts for a single transaction.

A suggestion, could you try putting I2C IRQs at maximum priority (above other peripherals, do not use 0, 1 and 2)? You should do this because one of limitations of I2Cv1 (HW, not SW).

Giovanni

Giovanni wrote:Nope, anyway, it is strange it takes so many interrupts for a single transaction.

I've now captured what is happening with a logic analyser:
http://uav.tridgell.net/ChibiOS/ISR_Storm1.jpg
I setup the I2C_ISR line to toggle on each ISR. The I2C_OP line goes high during a I2C transaction, then it has SCL/SDA for I2C.
The capture shows a write to device 0x0B, register 0x28, followed by a 10 byte read. On the first read byte you can see the ISR storm in the I2C_ISR trace.
I think this corresponds to the I2C_EV8_2_MASTER_BYTE_TRANSMITTED case. I think we're just lacking an ACK of the BTF in that case.
I tested this by adding:
(void)dp->DR; // clear BTF
in the I2C_EV8_2_MASTER_BYTE_TRANSMITTED case in the switch statement and I no longer get the interrupt storm. The trace now looks like this:
http://uav.tridgell.net/ChibiOS/ISR_fixed.jpg
that shows a single interrupt while waiting for the register address to be acknowledged and a successful transaction.

A suggestion, could you try putting I2C IRQs at maximum priority (above other peripherals, do not use 0, 1 and 2)? You should do this because one of limitations of I2Cv1 (HW, not SW).

I think they are already at highest, at priority 5. ADC priority is 6, EXT priorities are 6, TIM priorities are 7, SPI priorities are 10, ICU is 7
the full list is here:
https://github.com/ArduPilot/ardupilot/ ... _mcuconf.h

The interesting thing is why this bug doesn't bite more often. It looks like on all I2C write-then-read transactions we will get continuous interrupts until the remote device acks the address. This doesn't actually seem to be specific to SMBus. So why aren't we crashing a lot more?
Also, we always run our I2C transactions with a timeout so why didn't the timeout save the aircraft? For this device we use a timeout of 20ms.

here is the patch that fixes it for me:
https://github.com/ArduPilot/ChibiOS/co ... 76394316af

Wow, this is way simpler, does this replace the count/reset procedure safely?

Giovanni

Giovanni wrote:Wow, this is way simpler, does this replace the count/reset procedure safely?

I still want to have the storm prevention code, so that when the next bug like this turns up we will find it. I suspect this bug has been lurking in ChibiOS for a long time, and costing people CPU cycles whenever a I2C device is a bit slow responding, and causing lockups if the device completely fails to respond. Unless you're looking for it this type of short term ISR flood won't show up in regular testing.
I will develop an equivalent storm prevention patch for SPI and post it soon, unless you are planning to go ahead with the more generic mechanism.

SPI is way less critical than I2C, I think it has different failure modes too. The way it can fail is RX DMA not giving an interrupt so the thread remains stuck into the driver. It can be "guarded" using a timer (virtual or GPT).

About timeouts, do you use the I2C timeout feature? didn't it help in that case? it is meant for guarding clock stretching.

Giovanni

Giovanni wrote:SPI is way less critical than I2C, I think it has different failure modes too. The way it can fail is RX DMA not giving an interrupt so the thread remains stuck into the driver. It can be "guarded" using a timer (virtual or GPT).

yes, this sort of storm is a lot less likely on SPI. It could really only happen if the peripheral internal state becomes corrupted. I still think we should guard against that as an optional feature.

About timeouts, do you use the I2C timeout feature? didn't it help in that case? it is meant for guarding clock stretching.

yes, we had timeouts enabled. For these particular transactions the timeout was 20ms. That should have prevented us falling out of the sky, unless perhaps the retries in the battery driver that uses this I2C bus happened often enough that we just didn't get any CPU time to actually fly as we spent all the CPU in the i2c interrupt handler.
Our code assumes that when the thread associated with an I2C bus is waiting for a transaction to complete that the CPU is free to get on with other things. That isn't true with this bug.
(we use a "thread per hardware bus" architecture in ArduPilot)