Skip to content

drivers/lsm6dsxx: refactoring Lsm6dsl into common driver Lsm6dsxx#20170

Merged
miri64 merged 11 commits intoRIOT-OS:masterfrom
miquel-b:lsm6dsxx
Jan 30, 2024
Merged

drivers/lsm6dsxx: refactoring Lsm6dsl into common driver Lsm6dsxx#20170
miri64 merged 11 commits intoRIOT-OS:masterfrom
miquel-b:lsm6dsxx

Conversation

@miquel-b
Copy link
Copy Markdown

@miquel-b miquel-b commented Dec 12, 2023 •
edited
Loading

Contribution description

RIOT already offers support for the Gyro-Accelerometer sensor LSM6DSL found in the b-l475e-iot01a board. There is a very similar sensor (LSM6DS33) in the feather-nrf52840-sense board that shares its characteristics. This PR intends to merge both drivers.

Testing procedure

feather-nrf52840-sense Input: 
BUILD_IN_DOCKER=1 DOCKER='sudo docker' BOARD=feather-nrf52840-sense DRIVER=lsm6ds33 make -C ~/RIOT/tests/drivers/lsm6dsxx all flash term -j

Output:

Accelerometer x: -4 y: 43 z: 1038
2024-01-18 14:32:07,762 # Gyroscope x: 40 y: -48 z: -55
2024-01-18 14:32:07,763 # Temperature [in °C x 100]: 2491 
2024-01-18 14:32:07,763 # 
2024-01-18 14:32:08,263 # Accelerometer x: -4 y: 43 z: 1040
2024-01-18 14:32:08,266 # Gyroscope x: 39 y: -49 z: -55
2024-01-18 14:32:08,267 # Temperature [in °C x 100]: 2491 
2024-01-18 14:32:08,268 # 
2024-01-18 14:32:08,767 # Accelerometer x: -5 y: 44 z: 1039
2024-01-18 14:32:08,771 # Gyroscope x: 39 y: -48 z: -55
2024-01-18 14:32:08,772 # Temperature [in °C x 100]: 2491 
2024-01-18 14:32:08,772 # 
2024-01-18 14:32:09,272 # Accelerometer x: -4 y: 44 z: 1039
2024-01-18 14:32:09,275 # Gyroscope x: 39 y: -48 z: -55
2024-01-18 14:32:09,276 # Temperature [in °C x 100]: 2491
b-l475e-iot01a Input 
BUILD_IN_DOCKER=1 DOCKER='sudo docker' BOARD=b-l475e-iot01a DRIVER=lsm6dsl  make -C ~/RIOT/tests/drivers/lsm6dsxx all flash term -j

Output:

2024-01-18 14:33:25,059 # LSM6DSXX test application
2024-01-18 14:33:25,081 # Initializing LSM6DSXX sensor at I2C_1... [SUCCESS]
2024-01-18 14:33:25,081 # 
2024-01-18 14:33:25,084 # Powering down LSM6DSXX sensor...
2024-01-18 14:33:25,086 # [SUCCESS]
2024-01-18 14:33:25,087 # 
2024-01-18 14:33:26,089 # Powering up LSM6DSXX sensor...
2024-01-18 14:33:26,092 # [SUCCESS]
2024-01-18 14:33:26,092 # 
2024-01-18 14:33:26,098 # Accelerometer x: -70 y: 83 z: 1013
2024-01-18 14:33:26,105 # Gyroscope x: -1712 y: 2237 z: -1820
2024-01-18 14:33:26,108 # Temperature [in °C x 100]: 2425 
2024-01-18 14:33:26,108 # 
2024-01-18 14:33:26,615 # Accelerometer x: -8 y: -7 z: 1018
2024-01-18 14:33:26,620 # Gyroscope x: -4 y: -5 z: -4
2024-01-18 14:33:26,624 # Temperature [in °C x 100]: 2408 
2024-01-18 14:33:26,624 # 
2024-01-18 14:33:27,131 # Accelerometer x: -8 y: -7 z: 1020
2024-01-18 14:33:27,136 # Gyroscope x: -4 y: -5 z: -4
2024-01-18 14:33:27,140 # Temperature [in °C x 100]: 2410 
2024-01-18 14:33:27,140 # 
2024-01-18 14:33:27,647 # Accelerometer x: -7 y: -7 z: 1020
2024-01-18 14:33:27,652 # Gyroscope x: -4 y: -5 z: -4
2024-01-18 14:33:27,656 # Temperature [in °C x 100]: 2414 
2024-01-18 14:33:27,656 # 
2024-01-18 14:33:28,163 # Accelerometer x: -8 y: -7 z: 1021
2024-01-18 14:33:28,168 # Gyroscope x: -4 y: -5 z: -4
2024-01-18 14:33:28,172 # Temperature [in °C x 100]: 2412 
2024-01-18 14:33:28,172 # 
2024-01-18 14:33:28,679 # Accelerometer x: -9 y: -7 z: 1021
2024-01-18 14:33:28,684 # Gyroscope x: -4 y: -5 z: -4
2024-01-18 14:33:28,688 # Temperature [in °C x 100]: 2417

