Unable to leave off stop bit for I2C transactions on STM32

[RobMeades]

Raised in Discord https://discord.com/channels/720317445772017664/883445358821249084/1209677653259329568 but no response and this seems to pretty clearly be a problem in the STM32 driver inside the Zephyr code base, hence raising it here.

We talk to I2C HW, a u-blox GNSS device (any one, they all work the same way), which behaves like some I2C flash devices do, in that, to set the register that you are addressing inside the device you send the sequence:

Start bit I2C address I2C write register address X Start bit I2C read (and the device will return the value at register address X)

This is sometimes called a "repeated start".

The i2c_transfer() API allows this: we simply do not set the I2C_MSG_STOP flag set in the call to i2c_transfer() for the write operation.

However, as far as we can tell, the STM32 I2C driver ALWAYS sets I2C_MSG_STOP at the end of a transaction (see here), irrespective of whether the application intended it or not

This means we are unable to use STM32 MCUs with such I2C HW. nRF52/nRF53 works fine with this pattern, and in fact we can use the STM32 LL API outside Zephyr and that works fine also, it is only the STM32 I2C driver within Zephyr that has this problem.

Correct behaviour:

Incorrect behaviour:

Our GNSS code can be found here, where you can see the last parameter to uPortI2cControllerSend() (which calls i2c_transfer()) when performing this "set register address" operation is set true, meaning noStop. In case it is of interest, our "STM32 native" equivalent code, which performs the operation correctly, can be found here.

[RobMeades]

I would argue with "priority low": it is physically impossible for any of our customers to use an STM32 MCU in Zephyr to talk to any u-blox GNSS device over I2C, likely also most I2C-based EEPROM devices, until this behaviour is fixed; it is a show-stopper for at least two projects that we know of.

EDIT: and of course it means that STM32 devices don't properly support the i2c_transfer() Zephyr API.

[erwango]

I would argue with "priority low"

I can understand you don't have the same view, but severity is set from whole project point of view, only one case of one driver of one type of target is impacted (which is a different impact as kernel scheduling is broken). If it was proven that the driver is fully broken, we can raise it to medium but this is not the case yet from what I know. Also note that almost anyone that raises an issue sees the issue as blocking in some ways.

This being said, please be sure this doesn't mean that we won't consider this point. We will have a look as soon as possible. But you need to understand as well that there are other activities going on and other issues in the backlog. We can't stop everything each time a bug is raised.

[RobMeades]

Understood, I just wanted to point out that this is not an optional thing, it stops everything working for an STM32 MCU talking to a u-blox GNSS over I2C, which seems a popular combination amongst our customers.

[erwango]

@RobMeades Before digging deeper into your specific case. Did you see that I2C API opens the door to implementation specific behavior around use of I2C_MSG_RESTART flag ? See https://github.com/zephyrproject-rtos/zephyr/blob/53f527cc2dc322302a11e2a524126e62a4dc6834/include/zephyr/drivers/i2c.h#L169-L182

[RobMeades]

Yes, I don't believe we can use this; our port API (which we use for all [fiveish] supported SDKs, of which Zephyr is one) is just-send-with-a-no-stop-option and send/receive, each supporting a single transaction only. Not sure how I2C_MSG_RESTART could be made to fit into that.

Plus it says that "not all I2C drivers have or require explicit support for this feature", which means Zephyr would no longer be a platform independent API, we would need to talk to Zephyr differently for different MCUs, kind of defeating the object of the use of Zephyr in the first place.

[RobMeades]

As a suggestion, since I guess you would be afraid to break customers who never bothered setting I2C_MSG_STOP in their I2C message transactions if you just removed the offending line in the I2C driver, you could probably invent a CONFIG_STM32_I2C_ALWAYS_STOP_DISABLE configuration flag or some such [EDIT: maybe CONFIG_STM32_I2C_HANDS_OFF_MY_FLAGS :-)] and put it around that line. We could then advise customers what to do to make things work for STM32 on Zephyr.

EDIT: it is probably not quite that simple: in our native STM32Cube driver code we also need to remember to ignore the busy flag on the next transaction as the HW is "stop bit aware".

[erwango]

I guess you would be afraid to break customers [...]

You nailed it.

Plus it says that "not all I2C drivers have or require explicit support for this feature", which means Zephyr would no longer be a platform independent API, we would need to talk to Zephyr differently for different MCUs, kind of defeating the object of the use of Zephyr in the first place.

I understand this is unfortunate, but that's the way I2C api is designed. You can discuss this with I2C maintainer.

