Skip to content

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

Merged
merged 17 commits into from
Mar 3, 2025
Merged

Changes to the GPS logic for newer GPS. #374

merged 17 commits into from
Mar 3, 2025

Conversation

gluap
Copy link
Contributor

@gluap gluap commented Feb 25, 2025

This adds a few changes to the GPS logic

  • on non-neo6m we coldstart on bootup
  • there is an extra http endpoint to trigger a coldstart (by @Jens-Kassel)
  • we only do aid_ini and aid_alp on neo6m
  • we interpret MON_HW in the way it is sent on neo8m (which only differs in bytes that we weren't using on neo6m) (by @Jens-Kassel)
  • We store gain and jamming indicator for display and logging (by @Jens-Kassel)
  • We display more information from MON-HW
  • we give more debug data on-screen for non-neo6m
  • Instead of ignoring datetimes below a minimum precision we keep asking for the datetime messge in short intervals until we receive one with a high precision (based on @Jens-Kassel s findings from last november).

@amandel what do you think?

Copy link
Member

@amandel amandel left a 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)) {
Copy link
Member

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'?

Copy link
Contributor Author

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};
Copy link
Member

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

Copy link
Contributor Author

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.

@Jens-Kassel
Copy link

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).

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)

@amandel
Copy link
Member

amandel commented Feb 28, 2025

I thought about storing the hardware-variant in the config.

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?

@gluap
Copy link
Contributor Author

gluap commented Feb 28, 2025

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).

@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.

@amandel
Copy link
Member

amandel commented Feb 28, 2025

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've triggered gemini :) - I see it just as bonus no blocker if it is red.

Copy link
Member

@amandel amandel left a 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

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 triggers gps.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(). If gps.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. The hwString appears to already contain the hardware version information. Directly comparing substrings of hwString is simpler.
  • Suggestion: Remove the is_neoX() functions and use direct comparisons in the hw() 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 conditional if((!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 or cold_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:

  1. 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();
    }
  2. 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 of AID_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.

  3. Potential Redundant handle() Calls: (Gps.cpp lines 305, 355, 362) Multiple calls to handle() close together might be redundant. Consider consolidating them if they are intended to serve the same purpose. Explain in a comment why each handle() is necessary or remove redundancies.

  4. 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 and 0xFF, 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};
  5. Inconsistent waitForData duration: In softResetGps(), waitForData is called with 1000ms, while in coldStartGps(), it's also 1000ms. If there's a reason for the different durations, document it. Otherwise, use a consistent value or a named constant.

  6. Unclear Display Logic: (Gps.cpp lines 843-861) The display logic in showWaitStatus is fragmented and difficult to understand. The purpose of clear and the conditions for displaying different messages aren't immediately obvious. Simplify and document this section. Explain what obsDisplay is. The code is also incomplete, ending with a comment.

  7. 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 access satellitesString[2] if mLastTimeTimeSet != 0. If there are conditions where both satellitesString[1] and satellitesString[2] would be set, then it would try to access satellitesString[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:

  1. 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");
  2. Unclear hw() function: The hw() 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.

  3. Potential Buffer Overflow (hwString): The code copies mGpsBuffer.monVer.hwVersion into hwString without checking the size of hwString. This can lead to a buffer overflow if mGpsBuffer.monVer.hwVersion is larger than hwString. Use strncpy 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
  4. is_neo6() function missing: The is_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.

  5. String concatenation inefficiency: Repeatedly concatenating strings using + can be inefficient, especially within a loop. Consider using sprintf or a stringstream for better performance when building complex strings.

  6. Inconsistent Logging Levels: The code mixes log_v (verbose) and log_d (debug). Choose a consistent logging level or use different levels appropriately based on the importance of the message.

  7. aStatus String Conversion: The switch case for aStatus within the UBX_MSG::MON_HW handling creates temporary strings. Consider using a lookup table or an array of strings for better efficiency.

  8. Duplicate Code for Logging: The logging statements for NAV_SOL and NAV_PVT are very similar. Consider refactoring them into a helper function to reduce code duplication.

  9. mAidIniSent and is_neo6() Logic: The conditional if (!mAidIniSent and is_neo6()) suggests that aidIni() should only be sent for NEO-6 modules. Is this the intended behavior? Document this logic clearly.

  10. 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 to log_d for VELNED 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 to log_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 enable log_d for troubleshooting purposes.
  • 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 when delayMs > 100, could create excessive traffic on the GPS interface. The condition delayMs > 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.
  • 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(), and is_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 of vp 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, and hwString, consider initializing them in the constructor to prevent uninitialized values.
  • Suggestion (src/gps.cpp): The hw() function should be const as indicated by the change. This is good practice. However, hwString must also be mutable if you intend to modify it within the hw() function. If it's only being assigned once, consider making it const 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 in loadMessage() 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 to loadMessage) 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>

@gluap
Copy link
Contributor Author

gluap commented Mar 1, 2025

@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.

@gluap
Copy link
Contributor Author

gluap commented Mar 1, 2025

I think I went through all improvement suggestions now.

A quick smoke test still gets a fix with reasonable speed.
firmware.zip

@amandel
Copy link
Member

amandel commented Mar 2, 2025

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?

@amandel
Copy link
Member

amandel commented Mar 2, 2025

@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.".

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.

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.

If you think we should give us some rules here we might need to discuss it in a Monday meeting?

@gluap
Copy link
Contributor Author

gluap commented Mar 2, 2025

I tested on neo7 and neo8.

Things to test on neo6:

  • does alp download still happen? (if there are messages about alp while starting server mode it does, the whole method is skipped on other neos)

One todo left i noticed yesterday is to clean up the gps info on the screen.
IMG_20250302_113007~2.jpg

@gluap
Copy link
Contributor Author

gluap commented Mar 2, 2025

I tested on neo6, neo7 and neo8.

  • does alp download still happen on neo6? (yes)
  • is ALP download skipped on neo8?(yes)
  • clean up the gps info on the screen.

@gluap gluap merged commit 544d2ac into main Mar 3, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants