Starting new I2C transaction via master callback

[dsptech]

Hi, I see now the last release (but I've not installed it yet). I'm thinking about a "little" issue in "acquireBus(..)" method. The method raise the interrupt priority against the caller routine. This prevent the use of the I2C library from the callback functions because these functions are interrupt routines themselves (I see no other issues). As workaround, I thinking about restoring the original priority after calling the library routines, but avoid the change by "acquireBus(..)" is better. Is there a way to do it ? EDIT: without change the library sourcecode.

..Obviously, the callback routine need to exit fastest as possible to avoid delays on new I2C interrupts. Thank you in advance.

[nox771]

This is an interesting problem. I think a simple way to deal with this is to add in a user #define that disables the priority escalation (just wrap the whole priority check in acquireBus_() function). For people that don't want it then it will be faster too. I'll think about it some more, but short-term if it causes you a problem then just comment out that priority check block of code.

[dsptech]

I agree, It's also help to reduce the code size if the check is not needed. Howewer, moving the irqPriority and currPriority declarations inside the priority check section is needed to avoid compiler warnings (unused vars).

if(i2c->opMode == I2C_OP_MODE_ISR || i2c->opMode == I2C_OP_MODE_DMA)   {
   int irqPriority, currPriority; //declarations moved here
   currPriority = nvic_execution_priority();
   ...
}

If I may say, another way is to add a public flag "disablePriorityCheck" in the class declaration (default initializer to false to mantain backward compatibility) and add it in the above conditional clause, so, the user will be able to exclude and reactivate the check at runtime only when a such exclusion is needed (example: inside the callback functions). In the future, the same flag can be used also by apposite wrappers of SendTransmission and others methods. example: if (!disablePriorityCheck && (i2c->opMode == I2C_OP_MODE_ISR || i2c->opMode == I2C_OP_MODE_DMA)) ....

[dsptech]

Hi, As you suggested, I patched the sourcecode by a #if code exclusion and I tested the library using interrupt and dma modes at 400kHz I2C clock on a Teensy 3.1 clocked at 72Mhz The library work fine even if the I2C transactions are started from callbacks. I have sought for interrupts latencies (the interrupt is masked inside the callbacks) but I seen none on them. Note: In my code, the new I2C operation is launched as last callback instruction, so, there is a 25us margin against the next interrupt event ( 400kHz clk ). Thank you.

[nox771]

I am re-opening this as it is not solved yet on the main lib.

I'm thinking to add in a define to disable the priority escalation on a future release. Do you know if library as-is ramps priority to 0 (highest) when using callbacks? I suspect this might happen, but I did not have a chance to test it yet. This is a problem because it then reverts to immediate mode instead of ISR.

If this is true I would need to patch in a wrapper to dynamically disable priority (as you mentioned earlier) around the callback. It is more complex, so it would take some time to implement and test.

[dsptech]

Hi, As I already written, when I leaved the first message I had not still downloaded the new version of the library. I deduced the issue by inspecting the code, so , I requested a confirmation here. Anyway .. no. I've not truly created the problem. Tomorrow I will run a true test to confirm that.

[dsptech]

Confirmed! After starting nine times an i2c transaction from the callbacks, the library "switch" to the immediate mode.

Meantime, I thinked about an alternative solution to avoid wrappers:

• a volatile uint8_t IsrRunning flag may be added to the i2cStruct or added elsewhere. The flag must be initialized with a zero value.
• the i2cX_isr routines may set the flag before calling the handler and clear the flag on exit (the handler has multiple exit points, so, changing it may be less affordable).
• in acquirebus_(...) if the current priority match the irq priority, an additive check on IsrRunning may tell us if we are running inside the I2C isr environment. In this case, the priority escalation can be skipped.

Could this solve the problem ? Note: the NVIC controller may produce the same information of the flag, (active interrupt bit ?) but I don't know this device, nor if the information is available il all cortexM processor (example M0 for Teensy LC).

[dsptech]

Addendum to the previous message and clarification (I may have misinterpreted your reply). The previous tests was perfomed with NO code exclusions.

Anyway, the code exclusion by the #define and #if (removal of the priority check and escalation section) solve all problems and the library fully work in background mode, even when called from it's callbacks. Escalation does not occur at all the library cannot fall in the immediate mode.
I agree that it is the simpler way.

[nox771]

Thanks for running all these tests. I'm thinking I'll patch it with two things. To fix the callback problem I'll put a dynamic disable at the points where the callbacks occur, including the Slave callbacks (there are only a few of these in the ISR so it should not be difficult). Secondly I'll also add a #define to disable priority escalation entirely (if someone wants that), with default being not defined (so enabled, as-is now). I'll try to push an update soon.

[dsptech]

Hi, I agree at all. For example, the code exclusion may be also useful if the library is used in a FreeRTOS environment (I've a ... future planning about this) . The task scheduler is not happy if an it's API is called with a priority higher than the task scheduler itself. Thank you.

[nox771]

I pushed a v10.1 update for this. It has a new #define to disable the check, and a new class static var (i2c_t3::isrActive) is used to dynamically detect if the ISR is running and block the check during that time.

I started to implement the ISR check as a class variable but I converted it to a shared class static (global) instead. The reason for this is because a class var would not prevent cross-bus escalation (eg. Wire calling Wire1 in callback, and Wire1 calling Wire in callback). So instead the shared var will prevent priority changes any time any I2C ISR is running, regardless of bus. In these complex cases it is better to use the #define to shut off priority checks entirely and just manually set ISR priorities as needed.

It is not documented, but isrActive can also be used as a dynamic disable by incrementing/decrementing it (since it is shared, do not set it, just incr/decr). So this could work:

i2c_t3::isrActive++;  // disable priority check (make it think ISR is running)
...
i2c_t3::isrActive--;  // enable priority check (revert change, no ISR running)

If you think this new release fixes your bug you can close this again.

[dsptech]

Thank you, I will download the new v10.1 and send a report in few days.

[dsptech]

Hi, I tested the 10.1 priority check exclusion via i2c_t3::isrActive.

Setting isrActive to 1 by the main program (Wire.isrActive++ as you suggested), the priority check is totally disabled and never performed. In a second istance, isrActive was leaved to it's default and I seen that the priority check was performed only if the library was called from main program. The check was never performed if the library was called from the callbacks. Definitely, it's all ok for me. Thank you.

[nox771]

Ok thanks for verifying it. I'll close this.