drivers: sensors: stmemsc: Don't use i2c_burst_write on Nordic #34066
Conversation
|
Why can not you use "zephyr,concat-buf-size" property for |
I would guess because that's a controller configuration, and it's needed only if there happens to be an ST device on the bus. |
We do not do it per device and we should not. This solution here does not even distinguish which controller is used. |
I didn't know this property was even existing (I don't use nordic platforms). Now I feel that using this property is much cleaner, as it moves the proper solution inside where the issue lies. But I let you guys discuss more about it. |
I agree with @jfischer-no. Using this property is a better solution for the issue. |
@richardbarlow |
@pabigot |
My thought was: The buffer configuration is within the TWIM devicetree node. A workaround for burst write is only necessary for a bus controlled by a Nordic TWIM peripheral. Since stmemsc uses the burst write API all sensors it supports are susceptible to failure on Nordic TWIM buses. So if you take a Nordic-based board, attach an ST sensor to an I2C bus, and try to use it, it will fail to work correctly unless you modify the board devicetree (directly, or via an overlay for the application) to change the controller's concatenation buffer size. Unless you do that proactively for all Nordic boards that use TWIM, any ST sensor on any of those boards may fail. Which is a painful user experience, resulting in reports that the sensor doesn't work on Nordic hardware. While yes, every application where that occurs could be fixed by modifying the devicetree files once the problem is (re)discovered for each user, the fix here solves it globally for the Nordic/ST combination without requiring any special handling for particular board/sensor combinations. But I may be misunderstanding what's going on, and I'm not maintaining I2C or Zephyr anymore, so feel free to do whatever y'all think is best. |
So, let's say that every device on the Nordic I2C bus with a driver using the burst write is susceptible of failure. Correct?
That's clear.
Yes, I understand your rational of concentrating in only one place the change. And to be honest I'm not 100% against it. But let's face the fact that there is already a clean solution to the TWIM burst write issue (if I understand well), which is using that property which is there exactly to solve this issue. Isn't it? So, if everyone agrees on my statements above my feeling is that we need to change all the Nordic affected boards including that property in the DT. That can be done in a single PR.
I have nothing against this patch, but again my feeling is that there is a cleaner solution. EDIT: |
|
Just to chip in with my 2 cents (after uncovering a potentially contentious topic). Please note that I would class myself as a 'user' of Zephyr, rather than a Zepher developer. A brief historyI ended up here after attempting to use one of the ST sensor drivers with an nRF53 SoC. As I'm sure will be obvious, it did not work. It took a little time to track down, but after staring at the logic analyser output for a while it occurred to me that the behaviour I was observing (repeated start) may be the cause of the issue. This led me to look for anything (Issues/Pull Requests) that may be related to what I was seeing. The outcome of this can be seen in #33440. As noted in that issue, I discovered that it was already a known problem and others had previously been bitten by it. Specifically the problem being that the TWIM peripheral makes it impossible to implement the Based on my findings, I suggested that it may be sensible to factor out the stmemsc interface routines so that all ST sensors using the stmemsc library may benefit from the fix that I am now proposing. @avisconti very kindly implemented the refactoring. My observations
Note that #12588 added documentation to warn that |
+1 Perhaps we need an |
Why is it buggy, who decided it? |
Not a specific driver should fall back but probably I2C API / I2C transfer manager. |
drivers/sensor/stmemsc/stmemsc_i2c.c
Outdated
| * I2C master uses TWI or TWIM, so use a substitute implementation | ||
| * unconditionally on Nordic. | ||
| */ | ||
| if (IS_ENABLED(CONFIG_I2C_NRFX)) { |
There was a problem hiding this comment.
Since it seems that using i2c_burst_write() is deprecated (#12588) why not calling always the i2c_write() and get rid of the i2c_burst_write()? I mean, using this piece of code not only for Nordic platform but always, following the i2c API comment from @pabigot:
- @warning The combined write synthesized by this API may not be
- supported on all I2C devices. Uses of this API may be made more
- portable by replacing them with calls to i2c_write() passing a
- buffer containing the combined address and data.
There was a problem hiding this comment.
To be honest, as ST sensors maintainer I would like to see them working properly with all platforms. And it is not very clean to have a check of the platfrom here in this code. Better get rid of it. @richardbarlow would you kindly re-implement this PR so that I could also verify it on my platforms?
There was a problem hiding this comment.
Since it seems that using i2c_burst_write() is deprecated (#12588)
I think that is a rumor.
why not calling always the i2c_write() and get rid of the i2c_burst_write()
Because it is generally very useful function and many operating systems offer it?
@jfischer-no |
No one was asked to do it here.
No, it is not deprecated. |
This was already clear. I was just summarizing...
Still, as i2c API user, I'm evaluating to replace inside stmemsc the i2c_burst_write() API with i2c_write(), copying in a single buffer register and data, What would the advantage be of using one approach vs the other? I guess that avoiding the copy is of course one of them, but this happens only when programming the sensors. Other advantages? |
The TWIM peripheral in Nordic SoCs (nRF5{1,2,3}) cannot correctly
support the i2c_burst_write API. Using it results in a repeated start,
rather than a single I2C write transaction.
Use i2c_write instead, which will always work correctly.
Fixes zephyrproject-rtos#33440
Signed-off-by: Rich Barlow <rich@bennellick.com>
richardbarlow
force-pushed
the
fix_33440_stmemsc_i2c_on_nrf
branch
from
efcba08
to
d454133
Compare
I've just updated the PR to always use |
Is it not a sufficient argument? There is also counterpart i2c_burst_read() to read sensor (array) values. |
@jfischer-no The observation above is also stated in #20154:
But what I seem to understand is that also using the "zephyr,concat-buf-size" property is not satisfactory, as it is imposing the i2c_nrfx_twim driver to have a behaviour that is efficient only for a subset of I2C devices attached to the bus. So, I'm a bit confused about how to proceed. |
Thanks @richardbarlow . I still have some doubts regarding the fixed buffer size to 10 bytes. In the future there might be other ST sensors with different size requirements. So, confused a bit now... |
|
|
||
| __ASSERT((1U + len) <= sizeof(buffer), "stmemsc i2c buffer too small"); | ||
| buffer[0] = reg_addr; | ||
| memcpy(buffer + 1, value, len); |
There was a problem hiding this comment.
__ASSERTs are useless. memcpy(&buffer[1], value, MIN(len, sizeof(buffer) - 1)); ?
There was a problem hiding this comment.
But here you should at least report that you are writing less than requested. Something wrong will happen somewhere in any case. Moreover, as I said earlier, I feel that imposing a maximum size (reg + 9 byte of data here) may lead to issues in the next future.
There was a problem hiding this comment.
Don't use a literal 9 or 10; define a constant in a header that specifies the maximum data length for one of the relevant operations (e.g. maximum size of config structures, if those are the ones that are a problem), and document it needs to be updated when things change.
If you put that define in the upstream stmemsc package then everything should be good because the stmemsc maintainers should know when new sensors that have larger transfers are added.
There was a problem hiding this comment.
@pabigot
I am the stmemsc maintainer (sigh!).
The issue here is that things may get more complex in the future, with sensor that has the capability to drive external sensors (the shub feature is already there in some of the current sensor drivers) and maybe other things. Calculating the maximum size and adapting the macro evrytime a new sensor (or a new feature in a sensor) is added can lead to trivial errors.
Frankly speaking I would rather keep the i2c_burst_write() for the SoC that have I2C scatter-gather capability and the i2c_write implementation for the others. If we want to avoid the NFRX platform test, maybe we can add in stmemsc a new property that provides the burst-skipping info (and maybe another with the max buffer size).
In this way, we confine the work to stmemsc w/o affecting the I2C TWIM driver behaviour. Of course the default of that property will be "use-burst". The NFRx platforms will have to indicate the alternative usage.
What do you all think?
There was a problem hiding this comment.
Something like "stmemsc,buf-size". When is 0 (the default) means go with i2c_burst_write. The property must be included in stmemsc based sensors when they are on a NFRx platform.
There was a problem hiding this comment.
Responding only because I was directly asked for this: I don't have a horse in this race because I've given up on Zephyr being useful to me personally, so my patience with wicked issues like this is exhausted.
There are so many ways this could be resolved; each has different trade-offs of (a) amount of cooperation required from reviewers and other maintainers to get the solution merged, and (b) impact on Zephyr maintainability and usability.
I don't see where stmemsc,buf-size could be provided or how its value could be determined by most users, so I would do this:
#if DT_HAS_COMPAT_STATUS_OKAY(nordic_nrf_twim)
/* The underlying I2C driver might be nordic,nrf-twim, and if so it might
* not have a zephyr,concat-buf-size large enough to correctly implement
* i2c_burst_write of len bytes. Since we can't verify that easily at
* either build or run time, pack the write payload locally.
*/
uint8_t buf[1 + len];
buf[0] = reg_addr;
memcpy(buf+1, value, len);
return i2c_write(...);
#else
return i2c_burst_write(...);
#endif
This is conservative in that on Nordic platforms it sometimes uses non-burst write in cases where burst-write would have worked, but checking for that at build time (or even at runtime) is IMO not feasible or doesn't solve the problem of clearly indicating to the user that there's a knob that could be set to make the code work.
But depending on Zephyr coding guidelines the use of a VLA might be rejected, in which case the alternative is a constant that comes from a header file or Kconfig and a runtime check against length. But that still wouldn't solve the problem of a novice user wondering why their sensor doesn't work.
There was a problem hiding this comment.
Responding only because I was directly asked for this: I don't have a horse in this race because I've given up on Zephyr being useful to me personally, so my patience with wicked issues like this is exhausted.
Sorry for that.
There are so many ways this could be resolved; each has different trade-offs of (a) amount of cooperation required from reviewers and other maintainers to get the solution merged, and (b) impact on Zephyr maintainability and usability.
I don't see where
stmemsc,buf-sizecould be provided or how its value could be determined by most users, so I would do this:#if DT_HAS_COMPAT_STATUS_OKAY(nordic_nrf_twim)
I think you need a DT_DRV_COMPAT to do that, but we do not have in stmemsc as this is more like a library used by many drivers. I would prefer the original check as before (i.e. IS_ENABLED(CONFIG_I2C_NRFX)
/* The underlying I2C driver might be nordic,nrf-twim, and if so it might
- not have a zephyr,concat-buf-size large enough to correctly implement
- i2c_burst_write of len bytes. Since we can't verify that easily at
- either build or run time, pack the write payload locally.
*/
uint8_t buf[1 + len];
I think we should avoid arrays with variable length, and accept the fact that, in case of nfrx TWIM platforms, we will handle the normal 9axis case (9 + 1 byte) as it was before.
More complex sensor features will not be handled here.
buf[0] = reg_addr;
memcpy(buf+1, value, len);
return i2c_write(...);
#else
return i2c_burst_write(...);
#endifThis is conservative in that on Nordic platforms it sometimes uses non-burst write in cases where burst-write would have worked, but checking for that at build time (or even at runtime) is IMO not feasible or doesn't solve the problem of clearly indicating to the user that there's a knob that could be set to make the code work. But depending on Zephyr coding guidelines the use of a VLA might be rejected, in which case the alternative is a constant that comes from a header file or Kconfig and a runtime check against length. But that still wouldn't solve the problem of a novice user wondering why their sensor doesn't work.
In my opinion it would be better to avoid VLAs.
|
@richardbarlow In this we should be able to make th standard 9 axis (or less) in the usual "flavor" working fine. |
That is not correct, again, there is "zephyr,concat-buf-size" property for compatible = "nordic,nrf-twim" to resolve this. |
@jfischer-no
|
It's a property in the controller devicetree node (parent of the "slave"s); the driver instance reserves memory to use as a buffer. The weakness of that approach is that anybody using a Nordic-based board with ST sensors on it must somehow be aware that for the sensor to work they have to override their devicetree bindings: they must set that property in the parent controller(s) to an undocumented value (being the maximum of the undocumented maximum buffer length required over all attached sensors). Or:
What value would that be? Should all instances of Nordic TWIM pay the cost of supporting ST sensors that are only rarely present? |
|
Sorry for the silence at my end. I've had to move on to other parts of the firmware that I am currently writing. @avisconti I have switched to using the @jfischer-no I also mildly agree that the fix that I am proposing here isn't the best solution to the problem. Now that I know of the existence of the
@pabigot While the focus of this PR has been ST sensors, surely any I2C device that expects a single write transaction is susceptible? (without extensive research, we don't know if/how many devices that may be). Anywhere where a device driver author has used |
OK, thanks for confirming it.
That was exactly my point: every driver using i2c_burst_write will fall into the same scenario. |
@avisconti to be even more technically correct, every driver using i2c_burst_write might fall into the same scenario, and we have no way of knowing without lots of tedious datasheet reading. The harsh reality is that different people may have made different assumptions when using i2c_burst_write and those assumptions are not documented |
|
@anangl could you please comment on this one? |
|
This issue is not unique to scenarios that combine an nRF I2C controller with an STM sensor; NXP I2C drivers also generate a repeated start and NXP sensors cannot tolerate it. I handled this in the NXP sensor drivers (e.g., fxos8700) by using a series of |
|
API meeting 2021.06.01: Steps proposed:
|
Any news on this? |
|
@carlescufi - just checking in - is this one still moving forward? |
No, agreement is to take a different approach. We will follow-up in the coming days. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
The TWIM peripheral in Nordic SoCs (nRF5{1,2,3}) cannot correctly
support the i2c_burst_write API. Using it results in a repeated start,
rather than a single I2C write transaction.
This implements the workaround noted in #20154.
Fixes #33440
Signed-off-by: Rich Barlow rich@bennellick.com