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

drivers/sensors: Add Velocity sensor type to UORB #15293

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

acassis
Copy link
Contributor

@acassis acassis commented Dec 19, 2024

Summary

This patch adds support for velocity measurement sensors. I plan to use it as a generic speed type to be used for Renesas FS3000 (Air Velocity Flow).

Related PR: apache/nuttx-apps#2908

Impact

Now uORB will support Velocity sensors

Testing

Tested with FS3000 air velocity flow sensor

@github-actions github-actions bot added Area: OS Components OS Components issues Area: Sensors Sensors issues Size: S The size of the change in this PR is small labels Dec 19, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 19, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. While it provides a basic summary and mentions testing, it lacks crucial details required for proper review.

Here's a breakdown of the missing information:

  • Summary:

    • Lacks detail on how the speed measurement is implemented. What driver is modified/added? What specific changes were made to uORB? Just stating "adds support" is insufficient.
    • No mention of related NuttX or NuttX Apps issues/PRs.
  • Impact: This section is severely lacking. All the "NO/YES" questions need to be explicitly answered and explained if "YES." Simply stating "Now uORB will support Speed sensors" doesn't address the potential impact on users, build process, hardware compatibility, documentation needs, security implications, or backward compatibility.

  • Testing: While it mentions testing, the crucial "Testing logs before change" and "Testing logs after change" sections are empty. These logs are essential for demonstrating the functionality and verifying that the changes have the intended effect. It also lacks information on the specific build host and target used for testing (OS, CPU, compiler, architecture, board, configuration).

Example of improved descriptions for some sections:

Summary:

This patch adds support for speed measurement sensors by introducing a new sensor_speed uORB topic and a driver framework for these sensors. Specifically, this implementation adds the Renesas FS3000 airflow speed sensor driver as an example. The sensor_speed topic publishes speed data in meters per second. This change is being made to support a wider range of sensors within NuttX. No related NuttX or NuttX Apps issues/PRs exist at this time.

Impact:

  • Is new feature added? YES. A new uORB topic (sensor_speed) and a driver framework for speed sensors are added.
  • Impact on user (will user need to adapt to change)? YES. Users wishing to utilize speed sensors will need to configure their build to include the new driver(s) and can access data via the sensor_speed uORB topic.
  • Impact on build (will build process change)? YES. The build system needs to be updated to include the new driver(s) and the sensor_speed uORB topic definition. Kconfig options may need adjustments.
  • Impact on hardware (will arch(s) / board(s) / driver(s) change)? YES. This change introduces support for new hardware, specifically speed sensors like the Renesas FS3000. Board configurations will need to be updated to enable this driver.
  • Impact on documentation (is update required / provided)? YES. Documentation will be updated to describe the new driver framework, the sensor_speed uORB topic, and how to configure and use speed sensors. (Documentation updates provided in this PR).
  • Impact on security (any sort of implications)? NO.
  • Impact on compatibility (backward/forward/interoperability)? NO.
  • Anything else to consider? The driver framework is designed to be extensible for other types of speed sensors.

Testing:

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Linux (Ubuntu 22.04), x86_64 (Intel Core i7), GCC 11.2
  • Target(s): sim:stm32f4discovery

Testing logs before change:

$ uorb top sensor_speed # Topic does not exist

Testing logs after change:

$ uorb top sensor_speed
topic: sensor_speed, instance 0, listener count 0
...  (Actual sensor data output here) ...

By providing more specific information as demonstrated above, the PR will be much clearer and easier for reviewers to evaluate, ultimately leading to a quicker and smoother integration process.

@raiden00pl
Copy link
Member

raiden00pl commented Dec 19, 2024

shouldn't this be a velocity sensor ? "Velocity" is a vector quantity, so it can be signed, "speed" can't.

EDIT: FS3000 is Air Velocity Sensor Module. Velocity and speed are not the same thing (but often confused and used interchangeably)

@cederom
Copy link
Contributor

cederom commented Dec 19, 2024

Good point @raiden00pl :-) The same way we use accelerometers to measure acceleration :-)

This patch adds support for velocity measurement sensors.
I plan to use it as a generic velocity type to be used for
Renesas FS3000 (Air Velocity Flow).

Signed-off-by: Alan C. Assis <[email protected]>
@acassis
Copy link
Contributor Author

acassis commented Dec 19, 2024

shouldn't this be a velocity sensor ? "Velocity" is a vector quantity, so it can be signed, "speed" can't.

EDIT: FS3000 is Air Velocity Sensor Module. Velocity and speed are not the same thing (but often confused and used interchangeably)

Good point, I'll update it

@acassis acassis changed the title drivers/sensors: Add SPEED sensor type to UORB drivers/sensors: Add Velocity sensor type to UORB Dec 19, 2024
Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you @acassis :-)

After this PR is merged, next is apache/nuttx-apps#2908.

@cederom cederom merged commit 0e162ee into apache:master Dec 19, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Area: Sensors Sensors issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants