GPS refactor & license text updates - #601
Conversation
The code that was left behind was code that I wrote myself, so I'm relicensing that to Apache
📝 WalkthroughWalkthroughThe change separates generic GPS interfaces from the Meshtastic GPS implementation, adds module headers and build wiring, and starts both modules during boot. It introduces CAS message definitions and Meshtastic-specific logging identifiers. README, device-tree, license metadata, and repository license documentation are also updated. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be7b9b76-c057-4e2a-bfcc-7dfab93942aa
📒 Files selected for processing (31)
Drivers/gps-generic-module/CMakeLists.txtDrivers/gps-generic-module/LICENSE-Apache-2.0.mdDrivers/gps-generic-module/README.mdDrivers/gps-generic-module/bindings/tactility,gps-generic.yamlDrivers/gps-generic-module/include/gps_generic/module.hDrivers/gps-generic-module/source/module.cppDrivers/gps-meshtastic-module/CMakeLists.txtDrivers/gps-meshtastic-module/LICENSE-GPL-3.0.mdDrivers/gps-meshtastic-module/README.mdDrivers/gps-meshtastic-module/devicetree.yamlDrivers/gps-meshtastic-module/include/gps_meshtastic/module.hDrivers/gps-meshtastic-module/private/gps_generic/private/cas_messages.hDrivers/gps-meshtastic-module/private/gps_generic/private/gps_response.hDrivers/gps-meshtastic-module/private/gps_generic/private/init.hDrivers/gps-meshtastic-module/private/gps_generic/private/probe.hDrivers/gps-meshtastic-module/private/gps_generic/private/ublox.hDrivers/gps-meshtastic-module/private/gps_generic/private/ublox_messages.hDrivers/gps-meshtastic-module/source/gps_meshtastic.cppDrivers/gps-meshtastic-module/source/init.cppDrivers/gps-meshtastic-module/source/module.cppDrivers/gps-meshtastic-module/source/probe.cppDrivers/gps-meshtastic-module/source/ublox.cppLICENSE.mdTactility/CMakeLists.txtTactility/Source/Tactility.cppTests/SdkIntegration/LICENSE-Apache-2.0.mdTests/Tactility/LICENSE-GPL-3.0.mdTests/TactilityFreeRtos/LICENSE-Apache-2.0.mdTests/TactilityKernel/LICENSE-Apache-2.0.mdTests/crypt-module/LICENSE-Apache-2.0.mdTranslations/LICENSE-Apache-2.0.md
💤 Files with no reviewable changes (1)
- Drivers/gps-generic-module/CMakeLists.txt
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be7b9b76-c057-4e2a-bfcc-7dfab93942aa
📒 Files selected for processing (31)
Drivers/gps-generic-module/CMakeLists.txtDrivers/gps-generic-module/LICENSE-Apache-2.0.mdDrivers/gps-generic-module/README.mdDrivers/gps-generic-module/bindings/tactility,gps-generic.yamlDrivers/gps-generic-module/include/gps_generic/module.hDrivers/gps-generic-module/source/module.cppDrivers/gps-meshtastic-module/CMakeLists.txtDrivers/gps-meshtastic-module/LICENSE-GPL-3.0.mdDrivers/gps-meshtastic-module/README.mdDrivers/gps-meshtastic-module/devicetree.yamlDrivers/gps-meshtastic-module/include/gps_meshtastic/module.hDrivers/gps-meshtastic-module/private/gps_generic/private/cas_messages.hDrivers/gps-meshtastic-module/private/gps_generic/private/gps_response.hDrivers/gps-meshtastic-module/private/gps_generic/private/init.hDrivers/gps-meshtastic-module/private/gps_generic/private/probe.hDrivers/gps-meshtastic-module/private/gps_generic/private/ublox.hDrivers/gps-meshtastic-module/private/gps_generic/private/ublox_messages.hDrivers/gps-meshtastic-module/source/gps_meshtastic.cppDrivers/gps-meshtastic-module/source/init.cppDrivers/gps-meshtastic-module/source/module.cppDrivers/gps-meshtastic-module/source/probe.cppDrivers/gps-meshtastic-module/source/ublox.cppLICENSE.mdTactility/CMakeLists.txtTactility/Source/Tactility.cppTests/SdkIntegration/LICENSE-Apache-2.0.mdTests/Tactility/LICENSE-GPL-3.0.mdTests/TactilityFreeRtos/LICENSE-Apache-2.0.mdTests/TactilityKernel/LICENSE-Apache-2.0.mdTests/crypt-module/LICENSE-Apache-2.0.mdTranslations/LICENSE-Apache-2.0.md
💤 Files with no reviewable changes (1)
- Drivers/gps-generic-module/CMakeLists.txt
🛑 Comments failed to post (4)
Drivers/gps-meshtastic-module/private/gps_generic/private/ublox_messages.h (1)
218-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the GSA payload with its initialization intent.
Line 223 configures a zero UART1 rate, so this disables GSA, while
init_ublox_789sends it as “enable NMEA GSA”. Enable the UART1 rate or update the initialization behavior/logging to intentionally disable it.Drivers/gps-meshtastic-module/source/ublox.cpp (3)
119-190: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm the return type/semantics of uart_controller_read_bytes rg -n "uart_controller_read_bytes" --type=cpp -g '*.h' -B2 -A5Repository: TactilityProject/Tactility
Length of output: 164
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== tracked files matching ublox / uart_controller_read_bytes ==" git ls-files | rg '(^|/)(ublox\.(cpp|hpp|h)|.*)' || true echo "== broad search uart_controller_read_bytes ==" rg -n "uart_controller_read_bytes" . || true echo "== locate ublox.cpp ==" fd -a 'ublox\.cpp$' . || true echo "== list source files in Drivers/gps-meshtastic-module/source if present ==" fd . Drivers/gps-meshtastic-module/source 2>/dev/null | sed -n '1,120p' || trueRepository: TactilityProject/Tactility
Length of output: 50382
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== ublox.cpp relevant sections ==" sed -n '1,230p' Drivers/gps-meshtastic-module/source/ublox.cpp sed -n '230,520p' Drivers/gps-meshtastic-module/source/ublox.cpp echo "== declarations/usages of read_chars/readbytes/read_bytes/read_bytes_return-like names ==" rg -n "uart_controller_.*(read|write|available)|read_bytes|read_chars|size_t|auto .*read" Drivers/gps-meshtastic-module/source Drivers/gps-meshtastic-module/include Drivers -g '*.{h,hpp,cpp,c}' || trueRepository: TactilityProject/Tactility
Length of output: 50382
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== uart_controller declarations ==" fd -a 'uart_controller\.(h|hpp|cpp|c)$' . | sed -n '1,40p' rg -n "uart_controller_get_available|uart_controller_read_byte|uart_controller_write_bytes|uart_controller_flush_input|typedef.*uart_controller|error_t.*uart_controller" Drivers include . -g '*.{h,hpp,c,cpp}' --glob '!**/.git/**' --glob '!**/build/**' | sed -n '1,220p' echo "== gps meshtastic uart read/write/call patterns ==" rg -n "uart_controller_(read|write|get_available|flush_input)|get_ack\\(" Drivers/gps-meshtastic-module -g '*.{h,hpp,c,cpp}'Repository: TactilityProject/Tactility
Length of output: 22309
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== read_bytes implementation ==" sed -n '1,45p' TactilityKernel/source/drivers/uart_controller.cpp echo "== read_until success return path around bytes_read ==" sed -n '35,55p' TactilityKernel/source/drivers/uart_controller.cpp echo "== read_until declaration ==" sed -n '175,188p' TactilityKernel/include/tactility/drivers/uart_controller.hRepository: TactilityProject/Tactility
Length of output: 3579
Assign and check
uart_controller_read_bytes’s returned status.
uart_controller_read_bytes()returns anerror_t, but the return here is ignored, soread_bytesremains the stale0Uand the success check becomes a no-op. Capture the returned error code and reject the UBX frame path when the read did not complete successfully.
192-249: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ublox_info.extensionNois never reset at the start ofprobe()— risk of out-of-bounds write on repeated calls.
ublox_infoisstatic, so its state persists across invocations ofprobe().extensionNois only ever incremented (lines 246-248) and bounded viaif (ublox_info.extensionNo > 9) break;, but never reset to0at the top ofprobe(). Ifprobe()runs a second time within the same power cycle (driver restart/retry) after a prior call leftextensionNoat10, the next accumulation loop iteration will write toublox_info.extension[10][...], one past the bound ofchar extension[10][30], corrupting the adjacentprotocol_versionfield (or beyond).🔧 Suggested fix
GpsModel probe(Device* uart) { LOG_I(TAG, "Probing for U-blox"); + ublox_info.extensionNo = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.static struct UbloxGnssModelInfo { char swVersion[30]; char hwVersion[10]; uint8_t extensionNo; char extension[10][30]; uint8_t protocol_version; } ublox_info; GpsModel probe(Device* uart) { LOG_I(TAG, "Probing for U-blox"); ublox_info.extensionNo = 0; uint8_t cfg_rate[] = {0xB5, 0x62, 0x06, 0x08, 0x00, 0x00, 0x00, 0x00}; checksum(cfg_rate, sizeof(cfg_rate)); uart_controller_flush_input(uart); uart_controller_write_bytes(uart, cfg_rate, sizeof(cfg_rate), 500 / portTICK_PERIOD_MS); // Check that the returned response class and message ID are correct GpsResponse response = get_ack(uart, 0x06, 0x08, 750); if (response == GpsResponse::None) { LOG_W(TAG, "No GNSS Module"); return GpsModel::GPS_MODEL_UNKNOWN; } else if (response == GpsResponse::FrameErrors) { LOG_W(TAG, "UBlox Frame Errors"); } uint8_t buffer[256]; memset(buffer, 0, sizeof(buffer)); uint8_t message_monver[8] = { 0xB5, 0x62, // Sync message for UBX protocol 0x0A, 0x04, // Message class and ID (UBX-MON-VER) 0x00, 0x00, // Length of payload (we're asking for an answer, so no payload) 0x00, 0x00 // Checksum }; // Get Ublox gnss module hardware and software info checksum(message_monver, sizeof(message_monver)); uart_controller_flush_input(uart); uart_controller_write_bytes(uart, message_monver, sizeof(message_monver), 500); uint16_t ack_response_len = get_ack(uart, buffer, sizeof(buffer), 0x0A, 0x04, 1200); if (ack_response_len) { uint16_t position = 0; for (char& i: ublox_info.swVersion) { i = buffer[position]; position++; } for (char& i: ublox_info.hwVersion) { i = buffer[position]; position++; } while (ack_response_len >= position + 30) { for (int i = 0; i < 30; i++) { ublox_info.extension[ublox_info.extensionNo][i] = buffer[position]; position++; } ublox_info.extensionNo++; if (ublox_info.extensionNo > 9) break; }
226-227: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm expected units (ticks vs ms) for uart_controller_write_bytes's timeout parameter rg -n "uart_controller_write_bytes" --type=cpp -g '*.h' -B2 -A5Repository: TactilityProject/Tactility
Length of output: 164
🏁 Script executed:
#!/bin/bash set -u echo "== locate ublox.cpp and related files ==" git ls-files | rg '(^|/)ublox\.cpp$|((uart_controller|Uart|UART).*\.(h|c|cxx|cpp)|\bdrivers?|Drivers)' | sed -n '1,200p' echo echo "== search for uart_controller_write_bytes across repo ==" rg -n "uart_controller_write_bytes|UART_CONTROLLER|uart_controller" . -g '!build' -g '!node_modules' -g '!dist' | sed -n '1,240p' echo echo "== outline ublox if present ==" file="$(git ls-files | rg '(^|/)ublox\.cpp$' | head -n1)" if [ -n "${file:-}" ]; then echo "FILE=$file" wc -l "$file" ast-grep outline "$file" --view compact | sed -n '1,200p' fiRepository: TactilityProject/Tactility
Length of output: 28818
🏁 Script executed:
#!/bin/bash set -u echo "== ublox.cpp relevant sections ==" sed -n '1,80p;190,235p;300,340p;350,370p;455,472p' Drivers/gps-meshtastic-module/source/ublox.cpp echo echo "== uart_controller signature/implementation and type constants ==" sed -n '1,220p' TactilityKernel/include/tactility/drivers/uart_controller.h sed -n '1,320p' TactilityKernel/source/drivers/uart_controller.cpp rg -n "portTICK_PERIOD_MS|typedef.*TickType|struct TickType|TickType_t" --glob '*.{h,c,cpp}' --glob '!Drivers/gps-meshtastic-module/source/*' -B1 -A1 | sed -n '1,160p' echo echo "== all direct millis values passed to uart_controller write calls in the gps module ==" python3 - <<'PY' from pathlib import Path p = Path("Drivers/gps-meshtastic-module/source/ublox.cpp") for i,line in enumerate(p.read_text().split("\n"),1): if "uart_controller_write_bytes" in line: print(f"{i}: {line.strip()}") PYRepository: TactilityProject/Tactility
Length of output: 19839
🏁 Script executed:
#!/bin/bash set -u echo "== esp32 uart write implementation around write_bytes ==" sed -n '180,340p' Platforms/platform-esp32/source/drivers/esp32_uart.cpp echo echo "== portTICK_PERIOD_MS definition references ==" rg -n "portTICK_PERIOD_MS|pdMS_TO_TICKS|pdTICKS_TO_MS|CONFIG_FREERTOS_HZ" . -g '!node_modules' -g '!build' -g '!dist' | sed -n '1,200p'Repository: TactilityProject/Tactility
Length of output: 28692
🏁 Script executed:
#!/bin/bash set -u echo "== esp32 uart write_bytes definition ==" rg -n "write_bytes|uart_write_bytes|write_byte|xQueueSend|pdMS_TO_TICKS|TickType_t timeout" Platforms/platform-esp32/source/drivers/esp32_uart.cpp -B3 -A3 echo echo "== targeted lines 1-180 esp32_uart.cpp ==" sed -n '1,180p' Platforms/platform-esp32/source/drivers/esp32_uart.cppRepository: TactilityProject/Tactility
Length of output: 8070
Pass tick values to
uart_controller_write_bytes.
uart_controller_*timeout parameters areTickType_t, and the ESP32 driver forwards them touart_wait_tx_done. Keep these raw millisecond values consistent with the other calls in this file, but pass500 / portTICK_PERIOD_MShere and2000 / portTICK_PERIOD_MSfor the SAVE call so the timeout duration is preserved.Also applies to: 466-467
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Drivers/gps-meshtastic-module/source/module.cpp (1)
14-16: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDefine the Meshtastic module symbol here.
Changing
.namedoes not change the exported symbol: this file still definesgps_generic_module, while the Meshtastic header, driver owner, and startup path usegps_meshtastic_module. This can duplicate the generic module and leave the Meshtastic module undefined or unregistered.Proposed fix
-Module gps_generic_module = { +Module gps_meshtastic_module = {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 16f70c30-da64-4c54-a437-9208ee5ba519
📒 Files selected for processing (14)
Drivers/gps-meshtastic-module/README.mdDrivers/gps-meshtastic-module/include/gps_meshtastic/module.hDrivers/gps-meshtastic-module/private/gps_generic/private/cas_messages.hDrivers/gps-meshtastic-module/private/gps_generic/private/gps_response.hDrivers/gps-meshtastic-module/private/gps_generic/private/init.hDrivers/gps-meshtastic-module/private/gps_generic/private/probe.hDrivers/gps-meshtastic-module/private/gps_generic/private/ublox.hDrivers/gps-meshtastic-module/private/gps_generic/private/ublox_messages.hDrivers/gps-meshtastic-module/source/gps_meshtastic.cppDrivers/gps-meshtastic-module/source/init.cppDrivers/gps-meshtastic-module/source/module.cppDrivers/gps-meshtastic-module/source/probe.cppDrivers/gps-meshtastic-module/source/ublox.cppLICENSE.md
🚧 Files skipped from review as they are similar to previous changes (10)
- Drivers/gps-meshtastic-module/private/gps_generic/private/ublox_messages.h
- Drivers/gps-meshtastic-module/private/gps_generic/private/ublox.h
- Drivers/gps-meshtastic-module/private/gps_generic/private/gps_response.h
- Drivers/gps-meshtastic-module/source/ublox.cpp
- Drivers/gps-meshtastic-module/source/init.cpp
- Drivers/gps-meshtastic-module/source/gps_meshtastic.cpp
- Drivers/gps-meshtastic-module/private/gps_generic/private/init.h
- Drivers/gps-meshtastic-module/private/gps_generic/private/cas_messages.h
- Drivers/gps-meshtastic-module/source/probe.cpp
- Drivers/gps-meshtastic-module/private/gps_generic/private/probe.h
1 participant
gps-generic-moduleinto:gps-generic-modulethat contains only interfaces/configs/bindings (Apache license)gps-meshtastic-modulethat contains an implementation (GPL license)LICENSE.mdfor changes, but also added clarifications