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

testing/sensortest: Add support to Velocity Sensors #2908

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

acassis
Copy link
Contributor

@acassis acassis commented Dec 19, 2024

Summary

Add support to Speed Sensors on sensortest

Depends on: apache/nuttx#15293

Impact

User will be able to use speed sensors

Testing

FS3000

@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 some information, it lacks crucial details in several sections.

Here's a breakdown of what's missing and how it can be improved:

Summary:

  • Missing: While it mentions adding support for speed sensors, it doesn't specify why this change is necessary. Is it a new feature request? Does it fix a bug? It also lacks details about how the change works. What specific code was modified? What algorithms or drivers were implemented or changed? Finally, linking a related NuttX issue would be beneficial if one exists.

Example of a better Summary:

This PR adds support for speed sensors to the sensortest utility, addressing a feature request from [link to issue if applicable]. Currently, sensortest does not handle speed sensor data. This implementation adds a new command-line option to sensortest to read and display data from speed sensors using the existing sensor framework. The read_sensor function is modified to recognize and handle speed sensor data.

Impact:

  • Insufficient Detail: While mentioning user impact, it lacks specifics. How exactly will the user interact with this new feature? Are there new command-line options? The other impact categories are completely omitted. Even if there's no impact, stating "NO" explicitly for each category is necessary.

Example of a better Impact:

  • Is new feature added? YES - Adds support for speed sensors to sensortest.
  • Impact on user? YES - Users can now access speed sensor data using the -s option in sensortest. See the updated sensortest help for usage details.
  • Impact on build? NO
  • Impact on hardware? NO (Assumes existing speed sensor drivers are present) If new drivers are added, specify here.
  • Impact on documentation? YES - The sensortest documentation will be updated to include the new -s option and usage examples. This documentation update is included in this PR.
  • Impact on security? NO
  • Impact on compatibility? NO
  • Anything else to consider? NO

Testing:

  • Severely lacking: Simply stating "FS3000" is not enough. What platform is this? What build host was used? Most importantly, where are the logs demonstrating the change? "Before" and "after" logs are essential to demonstrate that the change works as expected.

Example of a better Testing:

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

  • Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): sim:nsh config

Testing logs before change:

$ sensortest -l
... (list of sensors, no speed sensor present)

Testing logs after change:

$ sensortest -l
... (list of sensors, including the new speed sensor)
$ sensortest -s
Speed Sensor: <value> <units>

In short, the provided PR description is a starting point but needs significant expansion to meet the NuttX requirements. Provide specific details, logs, and consider all the impact categories.

Copy link

@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.

Thank you @acassis :-) Please take a look at CI build complains :-P

@acassis
Copy link
Contributor Author

acassis commented Dec 19, 2024

Thank you @acassis :-) Please take a look at CI build complains :-P

It is expected because this PR depends on apache/nuttx#15293 be merged first

@acassis acassis changed the title testing/sensortest: Add support to Speed Sensors testing/sensortest: Add support to Velocity Sensors Dec 19, 2024
@cederom
Copy link

cederom commented Dec 19, 2024

Okay apache/nuttx#15293 is merged, I have restarted CI, should work this time :-) Thanks @acassis :-)

Copy link

@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.

All clear, thank you @acassis ! :-)

@lupyuen lupyuen merged commit 9500938 into apache:master Dec 20, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants