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.

Sign up for GitHub

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

Jump to bottom

drivers/sensor: add new sensot type to align android sensor type #15610

Merged
merged 1 commit into from
Jan 20, 2025
Merged

drivers/sensor: add new sensot type to align android sensor type #15610

merged 1 commit into from
Jan 20, 2025

Conversation

Donny9
Copy link
Contributor

@Donny9 Donny9 commented Jan 20, 2025

Summary

new sensor type:
SENSOR_TEMPERATURE
SENSOR_TYPE_ACCELEROMETER_UNCALIBRATED
SENSOR_TYPE_GYROSCOPE_UNCALIBRATED
SENSOR_TYPE_MAGNETIC_FILED_UNCALIBRATED

refs:
https://cs.android.com/android/_/android/platform/hardware/libhardware/+/0e67aa0caee9500b61b9c1c8b6e5cab18301364c:include_all/hardware/sensors-base.h

Impact

add new sensor type

Testing

local test

@github-actions github-actions bot added Area: OS Components OS Components issues Area: Sensors Sensors issues Size: M The size of the change in this PR is medium labels Jan 20, 2025
@Donny9 Donny9 mentioned this pull request Jan 20, 2025
Merged
@Donny9 Donny9 force-pushed the type branch 2 times, most recently from 007ad49 to 51aba1b Compare January 20, 2025 07:31
@nuttxpr
Copy link

nuttxpr commented Jan 20, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the changes, it lacks crucial details.

Missing Information:

  • Summary: While the new sensor types are listed, the reason for adding them is missing. Why are these new types necessary? What problem do they solve or what functionality do they enable? How exactly do these new types work within the existing sensor framework? There's no mention of related NuttX issues.
  • Impact: Simply stating "add new sensor type" is insufficient. The PR needs to address all the impact points explicitly. Will users need to adapt their code? Will the build process be affected? Which architectures/boards/drivers are impacted? Does this require documentation updates (and have they been provided)? Are there security implications? Does this affect compatibility?
  • Testing: "local test" is far too vague. The requirements ask for specific details about the build host and target, including OS, CPU, compiler, architecture, board, and configuration. Crucially, the actual testing logs before and after the change are missing. Without these logs, there's no way to verify the changes work as intended.

The PR author needs to provide substantially more detail to meet the requirements. They need to clearly explain the motivation, thoroughly analyze the impact, and provide concrete evidence of testing.

@raiden00pl
Copy link
Member

@Donny9 Is there any advantage in compatibility with Android here? If so, maybe it's worth adding a note in uorb.h and Documentation that compatibility with Android is intentional and should be preserved (probably with a link to Android sensors-base.h). So that in the future will be no doubts about this.

@Donny9
Copy link
Contributor Author

Donny9 commented Jan 20, 2025

@Donny9 Is there any advantage in compatibility with Android here? If so, maybe it's worth adding a note in uorb.h and Documentation that compatibility with Android is intentional and should be preserved (probably with a link to Android sensors-base.h). So that in the future will be no doubts about this.

@raiden00pl Good idea, Android has accumulated valuable experience and set fine examples, and we can draw closer to it in the direction of smart devices.

@Donny9
drivers/sensor: add new sensot type to align android sensor type
ccb1c03 
new sensor type:
SENSOR_TEMPERATURE
SENSOR_TYPE_ACCELEROMETER_UNCALIBRATED
SENSOR_TYPE_GYROSCOPE_UNCALIBRATED
SENSOR_TYPE_MAGNETIC_FILED_UNCALIBRATED

refs:
https://cs.android.com/android/_/android/platform/hardware/libhardware/+/
0e67aa0caee9500b61b9c1c8b6e5cab18301364c:include_all/hardware/sensors-base.h

Signed-off-by: dongjiuzhu1 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit e7f38fe into apache:master Jan 20, 2025
39 checks passed
linguini1
linguini1 reviewed Jan 20, 2025
View reviewed changes
drivers/sensors/sht4x_uorb.c
@@ -875,7 +875,6 @@ int sht4x_register(FAR struct i2c_master_s *i2c, int devno, uint8_t addr)

priv->hum.sensor_lower.ops = &g_sensor_ops;
priv->hum.sensor_lower.type = SENSOR_TYPE_RELATIVE_HUMIDITY;
priv->hum.sensor_lower.uncalibrated = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to update the recent documentation on UORB to mention the removal of the uncalibrated member.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#15619 @linguini1 Done

@Donny9 Donny9 mentioned this pull request Jan 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@linguini1 linguini1 linguini1 left review comments

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 approved these changes

@raiden00pl raiden00pl raiden00pl approved these changes

Assignees
No one assigned
Labels
Area: OS Components OS Components issues Area: Sensors Sensors issues Size: M The size of the change in this PR is medium
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Donny9 @nuttxpr @raiden00pl @xiaoxiang781216 @linguini1