you could probably invent a CONFIG_STM32_I2C_ALWAYS_STOP_DISABLE configuration flag or some such and put it around that line. We could then advise customers what to do to make things work for STM32 on Zephyr.

Note that there is NRF, STM32 but others as well... I would be surprised we're the only one exposing this behavior

[RobMeades]

nRF52/nRF53 work fine, we already have customers using that.

EDIT: oh, I see you, mean MCUs other than Nordic and ST - I hope not! If so, the Zephyr I2C API isn't much of an API: having the driver override flags that the application is meant to be in control of is not something I would normally expect.

[RobMeades]

Might be worth noting that the mbed API, which I know ST supports pretty thoroughly, adopts the same approach as ours with write(int address, const char * data, int length, bool repeated) (repeated being the same as noStop), so there is an existing driver implementation that could be looked at there also.

[erwango]

Adding @teburd as the I2C maintainer, as I think that if there's an issue with the API leaving too much place to driver specific behavior, he should be involved. @teburd What's your take on this ?

[Desvauxm-st]

@RobMeades
do you use CONFIG_I2C_STM32_V1 or CONFIG_I2C_STM32_V2 ?

[RobMeades]

@Desvauxm-st: we don't specify either of these in our config so I guess whatever the default is...?

[erwango]

@Desvauxm-st: we don't specify either of these in our config so I guess whatever the default is...?

That actually depends on the STM32 series you will use. There are 2 I2C versions (which 2 different drivers) amongst STM32 series. So for instance STM32F4 will use V1 and STM32L4 will use V2.

[RobMeades]

We happen to be testing with a Nucleo F767zi board but the two customers that have raised this issue with is so far have been using F466RE and WL.

[RobMeades]

@erwango: thanks for adding @teburd. @teburd: maybe the right question to ask here is:

• there is HW currently in use in the world which requires a "send-with-no-stop-bit" operation to work properly (our u-blox GNSS modules, some I2C-based EEPROMs, maybe others),
• what is the "canonical" way to achieve a "send-with-no-stop-bit" operation at the Zephyr I2C API?

I had assumed the way was to not set the I2C_MSG_STOP flag, but maybe there is another way? Whatever the way is, it must be independent of the underlying platform since that's what we are relying on Zephyr for.

[teburd]

Are you attemping to not stop in one i2c_transfer() call and then do a start in a seperate i2c_transfer() call? That's not portable, and in the worst case could cause other issues if multiple devices are on the bus and multiple call contexts want to use that bus.

Otherwise i2c_transfer lets you do full start->write->start->read->stop style transactions already and should be the way to do it. See the i2c_write_read helper https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/i2c.h#L880

That does exactly what you are asking for in a way that's guaranteed to work on all drivers.

For some additional reasoning... most/all drivers now have a semaphore or mutex that is taken in i2c_transfer and released just before return. Once the call returns, its perfectly possible for any other thread to come in and start a new transfer. An i2c_transfer() that leaves the bus in a state that might break the next caller is arguably a bug because of this. I actually think the st driver might be more correct in this case then if others are not doing this.

[RobMeades]

That's not portable, and in the worst case could cause other issues if multiple devices are on the bus and multiple call contexts want to use that bus.

Well it is the API we have used for a quite a few years now and it works on all of the SDKs we support (currently ESP-IDF, STM32Cube, nRF5SDK and Linux as well as, up to now at least, Zephyr). Understood that it would cause an issue if the application does not protect access to the I2C bus during the transaction.

So is it NOT true that the flags in message.flags are the property of the application, i.e. not setting a flag doesn't mean anything, the driver may always set that flag?

EDIT: or maybe, in a more limited sense, an I2C transaction will ALWAYS END WITH A STOP bit; this is not under the control of the caller? EDIT TO THE EDIT: though this is clearly not the case with the Nordic I2C driver.

[teburd]

I’d say the final message yes, should probably imply a stop. That doesn’t mean a transfer with multiple starts and stops isn’t supported so the bit flags still do make sense.

I do think we can agree this isn’t specified in the docs though and should be. I will fix that tomorrow.

[RobMeades]

OK, in that case I will tell the customers that are stuck on this that STM32-under-Zephyr won't work for them for now.

We will need to migrate to a new function in our I2C port API for the I2C-send-without-stop case (basically to an I2C-send-and-receive-without-intermediate-stop instead). This will have to be implemented and tested on all the SDKs we use which may take a little while.

[teburd]

@RobMeades you mentioned this is not how the nordic driver works, please file a bug and note #69484