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