Issues/PRs references

@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration labels Dec 12, 2023
@MrKevinWeiss MrKevinWeiss changed the title drivers/Rrefactoring Lsm6dsl into common driver Lsm6dsxx drivers/lsm6dsxx: refactoring Lsm6dsl into common driver Lsm6dsxx Dec 13, 2023
MrKevinWeiss
MrKevinWeiss reviewed Dec 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Hmmm I think there is a lot of code duplication here.

Maybe it would be better to just make one module lsm6dsxx and ust use PSEUDOMODULES for the specific devices.

Just browsing around I found the lpsxxx a good example.

@MrKevinWeiss MrKevinWeiss self-assigned this Dec 13, 2023
MrKevinWeiss
MrKevinWeiss reviewed Dec 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Much better, still a few things to note, I would suggest breaking it up into the following commits:

  1. Rename all files and dirs
  2. Replace all lsm6dsl with lsm6dsxx in the files
  3. Add the lsm6ds33 ifdefs, register splitting, and makefile changes
  4. Adapt the feather-nrf52840-sense params and add the saul device to the makefile.dep
  5. Adapt the test

Remember to keep the credit of others and add your own.

boards/feather-nrf52840-sense/include/board.h Outdated
*/
#define LSM6DSXX_PARAM_I2C I2C_DEV(0)
#define LSM6DSXX_PARAM_ADDR (0x6A)
#define LSM6DSXX_WHO_AM_I (0b01101001)
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss Dec 18, 2023

Choose a reason for hiding this comment

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

I guess we don't need that now.

@miquel-b miquel-b force-pushed the lsm6dsxx branch 2 times, most recently from d8ba167 to 5ae9e1f Compare December 21, 2023 12:11
MrKevinWeiss
MrKevinWeiss reviewed Jan 3, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Still a few minor things but looking good!

MrKevinWeiss
MrKevinWeiss reviewed Jan 16, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Very nice, just a few more nit-picks!

@miquel-b miquel-b force-pushed the lsm6dsxx branch 2 times, most recently from 975d584 to 61201b8 Compare January 18, 2024 13:18
MrKevinWeiss
MrKevinWeiss approved these changes Jan 18, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Look's good, I saw the tests being run, maybe it would be good to paste the logs. ACK.

@MrKevinWeiss MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 18, 2024
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Jan 19, 2024

I have added the last few fixups, please go through them and if you are happy you may squash

git rebase master -i --autosquash

The murdock failure looks unrelated.

@github-actions github-actions bot removed the Area: SAUL Area: Sensor/Actuator Uber Layer label Jan 24, 2024
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Jan 24, 2024

hmmm something didn't work out right... I am just adding the commits directly

@github-actions github-actions bot added the Area: SAUL Area: Sensor/Actuator Uber Layer label Jan 24, 2024
@MrKevinWeiss MrKevinWeiss added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 24, 2024
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Jan 24, 2024

Hmmm native timer tests failing again... This PR should not touch anything in native.

@MrKevinWeiss MrKevinWeiss added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 25, 2024
miri64
miri64 approved these changes Jan 30, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I did not look deeply into the code, but from what I can see it looks good. Much more important: I ran the test on a feather-nrf52840-sense and I was able to confirm that the readouts were correct.

@miri64 miri64 added this pull request to the merge queue Jan 30, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 30, 2024
@miri64 miri64 added this pull request to the merge queue Jan 30, 2024
Merged via the queue into RIOT-OS:master with commit 34fcffe Jan 30, 2024
@mguetschow mguetschow mentioned this pull request Feb 27, 2024
Merged
@MichelRottleuthner MichelRottleuthner mentioned this pull request Mar 22, 2024
Merged
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

@MrKevinWeiss MrKevinWeiss MrKevinWeiss approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@smlng smlng Awaiting requested review from smlng

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees

@MrKevinWeiss MrKevinWeiss

Labels

Area: boards Area: Board ports Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: SAUL Area: Sensor/Actuator Uber Layer Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

Release 2024.04

Development

Successfully merging this pull request may close these issues.

4 participants

@miquel-b @riot-ci @MrKevinWeiss @miri64