-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
drivers/sensors: Added uORB driver for LSM6DSO32 IMU. #15789
Conversation
[Experimental Bot, please feedback here] Yes, this PR substantially meets the NuttX requirements. The summary clearly explains the change, its scope (limited support), and mentions accompanying documentation. The impact section identifies affected parts (build system, kernel) and correctly notes the addition of a new driver as a feature. The testing section is comprehensive, detailing the host and target environments, providing the test script, expected behavior, observed output in different modes (interrupt/polling), and even includes relevant system logs. This thorough testing significantly strengthens the PR. Minor suggestions for improvement:
By addressing these minor points, the PR will even more closely adhere to the NuttX requirements and be easier for reviewers to assess. |
a9ad465
to
09fc260
Compare
|
||
/* If we got here, there was no error, we can record the activation state */ | ||
|
||
sens->enabled = enable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use interrupt ways at all times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean, as opposed to polling from a thread? This allows flexibility for users to choose to poll.
09fc260
to
a3ef983
Compare
Adds support for the LSM6DSO32 IMU by STM using the uORB framework. This only contains limited support for the I2C interface, interrupt and polling driven measurement and standard modes of operation (high performance ODRs). Features like interrupts besides DRDY, filtering, gesture recognition and acting as a bus master are not implemented.
a3ef983
to
58ff6ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move the documentation to a new PR for PR
May I ask why? Wouldn't it be best to merge the documentation about this new feature at the same time that it's merged? |
For a LTS release we will rebase commits for several years and there is a high chance that documentation or other areas will need changes. I think that having separate PR with same name but numbers at the end {1/2}, {2/2} will make the rebase and conflict resolution easy. Best regards |
From my perspective this will make it more difficult to ensure that contributors are putting forward documentation with their PRs, and it adds more work for contributors and reviewers to look between two PRs and have to update two PRs when changes are made. For this PR I've already had to update the docs to match changes I made based on the reviews. If we introduce dual PRs I'll have to swap between two branches to make changes and push to two branches. It makes more sense to me to have the docs be combined with the changes being made. If this is going to be a hard requirement from NuttX I will split the PR, but this should be mentioned in the contribution guidelines as well. |
@jerpelea I think this "enforcement" doesn't exist in others more "stable" projects with LTS, see this example: Board support and documentation in the same PR (and he is not a random contributor) |
@acassis during the 12.8.0 stabilization period there were some conflicts that made me ask for this change. If we plan to maintain the LTS release we should make it manageable. Unless we have PR and commits affecting only 1 area they will create "hidden" requirements and add "hidden" changes which will impact future PRs affecting the same area. Please remember that we are talking about 2400+ commits / year that must be checked and ported for LTS |
LTS should avoid backport the change except the bug fix report by end user as much as possible, I think. |
@jerpelea @xiaoxiang781216 maybe it is worth to investigate how other projects that have LTS are solving it. Forcing our (few) contributors to create a single PR for each modified are seems no feasible. |
@jerpelea If we don't have the resources to deal with LTS, why do it at all? making life difficult for (few) contributors is a tragic solution. Another issue is that some changes must affect more than one area (eg. arch + boards), otherwise CI will be useless. |
Summary
Adds support for the LSM6DSO32 IMU by STM using the uORB framework. This only contains limited support for the I2C interface, interrupt and polling driven measurement and standard modes of operation (high performance ODRs). Features like interrupts besides DRDY, filtering, gesture recognition and acting as a bus master are not implemented.
Accompanying documentation included.
Impact
This PR adds another available uORB driver to the NuttX sensor drivers.
It impacts the build system by adding more Kconfig options and impacts the kernel with more driver source files.
Testing
All builds were performed on a Linux host. All testing was performed on an RP2040 MCU on a custom flight computer board.
Below is the script that was used for testing.
This script tests the following:
WHOAMI
register is read and the value is correctSETFULLSCALE
changes the range of the sensor correctly and measurements are correct following this changeSET_CALIBVALUE
get_info
get_info
returns the correct information depending on accel/gyroHere is the output of the script in interrupt-driven mode:
Here is the output of the script in polling mode:
Here is the output of
uorb_listener
in polling mode (there is also a magnetometer on this board, only the accel and gyro readings are from this driver).Here is the output of
uorb_listener
in interrupt-driven mode (there is also a magnetometer and barometer on this board, only the accel and gyro readings are from this driver).Here are the system logs in debug mode when the driver is registered in polling mode:
Here are the system logs in debug mode when the driver is registered in interrupt-driven mode:
Documentation was built and rendered locally and verified for correct formatting.