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
| #include <format> | ||
| #include <string> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the GCC/libstdc++ toolchain typically used by ESP-IDF on ESP32 fully support C++20 /std::format?
💡 Result:
No, the GCC/libstdc++ toolchain typically used by ESP-IDF on ESP32 does not fully support C++20 <format> / std::format.
Citations:
- 1: use fmt library for C++ logging (IDFGH-8779) espressif/esp-idf-cxx#17
- 2: https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-guides/cplusplus.html
- 3: https://docs.espressif.com/projects/esp-idf/en/v5.1-beta1/esp32/api-guides/cplusplus.html
- 4: C++ Compilation Error with ESP-IDF 5.5 and GCC 14.2.0 Toolchain (IDFGH-17157) espressif/esp-idf#18172
- 5: Recent GCC v14 conflicts with stdatomic.h from the xtensa toolchain (esp-13.2.0_20240305) (IDFGH-13033) espressif/esp-idf#13979
- 6: xtensa: Size optimization regression between GCC 8.4.0 and 13.2.0 espressif/crosstool-NG#52
- 7: Libstdc++ Nano? (IDFGH-17235) espressif/esp-idf#18232
Remove #include <format> and std::format usage—incompatible with ESP-IDF's libstdc++ toolchain.
The ESP-IDF GCC/libstdc++ toolchain does not support C++20 <format> library. Replace with snprintf() or std::to_string() to ensure the code compiles.
Also applies to: 96-101
🧰 Tools
🪛 Clang (14.0.6)
[error] 1-1: 'format' file not found
(clang-diagnostic-error)
KenVanHoeylandt
left a comment
KenVanHoeylandt
left a comment
There was a problem hiding this comment.
Great job! I've finished my feedback.
| return std::format(DEVICE_SETTINGS_FORMAT, DATA_DIR, addr_hex); | ||
| } | ||
|
|
||
| bool contains(const std::string& addr_hex) { |
There was a problem hiding this comment.
Let's rename this to hasFileForDevice() or something similar.
| if (bluetooth_serial_start(dev) != ERROR_NONE) return false; | ||
| settings::setSppAutoStart(true); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Same here: can be removed and settings can be called in-place.
| if (dev == nullptr) return; | ||
| settings::setSppAutoStart(false); | ||
| bluetooth_serial_stop(dev); | ||
| } |
There was a problem hiding this comment.
Same here: can be removed and settings can be called in-place.
Tactility/Source/bluetooth/README.md
Outdated
| @@ -0,0 +1,52 @@ | |||
| # Bluetooth Bridge (Tactility Layer) | |||
There was a problem hiding this comment.
File can be removed
|
|
||
| // ---- Device lookup ---- | ||
|
|
||
| struct Device* bluetooth_get_device() { |
There was a problem hiding this comment.
Let's rename this to bluetooth_find_first_ready_device()
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
|
|
||
| device.name = map.contains(KEY_NAME) ? map[KEY_NAME] : ""; | ||
|
|
||
| device.autoConnect = !map.contains(KEY_AUTO_CONNECT) || (map[KEY_AUTO_CONNECT] == "true"); |
There was a problem hiding this comment.
Missing autoConnect key currently enables auto-connect unexpectedly.
Line 70 treats absent autoConnect as true, which contradicts PairedDevice.autoConnect = false and can silently change behavior for legacy/incomplete files.
Proposed fix
- device.autoConnect = !map.contains(KEY_AUTO_CONNECT) || (map[KEY_AUTO_CONNECT] == "true");
+ device.autoConnect = map.contains(KEY_AUTO_CONNECT) && (map[KEY_AUTO_CONNECT] == "true");📝 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.
| device.autoConnect = !map.contains(KEY_AUTO_CONNECT) || (map[KEY_AUTO_CONNECT] == "true"); | |
| device.autoConnect = map.contains(KEY_AUTO_CONNECT) && (map[KEY_AUTO_CONNECT] == "true"); |
| 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; | |
| } | |
| } |
| /** Data was received on the BLE SPP (NUS) RX characteristic */ | ||
| BT_EVENT_SPP_DATA_RECEIVED, | ||
| /** Data was received on the BLE MIDI I/O characteristic */ | ||
| BT_EVENT_MIDI_DATA_RECEIVED, | ||
| }; |
There was a problem hiding this comment.
SPP/MIDI “data received” events have no payload contract.
BT_EVENT_SPP_DATA_RECEIVED and BT_EVENT_MIDI_DATA_RECEIVED are declared, but struct BtEvent has no corresponding payload fields. As-is, handlers cannot access received bytes safely/portably through this API. Add explicit payload structs (data pointer/length + source context) or provide dedicated read APIs and document ownership/lifetime.
Also applies to: 126-135
| error_t (*get_paired_peers)(struct Device* device, struct BtPeerRecord* out, size_t* count); | ||
|
|
There was a problem hiding this comment.
get_paired_peers exists internally but is missing from the public C API.
struct BluetoothApi exposes get_paired_peers, but there is no corresponding public wrapper declaration, which leaves paired-peer enumeration inaccessible through the documented external API surface. Please either expose bluetooth_get_paired_peers(...) publicly (and export it) or remove this from the internal contract if intentionally private.
Suggested API completion
--- a/TactilityKernel/include/tactility/drivers/bluetooth.h
+++ b/TactilityKernel/include/tactility/drivers/bluetooth.h
@@
error_t bluetooth_pair(struct Device* device, const BtAddr addr);
error_t bluetooth_unpair(struct Device* device, const BtAddr addr);
+error_t bluetooth_get_paired_peers(struct Device* device, struct BtPeerRecord* out, size_t* count);
error_t bluetooth_connect(struct Device* device, const BtAddr addr, enum BtProfileId profile);--- a/TactilityKernel/source/drivers/bluetooth.cpp
+++ b/TactilityKernel/source/drivers/bluetooth.cpp
@@
error_t bluetooth_unpair(struct Device* device, const BtAddr addr) {
return BT_API(device)->unpair(device, addr);
}
+
+error_t bluetooth_get_paired_peers(struct Device* device, struct BtPeerRecord* out, size_t* count) {
+ return BT_API(device)->get_paired_peers(device, out, count);
+}Also applies to: 286-303
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 (settings::shouldSppAutoStart()) { | ||
| LOGGER.info("Auto-starting SPP server"); | ||
| if (struct Device* dev = bluetooth_serial_get_device()) { | ||
| bluetooth_serial_start(dev); | ||
| } | ||
| } else if (settings::shouldMidiAutoStart()) { | ||
| LOGGER.info("Auto-starting MIDI server"); | ||
| if (struct Device* dev = bluetooth_midi_get_device()) { | ||
| bluetooth_midi_start(dev); | ||
| } |
There was a problem hiding this comment.
Start SPP and MIDI independently after radio-up.
These preferences are persisted independently, but this else if chain restores only one of them. If both were enabled, MIDI never comes back after Bluetooth is turned on.
Suggested fix
- } else if (settings::shouldSppAutoStart()) {
- LOGGER.info("Auto-starting SPP server");
- if (struct Device* dev = bluetooth_serial_get_device()) {
- bluetooth_serial_start(dev);
- }
- } else if (settings::shouldMidiAutoStart()) {
- LOGGER.info("Auto-starting MIDI server");
- if (struct Device* dev = bluetooth_midi_get_device()) {
- bluetooth_midi_start(dev);
- }
+ } else {
+ if (settings::shouldSppAutoStart()) {
+ LOGGER.info("Auto-starting SPP server");
+ if (struct Device* dev = bluetooth_serial_get_device()) {
+ bluetooth_serial_start(dev);
+ }
+ }
+ if (settings::shouldMidiAutoStart()) {
+ LOGGER.info("Auto-starting MIDI server");
+ if (struct Device* dev = bluetooth_midi_get_device()) {
+ bluetooth_midi_start(dev);
+ }
+ }
}📝 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.
| } else if (settings::shouldSppAutoStart()) { | |
| LOGGER.info("Auto-starting SPP server"); | |
| if (struct Device* dev = bluetooth_serial_get_device()) { | |
| bluetooth_serial_start(dev); | |
| } | |
| } else if (settings::shouldMidiAutoStart()) { | |
| LOGGER.info("Auto-starting MIDI server"); | |
| if (struct Device* dev = bluetooth_midi_get_device()) { | |
| bluetooth_midi_start(dev); | |
| } | |
| } else { | |
| if (settings::shouldSppAutoStart()) { | |
| LOGGER.info("Auto-starting SPP server"); | |
| if (struct Device* dev = bluetooth_serial_get_device()) { | |
| bluetooth_serial_start(dev); | |
| } | |
| } | |
| if (settings::shouldMidiAutoStart()) { | |
| LOGGER.info("Auto-starting MIDI server"); | |
| if (struct Device* dev = bluetooth_midi_get_device()) { | |
| bluetooth_midi_start(dev); | |
| } | |
| } | |
| } |
| } 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.
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
| output_file.write(f"CONFIG_BT_NIMBLE_TRANSPORT_UART=n\n") | ||
| output_file.write(f"CONFIG_ESP_HOSTED_ENABLE_BT_NIMBLE=y\n") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import ast
from pathlib import Path
path = Path("device.py")
src = path.read_text()
tree = ast.parse(src, filename=str(path))
for node in ast.walk(tree):
if isinstance(node, ast.JoinedStr) and not any(isinstance(v, ast.FormattedValue) for v in node.values):
print(f"line {node.lineno}: {ast.get_source_segment(src, node)}")
PYRepository: TactilityProject/Tactility
Length of output: 170
🏁 Script executed:
cd . && head -320 device.py | tail -10Repository: TactilityProject/Tactility
Length of output: 695
🏁 Script executed:
python - <<'PY'
import ast
from pathlib import Path
path = Path("device.py")
src = path.read_text()
tree = ast.parse(src, filename=str(path))
count = 0
for node in ast.walk(tree):
if isinstance(node, ast.JoinedStr) and not any(isinstance(v, ast.FormattedValue) for v in node.values):
count += 1
if count <= 20: # Limit output
print(f"line {node.lineno}: {ast.get_source_segment(src, node)}")
print(f"\nTotal placeholder-free f-strings: {count}")
PYRepository: TactilityProject/Tactility
Length of output: 207
Remove the unused f prefixes from lines 318-319.
These are plain string literals without placeholders. Lines 318-319 will continue to trigger Ruff F541 until the f prefix is removed.
🧰 Tools
🪛 Ruff (0.15.9)
[error] 318-318: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 319-319: f-string without any placeholders
Remove extraneous f prefix
(F541)
| int rc = ble_gatts_count_cfg(svcs); | ||
| if (rc == 0) { | ||
| rc = ble_gatts_add_svcs(svcs); | ||
| if (rc != 0) { | ||
| LOG_E(TAG, "switchGattProfile: gatts_add_svcs failed rc=%d", rc); | ||
| return; // don't update profile — GATT state is inconsistent | ||
| } | ||
| } else { | ||
| LOG_E(TAG, "switchGattProfile: gatts_count_cfg failed rc=%d", rc); | ||
| return; | ||
| } | ||
|
|
||
| // ble_gatts_add_svcs() only adds definitions to a pending list. | ||
| // ble_gatts_start() converts them into live ATT entries. | ||
| // Without this call, all GATT reads return ATT errors and Windows | ||
| // cannot install the HID driver → Phase 2 reconnect never occurs. | ||
| rc = ble_gatts_start(); | ||
| if (rc != 0) { | ||
| LOG_E(TAG, "switchGattProfile: gatts_start failed rc=%d", rc); | ||
| return; |
There was a problem hiding this comment.
Propagate GATT switch failures back to start().
ble_hid_switch_profile() logs and returns on ble_gatts_count_cfg(), ble_gatts_add_svcs(), or ble_gatts_start() failure, but hid_device_start() still marks HID active and starts HID advertising. That can advertise a HID service UUID even though the live GATT table never switched. Make ble_hid_switch_profile() return a status and abort cleanup/advertising on failure.
Also applies to: 454-458
| 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
| 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); | ||
| strncpy(buf, ctx->device_name, buf_len - 1); | ||
| buf[buf_len - 1] = '\0'; | ||
| xSemaphoreGive(ctx->radio_mutex); | ||
| return ERROR_NONE; |
There was a problem hiding this comment.
Reject undersized buffers in get_device_name().
The API contract requires buf_len >= BLE_DEVICE_NAME_MAX + 1, but this implementation silently truncates shorter buffers and still returns ERROR_NONE. That makes it impossible for callers to distinguish a full device name from a truncated one.
| 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.
| #include <tactility/bindings/bindings.h> | ||
| #include <tactility/drivers/esp32_ble.h> | ||
|
|
||
| DEFINE_DEVICETREE(ble, struct Esp32BleNimbleConfig) |
There was a problem hiding this comment.
Can you rename Esp32BleNimbleConfig to Esp32BleConfig?
| #include <tactility/error.h> | ||
|
|
||
| #include <esp_timer.h> | ||
| #include <cstdint> |
There was a problem hiding this comment.
Should be stdint.h and not cstdint (C <> C++)
| #if defined(CONFIG_BT_NIMBLE_ENABLED) | ||
|
|
||
| #include <tactility/drivers/bluetooth_serial.h> | ||
| #include <cstdint> |
| void ble_spp_set_conn_handle(struct Device* device, uint16_t h); | ||
|
|
||
|
|
||
| #endif // CONFIG_BT_NIMBLE_ENABLED |
There was a problem hiding this comment.
Speaking of C code: all the header files should have "extern "C" {}" with idfdefs.
Except for the bt internal header.
| #include <atomic> | ||
| #include <cstring> | ||
|
|
||
| #define TAG "esp32_ble" |
There was a problem hiding this comment.
TAG in cpp files can be constexpr instead
| // ble_midi_ensure_keepalive creates the timer if needed and starts it periodically. | ||
| // ble_midi_stop_keepalive stops (but does not delete) the timer. | ||
| error_t ble_midi_ensure_keepalive(struct Device* device, esp_timer_cb_t cb, uint64_t period_us); | ||
| void ble_midi_stop_keepalive(struct Device* device); |
There was a problem hiding this comment.
This functions shouldn't be public in this header. They should be part of BtMidiApi and exposed via BT midi driver functions (in TactilityKernel).
| bool ble_spp_get_active(struct Device* device); | ||
| void ble_spp_set_active(struct Device* device, bool v); | ||
| uint16_t ble_spp_get_conn_handle(struct Device* device); | ||
| void ble_spp_set_conn_handle(struct Device* device, uint16_t h); |
There was a problem hiding this comment.
This functions shouldn't be public in this header. They should be part of BtSppApi and exposed via BT SPP driver functions (in TactilityKernel).
|
|
||
| // device must be the hid_device child Device*. | ||
| void ble_hid_init_gatt(); | ||
| void ble_hid_switch_profile(struct Device* hid_child, BleHidProfile profile); |
There was a problem hiding this comment.
This functions shouldn't be public in this header. They should be part of BtHidApi and exposed via BT HID driver functions (in TactilityKernel).
| @@ -0,0 +1,2 @@ | |||
| // HID device start/stop are now called directly via bluetooth_hid_device_start/stop | |||
| // from TactilityKernel. This file is intentionally empty. | |||
There was a problem hiding this comment.
Empty file can be removed?
| @@ -0,0 +1,3 @@ | |||
| // MIDI start/stop are now called directly via bluetooth_midi_start/stop | |||
There was a problem hiding this comment.
Empty file can be removed?
| @@ -0,0 +1,3 @@ | |||
| // SPP start/stop are now called directly via bluetooth_serial_start/stop | |||
There was a problem hiding this comment.
Empty file can be removed?
| DEFINE_MODULE_SYMBOL(bluetooth_hid_device_send_mouse), | ||
| DEFINE_MODULE_SYMBOL(bluetooth_hid_device_send_gamepad), | ||
| DEFINE_MODULE_SYMBOL(bluetooth_hid_device_is_connected), | ||
| DEFINE_MODULE_SYMBOL(BLUETOOTH_HID_DEVICE_TYPE), |
There was a problem hiding this comment.
All these bluetooth_hid_, serial, spp and midi functions don't exist in the kernel yet. They should be created and made part of their respective APIs and moved here.
Then platform-esp32 can implement these APIs.
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
Platforms/platform-esp32/include/tactility/bindings/esp32_ble.h
Outdated
Show resolved
Hide resolved
| struct BleMidiCtx { | ||
| SemaphoreHandle_t rx_mutex; | ||
| std::deque<std::vector<uint8_t>> rx_queue; | ||
| }; |
There was a problem hiding this comment.
rx_queue and rx_mutex are never consumed.
The BleMidiCtx maintains an RX queue that is populated in midi_chr_access (line 118), but looking at the public BtMidiApi (from TactilityKernel/include/tactility/drivers/bluetooth_midi.h:22-51), there is no receive or dequeue method exposed. The queue fills up, drops old packets (line 119), but the data is never retrieved.
Either add a receive method to the API, or remove this dead code to avoid unnecessary memory allocation and mutex overhead.
| error_t ble_midi_ensure_keepalive(struct Device* device, esp_timer_cb_t cb, uint64_t period_us) { | ||
| BleCtx* ctx = midi_root_ctx(device); | ||
| if (ctx->midi_keepalive_timer == nullptr) { | ||
| esp_timer_create_args_t args = {}; | ||
| args.callback = cb; | ||
| args.arg = device; | ||
| args.dispatch_method = ESP_TIMER_TASK; | ||
| args.name = "ble_midi_as"; | ||
| int rc = esp_timer_create(&args, &ctx->midi_keepalive_timer); | ||
| if (rc != ESP_OK) { | ||
| LOG_E(TAG, "midi keepalive timer create failed (rc=%d)", rc); | ||
| return ERROR_INVALID_STATE; | ||
| } | ||
| } | ||
| int rc = esp_timer_start_periodic(ctx->midi_keepalive_timer, period_us); | ||
| if (rc != ESP_OK) { | ||
| LOG_E(TAG, "midi keepalive timer start failed (rc=%d)", rc); | ||
| return ERROR_INVALID_STATE; | ||
| } | ||
| return ERROR_NONE; | ||
| } |
There was a problem hiding this comment.
Timer may fail to start if already running.
esp_timer_start_periodic returns ESP_ERR_INVALID_STATE if the timer is already running. If ble_midi_ensure_keepalive is called multiple times (e.g., during reconnection), the timer start will fail silently after the first call.
Suggested fix
}
+ // Stop first in case timer is already running
+ esp_timer_stop(ctx->midi_keepalive_timer);
int rc = esp_timer_start_periodic(ctx->midi_keepalive_timer, period_us);
if (rc != ESP_OK) {📝 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.
| error_t ble_midi_ensure_keepalive(struct Device* device, esp_timer_cb_t cb, uint64_t period_us) { | |
| BleCtx* ctx = midi_root_ctx(device); | |
| if (ctx->midi_keepalive_timer == nullptr) { | |
| esp_timer_create_args_t args = {}; | |
| args.callback = cb; | |
| args.arg = device; | |
| args.dispatch_method = ESP_TIMER_TASK; | |
| args.name = "ble_midi_as"; | |
| int rc = esp_timer_create(&args, &ctx->midi_keepalive_timer); | |
| if (rc != ESP_OK) { | |
| LOG_E(TAG, "midi keepalive timer create failed (rc=%d)", rc); | |
| return ERROR_INVALID_STATE; | |
| } | |
| } | |
| int rc = esp_timer_start_periodic(ctx->midi_keepalive_timer, period_us); | |
| if (rc != ESP_OK) { | |
| LOG_E(TAG, "midi keepalive timer start failed (rc=%d)", rc); | |
| return ERROR_INVALID_STATE; | |
| } | |
| return ERROR_NONE; | |
| } | |
| error_t ble_midi_ensure_keepalive(struct Device* device, esp_timer_cb_t cb, uint64_t period_us) { | |
| BleCtx* ctx = midi_root_ctx(device); | |
| if (ctx->midi_keepalive_timer == nullptr) { | |
| esp_timer_create_args_t args = {}; | |
| args.callback = cb; | |
| args.arg = device; | |
| args.dispatch_method = ESP_TIMER_TASK; | |
| args.name = "ble_midi_as"; | |
| int rc = esp_timer_create(&args, &ctx->midi_keepalive_timer); | |
| if (rc != ESP_OK) { | |
| LOG_E(TAG, "midi keepalive timer create failed (rc=%d)", rc); | |
| return ERROR_INVALID_STATE; | |
| } | |
| } | |
| // Stop first in case timer is already running | |
| esp_timer_stop(ctx->midi_keepalive_timer); | |
| int rc = esp_timer_start_periodic(ctx->midi_keepalive_timer, period_us); | |
| if (rc != ESP_OK) { | |
| LOG_E(TAG, "midi keepalive timer start failed (rc=%d)", rc); | |
| return ERROR_INVALID_STATE; | |
| } | |
| return ERROR_NONE; | |
| } |
and some bonus symbols
2 participants
Bluetooth Host, Device, HID, Midi, SPP
Ahhhhhhhhhhhhhhhhhh!
Summary by CodeRabbit
Release Notes