You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "JianyuWang0623 (via GitHub)" <gi...@apache.org> on 2024/09/19 11:12:17 UTC
[PR] gnssutils/minmea: Add config for extended sentnce LOR LSQ [nuttx]
JianyuWang0623 opened a new pull request, #13544:
URL: https://github.com/apache/nuttx/pull/13544
## Summary
Add config for extended sentnce LOR LSQ.
Enable vendor extended sentnce "LOR LSQ" for the MINMEA NMEA library.
## Impact
gnssutils
## Testing
```
./tools/configure.sh -l lilygo_tbeam_lora_gps:gps
```
compilation ok, failed in link stage, need check CI result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] gnssutils/minmea: Add config for extended sentence LOR LSQ [nuttx]
Posted by "JianyuWang0623 (via GitHub)" <gi...@apache.org>.
JianyuWang0623 commented on code in PR #13544:
URL: https://github.com/apache/nuttx/pull/13544#discussion_r1766692980
##########
libs/libc/gnssutils/Kconfig:
##########
@@ -12,3 +12,9 @@ config GNSSUTILS_MINMEA_LIB
NOTE: This library depends on having some version of math.h
at include/nuttx. There are some different ways to accomplish
this. See the discussion in the top-level nuttx/README.txt file.
+
+config GNSSUTILS_MINMEA_LIB_EXTEND_LOR_LSQ
+ bool "Enable extended sentnce LOR LSQ"
+ default n
+ ---help---
+ Enable vendor extended sentnce "LOR LSQ" for the MINMEA NMEA library.
Review Comment:
Thank you.
As related PR(apache/nuttx-apps#2584) updated(vendor extended sentence should not added to general examples), this PR will be closed, thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] gnssutils/minmea: Add config for extended sentnce LOR LSQ [nuttx]
Posted by "JianyuWang0623 (via GitHub)" <gi...@apache.org>.
JianyuWang0623 closed pull request #13544: gnssutils/minmea: Add config for extended sentnce LOR LSQ
URL: https://github.com/apache/nuttx/pull/13544
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] gnssutils/minmea: Add config for extended sentnce LOR LSQ [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #13544:
URL: https://github.com/apache/nuttx/pull/13544#issuecomment-2360711736
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13494&type=pullrequests)
**No, this PR does not fully meet the NuttX requirements.**
**Here's why and how to improve it:**
* **Insufficient Summary:**
* **Why?** The summary lacks a clear explanation of *why* adding this configuration for the extended sentence is necessary. What problem does it solve? What benefit does it provide?
* **What functional part?** Be more specific than "gnssutils." Which file(s) are modified?
* **Missing Impact Details:**
* **New feature?** Is this a new feature, or is it enabling an existing feature that was previously unavailable?
* **User impact?** Will users need to change any configurations or will this "just work"?
* **Build/Hardware/Documentation/Security/Compatibility:** You've marked "gnssutils" under impact, but you need to address each specific impact category with "YES/NO" and provide details if applicable.
* **Incomplete Testing:**
* **Build Host Details:** Provide specifics about your build host environment (OS, CPU architecture, compiler version).
* **Target Details:** Specify the target architecture (e.g., "sim" for simulator, "ARMv7-M") and the board configuration you used for testing.
* **Insufficient Logs:** The provided testing logs are too vague. Include relevant snippets of logs *before* and *after* your changes, clearly demonstrating the issue and the fix. For example:
* **Before:** Show that the extended sentence was NOT being parsed.
* **After:** Show that the extended sentence IS now being parsed correctly.
**Example of Improved Summary:**
```
## Summary
This PR enables support for the vendor-specific "LOR LSQ" extended sentence in the MINMEA NMEA library. This sentence provides [explain the data provided by the sentence and why it's useful].
The change adds a new configuration option to enable parsing of this sentence and updates the MINMEA library to handle the new sentence type.
Related NuttX Issue: [If applicable, link to an issue discussing the need for this feature]
```
**Remember:** A good PR description saves reviewers time and helps ensure your changes are accepted quickly!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] gnssutils/minmea: Add config for extended sentnce LOR LSQ [nuttx]
Posted by "lupyuen (via GitHub)" <gi...@apache.org>.
lupyuen commented on code in PR #13544:
URL: https://github.com/apache/nuttx/pull/13544#discussion_r1766661554
##########
libs/libc/gnssutils/Kconfig:
##########
@@ -12,3 +12,9 @@ config GNSSUTILS_MINMEA_LIB
NOTE: This library depends on having some version of math.h
at include/nuttx. There are some different ways to accomplish
this. See the discussion in the top-level nuttx/README.txt file.
+
+config GNSSUTILS_MINMEA_LIB_EXTEND_LOR_LSQ
+ bool "Enable extended sentnce LOR LSQ"
+ default n
+ ---help---
+ Enable vendor extended sentnce "LOR LSQ" for the MINMEA NMEA library.
Review Comment:
```suggestion
Enable vendor extended sentence "LOR LSQ" for the MINMEA NMEA library.
```
Same here. Thanks!
##########
libs/libc/gnssutils/Kconfig:
##########
@@ -12,3 +12,9 @@ config GNSSUTILS_MINMEA_LIB
NOTE: This library depends on having some version of math.h
at include/nuttx. There are some different ways to accomplish
this. See the discussion in the top-level nuttx/README.txt file.
+
+config GNSSUTILS_MINMEA_LIB_EXTEND_LOR_LSQ
+ bool "Enable extended sentnce LOR LSQ"
Review Comment:
```suggestion
bool "Enable extended sentence LOR LSQ"
```
I think there's a typo?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org