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

sensor:Update sensor type #13639

Merged
merged 4 commits into from
Sep 27, 2024
Merged

Conversation

Otpvondoiats
Copy link
Contributor

Summary

  1. Sort the existing sensor types and align them to aosp
  2. Add sensor info to get structure, align parameters with aosp
  3. Add some sensor types in aosp.

Impact

n/a

Testing

  • Environment:

OS and Version: Ubuntu 20.04.6 LTS x86_64
GCC Version: 13.1.0
SIM: ./tools/configure.sh sim:nsh

  • Order:

./nuttx

@github-actions github-actions bot added Area: Sensors Sensors issues Size: L The size of the change in this PR is large labels Sep 26, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 26, 2024

[Experimental Bot, please feedback here]

The PR summary is somewhat informative but could be more specific. Mentioning specific sensor types or groups and the reasoning behind aligning with AOSP would make it stronger.

However, the major issue is the lack of detail in the Impact and Testing sections.

Here's a breakdown of what's missing and how to improve:

Impact:

  • Be specific. "n/a" isn't acceptable. Address every bullet point. Even if the answer is NO, state it explicitly (e.g., "Impact on build: NO").
  • For YES answers, provide details.
    • New feature: Which sensor types are added? What functionality do they enable?
    • User impact: Will any existing APIs change? Will users need to modify their applications?
    • Hardware impact: Which architectures/boards are affected? Any new driver requirements?
    • Documentation: Is new documentation needed? Where is it located in the PR?
    • Compatibility: Any potential issues with older NuttX versions or other software that interacts with sensors?

Testing:

  • Insufficient information. The provided details are too generic.
  • Be explicit about the changes. What tests were run to verify the sensor sorting, info structure, and new sensor types?
  • Show before/after logs relevant to the changes. Include snippets that demonstrate the problem being solved or the new functionality in action.
  • Test on real hardware if possible. Simulation is a good start, but testing on a real board relevant to the added sensor types strengthens the PR.

In short, the PR currently doesn't meet the NuttX requirements due to the lack of detail in the Impact and Testing sections. Providing specific and comprehensive information in these areas is crucial for reviewers to understand and approve the changes.

…droid type.

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

Nuttx    <-------------------------------> Android
int32_t  <-- version                   --> int
float    <-- power                     --> float
float    <-- max_range                 --> float
float    <-- resolution                --> float
int32_t  <-- min_delay                 --> int32_t
int32_t  <-- max_delay                 --> int32/64_t
uint32_t <-- fifo_reserved_event_count --> uint32_t
uint32_t <-- fifo_max_event_count      --> uint32_t
char[]   <-- name                      --> char*
char[]   <-- vendor                    --> char*

Signed-off-by: likun17 <[email protected]>
@acassis
Copy link
Contributor

acassis commented Sep 26, 2024

@Otpvondoiats @xiaoxiang781216 why is this error:

Configuration/Tool: sim/sqlite
2024-09-26 12:04:48
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 5633k    0 5633k    0     0  6005k      0 --:--:-- --:--:-- --:--:-- 5999k
100 9795k    0 9795k    0     0  5010k      0 --:--:--  0:00:01 --:--:-- 5010k
100 13.4M    0 13.4M    0     0  5445k      0 --:--:--  0:00:02 --:--:-- 5446k
configure: WARNING: Can't find Tcl configuration definitions
configure: WARNING: *** Without Tcl the regression tests cannot be executed ***
configure: WARNING: *** Consider using --with-tcl=... to define location of Tcl ***
make[4]: warning: -j0 forced in submake: resetting jobserver mode.
  Normalize sim/sqlite

@xiaoxiang781216
Copy link
Contributor

@Otpvondoiats @xiaoxiang781216 why is this error:

Configuration/Tool: sim/sqlite
2024-09-26 12:04:48
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 5633k    0 5633k    0     0  6005k      0 --:--:-- --:--:-- --:--:-- 5999k
100 9795k    0 9795k    0     0  5010k      0 --:--:--  0:00:01 --:--:-- 5010k
100 13.4M    0 13.4M    0     0  5445k      0 --:--:--  0:00:02 --:--:-- 5446k
configure: WARNING: Can't find Tcl configuration definitions
configure: WARNING: *** Without Tcl the regression tests cannot be executed ***
configure: WARNING: *** Consider using --with-tcl=... to define location of Tcl ***
make[4]: warning: -j0 forced in submake: resetting jobserver mode.
  Normalize sim/sqlite

look like the ci temp failure, let's restart ci.

@cederom
Copy link
Contributor

cederom commented Sep 27, 2024

@xiaoxiang781216 xiaoxiang781216 merged commit 329d121 into apache:master Sep 27, 2024
9 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Sensors Sensors issues Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants