Bluetooth#518
Conversation
helps a little on S3 (t-deck)
Fixes external app start/stop server (child devices) Fixes BtManage causing a full system hang upon disabling bt when a device is connected to the host.
This reverts commit d369436.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces comprehensive Bluetooth Low Energy (BLE) support via NimBLE across the platform. Changes include: device-tree and configuration updates for Bluetooth-capable devices (lilygo-tdeck, m5stack-tab5), kernel-level Bluetooth driver interfaces and wrapper implementations, an ESP32-specific NimBLE driver with support for BLE scanning, pairing, HID host/device, Serial, and MIDI profiles, new user-facing Bluetooth management applications, LVGL icon definitions and status bar integration, and build system updates to conditionally include Bluetooth components. Settings persistence enables Bluetooth state configuration. Device-tree bindings define the BLE hardware node. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
Tactility/Source/lvgl/Keyboard.cpp-35-35 (1)
35-35:⚠️ Potential issue | 🟠 MajorAdd
software_keyboard_deactivate()before deleting keyboard group in GuiService.At Keyboard.cpp lines 56-57,
pending_keyboard_groupis reused without validation when hardware keyboard reconnects. If GuiService deletes the keyboard group (GuiService.cpp line 215) without first callingsoftware_keyboard_deactivate(), the pointer becomes stale, causing a crash on hardware reconnect.The group lifecycle is not protected by widget cleanup alone—the group itself can be deleted while
pending_keyboard_groupstill references it. Ensuresoftware_keyboard_deactivate()is called beforelv_group_delete(keyboardGroup)in GuiService::stop().Also applies to: 56-57
Tactility/Source/Tactility.cpp-161-162 (1)
161-162:⚠️ Potential issue | 🟠 MajorGate the Bluetooth-only apps like the other optional hardware apps.
These manifests are registered unconditionally, unlike the USB/screenshot/chat/GPS entries around them. On builds where the Bluetooth stack is off, they will still be exposed from the app registry even though the feature is unavailable.
🛠️ Suggested fix
+#if defined(CONFIG_BT_NIMBLE_ENABLED) addAppManifest(app::btmanage::manifest); addAppManifest(app::btpeersettings::manifest); +#endifTactility/Source/bluetooth/BluetoothMidi.cpp-22-26 (1)
22-26:⚠️ Potential issue | 🟠 MajorPersist the MIDI disable request before checking for the child device.
Line 24 returns before
setMidiAutoStart(false)runs. If the device is already absent, the user can disable MIDI and still have it auto-start again when Bluetooth comes back.🛠️ Suggested fix
void midiStop() { - struct Device* dev = bluetooth_midi_get_device(); - if (dev == nullptr) return; settings::setMidiAutoStart(false); + struct Device* dev = bluetooth_midi_get_device(); + if (dev == nullptr) return; bluetooth_midi_stop(dev); }Tactility/Source/bluetooth/BluetoothSpp.cpp-22-26 (1)
22-26:⚠️ Potential issue | 🟠 MajorPersist the disable action even when the SPP child device is already gone.
Line 24 returns before
setSppAutoStart(false)runs. If the device disappears first, a user can still disable SPP and then have it auto-start again on the next Bluetooth restart.🛠️ Suggested fix
void sppStop() { - struct Device* dev = bluetooth_serial_get_device(); - if (dev == nullptr) return; settings::setSppAutoStart(false); + struct Device* dev = bluetooth_serial_get_device(); + if (dev == nullptr) return; bluetooth_serial_stop(dev); }Tactility/Source/service/statusbar/Statusbar.cpp-164-167 (1)
164-167:⚠️ Potential issue | 🟠 MajorInclude HID links in the Bluetooth “connected” state.
connectedis derived only from SPP and MIDI here. HID host/device sessions added in this PR can be active while the status bar still shows the idle/searching icon, so the Bluetooth indicator becomes misleading. Prefer a profile-agnostic “any Bluetooth connection up” check in this path.Tactility/Source/bluetooth/BluetoothSettings.cpp-67-72 (1)
67-72:⚠️ Potential issue | 🟠 MajorSave a snapshot taken under the mutex, not the unlocked global cache.
Each setter releases
settings_mutexand then callssave(cached). Another thread can modifycachedbetween those two steps, so the persisted flags can come from a different update than the caller just made.🔒 Suggested fix
void setEnableOnBoot(bool enable) { + BluetoothSettings snapshot; settings_mutex.lock(); cached.enableOnBoot = enable; cached_valid = true; + snapshot = cached; settings_mutex.unlock(); - if (!save(cached)) LOGGER.error("Failed to save"); + if (!save(snapshot)) LOGGER.error("Failed to save"); } void setSppAutoStart(bool enable) { + BluetoothSettings snapshot; settings_mutex.lock(); cached.sppAutoStart = enable; cached_valid = true; + snapshot = cached; settings_mutex.unlock(); - if (!save(cached)) LOGGER.error("Failed to save (setSppAutoStart)"); + if (!save(snapshot)) LOGGER.error("Failed to save (setSppAutoStart)"); } void setMidiAutoStart(bool enable) { + BluetoothSettings snapshot; settings_mutex.lock(); cached.midiAutoStart = enable; cached_valid = true; + snapshot = cached; settings_mutex.unlock(); - if (!save(cached)) LOGGER.error("Failed to save (setMidiAutoStart)"); + if (!save(snapshot)) LOGGER.error("Failed to save (setMidiAutoStart)"); }Also applies to: 79-84, 91-96
Tactility/Source/bluetooth/BluetoothSettings.cpp-33-35 (1)
33-35:⚠️ Potential issue | 🟠 MajorDon’t make the new
enableOnBootkey mandatory when loading old settings.
enableOnBootis introduced in this PR. Returningfalsewhen it is absent makes older/data/service/bluetooth/settings.propertiesfiles unreadable, so existing SPP/MIDI preferences get reset during migration instead of being preserved.♻️ Suggested fix
- auto it = map.find(KEY_ENABLE_ON_BOOT); - if (it == map.end()) return false; - out.enableOnBoot = (it->second == "true"); + auto it = map.find(KEY_ENABLE_ON_BOOT); + out.enableOnBoot = (it != map.end() && it->second == "true");Tactility/Source/app/btpeersettings/BtPeerSettings.cpp-71-80 (1)
71-80:⚠️ Potential issue | 🟠 MajorFail closed when the paired-device record cannot be loaded.
If
bluetooth::settings::load(addrHex, device)fails,addrstays00:00:00:00:00:00andprofileIdstaysBT_PROFILE_HID_HOST, but the screen still enables connect/disconnect/forget and the auto-connect toggle. That can target the wrong peer and also persist a replacement record with the wrong profile. Either rebuild the state from a trusted source (e.g. parsedaddrHexplus peer metadata), or stop/disable this screen until the record is valid.Also applies to: 108-119
Tactility/Source/bluetooth/BluetoothPairedDevice.cpp-8-13 (1)
8-13:⚠️ Potential issue | 🟠 MajorMake the settings parser failure-safe.
load()returnsfalsefor most malformed properties, butstd::stoionprofileIdat line 73 can throw exceptions instead, breaking this error-handling contract. Corrupt.device.propertiesfiles could crash the caller. Switch to a non-throwing parse routine to maintain consistency—std::from_charsis already used elsewhere in the codebase and fully supported by the C++23 standard.🛠️ Suggested change
`#include` <dirent.h> +#include <charconv> `#include` <format> `#include` <iomanip> `#include` <sstream> `#include` <string> `#include` <cstdio> @@ static bool hexToAddr(const std::string& hex, std::array<uint8_t, 6>& addr) { if (hex.size() != 12) { LOGGER.error("hexToAddr() length mismatch: expected 12, got {}", hex.size()); return false; } - char buf[3] = { 0 }; for (int i = 0; i < 6; ++i) { - buf[0] = hex[i * 2]; - buf[1] = hex[i * 2 + 1]; - char* endptr = nullptr; - addr[i] = static_cast<uint8_t>(strtoul(buf, &endptr, 16)); - if (endptr != buf + 2) { - LOGGER.error("hexToAddr() invalid hex at byte {}: '{}{}'", i, buf[0], buf[1]); + const char* begin = hex.data() + (i * 2); + const char* end = begin + 2; + unsigned value = 0; + const auto [ptr, ec] = std::from_chars(begin, end, value, 16); + if (ec != std::errc{} || ptr != end || value > 0xFF) { + LOGGER.error("hexToAddr() invalid hex at byte {}: '{}{}'", i, begin[0], begin[1]); return false; } + addr[i] = static_cast<uint8_t>(value); } return true; } @@ if (map.contains(KEY_PROFILE_ID)) { - device.profileId = std::stoi(map[KEY_PROFILE_ID]); + const auto& raw = map[KEY_PROFILE_ID]; + int value = 0; + const auto [ptr, ec] = std::from_chars(raw.data(), raw.data() + raw.size(), value); + if (ec != std::errc{} || ptr != raw.data() + raw.size()) { + LOGGER.error("Invalid profileId '{}'", raw); + return false; + } + device.profileId = value; }Also applies to: 35–51
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cpp-174-177 (1)
174-177:⚠️ Potential issue | 🟠 MajorDon't drop the unread tail on partial SPP reads.
If
max_lenis smaller than the queued packet, this pops the whole frame after copying only the prefix. Any larger NUS packet gets truncated permanently.Possible fix
auto& front = sctx->rx_queue.front(); size_t copy_len = std::min(front.size(), max_len); memcpy(data, front.data(), copy_len); - sctx->rx_queue.pop_front(); + if (copy_len == front.size()) { + sctx->rx_queue.pop_front(); + } else { + front.erase(front.begin(), front.begin() + copy_len); + }Tactility/Source/bluetooth/Bluetooth.cpp-124-134 (1)
124-134:⚠️ Potential issue | 🟠 MajorDon't make the radio-on auto-start branches mutually exclusive.
Right now only the first matching branch runs. A user with an auto-connect HID host peer and SPP/MIDI auto-start enabled will never get the peripheral servers started after radio-on.
Possible fix
if (has_hid_auto) { LOGGER.info("HID host auto-connect peer found — starting scan"); if (struct Device* dev = findFirstDevice()) { bluetooth_scan_start(dev); } - } else if (settings::shouldSppAutoStart()) { + } + if (settings::shouldSppAutoStart()) { LOGGER.info("Auto-starting SPP server"); sppStart(); - } else if (settings::shouldMidiAutoStart()) { + } + if (settings::shouldMidiAutoStart()) { LOGGER.info("Auto-starting MIDI server"); midiStart(); }Tactility/Source/app/btmanage/View.cpp-194-199 (1)
194-199:⚠️ Potential issue | 🟠 MajorFilter paired peers out of the "Available" section.
Already-paired devices can render twice here. The duplicate row is wired as an unpaired scan result, so tapping it will try to pair again instead of opening peer settings.
Possible fix
auto scan_results = state->getScanResults(); + bool has_available = false; + for (const auto& record : scan_results) { + bool already_paired = false; + for (const auto& peer : paired) { + if (peer.addr == record.addr) { + already_paired = true; + break; + } + } + if (!already_paired) { + has_available = true; + break; + } + } lv_list_add_text(peers_list, "Available"); - if (!scan_results.empty()) { + if (has_available) { for (size_t i = 0; i < scan_results.size(); ++i) { + bool already_paired = false; + for (const auto& peer : paired) { + if (peer.addr == scan_results[i].addr) { + already_paired = true; + break; + } + } + if (already_paired) continue; createPeerListItem(scan_results[i], false, i); } } else if (!state->isScanning()) {Tactility/Source/bluetooth/BluetoothHidHost.cpp-144-165 (1)
144-165:⚠️ Potential issue | 🟠 MajorRelease events need to reuse the translated key from the original press.
Line 150 remaps releases with
mod == 0, so shifted letters andShift+Tabrelease as a different LVGL key than they pressed ('A'→'a',LV_KEY_PREV→LV_KEY_NEXT). That can leave keys stuck or drive the wrong navigation path.Possible fix
static QueueHandle_t hid_host_key_queue = nullptr; static uint8_t hid_host_prev_keys[6] = {}; +static std::array<uint32_t, 256> hid_host_pressed_lv_keys = {}; ... for (int i = 0; i < 6; i++) { uint8_t kc = hid_host_prev_keys[i]; if (kc == 0) continue; bool still = false; for (int j = 0; j < nkeys; j++) { if (curr[j] == kc) { still = true; break; } } if (!still) { - uint32_t lv = hidHostMapKeycode(0, kc); - if (lv) { HidHostKeyEvt e{lv, false}; xQueueSend(hid_host_key_queue, &e, 0); } + uint32_t lv = hid_host_pressed_lv_keys[kc]; + if (lv) { + HidHostKeyEvt e{lv, false}; + xQueueSend(hid_host_key_queue, &e, 0); + hid_host_pressed_lv_keys[kc] = 0; + } } } for (int i = 0; i < nkeys; i++) { uint8_t kc = curr[i]; if (kc == 0) continue; bool had = false; for (int j = 0; j < 6; j++) { if (hid_host_prev_keys[j] == kc) { had = true; break; } } if (!had) { uint32_t lv = hidHostMapKeycode(mod, kc); - if (lv) { HidHostKeyEvt e{lv, true}; xQueueSend(hid_host_key_queue, &e, 0); } + if (lv) { + hid_host_pressed_lv_keys[kc] = lv; + HidHostKeyEvt e{lv, true}; + xQueueSend(hid_host_key_queue, &e, 0); + } } }Also clear
hid_host_pressed_lv_keysin the connection reset/disconnect path.Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_scan.cpp-243-248 (1)
243-248:⚠️ Potential issue | 🟠 MajorCheck the return value of
ble_hs_id_infer_auto()before usingown_addr_type.According to Apache NimBLE documentation, the output parameter is only valid when
ble_hs_id_infer_auto()returns 0. A non-zero return leavesown_addr_typeundefined, yet Line 247 passes it toble_gap_connect(), causing undefined behavior. Initialize a safe default and check the return value, or skip name resolution for this peer on failure.Possible fix
- uint8_t own_addr_type; - ble_hs_id_infer_auto(0, &own_addr_type); + uint8_t own_addr_type = BLE_OWN_ADDR_PUBLIC; + if (ble_hs_id_infer_auto(0, &own_addr_type) != 0) { + LOG_W(TAG, "Name resolution: own addr inference failed, skipping idx=%u", (unsigned)i); + ++i; + continue; + } void* idx_arg = (void*)(uintptr_t)i; int rc = ble_gap_connect(own_addr_type, &addr, 1500, nullptr, name_res_gap_callback, idx_arg);Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cpp-403-407 (1)
403-407:⚠️ Potential issue | 🟠 Major
BleHidDeviceCtxleaks after stop-on-connected sessions.The connected branch in
hid_device_stop()intentionally keepshid_ctxaround for the disconnect handler, but nothing releases it once that disconnect completes. A later start then overwritesdevice_get_driver_data()with a new allocation and the previous session context is leaked.Also applies to: 437-457
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cpp-343-363 (1)
343-363:⚠️ Potential issue | 🟠 MajorFail HID startup when the profile switch fails.
ble_hid_switch_profile()has several early-return failure paths, buthid_device_start()still setshid_activeand starts HID advertising afterward. That can leave the advertised role out of sync with the live GATT database.Also applies to: 430-434
TactilityKernel/include/tactility/drivers/bluetooth.h-44-52 (1)
44-52:⚠️ Potential issue | 🟠 MajorCarry BLE address type through the public API.
BtPeerRecordkeepsaddr_type, but the operational APIs and pairing/profile event payloads drop it and only expose the raw 6-byte address. The ESP32 backend already has to guessBLE_ADDR_PUBLICwhen unpairing, so random-address peers cannot be matched or removed reliably.Also applies to: 107-124, 184-219, 293-296
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp-828-853 (1)
828-853:⚠️ Potential issue | 🟠 MajorReject profile operations until the radio is fully on.
api_connect()andapi_disconnect()jump straight into advertising/GATT helpers even when the radio isOFF,ON_PENDING, or already shutting down. A caller that races enable/disable can end up touching NimBLE before init or after deinit.Suggested guard
static error_t api_connect(struct Device* device, const BtAddr addr, enum BtProfileId profile) { BleCtx* ctx = (BleCtx*)device_get_driver_data(device); if (!ctx) return ERROR_INVALID_STATE; + if (ctx->radio_state.load() != BT_RADIO_STATE_ON) return ERROR_INVALID_STATE; if (profile == BT_PROFILE_HID_DEVICE) { return nimble_hid_device_api.start(ctx->hid_device_child, BT_HID_DEVICE_MODE_KEYBOARD); } else if (profile == BT_PROFILE_SPP) { return ble_spp_start_internal(ctx->serial_child); } else if (profile == BT_PROFILE_MIDI) { @@ static error_t api_disconnect(struct Device* device, const BtAddr addr, enum BtProfileId profile) { BleCtx* ctx = (BleCtx*)device_get_driver_data(device); if (!ctx) return ERROR_INVALID_STATE; + if (ctx->radio_state.load() != BT_RADIO_STATE_ON) return ERROR_INVALID_STATE; if (profile == BT_PROFILE_HID_DEVICE) { return nimble_hid_device_api.stop(ctx->hid_device_child); } else if (profile == BT_PROFILE_SPP) { return nimble_serial_api.stop(ctx->serial_child); } else if (profile == BT_PROFILE_MIDI) {Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp-543-544 (1)
543-544:⚠️ Potential issue | 🟠 MajorUse the inferred own-address type when starting advertising.
Both advertising helpers hardcode
BLE_OWN_ADDR_PUBLICinble_gap_adv_start()calls (lines 543-544 and 606-607), while the codebase already usesble_hs_id_infer_auto()elsewhere (e.g., in the discovery code). This hardcoding breaks targets/configurations that advertise with a random or static identity address, and contradicts Apache NimBLE best practices which require inferring the address type based on the device's configured identity addresses. Replace the hardcodedBLE_OWN_ADDR_PUBLICwithown_addr_typeobtained fromble_hs_id_infer_auto(0, &own_addr_type).Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp-744-750 (1)
744-750:⚠️ Potential issue | 🟠 MajorUse the recursive mutex API consistently for
radio_mutex.Line 1062 creates a recursive mutex with
xSemaphoreCreateRecursiveMutex(), but all call sites usexSemaphoreTake()/xSemaphoreGive(). FreeRTOS requires that recursive mutexes usexSemaphoreTakeRecursive()/xSemaphoreGiveRecursive()instead; using the non-recursive API on a recursive mutex violates the API contract and will cause ownership tracking failures or assertions.Affected locations:
- Lines 744–750
- Lines 893–902
- Lines 909–912
Suggested fix
Replace all instances:
- xSemaphoreTake(ctx->radio_mutex, portMAX_DELAY); + xSemaphoreTakeRecursive(ctx->radio_mutex, portMAX_DELAY); ... - xSemaphoreGive(ctx->radio_mutex); + xSemaphoreGiveRecursive(ctx->radio_mutex);
🟡 Minor comments (4)
device.py-318-319 (1)
318-319:⚠️ Potential issue | 🟡 MinorRemove the unnecessary
fprefixes.Lines 318-319 use f-string literals without any placeholders, triggering Ruff's F541 rule. These should be plain string literals.
Suggested fix
- output_file.write(f"CONFIG_BT_NIMBLE_TRANSPORT_UART=n\n") - output_file.write(f"CONFIG_ESP_HOSTED_ENABLE_BT_NIMBLE=y\n") + output_file.write("CONFIG_BT_NIMBLE_TRANSPORT_UART=n\n") + output_file.write("CONFIG_ESP_HOSTED_ENABLE_BT_NIMBLE=y\n")Tactility/Source/bluetooth/README.md-17-19 (1)
17-19:⚠️ Potential issue | 🟡 MinorFix the kernel header path in the architecture block.
Line 18 uses
TactilityKernel/drivers/bluetooth.h, which is inconsistent with the include path used elsewhere (tactility/drivers/bluetooth.h/TactilityKernel/include/tactility/drivers/bluetooth.h). Keeping this exact avoids confusion during integration.Suggested doc fix
- │ (TactilityKernel/drivers/bluetooth.h + bluetooth_serial/midi/hid_device.h) + │ (TactilityKernel/include/tactility/drivers/bluetooth.h + + │ tactility/drivers/bluetooth_serial.h, tactility/drivers/bluetooth_midi.h, + │ tactility/drivers/bluetooth_hid_device.h)Tactility/Include/Tactility/bluetooth/BluetoothPairedDevice.h-14-16 (1)
14-16:⚠️ Potential issue | 🟡 MinorUse
BT_PROFILE_SPPconstant instead of magic number2.The literal default value
2is brittle and duplicates the enum value. Use theBT_PROFILE_SPPconstant for clarity and forward compatibility. Theinttype is intentional here—the Tactility wrapper layer usesint profileIdconsistently across all bluetooth APIs and for serialization/deserialization support (seeBluetoothPairedDevice.cppwhere profileId is parsed/saved as an integer string).Suggested direction
- /** Profile used to pair (BtProfileId value). Defaults to BT_PROFILE_SPP=2. */ - int profileId = 2; + /** Profile used to pair (BtProfileId value). Defaults to BT_PROFILE_SPP. */ + int profileId = BT_PROFILE_SPP;Tactility/Source/app/btmanage/BtManage.cpp-90-92 (1)
90-92:⚠️ Potential issue | 🟡 MinorClear the app-state scan list when a new scan starts.
The driver cache is reset on
BT_EVENT_SCAN_STARTED, but the app state is not. The view therefore keeps showing the previous scan's devices until the first newBT_EVENT_PEER_FOUNDor scan completion.Possible fix
case BT_EVENT_SCAN_STARTED: getState().setScanning(true); + getState().updateScanResults(); break;
🧹 Nitpick comments (2)
Tactility/Private/Tactility/app/btmanage/Bindings.h (1)
15-22: Add in-class initialization to callback members to prevent misuse.The
Bindingsstruct definition lacks default initialization for function pointer members. While current instantiations inBtManagePrivate.h,WifiManagePrivate.h, andWifiConnect.huse aggregate initialization (= { }), which safely zero-initializes pointers, explicitly initializing struct members tonullptrwould prevent accidental misuse if the struct is ever instantiated without proper initialization in the future.Proposed fix
struct Bindings { - OnBtToggled onBtToggled; - OnScanToggled onScanToggled; - OnConnectPeer onConnectPeer; - OnDisconnectPeer onDisconnectPeer; - OnPairPeer onPairPeer; - OnForgetPeer onForgetPeer; + OnBtToggled onBtToggled = nullptr; + OnScanToggled onScanToggled = nullptr; + OnConnectPeer onConnectPeer = nullptr; + OnDisconnectPeer onDisconnectPeer = nullptr; + OnPairPeer onPairPeer = nullptr; + OnForgetPeer onForgetPeer = nullptr; };Platforms/platform-esp32/source/module.cpp (1)
1-3: Remove the#ifdef ESP_PLATFORMguard aroundsdkconfig.h.The
Platforms/platform-esp32directory is compiled only for ESP targets by the build system, so this guard is redundant and creates an unnecessary alternate preprocessing path around a required ESP-IDF header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0ecf1bb-a02e-4cd5-b69a-e20569c6d9fe
📒 Files selected for processing (73)
Data/data/service/bluetooth/settings.propertiesDevices/lilygo-tdeck/device.propertiesDevices/lilygo-tdeck/lilygo,tdeck.dtsDevices/m5stack-tab5/device.propertiesDevices/m5stack-tab5/m5stack,tab5.dtsFirmware/CMakeLists.txtModules/lvgl-module/assets/generate-all.pyModules/lvgl-module/include/tactility/lvgl_icon_shared.hModules/lvgl-module/include/tactility/lvgl_icon_statusbar.hModules/lvgl-module/source-fonts/material_symbols_shared_12.cModules/lvgl-module/source-fonts/material_symbols_shared_16.cModules/lvgl-module/source-fonts/material_symbols_shared_20.cModules/lvgl-module/source-fonts/material_symbols_shared_24.cModules/lvgl-module/source-fonts/material_symbols_shared_32.cModules/lvgl-module/source-fonts/material_symbols_statusbar_12.cModules/lvgl-module/source-fonts/material_symbols_statusbar_16.cModules/lvgl-module/source-fonts/material_symbols_statusbar_20.cModules/lvgl-module/source-fonts/material_symbols_statusbar_30.cModules/lvgl-module/source/symbols.cPlatforms/platform-esp32/CMakeLists.txtPlatforms/platform-esp32/bindings/esp32,ble-nimble.yamlPlatforms/platform-esp32/include/tactility/bindings/esp32_ble_nimble.hPlatforms/platform-esp32/include/tactility/drivers/esp32_ble_nimble.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_hid.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_internal.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_midi.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_spp.hPlatforms/platform-esp32/source/drivers/bluetooth/README.mdPlatforms/platform-esp32/source/drivers/bluetooth/bluetooth.pumlPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_midi.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_scan.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cppPlatforms/platform-esp32/source/module.cppTactility/CMakeLists.txtTactility/Include/Tactility/app/btmanage/BtManage.hTactility/Include/Tactility/bluetooth/Bluetooth.hTactility/Include/Tactility/bluetooth/BluetoothPairedDevice.hTactility/Include/Tactility/bluetooth/BluetoothSettings.hTactility/Private/Tactility/app/btmanage/Bindings.hTactility/Private/Tactility/app/btmanage/BtManagePrivate.hTactility/Private/Tactility/app/btmanage/State.hTactility/Private/Tactility/app/btmanage/View.hTactility/Private/Tactility/app/btpeersettings/BtPeerSettings.hTactility/Private/Tactility/bluetooth/BluetoothPrivate.hTactility/Source/Tactility.cppTactility/Source/app/btmanage/BtManage.cppTactility/Source/app/btmanage/State.cppTactility/Source/app/btmanage/View.cppTactility/Source/app/btpeersettings/BtPeerSettings.cppTactility/Source/bluetooth/Bluetooth.cppTactility/Source/bluetooth/BluetoothHidDevice.cppTactility/Source/bluetooth/BluetoothHidHost.cppTactility/Source/bluetooth/BluetoothMidi.cppTactility/Source/bluetooth/BluetoothMock.cppTactility/Source/bluetooth/BluetoothPairedDevice.cppTactility/Source/bluetooth/BluetoothSettings.cppTactility/Source/bluetooth/BluetoothSpp.cppTactility/Source/bluetooth/README.mdTactility/Source/lvgl/Keyboard.cppTactility/Source/service/statusbar/Statusbar.cppTactilityC/Source/tt_init.cppTactilityKernel/include/tactility/drivers/bluetooth.hTactilityKernel/include/tactility/drivers/bluetooth_hid_device.hTactilityKernel/include/tactility/drivers/bluetooth_midi.hTactilityKernel/include/tactility/drivers/bluetooth_serial.hTactilityKernel/source/drivers/bluetooth.cppTactilityKernel/source/drivers/bluetooth_hid_device.cppTactilityKernel/source/drivers/bluetooth_midi.cppTactilityKernel/source/drivers/bluetooth_serial.cppTactilityKernel/source/kernel_symbols.cdevice.py
KenVanHoeylandt
left a comment
KenVanHoeylandt
left a comment
There was a problem hiding this comment.
Great job! I've finished my feedback.
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ca15668-7dd7-4de1-86e8-25bfe74e78c4
📒 Files selected for processing (15)
Devices/lilygo-tdeck/lilygo,tdeck.dtsDevices/m5stack-tab5/m5stack,tab5.dtsFirmware/CMakeLists.txtPlatforms/platform-esp32/bindings/esp32,ble.yamlPlatforms/platform-esp32/include/tactility/bindings/esp32_ble.hPlatforms/platform-esp32/include/tactility/drivers/esp32_ble.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_hid.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_internal.hPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppTactility/Include/Tactility/bluetooth/BluetoothPairedDevice.hTactility/Source/bluetooth/Bluetooth.cppTactility/Source/bluetooth/BluetoothPairedDevice.cppTactilityKernel/include/tactility/drivers/bluetooth.hTactilityKernel/source/drivers/bluetooth.cppTactilityKernel/source/kernel_symbols.c
✅ Files skipped from review due to trivial changes (6)
- Platforms/platform-esp32/bindings/esp32,ble.yaml
- Devices/m5stack-tab5/m5stack,tab5.dts
- Platforms/platform-esp32/include/tactility/drivers/esp32_ble.h
- Firmware/CMakeLists.txt
- Platforms/platform-esp32/private/bluetooth/esp32_ble_hid.h
- Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- Devices/lilygo-tdeck/lilygo,tdeck.dts
- Tactility/Source/bluetooth/Bluetooth.cpp
- TactilityKernel/source/kernel_symbols.c
- Platforms/platform-esp32/private/bluetooth/esp32_ble_internal.h
| if (map.contains(KEY_PROFILE_ID)) { | ||
| device.profileId = std::stoi(map[KEY_PROFILE_ID]); | ||
| } |
There was a problem hiding this comment.
Guard profileId parsing from invalid persisted values.
Line 73 uses std::stoi without validation. A malformed properties value can throw and break loading flow.
Proposed fix
if (map.contains(KEY_PROFILE_ID)) {
- device.profileId = std::stoi(map[KEY_PROFILE_ID]);
+ try {
+ device.profileId = std::stoi(map[KEY_PROFILE_ID]);
+ } catch (...) {
+ LOGGER.error("Invalid profileId '{}'", map[KEY_PROFILE_ID]);
+ return false;
+ }
}📝 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.
| if (map.contains(KEY_PROFILE_ID)) { | |
| device.profileId = std::stoi(map[KEY_PROFILE_ID]); | |
| } | |
| if (map.contains(KEY_PROFILE_ID)) { | |
| try { | |
| device.profileId = std::stoi(map[KEY_PROFILE_ID]); | |
| } catch (...) { | |
| LOGGER.error("Invalid profileId '{}'", map[KEY_PROFILE_ID]); | |
| return false; | |
| } | |
| } |
There was a problem hiding this comment.
@Shadowtrance Can you can add a // TODO: Handle incorrect parsing input?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp (1)
1028-1037:⚠️ Potential issue | 🔴 CriticalStop NimBLE before destroying child devices to prevent use-after-free.
The child
Device*pointers (serial_child,midi_child,hid_device_child) are stored in GATT callback args (e.g.,nus_chars_with_handle[].arg) and timer payloads. Destroying children while NimBLE is still running creates a race where callbacks can dereference freed memory.Suggested fix: move dispatch_disable before child destruction
static error_t esp32_ble_stop_device(struct Device* device) { BleCtx* ctx = (BleCtx*)device_get_driver_data(device); if (!ctx) return ERROR_NONE; - // Destroy child devices before stopping the radio and freeing the context. - // device_stop() on each child will invoke stop_device (for serial/midi/hid) - // which frees their driver data. - destroy_child_device(ctx->hid_device_child); - destroy_child_device(ctx->midi_child); - destroy_child_device(ctx->serial_child); - if (ctx->radio_state.load() != BT_RADIO_STATE_OFF) { dispatch_disable(ctx); } + // Once NimBLE is stopped, no late GATT/timer callback can dereference child pointers. + destroy_child_device(ctx->hid_device_child); + destroy_child_device(ctx->midi_child); + destroy_child_device(ctx->serial_child); + if (ctx->scan_mutex != nullptr) {
🧹 Nitpick comments (1)
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp (1)
86-99: Consider removing the null check on ctx in internal event publishing.Per the codebase's Linux kernel style guideline, internal APIs should assume valid pointers. The
ble_publish_eventfunction is called from contexts wheredeviceand thusctxshould always be valid. The check at line 88 could mask bugs during development.Based on learnings: "Adopt Linux kernel style: do not add null pointer guards in internal APIs. Assume callers pass valid pointers; crashes on invalid pointers are bugs detected during development."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 784955cf-06e2-460c-8ae0-b16a384de123
📒 Files selected for processing (16)
Platforms/platform-esp32/private/bluetooth/esp32_ble_hid.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_internal.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_midi.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_spp.hPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_midi.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_scan.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cppPlatforms/platform-esp32/source/module.cppTactility/Include/Tactility/bluetooth/Bluetooth.hTactility/Source/bluetooth/Bluetooth.cppTactility/Source/bluetooth/BluetoothHidDevice.cppTactility/Source/bluetooth/BluetoothMidi.cppTactility/Source/bluetooth/BluetoothMock.cppTactility/Source/bluetooth/BluetoothSpp.cpp
✅ Files skipped from review due to trivial changes (3)
- Tactility/Source/bluetooth/BluetoothHidDevice.cpp
- Tactility/Source/bluetooth/BluetoothMidi.cpp
- Tactility/Source/bluetooth/BluetoothSpp.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- Platforms/platform-esp32/private/bluetooth/esp32_ble_spp.h
- Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_midi.cpp
- Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cpp
| } else if (event.profile_state.state == BT_PROFILE_STATE_IDLE && | ||
| event.profile_state.profile == BT_PROFILE_HID_HOST) { | ||
| // HID host disconnected — check if any peer has autoConnect and re-scan | ||
| // so that autoConnectHidHost() fires when the scan finishes. | ||
| getMainDispatcher().dispatch([] { | ||
| auto peers = settings::loadAll(); | ||
| bool has_auto = false; | ||
| for (const auto& p : peers) { | ||
| if (p.autoConnect && p.profileId == BT_PROFILE_HID_HOST) { | ||
| has_auto = true; | ||
| break; | ||
| } | ||
| } | ||
| if (has_auto) { | ||
| if (struct Device* dev = findFirstDevice()) { | ||
| if (!bluetooth_is_scanning(dev)) { | ||
| bluetooth_scan_start(dev); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Manual HID disconnects will immediately reconnect.
disconnect(..., BT_PROFILE_HID_HOST) drives the profile back to BT_PROFILE_STATE_IDLE, which lands here and starts a new scan whenever any HID-host peer still has autoConnect enabled. That makes “disconnect” non-sticky for auto-connect peers. Please gate this branch on disconnect reason or a one-shot suppression flag.
There was a problem hiding this comment.
If I understand this correctly, a manual disconnect will automatically reconnect in this scenario, while it should not do that temporarily until a manual connect is done again. (or until after reboot)
There was a problem hiding this comment.
@KenVanHoeylandt Yes, your understanding is exactly right. The flow is:
- User calls
disconnect(..., BT_PROFILE_HID_HOST)→hidHostDisconnect()is called. - NimBLE fires
BT_EVENT_PROFILE_STATE_CHANGEDwithBT_PROFILE_STATE_IDLEforBT_PROFILE_HID_HOST. - The bridge handler unconditionally starts a new scan if any peer has
autoConnect = true. - When the scan finishes,
autoConnectHidHost()reconnects the peer automatically.
The simplest fix is a static one-shot suppression flag, cleared on the next manual connect or on radio power-on:
// near the top of Bluetooth.cpp, alongside scan_cache_mutex
static std::atomic<bool> s_hid_host_manual_disconnect{false};In disconnect(), when routing to HID host:
void disconnect(const std::array<uint8_t, 6>& addr, int profileId) {
if (profileId == BT_PROFILE_HID_HOST) {
s_hid_host_manual_disconnect.store(true); // suppress auto-reconnect
hidHostDisconnect();
}
// ... other profiles
}Clear it on the next manual connect attempt (and on radio-on to handle reboots — it's already cleared implicitly since s_ is re-initialized):
void connect(const std::array<uint8_t, 6>& addr, int profileId) {
if (profileId == BT_PROFILE_HID_HOST) {
s_hid_host_manual_disconnect.store(false); // allow auto-reconnect again
hidHostConnect(addr);
}
// ...
}Gate the re-scan branch:
} else if (event.profile_state.state == BT_PROFILE_STATE_IDLE &&
event.profile_state.profile == BT_PROFILE_HID_HOST) {
if (s_hid_host_manual_disconnect.exchange(false)) {
// Manual disconnect — suppress auto-reconnect scan
break;
}
getMainDispatcher().dispatch([] {
// ... existing re-scan logic ...
});
}Using exchange(false) both reads and clears the flag atomically, so the suppression is truly one-shot (a subsequent unintentional disconnect, e.g. out-of-range, will still trigger the re-scan).
Would you like me to open a GitHub issue to track this fix, or shall I plan the code changes directly?
🧠 Learnings used
Learnt from: KenVanHoeylandt
Repo: TactilityProject/Tactility PR: 480
File: TactilityKernel/Source/drivers/i2s_controller.cpp:10-33
Timestamp: 2026-02-04T22:31:08.613Z
Learning: Adopt Linux kernel style: do not add null pointer guards in internal APIs. Assume callers pass valid pointers; crashes on invalid pointers are bugs detected during development. Apply this guideline to internal API implementations across the codebase (e.g., TactilityKernel/Source/drivers/*.cpp). Consider adding runtime comments or static analysis where preconditions are documented, but avoid defensive null checks in internal API paths.
Learnt from: NellowTCS
Repo: TactilityProject/Tactility PR: 521
File: Devices/cyd-2432s024r/Source/devices/Display.cpp:6-9
Timestamp: 2026-04-13T15:58:04.294Z
Learning: In ESP-IDF projects that use the `esp_lcd` component, don’t flag a type mismatch when a function expects `esp_lcd_spi_bus_handle_t` but the code passes a `spi_host_device_t` value such as `SPI2_HOST`. In ESP-IDF, `esp_lcd_spi_bus_handle_t` is a typedef alias compatible with `spi_host_device_t`, so this compiles and is type-correct—treat it as valid rather than an error.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp (1)
1062-1071:⚠️ Potential issue | 🔴 CriticalStop NimBLE before destroying the child devices.
The host task, GATT callback args, and BLE timers can still dereference
ctx->hid_device_child,ctx->midi_child, andctx->serial_childuntildispatch_disable()finishes. Tearing the children down first reintroduces a use-after-free window during shutdown.Suggested order
- // Destroy child devices before stopping the radio and freeing the context. - // device_stop() on each child will invoke stop_device (for serial/midi/hid) - // which frees their driver data. - destroy_child_device(ctx->hid_device_child); - destroy_child_device(ctx->midi_child); - destroy_child_device(ctx->serial_child); - if (ctx->radio_state.load() != BT_RADIO_STATE_OFF) { dispatch_disable(ctx); } + + // Once NimBLE is stopped, no late GATT/timer callback can dereference child pointers. + destroy_child_device(ctx->hid_device_child); + destroy_child_device(ctx->midi_child); + destroy_child_device(ctx->serial_child);Tactility/Source/bluetooth/Bluetooth.cpp (3)
127-147:⚠️ Potential issue | 🟠 MajorRestore persisted peripheral profiles independently after radio-up.
This
else ifchain still restores only one saved peripheral profile state. If HID device auto-start is enabled, SPP and MIDI are skipped entirely; if SPP is enabled, MIDI is skipped. Those settings are persisted independently, so they should not be first-match wins.
192-199:⚠️ Potential issue | 🟠 MajorKeep stored peer settings on
BT_PAIR_RESULT_BOND_LOST.This is the stale-bond repair flow, not an explicit forget. Removing the settings file here drops the saved name/profile/auto-connect state, and the later success path recreates the peer with defaults.
218-238:⚠️ Potential issue | 🟠 MajorManual HID-host disconnects still trigger auto-reconnect.
Any
BT_PROFILE_HID_HOSTtransition toIDLEstarts a new scan whenever an auto-connect HID-host peer exists, so a user-initiated disconnect is immediately undone. Gate this path on disconnect reason or a one-shot suppression flag.
🧹 Nitpick comments (1)
device.py (1)
310-313: Keep the Bluetooth gate consistent with the CMake gate.
write_bluetooth_variables()usesConfigParser, butFirmware/CMakeLists.txt:21-28still appendsbtonly when the file literally containsbluetooth=true. A valid entry likebluetooth = truewould enable NimBLE here without adding thebtcomponent, leaving the build config inconsistent. Consider sharing one parser or loosening the CMake match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5a11f02-3844-4848-aa01-57b2ca5ac5e7
📒 Files selected for processing (4)
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cppTactility/Source/bluetooth/Bluetooth.cppdevice.py
| static void adv_restart_callback(void* arg) { | ||
| struct Device* device = (struct Device*)arg; | ||
| BleCtx* ctx = ble_get_ctx(device); | ||
| if (ctx->radio_state.load() != BT_RADIO_STATE_ON) return; | ||
|
|
||
| uint16_t hid_conn = ble_hid_get_conn_handle(ctx->hid_device_child); | ||
|
|
||
| if (ctx->midi_active.load() && ctx->midi_conn_handle.load() == BLE_HS_CONN_HANDLE_NONE) { | ||
| ble_start_advertising(device, &MIDI_SVC_UUID); | ||
| } else if (ctx->spp_active.load() && ctx->spp_conn_handle.load() == BLE_HS_CONN_HANDLE_NONE) { | ||
| ble_start_advertising(device, &NUS_SVC_UUID); | ||
| } else if (ctx->hid_active.load() && hid_conn == BLE_HS_CONN_HANDLE_NONE) { | ||
| ble_start_advertising_hid(device, hid_appearance); | ||
| } |
There was a problem hiding this comment.
Restart name-only advertising after a rename.
When no profile is active, on_sync() starts plain name-only advertising (Lines 440-442), but adv_restart_callback() has no fallback branch for that state. api_set_device_name() therefore updates the GAP name without rebuilding the current advertising payload, so scanners keep seeing the old name until some later restart.
Also applies to: 918-930
There was a problem hiding this comment.
@Shadowtrance Legit issue, but a TODO to fix it later is fine by me.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| ctx->radio_mutex = xSemaphoreCreateRecursiveMutex(); | ||
| ctx->cb_mutex = xSemaphoreCreateMutex(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "radio_mutex|xSemaphoreCreateRecursiveMutex|xSemaphoreTakeRecursive|xSemaphoreGiveRecursive|xSemaphoreTake\(|xSemaphoreGive\(" Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppRepository: TactilityProject/Tactility
Length of output: 795
Use the recursive-mutex API consistently for radio_mutex.
radio_mutex is created with xSemaphoreCreateRecursiveMutex() at line 1004, but is taken/given with the non-recursive API (xSemaphoreTake()/xSemaphoreGive()) at lines 773–779, 922–931, and 938–941. This drops the recursive ownership semantics and can deadlock if any callback path re-enters a radio/name API on the same task. Either switch these call sites to xSemaphoreTakeRecursive()/xSemaphoreGiveRecursive() or create a plain mutex instead.
There was a problem hiding this comment.
Either switch these call sites to xSemaphoreTakeRecursive()/xSemaphoreGiveRecursive() or create a plain mutex instead.
This is a good fix. If a recursive mutex is needed then xSemaphoreTakeRecursive()/xSemaphoreGiveRecursive() should be used. Otherwise the mutex type can be changed to regular.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all radio_mutex usages and functions that might re-enter it
rg -n "radio_mutex|dispatch_enable|dispatch_disable|ble_schedule_adv_restart|api_set_radio_enabled|api_set_device_name|api_get_device_name" \
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp | head -80
Length of output: 1691
@KenVanHoeylandt, agreed. Looking at all the call chains, there are no re-entrant paths through radio_mutex:
api_set_radio_enabledholdsradio_mutexwhile callingdispatch_enable/dispatch_disable, and neither of those re-acquires it.api_set_device_nameholdsradio_mutexwhile callingble_schedule_adv_restart, which also doesn't touchradio_mutex.api_get_device_nameonly does astrncpyunder the lock.dispatch_disable_timer_cbcallsdispatch_disabledirectly (bypassingapi_set_radio_enabled) so it never tries to takeradio_mutex.
Since recursive semantics aren't needed, the cleanest fix is to switch to a plain mutex — change the creation at line 1020 and leave the existing xSemaphoreTake/Give call sites as-is:
- ctx->radio_mutex = xSemaphoreCreateRecursiveMutex();
+ ctx->radio_mutex = xSemaphoreCreateMutex();This eliminates the mismatch entirely without touching the three lock/unlock sites.
🧠 Learnings used
Learnt from: KenVanHoeylandt
Repo: TactilityProject/Tactility PR: 480
File: TactilityKernel/Source/drivers/i2s_controller.cpp:10-33
Timestamp: 2026-02-04T22:31:08.613Z
Learning: Adopt Linux kernel style: do not add null pointer guards in internal APIs. Assume callers pass valid pointers; crashes on invalid pointers are bugs detected during development. Apply this guideline to internal API implementations across the codebase (e.g., TactilityKernel/Source/drivers/*.cpp). Consider adding runtime comments or static analysis where preconditions are documented, but avoid defensive null checks in internal API paths.
Learnt from: NellowTCS
Repo: TactilityProject/Tactility PR: 521
File: Devices/cyd-2432s024r/Source/devices/Display.cpp:6-9
Timestamp: 2026-04-13T15:58:04.294Z
Learning: In ESP-IDF projects that use the `esp_lcd` component, don’t flag a type mismatch when a function expects `esp_lcd_spi_bus_handle_t` but the code passes a `spi_host_device_t` value such as `SPI2_HOST`. In ESP-IDF, `esp_lcd_spi_bus_handle_t` is a typedef alias compatible with `spi_host_device_t`, so this compiles and is type-correct—treat it as valid rather than an error.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp (3)
770-781:⚠️ Potential issue | 🟠 MajorUse recursive mutex API consistently for
radio_mutex.
radio_mutexis created withxSemaphoreCreateRecursiveMutex()at line 1004, but is taken/given with the non-recursivexSemaphoreTake()/xSemaphoreGive()API here. This breaks recursive ownership semantics and can cause undefined behavior or deadlock if any callback path re-enters a radio API on the same task.Use
xSemaphoreTakeRecursive()/xSemaphoreGiveRecursive(), or change line 1004 to create a plain mutex withxSemaphoreCreateMutex().Suggested fix (recursive API)
static error_t api_set_radio_enabled(struct Device* device, bool enabled) { BleCtx* ctx = (BleCtx*)device_get_driver_data(device); if (!ctx) return ERROR_INVALID_STATE; - xSemaphoreTake(ctx->radio_mutex, portMAX_DELAY); + xSemaphoreTakeRecursive(ctx->radio_mutex, portMAX_DELAY); if (enabled) { dispatch_enable(ctx); } else { dispatch_disable(ctx); } - xSemaphoreGive(ctx->radio_mutex); + xSemaphoreGiveRecursive(ctx->radio_mutex); return ERROR_NONE; }
918-942:⚠️ Potential issue | 🟠 MajorSame recursive mutex API mismatch, and
get_device_namesilently truncates.
Lines 922/931 and 938/941 use
xSemaphoreTake/xSemaphoreGiveon the recursiveradio_mutex.
api_get_device_namesilently truncates shorter buffers and returnsERROR_NONE, making it impossible for callers to distinguish a full name from a truncated one.Suggested fixes
static error_t api_set_device_name(struct Device* device, const char* name) { BleCtx* ctx = (BleCtx*)device_get_driver_data(device); if (!ctx || !name) return ERROR_INVALID_ARGUMENT; if (strlen(name) > BLE_DEVICE_NAME_MAX) return ERROR_INVALID_ARGUMENT; - xSemaphoreTake(ctx->radio_mutex, portMAX_DELAY); + xSemaphoreTakeRecursive(ctx->radio_mutex, portMAX_DELAY); strncpy(ctx->device_name, name, BLE_DEVICE_NAME_MAX); ctx->device_name[BLE_DEVICE_NAME_MAX] = '\0'; if (ctx->radio_state.load() == BT_RADIO_STATE_ON) { ble_svc_gap_device_name_set(ctx->device_name); ble_schedule_adv_restart(device, 0); } - xSemaphoreGive(ctx->radio_mutex); + xSemaphoreGiveRecursive(ctx->radio_mutex); return ERROR_NONE; } static error_t api_get_device_name(struct Device* device, char* buf, size_t buf_len) { BleCtx* ctx = (BleCtx*)device_get_driver_data(device); if (!ctx || !buf || buf_len == 0) return ERROR_INVALID_ARGUMENT; - xSemaphoreTake(ctx->radio_mutex, portMAX_DELAY); + if (buf_len < BLE_DEVICE_NAME_MAX + 1) { + return ERROR_INVALID_ARGUMENT; + } + xSemaphoreTakeRecursive(ctx->radio_mutex, portMAX_DELAY); strncpy(buf, ctx->device_name, buf_len - 1); buf[buf_len - 1] = '\0'; - xSemaphoreGive(ctx->radio_mutex); + xSemaphoreGiveRecursive(ctx->radio_mutex); return ERROR_NONE; }
1058-1071:⚠️ Potential issue | 🔴 CriticalStop NimBLE before destroying child devices.
Child devices (
hid_device_child,midi_child,serial_child) store theirDevice*pointers in GATT callback args and timer payloads. Destroying them at lines 1065-1067 while NimBLE is still running leaves those pointers dangling—late callbacks can race into freed memory.Move
dispatch_disable()before the child destruction:Suggested fix
static error_t esp32_ble_stop_device(struct Device* device) { BleCtx* ctx = (BleCtx*)device_get_driver_data(device); if (!ctx) return ERROR_NONE; - // Destroy child devices before stopping the radio and freeing the context. - // device_stop() on each child will invoke stop_device (for serial/midi/hid) - // which frees their driver data. - destroy_child_device(ctx->hid_device_child); - destroy_child_device(ctx->midi_child); - destroy_child_device(ctx->serial_child); - if (ctx->radio_state.load() != BT_RADIO_STATE_OFF) { dispatch_disable(ctx); } + + // Once NimBLE is stopped, no late GATT/timer callback can dereference child pointers. + destroy_child_device(ctx->hid_device_child); + destroy_child_device(ctx->midi_child); + destroy_child_device(ctx->serial_child);
🧹 Nitpick comments (1)
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_scan.cpp (1)
111-143: Consider null-terminating the name buffer explicitly afteros_mbuf_copydata.While
name_bufis zero-initialized, ifos_mbuf_copydatafails silently or returns partial data, the code still proceeds. The explicit null-termination at line 125 only covers the success path whereidx < ctx->scan_count.Suggested improvement
char name_buf[BT_NAME_MAX + 1] = {}; os_mbuf_copydata(attr->om, 0, len, name_buf); + name_buf[len] = '\0'; // ensure null-termination { xSemaphoreTake(ctx->scan_mutex, portMAX_DELAY);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 030df351-0c98-4233-82df-ff6fff227204
📒 Files selected for processing (10)
Platforms/platform-esp32/include/tactility/bindings/esp32_ble.hPlatforms/platform-esp32/include/tactility/drivers/esp32_ble.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_hid.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_midi.hPlatforms/platform-esp32/private/bluetooth/esp32_ble_spp.hPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_midi.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_scan.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cpp
✅ Files skipped from review due to trivial changes (3)
- Platforms/platform-esp32/private/bluetooth/esp32_ble_spp.h
- Platforms/platform-esp32/private/bluetooth/esp32_ble_hid.h
- Platforms/platform-esp32/private/bluetooth/esp32_ble_midi.h
🚧 Files skipped from review as they are similar to previous changes (3)
- Platforms/platform-esp32/include/tactility/drivers/esp32_ble.h
- Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cpp
- Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cpp
and some bonus symbols
100kHz i2c speed seems to be more compatible with m5stack modules...and probably in general. cdn message no longer applies
2 participants
Bluetooth Host, Device, HID, Midi, SPP
Ahhhhhhhhhhhhhhhhhh!
Summary by CodeRabbit
Release Notes