-
Notifications
You must be signed in to change notification settings - Fork 28
Changes to the GPS logic for newer GPS. #374
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
Conversation
for now as hidden http adress /cold
…o it is always visible. Keep Coldstarting on boot for neo8m
…tter display layout
… and others (always)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this and thanks for the lot of experiments with the GPS modules.
Long term we should also get rid of the ifdefs form UBX_M10 vs UBX_M6, but one thing after the other ;) Might be we should also not use the ALP data if a M8 or newer is detected. These modules can (if I remember correctly) collect the data over the time from the satellites and we could then persist it (AssistNow Autonomous).
src/gps.cpp
Outdated
@@ -31,14 +31,63 @@ const String Gps::INF_SEVERITY_STRING[] = { | |||
String("TST"), String("DBG") | |||
}; | |||
|
|||
bool Gps::is_neo6() const { | |||
if (String(hwString).substring(0,4) == String("00040007").substring(0,4)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have a module that returns '00040005'. Might be we should use only the 4 leading digits or use assume it is neo6 if none of the other patterns matches. Did you find references for the meaning of this number?
-> Which is what you do but (I just realize ;) ) this is rather confusing. If we do not know better, what about just comparing hwString[3] == '4'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to only look at the first 4 bytes and be more obvious about it - logic says that neo16m will have "0010" there, so just another 8 years or so.
src/gps.cpp
Outdated
@@ -249,6 +298,14 @@ void Gps::configureGpsModule() { | |||
} else { | |||
addStatisticsMessage("No ack for setting timepulse in either new or old format"); | |||
} | |||
const uint8_t UBX_CFG_RATE[] = {0xE8, 0x03, 0x01, 0x00, 0x00, 0x00}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong - below 'UBX_CFG_RATE' is not sent but TP5. A comment would help to understand what this should do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder where that came from - so I removed it.
I thought about storing the hardware-variant in the config. Then we can start fastly with an assumption. When we detect something else we can correct it an reboot. (This is very unlikely and acceptable for the first boot or changed hardware) |
On the other hand if the communication with the GPS module is up this info is just one message away. Would it be notable at all? |
@amandel I dropped some notes in the review where aid and alp are disabled unless neo6m is detected. I agree that we should aim to check for the GPS version at runtime. From my perspective I've addressed with the comments. Gemini is doing weird things. Earlier it was finished, showed red and left miles along, unrendered markdown comment, now it seems to have decided to re-run on its own. I'm unsure if its bad practice if I resolve therefore I left them unresolved. |
I've triggered gemini :) - I see it just as bonus no blocker if it is red. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR introduces a GPS cold start feature and adds GPS module details to the about page. However, it has several potential issues:
- CI: Removing
set -eux
from the CI reduces error visibility. Provide justification. Removing the AI code review workflow removes a quality check. Is there a replacement?
- GPS Cold Start: Lacks error handling and user feedback.
gpsColdIndex
's purpose is unclear and needs documentation. The handleColdStart
function declaration exists, but the implementation is missing.
Gps.cpp
: Redundant is_neoX()
functions. SD card dependency for initialization should be explained or removed. Inconsistent waitForData
durations. Unclear and incomplete showWaitStatus
logic. Potential satellitesString
array out-of-bounds access. Unnecessary cold start for non-NEO-6 modules. Unclear AID_INI
check. Potential redundant handle()
calls. Magic numbers for reset types.
Gps.cpp
(Display Logic): Magic numbers for grid indices. Unclear hw()
function. Potential hwString
buffer overflow. Missing is_neo6()
definition. Inefficient string concatenation. Inconsistent logging levels. Inefficient aStatus
string conversion. Duplicate logging code. Unclear mAidIniSent
and is_neo6()
logic. Missing error handling.
Gps.cpp
(Data Handling): Changing vp
size to 17 bytes assumes only M8 modules are used. This should be configurable or dynamically determined.
alpdata.cpp
: Deleting AID_INI_DATA_FILE_NAME
after reading might be problematic and needs justification.
Suggested Code Changes:
web_server.cpp
: Add error handling and user feedback to handleColdStartGPS()
. Clarify gpsColdIndex
's purpose. Implement the handleColdStart
function.
Gps.cpp
: Replace is_neoX()
functions with direct string comparisons. Explain or remove the SD card dependency in begin()
. Use consistent waitForData
durations. Clarify and complete the showWaitStatus
logic. Fix potential satellitesString
out-of-bounds access. Guard cold start logic with is_neo6()
. Clarify the AID_INI
check. Review and remove redundant handle()
calls. Use named constants for reset types. Use named constants for grid indices. Clarify the hw()
function and ensure hwString
handling is safe. Improve string concatenation efficiency. Use consistent logging levels. Improve aStatus
string conversion efficiency. Refactor duplicate logging code. Clarify mAidIniSent
logic. Add error handling. Make vp
size configurable or dynamically determined. Initialize member variables in the constructor.
alpdata.cpp
: Make deletion of AID_INI_DATA_FILE_NAME
optional or rename the file to indicate its temporary nature.
General Recommendations:
- Add unit tests for new functionality and error handling.
- Document all new functions and parameters, especially
coldStartGps()
.
- Justify the CI changes.
Addressing these issues will improve the code's robustness, readability, maintainability, and security.
## Review Summary
set -eux
from the CI reduces error visibility. Provide justification. Removing the AI code review workflow removes a quality check. Is there a replacement?gpsColdIndex
's purpose is unclear and needs documentation. The handleColdStart
function declaration exists, but the implementation is missing.Gps.cpp
: Redundant is_neoX()
functions. SD card dependency for initialization should be explained or removed. Inconsistent waitForData
durations. Unclear and incomplete showWaitStatus
logic. Potential satellitesString
array out-of-bounds access. Unnecessary cold start for non-NEO-6 modules. Unclear AID_INI
check. Potential redundant handle()
calls. Magic numbers for reset types.Gps.cpp
(Display Logic): Magic numbers for grid indices. Unclear hw()
function. Potential hwString
buffer overflow. Missing is_neo6()
definition. Inefficient string concatenation. Inconsistent logging levels. Inefficient aStatus
string conversion. Duplicate logging code. Unclear mAidIniSent
and is_neo6()
logic. Missing error handling.Gps.cpp
(Data Handling): Changing vp
size to 17 bytes assumes only M8 modules are used. This should be configurable or dynamically determined.alpdata.cpp
: Deleting AID_INI_DATA_FILE_NAME
after reading might be problematic and needs justification.web_server.cpp
: Add error handling and user feedback to handleColdStartGPS()
. Clarify gpsColdIndex
's purpose. Implement the handleColdStart
function.Gps.cpp
: Replace is_neoX()
functions with direct string comparisons. Explain or remove the SD card dependency in begin()
. Use consistent waitForData
durations. Clarify and complete the showWaitStatus
logic. Fix potential satellitesString
out-of-bounds access. Guard cold start logic with is_neo6()
. Clarify the AID_INI
check. Review and remove redundant handle()
calls. Use named constants for reset types. Use named constants for grid indices. Clarify the hw()
function and ensure hwString
handling is safe. Improve string concatenation efficiency. Use consistent logging levels. Improve aStatus
string conversion efficiency. Refactor duplicate logging code. Clarify mAidIniSent
logic. Add error handling. Make vp
size configurable or dynamically determined. Initialize member variables in the constructor.alpdata.cpp
: Make deletion of AID_INI_DATA_FILE_NAME
optional or rename the file to indicate its temporary nature.coldStartGps()
.This PR removes the AI code review workflow, removes a set -eux
from the CI, and removes/adds some seemingly minor lines in the C++ code. While the changes themselves are small, the removal of the AI code review workflow and the set -eux
raise concerns and warrant further explanation. The changes in the C++ code appear to be stylistic and functional improvements, but need further context to assess fully. A new GPS Cold Start endpoint has been added; its purpose, usage, and potential side effects should be documented.
File Comments
.github/workflows/ai-code-review.yml
:
- Major Issue: Deleting the AI code review workflow removes an automated quality check. While AI code review isn't perfect, it can catch potential issues. Justification for removal is required. Is there a replacement process? If so, this should be part of the PR and clearly explained. If not, reconsider removing this workflow. Are there performance or cost issues driving this removal?
.github/workflows/ci.yml
:
- Minor Issue: Removing
set -eux
within the SonarQube dependency installation step disables helpful shell debugging features (exit on error, print executed commands, expand variables). While this might be intentional, it reduces the CI's ability to pinpoint errors during the installation process. Justification for removal is needed. Is there a specific reason why these options are being removed? It's usually beneficial to keep them unless there's a compelling reason not to.
src/OpenBikeSensorFirmware.cpp
:
- Minor Issue: Removing the blank line after
setupBluetooth(cfg, trackUniqueIdentifier);
is purely stylistic. While consistent coding style is good, this change is so minor it doesn't warrant much discussion. Just be sure the overall project style guide is being followed.
src/configServer.cpp
:
- Minor Issue: Adding
static const char* const gpsColdIndex = "<h3>GPS cold start</h3>";
suggests a new feature related to GPS cold starting. This requires documentation. What does a "GPS cold start" entail in this context? How is this feature accessed/used? Are there any potential side effects? This should also be reflected in testing and potentially example usage. - Major Issue: The function
handleColdStart
is declared, but its definition is missing. This will lead to a linker error. The implementation needs to be added in this PR. - Minor Issue: The removal of blank lines within
configServer.cpp
is a style preference. Ensure consistency across the codebase.
General Recommendation:
- Testing: Any new features or changes to existing functionality should be accompanied by appropriate tests. If tests already exist, ensure they still pass after these changes. If not, add new tests to cover the modified code.
- Documentation: Update any relevant documentation (e.g., README, user manual, code comments) to reflect the changes introduced by this PR, particularly concerning the GPS cold start functionality.
This review focuses on potential issues and areas for improvement. Providing more context regarding the purpose and intended effects of these changes will help in conducting a more thorough and helpful review.
Review Summary:
This PR introduces a new feature to trigger a GPS cold start and adds information about the GPS module to the about page. While the functionality seems reasonable, there are some potential issues related to error handling, code clarity, and potential misuse of the SD card.
File: src/web_server.cpp
- Issue: The new
/cold
endpoint triggersgps.coldStartGps()
but doesn't handle potential errors or provide feedback to the user. What happens if the cold start fails? - Suggestion: Add error handling to
handleColdStartGPS()
. Ifgps.coldStartGps()
fails, return an appropriate error code and message to the user. Also consider providing some visual feedback on the page to indicate that the cold start has been initiated. - Example:
static void handleColdStartGPS(HTTPRequest *, HTTPResponse * res) {
String html = createPage(gpsColdIndex);
html = replaceDefault(html, "Navigation");
sendHtml(res, html);
if (gps.coldStartGps()) {
res->sendString("Cold start initiated."); // Provide user feedback
} else {
res->setStatusCode(500); // Internal Server Error
res->sendString("Error initiating cold start.");
}
res->finalize();
}
- Issue: Unclear purpose and usage of
gpsColdIndex
. Is it a dedicated HTML page? What content does it display? - Suggestion: Clarify the purpose of
gpsColdIndex
in a comment. Consider renaming it to something more descriptive.
File: src/gps.cpp
- Issue: The
is_neoX()
functions are redundant and potentially inefficient. ThehwString
appears to already contain the hardware version information. Directly comparing substrings ofhwString
is simpler. - Suggestion: Remove the
is_neoX()
functions and use direct comparisons in thehw()
function. - Example:
String Gps::hw() const {
if (String(hwString).substring(0,4) == "0004") {
return "Neo6";
} else if (String(hwString).substring(0,4) == "0008") {
return "Neo8";
} else if (String(hwString).substring(0,4) == "000A") {
return "NeoA";
} else {
return "Neo" + String(hwString).substring(3,4); // Is this correct logic for other versions?
}
}
- Issue: In
begin()
, the conditionalif((!is_neo6()) || (!SD.exists(AID_INI_DATA_FILE_NAME)))
checks for the existence of a file on the SD card. This introduces a dependency on the SD card and might cause issues if the SD card is not available or fails. It also seems incomplete. The code block ends abruptly. - Suggestion: Explain the purpose of this check. Why is it necessary to bypass certain initialization steps for non-Neo6 modules or when the
AID_INI_DATA_FILE_NAME
file doesn't exist? Consider alternatives that don't depend on the SD card if possible. Complete the code block and handle the case where the file exists on Neo6. What should happen then? Explain the rationale in a comment.
General Suggestions:
- Consistent Naming: Use a consistent naming convention for variables and functions (e.g.,
coldStartGPS
orcold_start_gps
). - Comments: Add comments to explain the logic behind the changes, particularly the new features and conditional checks.
- Testing: Add unit tests to verify the new functionality, especially error handling.
By addressing these issues, the code will be more robust, maintainable, and easier to understand.
File: Gps.cpp
Review Summary:
The changes aim to improve GPS initialization, especially for NEO-6 modules, and add some diagnostic information. While the intent is good, there are potential issues with the implementation and some areas could be improved.
Issues and Suggestions:
-
Unnecessary Cold Start for Non-NEO-6 Modules: (Gps.cpp lines 312-315)
if (is_neo6()) log_i("We found no AID_INI on with neo6 on boot - coldstart gps in case its in a state where it doesn't get fixes"); if (!is_neo6()) log_i("Coldstart because we found that newer neos profit from that."); coldStartGps();
A cold start is performed even for non-NEO-6 modules. The log message suggests this is intended to benefit "newer neos," but the code executes unconditionally. This should be guarded by
is_neo6()
.Suggested change:
if (is_neo6()) { log_i("We found no AID_INI on with neo6 on boot - coldstart gps in case its in a state where it doesn't get fixes"); coldStartGps(); } else if(/* Condition for other modules needing cold start */) { log_i("Coldstart for other reason."); coldStartGps(); }
-
Clarity on
AID_INI
Check: (Gps.cpp lines 307-315)The code implies that the absence of
AID_INI
triggers a cold start, specifically for NEO-6 modules. However, it's unclear how the absence ofAID_INI
is determined. There's no code snippet showing this check. Add a comment or variable to make this explicit.Suggested change: Add code or comment showing how the absence of
AID_INI
is determined. -
Potential Redundant
handle()
Calls: (Gps.cpp lines 305, 355, 362) Multiple calls tohandle()
close together might be redundant. Consider consolidating them if they are intended to serve the same purpose. Explain in a comment why eachhandle()
is necessary or remove redundancies. -
Magic Numbers for Reset Types: (Gps.cpp lines 291, 357) Using named constants instead of magic numbers for reset types (e.g.,
0x00, 0x00, 0x02, 0x00
and0xFF, 0xFF, 0x00, 0x00
) would improve readability and maintainability.const uint8_t WARM_START[] = {0x00, 0x00, 0x02, 0x00}; const uint8_t COLD_START[] = {0xFF, 0xFF, 0x00, 0x00};
-
Inconsistent
waitForData
duration: InsoftResetGps()
,waitForData
is called with 1000ms, while incoldStartGps()
, it's also 1000ms. If there's a reason for the different durations, document it. Otherwise, use a consistent value or a named constant. -
Unclear Display Logic: (Gps.cpp lines 843-861) The display logic in
showWaitStatus
is fragmented and difficult to understand. The purpose ofclear
and the conditions for displaying different messages aren't immediately obvious. Simplify and document this section. Explain whatobsDisplay
is. The code is also incomplete, ending with a comment. -
Potential Bug - String Index Out of Bounds: (Gps.cpp 857-861)
satellitesString
is an array of size 3, but the code seems to try and accesssatellitesString[2]
ifmLastTimeTimeSet != 0
. If there are conditions where bothsatellitesString[1]
andsatellitesString[2]
would be set, then it would try to accesssatellitesString[3]
which is out of bounds.
By addressing these issues, the code will be more robust, readable, and maintainable. Provide the complete code for showWaitStatus
so I can help you debug the display logic. Also, please explain what obsDisplay
represents.
File: (It seems you haven't provided the filename, please provide it in future diffs) - Assuming Gps.cpp
based on content.
Review Summary:
This change introduces additional debugging information and seemingly attempts to differentiate behavior between NEO-6 and other GPS modules. While the added information can be useful, the implementation has several issues related to code clarity, potential bugs, and memory management.
Issues and Suggestions:
-
Magic Numbers:
0
,1
,2
,3
,4
,5
are used as grid indices for displaying information. Replace these with named constants for better readability and maintainability. For example:#define ROW_GPS_DETAIL 0 #define ROW_GAIN_JAM 1 #define ROW_SATELLITES 2 // ... etc. obsDisplay->showTextOnGrid(0, ROW_GPS_DETAIL, String(hw())+" Detail");
-
Unclear
hw()
function: Thehw()
function is used without context. What does it return? A string representation of the hardware version? This needs clarification and potentially a more descriptive name. Also, directly converting its return value to a String might be inefficient. Consider pre-formatting the string. -
Potential Buffer Overflow (hwString): The code copies
mGpsBuffer.monVer.hwVersion
intohwString
without checking the size ofhwString
. This can lead to a buffer overflow ifmGpsBuffer.monVer.hwVersion
is larger thanhwString
. Usestrncpy
or another safe string copy method:strncpy(hwString, mGpsBuffer.monVer.hwVersion, sizeof(hwString) - 1); // Leave space for null terminator hwString[sizeof(hwString) - 1] = '\0'; // Ensure null termination
-
is_neo6()
function missing: Theis_neo6()
function is used but not defined in the provided code snippet. Include its definition in the diff or explain its purpose. Consider making the hardware detection more robust and less reliant on hardcoded checks. Perhaps store a hardware type enum during initialization. -
String concatenation inefficiency: Repeatedly concatenating strings using
+
can be inefficient, especially within a loop. Consider usingsprintf
or astringstream
for better performance when building complex strings. -
Inconsistent Logging Levels: The code mixes
log_v
(verbose) andlog_d
(debug). Choose a consistent logging level or use different levels appropriately based on the importance of the message. -
aStatus
String Conversion: The switch case foraStatus
within theUBX_MSG::MON_HW
handling creates temporary strings. Consider using a lookup table or an array of strings for better efficiency. -
Duplicate Code for Logging: The logging statements for
NAV_SOL
andNAV_PVT
are very similar. Consider refactoring them into a helper function to reduce code duplication. -
mAidIniSent
andis_neo6()
Logic: The conditionalif (!mAidIniSent and is_neo6())
suggests thataidIni()
should only be sent for NEO-6 modules. Is this the intended behavior? Document this logic clearly. -
Missing Error Handling: The code doesn't appear to handle potential errors during GPS communication or parsing. Add checks for invalid data and appropriate error handling.
Example incorporating some suggestions (for hwString and aStatus):
// ... other code ...
case (uint16_t) UBX_MSG::MON_VER: {
// ... other code
strncpy(hwString, mGpsBuffer.monVer.hwVersion, sizeof(hwString) - 1);
hwString[sizeof(hwString) - 1] = '\0';
// ... other code
}
break;
case (uint16_t) UBX_MSG::MON_HW: {
const char* aStatusStrings[] = {"init", "?", "ok", "short", "open", "invalid"};
int aStatusIndex = mGpsBuffer.monHw.aStatus;
if (aStatusIndex < 0 || aStatusIndex >= sizeof(aStatusStrings) / sizeof(aStatusStrings[0])) {
aStatusIndex = sizeof(aStatusStrings) / sizeof(aStatusStrings[0]) - 1; // Default to "invalid"
}
const char* aStatus = aStatusStrings[aStatusIndex];
log_d("MON-HW Antenna Status %d %s, Antenna Power %d, Gain (0-8191) %d, noise level %d", mGpsBuffer.monHw.aStatus, aStatus, mGpsBuffer.monHw.aPower, mGpsBuffer.monHw.agcCnt, mGpsBuffer.monHw.noisePerMs);
// ... other code
}
break;
// ... other code ...
By addressing these issues, the code will be more robust, readable, and maintainable. Please provide the complete file name in future diffs and the definition of is_neo6()
and hw()
for a more thorough review.
Review Summary:
This PR introduces several changes related to GPS data handling, logging, and time synchronization. While most changes seem reasonable, there are some potential issues and areas for improvement regarding logging levels, time synchronization logic, and potential security implications of jamming information.
File Comments:
src/gps.cpp:
-
Logging Level Change (Line 1277): Changing
log_v
tolog_d
forVELNED
messages increases the logging level from verbose to debug. This might be excessive for regular operation and could lead to performance issues or log flooding. Consider reverting tolog_v
or adding a configuration option to control the verbosity of these messages. Provide a justification for the increased logging level.- Suggestion: Keep
log_v
for regular operation and introduce a debug flag to enablelog_d
for troubleshooting purposes.
- Suggestion: Keep
-
Time Synchronization Tolerance (Line 1403): Increasing the delay tolerance from 250ms to 1000ms might negatively impact time accuracy. While it might improve robustness in some scenarios, it could also lead to accepting less accurate time updates. Carefully evaluate the trade-off between accuracy and robustness and provide a rationale for this change. Consider making this configurable.
- Suggestion: Add a configuration parameter for the maximum acceptable delay. Default it to a lower value (e.g., 250ms or 500ms) and document the implications of increasing it.
-
Repeated Time Sync Requests (Line 1419 and 1423): The logic for requesting
NAV_TIMEGPS
messages seems potentially problematic. Continuously requesting time updates until a "good" time is received, especially whendelayMs > 100
, could create excessive traffic on the GPS interface. The conditiondelayMs > 100
seems arbitrary and should be explained.- Suggestion: Limit the number of retry attempts for time synchronization and/or implement a backoff strategy to avoid flooding the GPS module with requests. Clarify the reasoning behind
delayMs > 100
.
- Suggestion: Limit the number of retry attempts for time synchronization and/or implement a backoff strategy to avoid flooding the GPS module with requests. Clarify the reasoning behind
-
Jamming Indicator Exposure (Lines 1506, 1508, and corresponding changes in gps.h): Exposing the jamming indicator (
mLastJamInd
) could have security implications. An attacker could potentially use this information to understand the effectiveness of their jamming attempts. Consider the security implications of exposing this data.- Suggestion: Carefully evaluate the need to expose this information. If it's crucial, consider adding access control mechanisms or obfuscating the data before exposing it.
src/gps.h:
-
New Methods for Specific GPS Modules (Lines 113-115): Adding methods like
is_neo6()
,is_neo8()
, andis_neo10()
creates a tight coupling to specific GPS module models. This could limit portability and make it harder to support different modules in the future.- Suggestion: Consider a more generic approach, such as querying the module's capabilities or using a configuration file to specify the module type. This will make the code more flexible and maintainable.
-
Cold Start (Line 111): The addition of the
coldStartGps()
method should be documented clearly, explaining its purpose, behavior, and potential side effects.
General Recommendations:
- Error Handling: The code seems to lack robust error handling for cases where the GPS module doesn't respond or sends invalid data. Consider adding checks for timeouts, invalid data lengths, and checksum errors.
- Unit Tests: Add unit tests to verify the correctness of the implemented logic, especially for the time synchronization and message parsing functionalities.
- Documentation: Improve the documentation for the new methods and configuration parameters, explaining their purpose, usage, and potential side effects.
By addressing these concerns, the code can be improved in terms of robustness, maintainability, and security.
File src/gps.cpp
- Potential Issue (src/gps.cpp): Changing the size of
vp
from 25 to 17 bytes based on the assumption that M8 only uses 17 bytes needs careful consideration. Is it confirmed that only the M8 will be used? If other modules (like the M6) might be used, this change will lead to data corruption or truncation. Consider making the size ofvp
configurable or dynamically determined based on the detected GPS module. - Suggestion (src/gps.cpp): Add a check for the GPS module type (M6, M8, etc.) and adjust the size of
vp
accordingly. This will prevent issues if different modules are used. This might involve sending a command to the GPS module to query its identity. - Minor Issue (src/gps.cpp): The comment "//M6 25bytes, M8 only 17bytes?" should be more definitive. Replace with something like "// M8 uses 17 bytes. M6 uses 25 bytes. Adjusting to 17 based on assumed M8 usage." This makes the reason for the change clearer and highlights the assumption.
- Minor Issue (src/gps.cpp): For new member variables like
mLastGain
,mLastJamInd
, andhwString
, consider initializing them in the constructor to prevent uninitialized values. - Suggestion (src/gps.cpp): The
hw()
function should beconst
as indicated by the change. This is good practice. However,hwString
must also be mutable if you intend to modify it within thehw()
function. If it's only being assigned once, consider making itconst char* hwString
and initializing it directly.
File src/gpsrecord.h
- Minor Issue (src/gpsrecord.h): Removing blank lines generally doesn't improve readability or functionality. Unless it's part of a larger code cleanup effort to enforce consistent style, it's a trivial change.
File src/utils/alpdata.cpp
- Potential Issue (src/utils/alpdata.cpp): Deleting
AID_INI_DATA_FILE_NAME
immediately after reading it inloadMessage()
might be problematic. Why is the file being deleted? Is this file only needed once? This could cause issues if the data is needed again. If it’s intended to be a temporary file, document this behavior clearly. - Suggestion (src/utils/alpdata.cpp): If the intent is to only use the file once, rename the file to clearly indicate its temporary nature (e.g.,
temp_aid_ini_data.dat
). Or provide an option (a boolean parameter toloadMessage
) to control whether or not the file is deleted. This provides flexibility.
Review Summary:
The changes introduce potential issues related to the vp
array size and the deletion of AID_INI_DATA_FILE_NAME
. The vp
array size change should handle different GPS module types. The deletion of AID_INI_DATA_FILE_NAME
should be carefully reviewed and justified or made optional. The other comments are minor style or good practice suggestions. Addressing these issues will significantly improve the robustness and maintainability of the code.
</details>
@amandel the AI is now posting in your name - I'd suggest to add a prefix the lines of "The following is the automatically-generated result of looking at the PR with gemini AI - sometimes these suggestions can be helpful, but don't be overly confused or put too much work in where the AI missed the point. We're currently testing how helpful it can be.". The AI suggestions are sometimes helpful, sometimes confusing, so I think we really want people to know when they get suggestions by an AI and when they are talking to real people. Also in the open source space usage of AI for code can be seen as controversial given that a lot of AI manufacturers and deter some contributors. |
I think I went through all improvement suggestions now. A quick smoke test still gets a fix with reasonable speed. |
Cool! I'm not at home and can not do a qick test with the NEO6 module, @gluap I assume you did only test with the NEO8 module? |
Actually intention was just to test this and not to create posts in my name 🙈. While the created content was mostly valid, I'll switch it off for now.
If you think we should give us some rules here we might need to discuss it in a Monday meeting? |
I tested on neo6, neo7 and neo8.
|
…ase of slow reset.
This adds a few changes to the GPS logic
@amandel what do you think?