Bluetooth and Wi-Fi improvements - #622
Conversation
📝 WalkthroughWalkthroughThe pull request replaces Bluetooth callback registration with caller-owned event subscriptions, bounded event queues, task-event notifications, and polling APIs. The ESP32 BLE driver manages subscription registration, event delivery, cleanup, and GAP profile synchronization. Tactility uses a dedicated event thread, while Bluetooth applications poll subscriptions directly. Wi-Fi subscription cleanup tracks closed and constructed states. Multiple ESP32 device-tree nodes no longer disable BLE explicitly. Documentation and kernel symbol exports were updated. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
TactilityKernel/source/drivers/bluetooth.cpp (1)
83-89: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDestroy
ring_mutexeven when the driver reports no matching subscription.The header documents
ERROR_NOT_FOUNDas a valid outcome ofevent_unsubscribe. On that path the mutex stays constructed, and the caller cannot release it becauseinternalis private. Each such call leaks one mutex object.♻️ Proposed change
error_t bluetooth_event_unsubscribe(struct Device* device, struct BtEventSubscription* sub) { error_t result = BT_API(device)->event_unsubscribe(device, sub); - if (result == ERROR_NONE) { - mutex_destruct(&sub->internal.ring_mutex); - } + mutex_destruct(&sub->internal.ring_mutex); return result; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e6082e1-d9f5-4f79-9f52-a9fe36a1a6f6
📒 Files selected for processing (45)
Devices/btt-panda-touch/bigtreetech,panda-touch.dtsDevices/cyd-4848s040c/cyd,4848s040c.dtsDevices/cyd-8048s043c/cyd,8048s043c.dtsDevices/elecrow-crowpanel-advance-28/elecrow,crowpanel-advance-28.dtsDevices/elecrow-crowpanel-advance-35/elecrow,crowpanel-advance-35.dtsDevices/elecrow-crowpanel-advance-50/elecrow,crowpanel-advance-50.dtsDevices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dtsDevices/es3c28p/es3c28p.dtsDevices/guition-jc1060p470ciwy/guition,jc1060p470ciwy.dtsDevices/guition-jc8048w550c/guition,jc8048w550c.dtsDevices/heltec-wifi-lora-32-v3/heltec,wifi-lora-32-v3.dtsDevices/lilygo-tdeck-plus/lilygo,tdeck-plus.dtsDevices/lilygo-tdeck/lilygo,tdeck.dtsDevices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dtsDevices/lilygo-tdongle-s3/lilygo,tdongle-s3.dtsDevices/lilygo-thmi/lilygo,thmi.dtsDevices/lilygo-tlora-pager/lilygo,tlora-pager.dtsDevices/m5stack-cardputer-adv/m5stack,cardputer-adv.dtsDevices/m5stack-cardputer/m5stack,cardputer.dtsDevices/m5stack-cores3/m5stack,cores3.dtsDevices/m5stack-papers3/m5stack,papers3.dtsDevices/m5stack-stackchan/m5stack,stackchan.dtsDevices/m5stack-sticks3/m5stack,sticks3.dtsDevices/m5stack-tab5/m5stack,tab5.dtsDevices/tulip-4r11/tulip-4r11.dtsDevices/unphone/unphone.dtsDevices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dtsDevices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dtsDevices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dtsDevices/waveshare-s3-touch-lcd-147/waveshare,s3-touch-lcd-147.dtsDevices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dtsDevices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dtsPlatforms/platform-esp32/private/bluetooth/esp32_ble_internal.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_spp.cppTactility/Include/Tactility/bluetooth/Bluetooth.hTactility/Private/Tactility/app/btmanage/BtManagePrivate.hTactility/Source/app/btmanage/BtManage.cppTactility/Source/app/btpeersettings/BtPeerSettings.cppTactility/Source/bluetooth/Bluetooth.cppTactilityKernel/include/tactility/drivers/bluetooth.hTactilityKernel/source/drivers/bluetooth.cppTactilityKernel/source/symbols.c
💤 Files with no reviewable changes (32)
- Devices/cyd-8048s043c/cyd,8048s043c.dts
- Devices/tulip-4r11/tulip-4r11.dts
- Devices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dts
- Devices/m5stack-cores3/m5stack,cores3.dts
- Devices/m5stack-papers3/m5stack,papers3.dts
- Devices/cyd-4848s040c/cyd,4848s040c.dts
- Devices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dts
- Devices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dts
- Devices/unphone/unphone.dts
- Devices/btt-panda-touch/bigtreetech,panda-touch.dts
- Devices/es3c28p/es3c28p.dts
- Devices/heltec-wifi-lora-32-v3/heltec,wifi-lora-32-v3.dts
- Devices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dts
- Devices/elecrow-crowpanel-advance-35/elecrow,crowpanel-advance-35.dts
- Devices/elecrow-crowpanel-advance-28/elecrow,crowpanel-advance-28.dts
- Devices/lilygo-tdeck-plus/lilygo,tdeck-plus.dts
- Devices/m5stack-cardputer/m5stack,cardputer.dts
- Devices/guition-jc8048w550c/guition,jc8048w550c.dts
- Devices/lilygo-thmi/lilygo,thmi.dts
- Devices/lilygo-tdeck/lilygo,tdeck.dts
- Devices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dts
- Devices/m5stack-tab5/m5stack,tab5.dts
- Devices/lilygo-tdongle-s3/lilygo,tdongle-s3.dts
- Devices/elecrow-crowpanel-advance-50/elecrow,crowpanel-advance-50.dts
- Devices/m5stack-sticks3/m5stack,sticks3.dts
- Devices/waveshare-s3-touch-lcd-147/waveshare,s3-touch-lcd-147.dts
- Devices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dts
- Devices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dts
- Devices/guition-jc1060p470ciwy/guition,jc1060p470ciwy.dts
- Devices/m5stack-stackchan/m5stack,stackchan.dts
- Devices/m5stack-cardputer-adv/m5stack,cardputer-adv.dts
- Devices/lilygo-tlora-pager/lilygo,tlora-pager.dts
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
KenVanHoeylandt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Tactility/Source/bluetooth/Bluetooth.cpp (1)
363-368: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftCancel deferred bridge actions before disabling the radio.
bt_event_bridge()posts radio-on and scan-finished work togetMainDispatcher().stopBtEventThread()joins only the polling thread. It does not drain or cancel posted tasks. A queued task can run after Line [365] disables the radio and start scanning or a profile.Add a radio-generation check to each deferred action, or synchronize with the dispatcher before disabling the radio.
Tactility/Source/app/btmanage/BtManage.cpp (1)
176-184: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle subscription failure before entering the event-driven UI.
If
bluetooth_event_subscribe()fails,ctx.btDeviceremains null, butappMain()still creates the window and accepts Bluetooth controls. No Bluetooth events are polled, so the state and view cannot update. Return after cleanup or disable event-dependent controls when subscription fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51959d84-eee5-48c7-a07f-3f8acfc9f017
📒 Files selected for processing (14)
Documentation/ideas.mdPlatforms/platform-esp32/private/bluetooth/esp32_ble_internal.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_spp.cppPlatforms/platform-esp32/source/drivers/esp32_wifi.cppPlatforms/platform-posix/source/drivers/mock_wifi.cppTactility/Source/app/btmanage/BtManage.cppTactility/Source/bluetooth/Bluetooth.cppTactilityKernel/include/tactility/drivers/bluetooth.hTactilityKernel/include/tactility/drivers/wifi.hTactilityKernel/source/drivers/bluetooth.cppTactilityKernel/source/drivers/wifi.cpp
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d69c0f8b-8f09-45a2-bd2d-92d724551925
📒 Files selected for processing (5)
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppTactilityKernel/include/tactility/drivers/bluetooth.hTactilityKernel/include/tactility/drivers/wifi.hTactilityKernel/source/drivers/bluetooth.cppTactilityKernel/source/drivers/wifi.cpp
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp (2)
1019-1025: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck for an already-constructed subscription before reinitializing it.
bluetooth_event_subscribe()constructs and resetssub->internal.ring_mutexbefore calling this handler. A second subscribe of the same livesubreaches this duplicate branch afterward. The wrapper then marks the subscription unconstructed and destructs the mutex, whilectx->subscriptionsstill points tosub. The next event or poll uses a destroyed mutex.Move the constructed-state check before initialization in
TactilityKernel/source/drivers/bluetooth.cpp, or reject an already-constructed subscription without modifying its fields. The full-list traversal fixes the earlier cycle issue but does not make this rejection safe. (raw.githubusercontent.com)
1235-1257: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftWake blocked waiters before releasing a closed subscription bit.
Teardown sets
closed, then callstask_event_group_release_bit(). That function clears and unclaims the bit; it does not signal it. A caller blocked intask_event_group_wait()can remain blocked forever and cannot callbluetooth_event_poll()to observe the closed state.Keep the bit claimed until the final unsubscribe, signal it after setting
closed, and release it from the unsubscribe path. Signaling and immediately releasing is not sufficient because release clears the event bit. (raw.githubusercontent.com)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e58321e-2314-4e4d-9023-35d78a5020dc
📒 Files selected for processing (1)
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.
1 participant
Uh oh!
There was an error while loading. Please reload this page.