Add kernel drivers for SPI and UART and make locking APIs more consistent #489
Conversation
📝 WalkthroughWalkthroughConsolidates mutex APIs to require a TickType_t timeout and updates callers and tests accordingly. Replaces the legacy tt::hal::uart C++/ESP/POSIX subsystem with a device-driven UART controller interface: new uart_controller API, kernel symbols, and C wrappers. Adds ESP32 platform drivers and bindings for SPI and UART (C drivers, headers, driver registrations) and many board DTS uart nodes using a new espressif,esp32-uart binding. Migrates GPS, HAL, and apps to the device-based UART APIs and adjusts an LVGL POSIX mutex callsite. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 4
🧹 Nitpick comments (5)
TactilityKernel/Include/tactility/drivers/spi_controller.h (1)
64-71: Clarify timeout units in the public docs.
TickType_timplies FreeRTOS ticks; documenting units avoids callers passing milliseconds by mistake.✏️ Suggested doc tweak
- * `@param`[in] timeout the maximum time to wait for the lock + * `@param`[in] timeout maximum time to wait for the lock, in FreeRTOS ticksPlatforms/PlatformEsp32/Source/drivers/esp32_spi.cpp (1)
16-27: Consider initializinginitializedin the struct.The
initializedmember is not initialized in the struct definition. Whilestart()sets it tofalsebefore use, adding a default initializer improves clarity and prevents potential issues if the struct is used differently in the future.♻️ Suggested improvement
struct InternalData { Mutex mutex; - bool initialized; + bool initialized = false; InternalData() {Platforms/PlatformEsp32/Source/drivers/esp32_uart.cpp (2)
34-35: Macro definitions include trailing semicolons.The
lockandunlockmacros include trailing semicolons. When invoked aslock(data);, this produces double semicolons. While harmless, it's unconventional and could cause issues with certain coding styles or static analyzers.♻️ Suggested improvement
-#define lock(data) mutex_lock(&data->mutex); -#define unlock(data) mutex_unlock(&data->mutex); +#define lock(data) mutex_lock(&data->mutex) +#define unlock(data) mutex_unlock(&data->mutex)
126-137: Partial reads returnERROR_TIMEOUTwithout indicating bytes actually read.When
uart_read_bytesreturns fewer bytes than requested (line 134), the function returnsERROR_TIMEOUT. The caller has no way to know how many bytes were actually read. Consider whether the API should provide this information, or document that partial reads discard the data.TactilityKernel/Include/tactility/drivers/uart_controller.h (1)
90-90: Addconstqualifier to write buffer parameters.The
bufferparameter inwrite_bytesis an input buffer that should not be modified. Addingconstimproves API safety and allows passing const buffers.♻️ Suggested fix in header
- error_t (*write_bytes)(struct Device* device, uint8_t* buffer, size_t buffer_size, TickType_t timeout); + error_t (*write_bytes)(struct Device* device, const uint8_t* buffer, size_t buffer_size, TickType_t timeout);-error_t uart_controller_write_bytes(struct Device* device, uint8_t* buffer, size_t buffer_size, TickType_t timeout); +error_t uart_controller_write_bytes(struct Device* device, const uint8_t* buffer, size_t buffer_size, TickType_t timeout);This change would also require updating the implementation in
uart_controller.cppandesp32_uart.cpp.Also applies to: 148-148
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
Modules/hal-device-module/Source/drivers/hal_device.cpp (1)
113-115: Consider using designated initializer for consistency.Other
DeviceTypedefinitions in this PR use.name = "..."syntax (e.g.,i2s_controller.cpp,gpio_controller.cpp). Using the same style here improves readability and consistency.♻️ Suggested change
const struct DeviceType HAL_DEVICE_TYPE { - "hal-device" + .name = "hal-device" };Platforms/PlatformEsp32/Source/drivers/esp32_spi.cpp (1)
89-101: Consider locking mutex during stop for consistency with UART driver.The
stop()function accesses and frees the SPI bus without holding the mutex. While the device framework may guarantee no concurrent access during stop, the UART driver (esp32_uart.cpplines 233-235) locks before cleanup. Consider adding locking for consistency and defensive safety.♻️ Suggested refactor
static error_t stop(Device* device) { ESP_LOGI(TAG, "stop %s", device->name); auto* driver_data = GET_DATA(device); auto* dts_config = GET_CONFIG(device); + mutex_lock(&driver_data->mutex); if (driver_data->initialized) { spi_bus_free(dts_config->host); } + mutex_unlock(&driver_data->mutex); device_set_driver_data(device, nullptr); delete driver_data; return ERROR_NONE; }Platforms/PlatformEsp32/Source/drivers/esp32_uart.cpp (2)
17-36: Minor: Lock/unlock macros include trailing semicolons.The macro definitions on lines 35-36 end with semicolons, causing double-semicolons at call sites (e.g.,
lock(driver_data);expands tomutex_lock(&driver_data->mutex);;). This is harmless but unconventional.♻️ Suggested fix
-#define lock(data) mutex_lock(&data->mutex); -#define unlock(data) mutex_unlock(&data->mutex); +#define lock(data) mutex_lock(&data->mutex) +#define unlock(data) mutex_unlock(&data->mutex)
219-219: Minor: Inconsistent logging functions used.
ESP_LOGIis used directly (lines 219, 229, 243) whileLOG_Efromtactility/log.his used elsewhere (line 191). Consider usingLOG_I/LOG_Econsistently throughout for uniformity with the rest of the codebase.Also applies to: 229-229, 243-243
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Devices/guition-jc8048w550c/guition,jc8048w550c.dts (1)
27-44:⚠️ Potential issue | 🟠 MajorUART1 pins conflict with I2C external bus.
GPIO 17 and 18 are assigned to both
i2c_external(SDA/SCL on lines 31–32) anduart1(RX/TX on lines 41–40). Enabling both nodes simultaneously will cause hardware contention. Consider disablinguart1by default withstatus = "disabled", or document that these are mutually exclusive configurations.🛠️ Example mitigation (disable UART until explicitly enabled)
uart1 { compatible = "espressif,esp32-uart"; port = <UART_NUM_1>; pin-tx = <18>; pin-rx = <17>; pin-cts = <GPIO_PIN_NONE>; pin-rtx = <GPIO_PIN_NONE>; + status = "disabled"; };Tactility/Source/hal/gps/Ublox.cpp (2)
61-85:⚠️ Potential issue | 🟠 MajorTimeout loop mixes getMillis with getTicks.
startTimeuseskernel::getMillis()while the loop compareskernel::getTicks()againstwaitMillis. If ticks aren’t milliseconds, the timeout becomes incorrect. Use a single time base (or convertwaitMillisto ticks).🛠️ Proposed fix (use millis consistently)
- uint32_t startTime = kernel::getMillis(); + uint32_t startTime = kernel::getMillis(); @@ - while (kernel::getTicks() - startTime < waitMillis) { + while (kernel::getMillis() - startTime < waitMillis) {
184-189:⚠️ Potential issue | 🟠 Major
uart_controller_read_bytesresult ignored; ACK payload path always fails.
read_bytesis never assigned, so theread_bytes != needReadcheck always fails and the ACK handler never returns payload data. Capture the API result and gate on success.🛠️ Proposed fix
- auto read_bytes = 0U; - uart_controller_read_bytes(uart, buffer, needRead, 250 / portTICK_PERIOD_MS); - if (read_bytes != needRead) { + const auto err = uart_controller_read_bytes(uart, buffer, needRead, 250 / portTICK_PERIOD_MS); + if (err != ERROR_NONE) { ubxFrameCounter = 0; } else { // return payload length `#ifdef` GPS_DEBUG LOGGER.info("Got ACK for class {:02X} message {:02X} in {}ms", requestedClass, requestedId, kernel::getMillis() - startTime); `#endif` return needRead; }Tactility/Source/hal/gps/GpsInit.cpp (1)
79-93:⚠️ Potential issue | 🟠 MajorTimeout loop mixes getMillis with getTicks.
startTimeis fromkernel::getMillis()but the loop compareskernel::getTicks()towaitMillis. If ticks aren’t milliseconds, the timeout is incorrect. Use the same time base or convertwaitMillisto ticks.🛠️ Proposed fix (use millis consistently)
- uint32_t startTime = kernel::getMillis(); + uint32_t startTime = kernel::getMillis(); @@ - while (kernel::getTicks() - startTime < waitMillis) { + while (kernel::getMillis() - startTime < waitMillis) {
🟡 Minor comments (8)
Devices/m5stack-stickc-plus2/m5stack,stickc-plus2.dts-42-42 (1)
42-42:⚠️ Potential issue | 🟡 MinorTypo:
pin-rtxshould bepin-rts.Standard UART flow control uses RTS (Request To Send), not "RTX".
Proposed fix
- pin-rtx = <GPIO_PIN_NONE>; + pin-rts = <GPIO_PIN_NONE>;Devices/m5stack-stickc-plus2/m5stack,stickc-plus2.dts-36-43 (1)
36-43:⚠️ Potential issue | 🟡 MinorAdd a clarifying comment to document Grove port mutual exclusivity.
The
uart_groveandi2c_grovenodes legitimately share GPIO pins 32 and 33 on M5Stack hardware—this is consistent with the M5Stack StickC Plus design where a single Grove connector supports both I2C and UART protocols but not simultaneously. However, the DTS file lacks documentation explaining this mutual exclusivity. Add an inline comment to thei2c_groveoruart_grovenode clarifying that the Grove port must be configured as either I2C or UART but not both.Devices/m5stack-core2/m5stack,core2.dts-57-57 (1)
57-57:⚠️ Potential issue | 🟡 MinorTypo:
pin-rtxshould bepin-rts.Proposed fix
- pin-rtx = <GPIO_PIN_NONE>; + pin-rts = <GPIO_PIN_NONE>;Devices/m5stack-cores3/m5stack,cores3.dts-79-79 (1)
79-79:⚠️ Potential issue | 🟡 MinorTypo:
pin-rtxshould bepin-rts.Proposed fix
- pin-rtx = <GPIO_PIN_NONE>; + pin-rts = <GPIO_PIN_NONE>;TactilityKernel/Include/tactility/device.h-49-49 (1)
49-49:⚠️ Potential issue | 🟡 MinorMinor typo in comment: trailing
::.Proposed fix
- * This is used by the devicetree code generator and the application init sequence::. + * This is used by the devicetree code generator and the application init sequence.Devices/cyd-2432s028rv3/cyd,2432s028rv3.dts-31-31 (1)
31-31:⚠️ Potential issue | 🟡 MinorTypo:
pin-rtxshould bepin-rts.Same issue as in other DTS files—RTS is the standard UART flow control signal name.
Proposed fix
- pin-rtx = <GPIO_PIN_NONE>; + pin-rts = <GPIO_PIN_NONE>;Devices/elecrow-crowpanel-basic-35/elecrow,crowpanel-basic-35.dts-33-33 (1)
33-33:⚠️ Potential issue | 🟡 MinorTypo:
pin-rtxshould bepin-rts.RTS (Request To Send) is the standard UART flow control signal name. This appears to be a typo.
Proposed fix
- pin-rtx = <GPIO_PIN_NONE>; + pin-rts = <GPIO_PIN_NONE>;Tactility/Private/Tactility/hal/gps/Ublox.h-17-22 (1)
17-22:⚠️ Potential issue | 🟡 Minor
sendPacketis unused code and never called anywhere in the codebase.The function builds a packet buffer but doesn't transmit it. However, this isn't a problem with hidden dependencies—
sendPackethas zero call sites. Either remove this dead code or, if it's intended as infrastructure for future use, implement it by callinguart_controller_write_bytes(as done elsewhere inUblox.cpp,Probe.cpp, andGpsInit.cpp).
🧹 Nitpick comments (3)
TactilityC/Include/tt_hal_uart.h (1)
12-17: Minor formatting inconsistency in comment block.Line 16 is missing a leading space before the asterisk, breaking the standard multi-line comment alignment.
Suggested fix
/** * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! * WARNING: THIS API IS NON-FUNCTIONAL AND DEPRECATED. * IT WILL BE REMOVED IN A FUTURE RELEASE ONCE OFFICIAL APPS ARE MIGRATED. -* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! */Tactility/Source/hal/gps/Probe.cpp (1)
100-107: Avoid hard-coded byte counts for NMEA commands.The manual lengths (e.g., Line 102-108) are easy to miscount and can truncate/extend writes. Consider
sizeof("...") - 1or aconstexprstring to keep the size in sync; apply the same pattern to the other literals in this function.♻️ Example refactor
- uart_controller_write_bytes(uart, (const uint8_t*)"$PCAS03,0,0,0,0,0,0,0,0,0,0,,,0,0*02\r\n", 40, 500); + constexpr char kCloseAllNmea[] = "$PCAS03,0,0,0,0,0,0,0,0,0,0,,,0,0*02\r\n"; + uart_controller_write_bytes(uart, (const uint8_t*)kCloseAllNmea, sizeof(kCloseAllNmea) - 1, 500);Tactility/Source/hal/gps/GpsInit.cpp (1)
175-186: Prefersizeoffor command byte counts.The hard-coded literal lengths (Line 176-186) are easy to miscount. Using
sizeof("...") - 1or aconstexprstring keeps the length correct and maintainable; apply the same pattern to the other literals in this file.♻️ Example refactor
- uart_controller_write_bytes(uart, (const uint8_t*)"$PAIR066,1,0,1,0,0,1*3B\r\n", 25, 250); // Enable GPS+GALILEO+NAVIC + constexpr char kEnableGnss[] = "$PAIR066,1,0,1,0,0,1*3B\r\n"; + uart_controller_write_bytes(uart, (const uint8_t*)kEnableGnss, sizeof(kEnableGnss) - 1, 250); // Enable GPS+GALILEO+NAVIC
Devices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dts
Outdated
Show resolved
Hide resolved
Devices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/hal/gps/GpsDevice.cpp (1)
46-63:⚠️ Potential issue | 🔴 CriticalUART resource leak on early-return paths after
open.If
uart_controller_opensucceeds (line 40) butprobe(line 48) orinit(line 59) fails,threadMainreturns -1 without callinguart_controller_close. This leaves the ESP UART driver installed and the device marked as open, preventing future re-opens.🐛 Proposed fix — close UART before early returns
GpsModel model = configuration.model; if (model == GpsModel::Unknown) { model = probe(uart); if (model == GpsModel::Unknown) { LOGGER.error("Probe failed"); setState(State::Error); + uart_controller_close(uart); return -1; } } mutex.lock(); this->model = model; mutex.unlock(); if (!init(uart, model)) { LOGGER.error("Init failed"); setState(State::Error); + uart_controller_close(uart); return -1; }
🧹 Nitpick comments (2)
Platforms/PlatformEsp32/Source/drivers/esp32_uart.cpp (2)
32-33: Parenthesize macro parameterdeviceinGET_CONFIG.Same pattern as the SPI driver —
device->configshould be(device)->configto be safe against complex macro arguments.♻️ Proposed fix
-#define GET_CONFIG(device) ((Esp32UartConfig*)device->config) -#define GET_DATA(device) ((Esp32UartInternal*)device_get_driver_data(device)) +#define GET_CONFIG(device) ((Esp32UartConfig*)(device)->config) +#define GET_DATA(device) ((Esp32UartInternal*)device_get_driver_data(device))
274-290:uart_driver_deletereturn value is unchecked.If
uart_driver_deletefails (e.g., due to pending I/O), the driver state is still marked as closed. Consider logging or propagating the error.♻️ Suggested improvement
- uart_driver_delete(dts_config->port); - driver_data->is_open = false; - unlock(driver_data); + esp_err_t err = uart_driver_delete(dts_config->port); + if (err != ESP_OK) { + LOG_E(TAG, "Failed to delete UART driver: %s", esp_err_to_name(err)); + unlock(driver_data); + return esp_err_to_error(err); + } + driver_data->is_open = false; + unlock(driver_data);
Devices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/hal/gps/GpsInit.cpp (1)
81-93:⚠️ Potential issue | 🔴 CriticalBug:
startTimeis in milliseconds but the loop compares it against ticks.Line 81 captures
startTimeviakernel::getMillis()(milliseconds), but line 93 subtracts it fromkernel::getTicks()(ticks) and compares againstwaitTicks(also ticks). WhenconfigTICK_RATE_HZ != 1000, this produces an incorrect timeout — either exiting immediately or spinning far too long.Use milliseconds consistently, or ticks consistently:
🐛 Proposed fix (use ticks consistently)
- uint32_t startTime = kernel::getMillis(); uint8_t buffer[CAS_ACK_NACK_MSG_SIZE] = {0}; uint8_t bufferPos = 0; TickType_t waitTicks = pdMS_TO_TICKS(waitMillis); + TickType_t startTick = kernel::getTicks(); ... - while (kernel::getTicks() - startTime < waitTicks) { + while (kernel::getTicks() - startTick < waitTicks) {Note: if
kernel::getMillis()is still needed for the debug log on line 121, you can capture it separately or compute elapsed from ticks.
🧹 Nitpick comments (3)
TactilityKernel/Include/tactility/device.h (1)
262-276: New lookup APIs look good; minor note ondevice_find_by_namelifetime guarantees.Both new functions are well-documented and the implementations correctly hold the ledger lock during iteration. One subtle point:
device_find_by_namereturns aDevice*after releasing the ledger lock, so callers face a TOCTOU window if the device can be concurrently removed. This is a common pattern in device registries so not necessarily a problem, but consider adding a brief doc note (e.g., "The caller must ensure the device is not removed concurrently, or lock the device before use") if this is a concern for your use cases.Tactility/Source/hal/gps/GpsInit.cpp (1)
94-97: Return values fromuart_controller_*calls are silently discarded throughout this file.All
uart_controller_write_bytes,uart_controller_read_byte, anduart_controller_get_availablecalls returnerror_t, but no call site checks the result. A failed write/read would go unnoticed, potentially causing silent GPS misconfiguration. Consider at least logging on error for the critical ACK-read path here, and optionally for the init write sequences.Tactility/Source/hal/gps/Ublox.cpp (1)
215-230: Minor: Log messages lost device context.Previously these warnings likely included baud rate info. The replacements on Lines 226 and 229 now just say "No GNSS Module" and "UBlox Frame Errors" without identifying which UART device. Consider logging the device name for easier debugging on boards with multiple UARTs.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Tactility/Source/hal/gps/Ublox.cpp (1)
185-195:⚠️ Potential issue | 🔴 CriticalCritical bug:
read_bytesis never assigned the result ofuart_controller_read_bytes, so the payload is always discarded.
auto read_bytes = 0U;is initialized to zero, and the return value ofuart_controller_read_bytes(anerror_t) is discarded on line 186. Consequently,read_bytes != needReadon line 187 is always true (whenneedRead > 0), causing every payload read to be treated as a failure. This function can never successfully return a parsed UBX response.The old code likely used a UART read that returned the byte count. The new
uart_controller_read_bytesreturnserror_t, so you need to check forERROR_NONEinstead of comparing byte counts.Additionally,
250 / portTICK_PERIOD_MSshould bepdMS_TO_TICKS(250)for correct and consistent ms-to-ticks conversion.🐛 Proposed fix
- auto read_bytes = 0U; - uart_controller_read_bytes(uart, buffer, needRead, 250 / portTICK_PERIOD_MS); - if (read_bytes != needRead) { + error_t err = uart_controller_read_bytes(uart, buffer, needRead, pdMS_TO_TICKS(250)); + if (err != ERROR_NONE) {Devices/m5stack-cardputer/Source/Configuration.cpp (1)
14-16:⚠️ Potential issue | 🟡 Minor
initBootis missing thestatickeyword.Every other device's
Configuration.cppdeclaresinitBootasstatic. Without it, this symbol has external linkage, which is inconsistent and could cause link-time symbol collisions.Proposed fix
-bool initBoot() { +static bool initBoot() { return driver::pwmbacklight::init(LCD_PIN_BACKLIGHT, 512); }
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
Platforms/PlatformEsp32/Source/drivers/esp32_uart.cpp (3)
32-33: Parenthesize macro parameter to prevent operator-precedence bugs.Same issue previously flagged in the SPI driver: if
GET_CONFIGorGET_DATAis ever invoked with an expression (e.g., pointer arithmetic), the->will bind incorrectly.Proposed fix
-#define GET_CONFIG(device) ((Esp32UartConfig*)device->config) -#define GET_DATA(device) ((Esp32UartInternal*)device_get_driver_data(device)) +#define GET_CONFIG(device) ((Esp32UartConfig*)(device)->config) +#define GET_DATA(device) ((Esp32UartInternal*)device_get_driver_data(device))Note:
GET_DATAis fine sincedeviceis passed as a function argument, butGET_CONFIGdirectly usesdevice->config.
326-340:stop()unlocks the mutex after clearing driver data, then deletes — potential use-after-free window.After
unlock(driver_data)on line 336, a concurrent thread that already obtained the pointer viaGET_DATA(before the lock) would unblock and operate on freed memory afterdelete driver_dataon line 338. This is a narrow race but exists ifstopand an I/O call overlap.If the lifecycle guarantees that
stopis only called when no other operations are in flight (similar to the SPI driver which doesn't lock at all instop), this is acceptable — but worth a brief comment documenting that precondition.
68-85: Mutex is held for the entire duration of blockinguart_read_bytes— all other device operations stall.
read_byteandread_byteshold the driver mutex acrossuart_read_bytes(…, timeout), which can block for the full timeout period. During that time,close(),get_available(),is_open(), etc. will all block. This is a deliberate correctness-vs-concurrency tradeoff, but for long timeouts it can cause unexpected stalls. Consider documenting this behavior or using a separate "open guard" (e.g., an atomic flag) for the open-state check so that the mutex only serializes lifecycle transitions.Also applies to: 148-166
Platforms/PlatformEsp32/Source/drivers/esp32_spi.cpp (1)
93-105:stop()does not lock the mutex before accessingdriver_data.Unlike the UART driver's
stop()which acquires the lock, this one directly accessesdriver_data->initializedwithout locking. This is consistent with the pattern thatstopis only called during exclusive lifecycle teardown, but it's inconsistent with the UART driver. Consider aligning the two for maintainability, or adding a brief comment noting the lifecycle precondition.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Platforms/PlatformEsp32/Source/drivers/esp32_spi.cpp (1)
111-120:constqualifier is silently cast away on line 116.
esp32_spi_apiis declaredconst staticbut is cast tovoid*, discardingconst. If theDriver::apifield can beconst void*, that would be preferable. If not, consider droppingconstfrom the declaration to avoid the inconsistency.Platforms/PlatformEsp32/Source/drivers/esp32_uart.cpp (2)
35-36: Consider inline functions instead oflock/unlockmacros.Function-like macros that expand to statements (
mutex_lock,mutex_unlock) can cause surprises with scoping and debuggability.static inlinehelpers would give the same zero-overhead behavior with type safety and proper debugger stepping.Example
-#define lock(data) mutex_lock(&data->mutex) -#define unlock(data) mutex_unlock(&data->mutex) +static inline void lock(Esp32UartInternal* data) { mutex_lock(&data->mutex); } +static inline void unlock(Esp32UartInternal* data) { mutex_unlock(&data->mutex); }
236-236: RX buffer is 1024 bytes with TX buffer at 0 (blocking TX) — consider a brief comment.
uart_driver_install(dts_config->port, 1024, 0, ...)uses a 1 KiB RX ring buffer and blocking TX (no TX buffer). This is a reasonable default, but a one-line comment would prevent future confusion about the intentional choice (especially since a past review iteration had an inaccurate comment here).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/hal/gps/Ublox.cpp (1)
19-26:⚠️ Potential issue | 🔴 CriticalCritical: Macro
SEND_UBX_PACKETreferences undefined symbolTIMEOUT.Line 23 calls
getAck(UART, TYPE, ID, TIMEOUT)butTIMEOUTis never defined anywhere in the codebase. The macro parameterTIMEOUT_MILLISis converted and passed touart_controller_write_bytes(line 22), but the undefinedTIMEOUTsymbol is passed togetAckinstead, causing a compilation error.Fix: Pass
TIMEOUT_MILLIStogetAckas well:if (getAck(UART, TYPE, ID, TIMEOUT_MILLIS) != GpsResponse::Ok) {Alternatively, if different timeouts are intended for the write and read operations, add a second parameter to the macro for the ACK timeout.
🧹 Nitpick comments (3)
Platforms/PlatformEsp32/Source/drivers/esp32_uart.cpp (1)
236-236: Consider documenting the RX buffer size choice.The RX buffer of 1024 bytes and TX buffer of 0 (blocking mode) are reasonable defaults, but the rationale isn't documented. A brief inline comment (e.g., why 1024 and not larger) would help future maintainers.
Tactility/Source/hal/gps/Ublox.cpp (2)
92-95: Uncheckederror_treturns fromuart_controller_get_availableanduart_controller_read_byte.Both calls discard their
error_treturn values. Forget_available, the fallback is safe (availablestays 0). Forread_byte, a silent failure leavesbwith stale data that feeds into the ACK state machine, potentially producing a false positive match. Consider at minimum logging or resettingackon failure.The same pattern repeats in the second
getAckoverload (lines 139–143).
61-67: Tick/ms unit consistency is improved but debug log on line 88 still mixes units.
startTimeis now correctly assigned fromkernel::getTicks()(line 66). However, the debug log on line 88 computeskernel::getMillis() - startTime, mixing milliseconds with a tick-based start time. This is numerically correct only whenconfigTICK_RATE_HZ == 1000. Since line 88 is in a#ifdef GPS_DEBUGblock and unchanged, this is very low priority.Also applies to: 85-85
1 participant
Uh oh!
There was an error while loading. Please reload this page.