Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver/lsm6dsxx: Allow generic LSM6DSXX driver #20504

Merged
merged 4 commits into from
Mar 27, 2024

Conversation

MrKevinWeiss
Copy link
Contributor

Contribution description

We noticed that the different variants of the feather-nrf52840-sense boards (B/C) have different LSM6DSXX variants. Instead of making 2 different boards, we would just allow the LSM6DSXX module to be used by itself and only the generic (so far all of them) functions can be used.

This also allows automated tests to be run... with some fun surprises along the way.

Testing procedure

Run the following on both variants:

BUILD_IN_DOCKER=1 make -C tests/drivers/lsm6dsxx flash test-with-config

both should pass...

Issues/PRs references

@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: build system Area: Build system Area: drivers Area: Device drivers Area: tools Area: Supplementary tools labels Mar 26, 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 Area: tests Area: tests and testing framework Area: build system Area: Build system Area: drivers Area: Device drivers Area: tools Area: Supplementary tools labels Mar 26, 2024
@MrKevinWeiss
Copy link
Contributor Author

ping @MichelRottleuthner @mguetschow

@MrKevinWeiss MrKevinWeiss force-pushed the pr/feather-nrf52840-sense/generic branch from 778af29 to 57e006d Compare March 26, 2024 11:40
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: build system Area: Build system Area: drivers Area: Device drivers Area: tools Area: Supplementary tools labels Mar 26, 2024
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! 👍 Two questions below.

Can confirm this (still) works here with board revision B.

drivers/lsm6dsxx/lsm6dsxx.c Outdated Show resolved Hide resolved
tests/drivers/lsm6dsxx/Makefile Show resolved Hide resolved
@MrKevinWeiss
Copy link
Contributor Author

Although, after looking a bit through the datasheet it would appear that this may not work.

The LSM6DS33 has this temp sensor
image

and the LSM6DSL has this:
image

@MrKevinWeiss
Copy link
Contributor Author

I guess this means we have to runtime check the whoami register and use those values for calculating the temp (and anything else that shows up).

@MrKevinWeiss
Copy link
Contributor Author

Also temperature offset of +-15 I don't understand

@MichelRottleuthner
Copy link
Contributor

Just to make sure there is no confusion: the driver currently supports two variants (LSM6DS33 and LSM6DSL). The new feather board rev. C uses neither of them. It uses the LSM6DS3TR-C. Although LSM6DSL and LSM6DS3TR-C seem very similar its probably still better to check against all datasheets ;)

Also temperature offset of +-15 I don't understand

I interpret it as "the temperature sensor will have pretty poor accuracy in terms of absolute temperature" i.e., a register value of 0 refers to something between 10 °C and 40 °C

warning: Member LSM6DSL_REG_X_OFS_USR (macro definition) of file lsm6dsxx_internal.h is not documented.
warning: Member LSM6DSL_REG_Y_OFS_USR (macro definition) of file lsm6dsxx_internal.h is not documented.
warning: Member LSM6DSL_REG_Z_OFS_USR (macro definition) of file lsm6dsxx_internal.h is not documented.
warning: Member LSM6DS33_REG_ORIENT_CFG_G (macro definition) of file lsm6dsxx_internal.h is not documented.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no way of telling doxygen that a define is only used internally? Something like @internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably but I don't know how... this is what we did for the others.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to just skip internal header files for the generated documentation? ala https://stackoverflow.com/questions/11025971/separate-internal-from-external-documentation-in-doxygen with @cond

or just skip those for the "missing documentation" check?

@MrKevinWeiss
Copy link
Contributor Author

May I squash (seeing as the first commit won't work for our use case)?

@riot-ci
Copy link

riot-ci commented Mar 26, 2024

Murdock results

✔️ PASSED

b98e5bd dist/tools/doccheck/exclude_simple: Add LSM6 excludes

Success Failures Total Runtime
10031 0 10031 09m:45s

Artifacts

@MrKevinWeiss
Copy link
Contributor Author

squashy squashy

@mguetschow
Copy link
Contributor

squashy squashy

Go for it!

@MrKevinWeiss MrKevinWeiss force-pushed the pr/feather-nrf52840-sense/generic branch from 8613bec to 00724b0 Compare March 26, 2024 14:25
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Two more minor things (you may squash directly).

drivers/lsm6dsxx/include/lsm6dsxx_internal.h Outdated Show resolved Hide resolved
drivers/lsm6dsxx/include/lsm6dsxx_internal.h Outdated Show resolved Hide resolved
@kfessel kfessel changed the title Allow generic LSM6DSXX driver /drivers/lsm6dsxx: Allow generic LSM6DSXX driver Mar 26, 2024
@kfessel kfessel changed the title /drivers/lsm6dsxx: Allow generic LSM6DSXX driver drivers/lsm6dsxx: Allow generic LSM6DSXX driver Mar 26, 2024
@kfessel kfessel changed the title drivers/lsm6dsxx: Allow generic LSM6DSXX driver driver/lsm6dsxx: Allow generic LSM6DSXX driver Mar 26, 2024
@MrKevinWeiss MrKevinWeiss force-pushed the pr/feather-nrf52840-sense/generic branch from 00724b0 to b98e5bd Compare March 26, 2024 17:13
@MrKevinWeiss
Copy link
Contributor Author

ping @mguetschow all good?

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Yes, LGTM, thanks! Also tested locally to (still) work on the rev. B board.

@mguetschow mguetschow added this pull request to the merge queue Mar 27, 2024
@MrKevinWeiss MrKevinWeiss removed this pull request from the merge queue due to a manual request Mar 27, 2024
@MrKevinWeiss MrKevinWeiss added this pull request to the merge queue Mar 27, 2024
@benpicco benpicco removed this pull request from the merge queue due to a manual request Mar 27, 2024
@maribu maribu added this pull request to the merge queue Mar 27, 2024
Merged via the queue into RIOT-OS:master with commit 42b68a5 Mar 27, 2024
29 checks passed
@MrKevinWeiss MrKevinWeiss deleted the pr/feather-nrf52840-sense/generic branch March 28, 2024 13:38
@MrKevinWeiss
Copy link
Contributor Author

Thanks! Next will be adding it to saul I guess :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: drivers Area: Device drivers 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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants