Skip to content

Commit

Permalink
STM I2C: manage Is Device Ready case
Browse files Browse the repository at this point in the history 
Some device drivers use a data lenght of 0 to check if device is ready.
STM32 HAL provides a dedicated service for that, so let's use it.
  • Loading branch information
@LMESTM
LMESTM committed Dec 16, 2016
1 parent c0ca0a7 commit 37c94a0
Showing 1 changed file with 16 additions and 2 deletions.
18 changes: 16 additions & 2 deletions targets/TARGET_STM/i2c_api.c
View file
Expand Up @@ -634,9 +634,16 @@ void i2c_reset(i2c_t *obj) {
int i2c_read(i2c_t *obj, int address, char *data, int length, int stop) {
struct i2c_s *obj_s = I2C_S(obj);
I2C_HandleTypeDef *handle = &(obj_s->handle);
int count = 0, ret = 0;
int count = I2C_ERROR_BUS_BUSY, ret = 0;
uint32_t timeout = 0;

if((length == 0) || (data == 0)) {
if(HAL_I2C_IsDeviceReady(handle, address, 1, 10) == HAL_OK)
return 0;
else
return I2C_ERROR_BUS_BUSY;
}

if ((obj_s->XferOperation == I2C_FIRST_AND_LAST_FRAME) ||
(obj_s->XferOperation == I2C_LAST_FRAME)) {
if (stop)
Expand Down Expand Up @@ -686,9 +693,16 @@ int i2c_read(i2c_t *obj, int address, char *data, int length, int stop) {
int i2c_write(i2c_t *obj, int address, const char *data, int length, int stop) {
struct i2c_s *obj_s = I2C_S(obj);
I2C_HandleTypeDef *handle = &(obj_s->handle);
int count = 0, ret = 0;
int count = I2C_ERROR_BUS_BUSY, ret = 0;
uint32_t timeout = 0;

if((length == 0) || (data == 0)) {
if(HAL_I2C_IsDeviceReady(handle, address, 1, 10) == HAL_OK)
return 0;
else
return I2C_ERROR_BUS_BUSY;
}

if ((obj_s->XferOperation == I2C_FIRST_AND_LAST_FRAME) ||
(obj_s->XferOperation == I2C_LAST_FRAME)) {
if (stop)
Expand Down

3 comments on commit 37c94a0

@bscott-zebra
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @LMESTM ,
I'm using an I2C mux that requires a special sequence of 0 length reads/writes (with repeated starts) to switch operational modes, a MAX7358 ; see figure 5. I've found that I can generate the necessary sequence through the mbed I2C driver on an STM32F3 with a sequence of zero length I2C writes and reads if I remove the HAL_I2C_IsDeviceReady() substitution that you added in this commit, but I can find no way to generate the sequence with the change from this commit in place (I tried the individual start() and byte read() and write() calls, but found the STM32 implementation requires at least 1 byte of data after the address to work). The HAL_I2C_IsDeviceReady() function forces the read/write address bit to 0 (i.e. write), and doesn't support repeated start functionality.
Keeping the "count = I2C_ERROR_BUS_BUSY" intialization from this commit, but getting rid of the two "if((length == 0) || (data == 0)) {" code blocks, I tested that the code still works for detecting the presence of a device (i.e. returns I2C_ERROR_BUS_BUSY when the device isn't responding, and 0 when the device does respond).
Do you see a need to specifically use the HAL_I2C_IsDeviceReady() function when the normal functionality appears to work properly for the zero length case, and doing so interferes with the ability to do some special zero data length transfers properly? My preference would be to push a commit that just removes these two code blocks.
Alternatively, I can get my case to work by limiting the HAL_I2C_IsDeviceReady() substitution to only occur in the (XferOperation I2C_FIRST_AND_LAST_FRAME or I2C_LAST_FRAME) and stop case, allowing normal, non-substituted transfers to be done in my repeated start sequence, but I worry that some other chip may require a 0 data length, non-repeated start I2C read, which would still get substituted (unless the substitution were ONLY done in the i2c_write() function).
Let me know what you think.

@LMESTM
Copy link
Contributor Author

@LMESTM LMESTM commented on 37c94a0 Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @monkiineko
I'm open to any proposal which makes most of the devices to work fine :-)

I've put this use of HAL_I2C_IsDeviceReady in place for the I2C Eeprom tests from: https://github.com/ARMmbed/ci-test-shield. Mainly this ensures that this was working for all of our STM32 targets, with all 9 families of slightly different HAL layers.

Now the EEPROM lib implements a waitForWrite in
https://developer.mbed.org/users/mbed_official/code/I2CEeprom/file/973c4289c44c/I2CEeprom.cpp
which is based on write operation with data and length of 0. while (m_i2c.write(m_i2cAddress, 0, 0) != 0) {

So basically I've added this fix for both read and write operations, although this is being used so far with write only. So maybe your last point is valid. What I tend to remember is that I had no success in making all my use cases work fine without the calls to HAL_I2C_IsDeviceReady, so I'm a bit reluctant in a first place to fully get rid of it, but maybe testing would prove otherwise.

So I'd propose you to send a Pull Request (or 2 versions if you like) and I'll review of course but mainly I'll run the I2C CI tests on our STM32 targets to check there aren't any regression. Would that be ok ?

@bscott-zebra
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LMESTM
Sounds good to me. I'll prepare a pull request for the simplest version and we'll see how the testing goes. If failures occur, I can resubmit one that keeps HAL_I2C_IsDeviceReady() in place for 0 length, non-repeated start writes to test.

Please sign in to comment.