Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32 I2C fails inside Ticker callback #3966

Closed
Willem23 opened this issue Mar 19, 2017 · 11 comments
Closed

STM32 I2C fails inside Ticker callback #3966

Willem23 opened this issue Mar 19, 2017 · 11 comments
Labels

Comments

@Willem23
Copy link
Contributor

@Willem23 Willem23 commented Mar 19, 2017

Description

  • Type: Bug
  • Related issue:
  • Priority: Major

Bug

I2C block writes or reads fail inside a function that is called by a Ticker
mbed lib 132 still works as expected, mbed 133 and higher fail.
See discussion here https://developer.mbed.org/questions/77029/I2C-read-from-ticker/#answer12089
Note that the Async versions (i2c.transfer) do seem to work

Target
STM32F103 (and probably others)

Toolchain version:
mbed lib 132 still works as expected, mbed 133 and higher fail.

Expected behavior
I2C operation inside Ticker should work

Actual behavior
No I2C comms. SDA and SCL stay at low level

Steps to reproduce
See example and discussion here https://developer.mbed.org/questions/77029/I2C-read-from-ticker/#answer12089
Change mbed revisions between 132 and 133 to show bug

@LMESTM

This comment has been minimized.

Copy link
Contributor

@LMESTM LMESTM commented Mar 20, 2017

Hello

A probable explanation is that ticker callback is an IRQ context and I2C also needs IRQs to work, because ticker IRQ priority is higher than I2C one, I2C IRQ is never handled and the application fails. I think the application will actually even get stuck here.

See the ticker documentation here and especially the Warnings:
https://developer.mbed.org/handbook/Ticker
Especially there is a warning that says: "No blocking code in ISR" and i2c_read() / i2c_write() are blocking calls. (whereas the async API i2c_trasnfer() is not a blocking call, so it will return immediately from the ISR and the i2c trasnfer will run just fine with the I2C interrupts).

So I would propose to either:

  • call the i2c_read/i2c_write from the main thread or another dedicated thread
  • use the async API as suggested.

Note that this was maybe working before, because IRQ priorities were different, or a previous I2C implementation wasn't using IRQ, nevertheless, this is not possible in up to date versions and I'd suggest to try out the possible proposals.

Let me know your feedback
Hope this helps.

@0xc0170 0xc0170 added the devices: st label Mar 20, 2017
@Willem23

This comment has been minimized.

Copy link
Contributor Author

@Willem23 Willem23 commented Mar 20, 2017

The Ticker callback function should indeed not block to avoid that the function execution takes longer than the interval until the next Ticker callback occurs. In that sense it seems to me that the I2C write should not be considered blocking. It just takes a bit of time to execute like any other piece of code. The Ticker uses a 1 second interval so plenty of time to finish the I2C write. It also doesnt actually crash the code. The Ticker keeps running, the LED keeps flashing inside the callback, there is just no I2C activity.

I did not have time to review the mbed-dev source code, but I don't see why the I2C synchronous code should be using interrupts. These functions typically just poll a register to check a status flag to see if the byte has been transmitted. I did some other tests and that shows that the i2c.start(), i2c.write() and i2c.stop() version does work for recent mbed libs. This should be working similar but slower compared to the blockwrite operation. In case there is an issue with IRQs in the STM synchronous I2C implementation it should at least be checked to prevent different behaviour compared to other mbed platforms.

Here is the example:

#include "mbed.h"
 
I2C i2c(D14, D15); //PB_9, PB_8
 
DigitalOut myled(LED1);
 
Ticker my_ticker; // ticker for interrupt

void write_i2c () { 
  static char cnt = 0;   
#if (0)
// This works for mbed 132, but fails for mbed 133+
  i2c.write(0x40, &cnt, 1);
#else
// This fails for mbed 132 and works for mbed 133+
  i2c.start();  
  i2c.write(0x40);  
  i2c.write(cnt);    
  i2c.stop();        
#endif

  cnt++;
  
  myled = 1;  
  wait(0.1);
  myled = 0;  
}
 
int main() {
  char data = 0xAA;
//  i2c.frequency(100000);

// This works for mbed 133
  i2c.write(0x40, &data, 1);
  
  wait(0.1);
 
 my_ticker.attach(&write_i2c, 1);
 
 while (true) {
    wait(0.5);
  }
}
@Willem23 Willem23 closed this Mar 20, 2017
@0xc0170

This comment has been minimized.

Copy link
Member

@0xc0170 0xc0170 commented Mar 21, 2017

@Willem23 Thanks for the description

I did not have time to review the mbed-dev source code, but I don't see why the I2C synchronous code should be using interrupts. These functions typically just poll a register to check a status flag to see if the byte has been transmitted. I did some other tests and that shows that the i2c.start(), i2c.write() and i2c.stop() version does work for recent mbed libs. This should be working similar but slower compared to the blockwrite operation. In case there is an issue with IRQs in the STM synchronous I2C implementation it should at least be checked to prevent different behaviour compared to other mbed platforms.

@LMESTM what do you think?

@LMESTM

This comment has been minimized.

Copy link
Contributor

@LMESTM LMESTM commented Mar 21, 2017

The STM I2C driver indeed needs interrupts for this API. This was the found compromise for supporting the bool repeated option for all STM32 families.

I think there are other possible issues with calling I2C from the callback. You may have the same I2C bus used by another driver (e.g. EEPROM and temperature in parallel), So, ticker would interrupt an ongoing I2C transfer and the ticker interrupt would try to get hold of the I2C lock which is not available.

@Willem23

This comment has been minimized.

Copy link
Contributor Author

@Willem23 Willem23 commented Mar 21, 2017

@LMESTM
I understand that you can get in trouble when an interrupt handler uses any resources that are also in use by the main thread. That would also happen for example when you access serial ports or a text LCD that needs to keep track of a cursor etc. It is not so obvious that this problem occurs when the resource is used exclusively in the handler. Note that such a multiple access problem would also occur for the async I2C.

If I understand correctly this compromise means that the I2C synchronous read and write would also not work when used in response to a falling edge on a pin or some other interrupt. You would have to use the async version, use a flag to trigger the main thread or use the I2C byte operations that I tried,
In case this behaviour cant be changed without breaking something else it should be clearly documented, especially when it only applies to I2C and does not happen for other platforms.

@LMESTM

This comment has been minimized.

Copy link
Contributor

@LMESTM LMESTM commented Mar 22, 2017

@Willem23 - It may not happen for other interrupts. This is a question of priorities of interrupts.
If I2C interrupt priority is higher than the handler one (failing edge triggered for instance) this should work ok. The main timer / ticker interrupt has the highest priority in the system so that the ticks increases in the various timeout case handling (BTW, this is a possible issue when calling drivers directly from the ticker interrupt handler, I'm not sure whether the error cases / timeout would be handled correctly).

As you have your test setup in place, would you mind making a change in the mbed driver.
If you change priority from 2 to 0 in the below line, is your test ok ?
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/i2c_api.c#L130
If you don't have time for it, which I'd understand, I will do the test later. Thank you for your time and feedback.

@AchmadFathoni

This comment has been minimized.

Copy link

@AchmadFathoni AchmadFathoni commented Mar 30, 2018

Is there any workaround to call i2c function periodically ticker like without ticker?

@LMESTM

This comment has been minimized.

Copy link
Contributor

@LMESTM LMESTM commented Mar 30, 2018

Is there any workaround to call i2c function periodically ticker like without ticker?

I2C transfers can be quite slow and long, so better to do it from another context than interrupt context. A good way to do it is to call I2C transfers from threads, which will be triggered from the interrupt: good description can be found here; https://os.mbed.com/blog/entry/Simplify-your-code-with-mbed-events/

@AchmadFathoni

This comment has been minimized.

Copy link

@AchmadFathoni AchmadFathoni commented Mar 30, 2018

I2C transfers can be quite slow and long, so better to do it from another context than interrupt context. A good way to do it is to call I2C transfers from threads, which will be triggered from the interrupt: good description can be found here; https://os.mbed.com/blog/entry/Simplify-your-code-with-mbed-events/

My device STM32F446RE doesn't support RTOS and event library.

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

@jeromecoutant jeromecoutant commented Mar 30, 2018

My device STM32F446RE doesn't support RTOS and event library

Maybe you can explain why ?

Thx

@LMESTM

This comment has been minimized.

Copy link
Contributor

@LMESTM LMESTM commented Mar 30, 2018

I agree with Jérôme, STM32_F446RE supports RTOS.

If you really don't want to move to RTOS, you may consider using the simpler I2C API which should work from interrupt context

/** Read a single byte from the I2C bus
 *
 *  @param ack indicates if the byte is to be acknowledged (1 = acknowledge)
 *
 *  @returns
 *    the byte read
 */
int read(int ack);

/** Write single byte out on the I2C bus
 *
 *  @param data data to write out on bus
 *
 *  @returns
 *    '0' - NAK was received
 *    '1' - ACK was received,
 *    '2' - timeout
 */
int write(int data);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.