search

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.48k stars 6.41k forks source link

lsm6dso sensor driver not working on nRF5340 #33440

Closed richardbarlow closed 3 years ago

richardbarlow commented 3 years ago

Describe the bug The LSM6DSO sensor driver does not function on the nRF5340 SoC. This appears to be the exact same problem as outlined in #11612, which was fixed for the LIS2DH in #19901 and issue #20154 was created as a 'marker' issue to reference when implementing the workaround.

The issue is that on the nRF51, nRF52 and nRF53 it is impossible to correctly implement the i2c_burst_write() API, which means that it is not cross-platform. What should be executed as a single I2C transaction ends up split with a repeated start. #17801 indicates that using the TWI, rather than TWIM peripheral may fix this, but the nRF53 only has TWIM.

Having applied the workaround noted in https://github.com/zephyrproject-rtos/zephyr/issues/20154#issuecomment-546356556 to drivers/sensor/lsm6dso/lsm6dso_i2c.c the sensor functions correctly.

I could submit a pull-required that adds this workaround into the LSM6DSO driver, however I feel that will be doing the world a disservice. The i2c_burst_write() API is used all over the place and I would prefer to prevent others like myself from falling into the same trap (and wasting a day tracking down the issue) when using any of the sensor drivers.

The LSM6DSO driver, uses the stmemsc library (https://github.com/zephyrproject-rtos/hal_st/tree/master/sensor/stmemsc). I haven't checked exhaustively, but I'm guessing that all of the other ST MEMS sensors drivers do the same. This library requires a read and write function to be provided to perform the actual read/write operations over the relevant hardware interface. Each Zephyr sensor driver that uses the stmemsc appears to have its own identical implementation of the read/write functions. Currently these functions are relatively simple and, in the case of the I2C write implementation, all buggy. One way of fixing the issue that I am encountering would be to apply the workaround to each of the write functions, but I think it would be better to factor these functions out somehow. Unfortunately this is at the limits of my experience with Zephyr and its architecture and I would appreciate some advice on how I might go about doing this. In particular the stmemsc library passes an opaque pointer through to the read/write functions and each sensor driver uses its own _data object. If the read/write functions were generalised then a similarly generalised type would need for the handle.

It is also of note that the fix made in #19901 for the LIS2DH was undone by 1ebb7089104156c63603797f05f3ae194e0307a2, which presumably means it's broken again when using it with an nRF51/2/3 SoC.

Environment (please complete the following information):

galak commented 3 years ago

@richardbarlow so I think a change to have a common implementation for stmdev_write_ptr and stmdev_read_ptr would be acceptable.

My suggestion would be to add something like:

drivers/sensor/stmemsc/i2c.{c,h} with the common implementation and update the various drivers to utilize that.

@avisconti is the maintainer of those drivers so he can add his 2 cents.

avisconti commented 3 years ago

@richardbarlow so I think a change to have a common implementation for stmdev_write_ptr and stmdev_read_ptr would be acceptable.

My suggestion would be to add something like:

drivers/sensor/stmemsc/i2c.{c,h} with the common implementation and update the various drivers to utilize that.

@avisconti is the maintainer of those drivers so he can add his 2 cents.

Not all the ST sensor drivers are currently using the stmemsc HAL interface. JUst some of them. Nevertheless could be not a bad idea to have a single couple of read/write routines for both i2c and spi and for all stmemsc based sensor driver. I can work on that tomorrow.

avisconti commented 3 years ago

@richardbarlow @galak would that be ok?

avisconti commented 3 years ago

Please take a look to #33506. I have introduced new stmemsc common routines, and tested it with iis2iclx driver sensor for my convenience. Please review it. After that next steps are to apply the same patch as #19901 and change lsm6dso to use this routines. I wil start asap with the lsm6dso change. I will let you apply the fix for I2C burst write after I'm done with the 6dso job. PLease have a review. Thanks!

richardbarlow commented 3 years ago

@avisconti I've just tried #33530 with the patch in #19901 applied locally and it all works perfectly! Thank you very much for sorting this out so quickly. Once #33530 is merged in I'll create a PR to fix i2c burst writes on nRF SoCs.

avisconti commented 3 years ago

@avisconti I've just tried #33530 with the patch in #19901 applied locally and it all works perfectly! Thank you very much for sorting this out so quickly. Once #33530 is merged in I'll create a PR to fix i2c burst writes on nRF SoCs.

This is the good part of working in open source projects: we are like a big sinergic team!

avisconti commented 3 years ago

@richardbarlow

33530 has been merged.

Can you prepare a PR to address this issue? I'm of course available to review it.

richardbarlow commented 3 years ago

@avisconti I've just seen. Thank you very much for sorting that out. It seems it turned out to be more work than I was expecting! I will create a PR right now.

galak commented 3 years ago

@richardbarlow any updates on this?

jfischer-no commented 3 years ago

The issue is that on the nRF51, nRF52 and nRF53 it is impossible to correctly implement the i2c_burst_write() API, which means that it is not cross-platform. What should be executed as a single I2C transaction ends up split with a repeated start. #17801 indicates that using the TWI, rather than TWIM peripheral may fix this, but the nRF53 only has TWIM.

That is not right, see my comment https://github.com/zephyrproject-rtos/zephyr/pull/34066#issuecomment-820386381, it also depends on controller type. The issue is rather the user experience with the proposed solution, as it is not obvious. cc @anangl

github-actions[bot] commented 3 years ago

This issue 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 issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.