Skip to content

nrf91: Update GPS to GNSS#13548

Merged
xiaoxiang781216 merged 2 commits intoapache:masterfrom
JianyuWang0623:br_wjy_nrf91_gps_gnss
Sep 20, 2024
Merged

nrf91: Update GPS to GNSS#13548
xiaoxiang781216 merged 2 commits intoapache:masterfrom
JianyuWang0623:br_wjy_nrf91_gps_gnss

Conversation

@JianyuWang0623
Copy link
Copy Markdown
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 <wangjianyu3@xiaomi.com>
@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
Copy Markdown

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.

Comment thread libs/libc/gnssutils/.gitignore Outdated
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 <wangjianyu3@xiaomi.com>
@JianyuWang0623
Copy link
Copy Markdown
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

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