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

nrf91: Update GPS to GNSS #13548

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

JianyuWang0623
Copy link
Contributor

Summary

Update GPS to GNSS.

  • Config
    • SENSORS_GPS => SENSORS_GNSS
  • Header
    • nuttx/sensors/gps.h => nuttx/sensors/gnss.h
  • Struct
    • gps_lowerhalf_s => gnss_lowerhalf_s
    • gps_ops_s => gnss_ops_s
    • sensor_gps => sensor_gnss
    • sensor_gps_satellite => sensor_gnss_satellite
  • Macro
    • SENSOR_GPS_SAT_INFO_MAX => SENSOR_GNSS_SAT_INFO_MAX
    • SENSOR_TYPE_GPS_SATELLITE => SENSOR_TYPE_GNSS_SATELLITE
    • SENSOR_TYPE_GPS => SENSOR_TYPE_GNSS
  • Function
    • gps_register() => gnss_register()
  • Related: 03f4ec7 (A mistake that not using the lasted version while rename GPS to GNSS, missing 42211cc maybe)

Impact

nrf91: arch/boards

Testing

./tools/configure.sh -l nrf9160-dk:modem_ns

Related: 03f4ec7

Signed-off-by: wangjianyu3 <[email protected]>
@github-actions github-actions bot added the Size: S The size of the change in this PR is small label Sep 20, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 20, 2024

[Experimental Bot, please feedback here]

The PR appears to meet the basic NuttX requirements, but could benefit from more details in a few areas:

Strengths:

  • Summary: Clearly outlines the "Why," "What," and "How" of the change. Good use of bullet points for readability.
  • Impact: Identifies the specific area of impact (nrf91: arch/boards).
  • Testing: Provides a specific configuration command used for testing.

Areas for Improvement:

  • Impact:
    • Feature Changes: Be more explicit. Is GNSS a replacement for GPS, or an addition? Will existing GPS functionality break?
    • User Impact: Does this change how users interact with the GPS/GNSS API? Any new configuration options?
    • Build Impact: Does the configure.sh command need any modifications due to this change?
    • Hardware Impact: Which specific boards are affected within arch/boards/nrf91?
    • Documentation Impact: Will documentation need updates to reflect the GNSS changes? If so, is this update included in the PR?
    • Other Impacts: Briefly address the remaining impact categories. Even a simple "NO" is better than leaving them blank.
  • Testing:
    • Build/Target Details: Provide more information about your testing environment:
      • Build Host OS, CPU Architecture, Compiler
      • Target Architecture (simulator or real hardware?), Specific board and configuration used
    • Testing Logs: The current logs are placeholders. Include actual log snippets from before and after the change. Focus on demonstrating that the GNSS functionality works as intended.

Recommendation:

Expand on the "Impact" and "Testing" sections with the details mentioned above. This will make your PR stronger and easier for reviewers to assess.

Log:
  Untracked files:
    (use "git add <file>..." to include in what will be committed)
          libs/libc/gnssutils/db46128e73cee26d6a6eb0482dcba544ee1ea9f5.zip

  nothing added to commit but untracked files present (use "git add" to track)

Signed-off-by: wangjianyu3 <[email protected]>
@JianyuWang0623
Copy link
Contributor Author

JianyuWang0623 commented Sep 20, 2024

Details for 622ddb5(libs/gnssutils: Ignore source zip):
libs/libc/gnssutils/Make.defs will delete *.zip:

	$(call DELFILE, $(MINMEA_VERSION).zip)

but libs/libc/gnssutils/CMakeLists.txt may not

@xiaoxiang781216 xiaoxiang781216 merged commit 44cce29 into apache:master Sep 20, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants