Conversation
- Added Bluetooth Files - Made edits to EmotiBit.h+.cpp - This build goes into bluetooth mode, does not stream
Update LEDS
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces Bluetooth Low Energy (BLE) support for EmotiBit on Arduino Feather ESP32 boards. New files EmotiBitBluetooth.h and EmotiBitBluetooth.cpp implement a complete BLE communication stack with server initialization, data transmission via characteristics, and connection lifecycle management. EmotiBit.h adds a new BLUETOOTH power mode and Bluetooth-related member variables. EmotiBit.cpp integrates Bluetooth initialization, control packet parsing from both WiFi and Bluetooth sources, conditional data transmission paths, LED status indicators, and power-mode transitions. The firmware and board configuration files are updated to support Bluetooth mode toggling and ESP32 partition allocation. Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
EmotiBit.cpp (3)
4045-4050: sendModePacket() always sends over WiFi, ignoring BLUETOOTH mode.When in BLE mode, this won’t reach the client and unnecessarily wakes WiFi code paths.
void EmotiBit::sendModePacket(String &sentModePacket, uint16_t &packetNumber) { createModePacket(sentModePacket, packetNumber); - // ToDo: This should probably be over TCP in response to specific messages from Host (but will require writing TCP ingest) - _emotiBitWiFi.sendData(sentModePacket); // Send packet immediately to update host + // Send over the active transport + if (getPowerMode() == PowerMode::BLUETOOTH) { + #ifdef ARDUINO_FEATHER_ESP32 + _emotiBitBluetooth.sendData(sentModePacket); + #endif + } else { + _emotiBitWiFi.sendData(sentModePacket); + } _outDataPackets += sentModePacket; // Add packet to slower data logging bucket }
2261-2273: Bitwise precedence bug causes incorrect overflow detection.
status & BufferFloat::ERROR_BUFFER_OVERFLOW == ...is parsed asstatus & (A==A). You’re effectively testing LSB of status.- if (status & BufferFloat::ERROR_BUFFER_OVERFLOW == BufferFloat::ERROR_BUFFER_OVERFLOW) { + if ((status & BufferFloat::ERROR_BUFFER_OVERFLOW) == BufferFloat::ERROR_BUFFER_OVERFLOW) {This affects reliability of overflow handling.
3995-4042: Add BLUETOOTH power mode case to createModePacket() using existing MODE_BLUETOOTH.The BLUETOOTH power mode case is missing from createModePacket(). EmotiBitPacket::TypeTag::MODE_BLUETOOTH already exists in the codebase and is actively used elsewhere (e.g., line 1601), so use it directly without a temporary string placeholder.
else if(getPowerMode() == PowerMode::WIRELESS_OFF) { payload += EmotiBitPacket::TypeTag::MODE_WIRELESS_OFF; } + else if (getPowerMode() == PowerMode::BLUETOOTH) + { + payload += EmotiBitPacket::TypeTag::MODE_BLUETOOTH; + } else if (getPowerMode() == PowerMode::HIBERNATE)EmotiBit.h (1)
266-274: Add PowerMode::BLUETOOTH handling to createModePacket().The
createModePacket()function (EmotiBit.cpp:3994-4042) handles NORMAL_POWER, LOW_POWER, MAX_LOW_POWER, WIRELESS_OFF, and HIBERNATE modes (lines 4017-4036), but lacks a case for PowerMode::BLUETOOTH. When the device is in BLUETOOTH mode, no MODE_BLUETOOTH tag is emitted, causing the host/UI to be unaware of the actual power state. Additionally,sendModePacket()(line 4048) always transmits via WiFi regardless of mode, which won't reach clients connected only over Bluetooth.Add this condition in createModePacket() before the final
dataCount++:else if (getPowerMode() == PowerMode::BLUETOOTH) { payload += EmotiBitPacket::TypeTag::MODE_BLUETOOTH; }Also review
sendModePacket()to ensure Bluetooth-connected clients receive mode updates (the ToDo comment at line 4047 hints at this broader design need).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
EmotiBit.cpp(10 hunks)EmotiBit.h(5 hunks)EmotiBitBluetooth.cpp(1 hunks)EmotiBitBluetooth.h(1 hunks)EmotiBit_stock_firmware/EmotiBit_stock_firmware.ino(1 hunks)board_feather_esp32.ini(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
EmotiBit.cpp
[note] 960-960: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 967-967: +1, nesting level increased to 1
(clang)
[warning] 960-960: redundant boolean literal supplied to boolean operator
(readability-simplify-boolean-expr)
[note] 1525-1525: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1601-1601: +1, nesting level increased to 3
(clang)
[note] 3227-3227: +5, including nesting penalty of 4, nesting level increased to 5
(clang)
[warning] 3860-3860: if with identical then and else branches
(bugprone-branch-clone)
[note] 3867-3867: else branch starts here
(clang)
[warning] 3860-3860: redundant boolean literal supplied to boolean operator
(readability-simplify-boolean-expr)
[warning] 3873-3873: repeated branch in conditional chain
(bugprone-branch-clone)
[note] 4452-4452: +1, nesting level increased to 2
(clang)
[note] 4451-4451: end of the original
(clang)
[note] 4453-4453: clone 1 starts here
(clang)
🪛 GitHub Actions: Build EmotiBit FeatherWing Firmware
EmotiBit.cpp
[error] 1601-1601: EmotiBitPacket::TypeTag does not contain MODE_BLUETOOTH. Compilation failed due to invalid enum value usage.
🔇 Additional comments (3)
board_feather_esp32.ini (1)
11-12: Partition change: confirm trade-offs.huge_app.csv reduces SPIFFS/OTA space. Verify this is acceptable for your SD/FTP flows and that no OTA or on-device FS is relied upon elsewhere.
EmotiBit_stock_firmware/EmotiBit_stock_firmware.ino (1)
13-24: BLE/Wireless Off toggle logic LGTM.The button toggles between BLUETOOTH and WIRELESS_OFF when Bluetooth is enabled; otherwise WiFi path toggles as before. Clear and consistent.
EmotiBitBluetooth.cpp (1)
31-33: I'm unable to locate EmotiBitBluetooth.cpp in the public repositories despite multiple attempts. Without access to the actual codebase, I cannot verify:
- Which BLE stack is in use (ESP32 Arduino Bluedroid vs NimBLE-Arduino)
- The current implementation at lines 31-33 and 112-121
- Whether security infrastructure is already configured elsewhere
- Whether the
MyCallbacksimplementation properly handles securityHowever, the web search results confirm that the review comment's suggested API patterns are accurate:
- For ESP32 Arduino (Bluedroid): Use
setAccessPermissions(ESP_GATT_PERM_WRITE_ENCRYPTED)orsetAccessPermissions(ESP_GATT_PERM_WRITE_ENC_MITM)after creating the characteristic- For NimBLE-Arduino: Use
NIMBLE_PROPERTY::WRITE_ENCwhen callingcreateCharacteristic()Both approaches require security/pairing infrastructure to be enabled on the device.
The security concern raised in the review comment is valid — writable BLE characteristics should require encryption and ideally MITM authentication to prevent unauthorized control writes.
| void EmotiBitBluetooth::end() | ||
| { | ||
| if (pServer && deviceConnected) | ||
| { | ||
| //tear down the old connection | ||
| pServer->disconnect(0); | ||
| Serial.println("BLE client disconnected by end()"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid hard-coded disconnect(0); track and use actual connection ID.
disconnect(0) may be wrong if the connId isn’t 0; store connId on connect and use it on end().
Search for an available API to retrieve the connection id:
If supported, in onConnect store server->_connId = pServer->getConnId(); and in end() call pServer->disconnect(server->_connId);. Otherwise, fall back to a parameterless disconnect if available.
Also applies to: 43-47
🏁 Script executed:
#!/bin/bash
rg -nP --type=cpp 'getConnId|disconnect\s*\(' -C2Length of output: 3051
I need to gather more information about the BLE library being used and examine the onConnect handler.
Now I need to examine the onConnect handler and verify how disconnect() is being called in the codebase.
🏁 Script executed:
#!/bin/bash
# Find the onConnect callback and surrounding context (lines 43-47)
rg -nA15 'onConnect\s*\(' EmotiBitBluetooth.cpp | head -40Length of output: 663
🏁 Script executed:
#!/bin/bash
# Find EmotiBitBluetooth class definition and member variables
rg -nB5 'class EmotiBitBluetooth' EmotiBitBluetooth.hLength of output: 191
🏁 Script executed:
#!/bin/bash
# Look for member variables and initialization in header file
rg -nP 'uint\d+_t|int\d+_t|bool|connId' EmotiBitBluetooth.h | head -30Length of output: 298
🏁 Script executed:
#!/bin/bash
# Check if getConnId is available in any BLE-related headers or code
rg -n 'getConnId|connId|connHandle' EmotiBitBluetooth.cpp EmotiBitBluetooth.hLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find BLE library includes and setup code
rg -n '#include.*BLE' EmotiBitBluetooth.cpp | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for BLEServer initialization and setup
rg -nB3A3 'pServer.*=.*new.*BLEServer|BLEServer::createServer|pServer->setCallbacks' EmotiBitBluetooth.cpp | head -40Length of output: 81
🏁 Script executed:
#!/bin/bash
# Look for disconnect method calls to understand the expected signature
rg -nB2A2 'pServer->disconnect' EmotiBitBluetooth.cppLength of output: 81
There was a problem hiding this comment.
I think our future selves may be significantly less disappointed in our present selves if we refactor setPowerMode to correctly handle BT. I've made suggested changes in the branch feat-blePrototype-Example-fix-setPowerMode
Make sure to highlight in your PR which data integrity tests have been run on SD card data using wifi (normal use-case) in addition to BLE situations.
Please also specify in the PR the battery life when using wifi (and wifi->wireless off) modes.
| static String packet; | ||
| static EmotiBitPacket::Header header; | ||
| int16_t dataStartChar = 0; | ||
| #ifdef ARDUINO_FEATHER_ESP32 |
There was a problem hiding this comment.
Could we remove this #ifdef and return zero from _emotiBitBluetooth.readControl(packet)?
There was a problem hiding this comment.
The entire bluetooth file is wrapped in an indef so the app will build on m0, it would break on m0.
EmotiBit.cpp
Outdated
| } | ||
|
|
||
| if (getPowerMode() == PowerMode::BLUETOOTH) { | ||
| #ifdef ARDUINO_FEATHER_ESP32 |
There was a problem hiding this comment.
Can we remove this #ifdef ARDUINO_FEATHER_ESP32?
If we got here, can't we just assume it's a BT compatible board?
There was a problem hiding this comment.
Removing the ifdefs here breaks m0
EmotiBit.cpp
Outdated
| // Battery LED | ||
| if (battIndicationSeq) | ||
| { | ||
| led.setState(EmotiBitLedController::Led::YELLOW, true); |
There was a problem hiding this comment.
Why do we need to have the Led::YELLOW and Led::RED controls duplicated for PowerMode::BLUETOOTH and not? Seems like one source of truth could be enough.
| _emotiBitWiFi.sendData(s); | ||
| } | ||
| if (getPowerMode() == PowerMode::BLUETOOTH) { | ||
| #ifdef ARDUINO_FEATHER_ESP32 |
There was a problem hiding this comment.
Could we get rid of #ifdef ARDUINO_FEATHER_ESP32 here and rely on the fact that we've already checked for BT compatibility before entering PowerMode::BLUETOOTH?
There was a problem hiding this comment.
This would also lead to m0 breaking
| #include <SD.h> | ||
| #include "driver/adc.h" | ||
| #include <esp_bt.h> | ||
| #include <esp_bt.h> //consider moving into bluetooth |
There was a problem hiding this comment.
How about creating EmotiBitBluetooth::disable() and moving #include <esp_bt.h> into EmotiBitBluetooth to avoid littering the place with bluetooth code?
3 participants
Description
This PR enables Bluetooth Low Energy (BLE) on the EmotiBit, adding the following functionality:
ARDUINO NOTE: If you are on Arduino, please set the partition table to Huge App
NOTE: This build does NOT have time syncing enabled so all data received does not account for start time/adjustments due to latency.
This PR allows users to press the side button on start up (same time frame to get into the debug mode) to switch the EmotiBit into bluetooth mode, which is signified by a faster blinking light. A synopsis of each change is outlined in the notes for reviewer. This code was built out with this arduino example here as an example.
Requirements
Issues Referenced
None
Documentation update
None - we are not publicly releasing this PR
Notes for Reviewer
EmotiBitBluetooth.h/.cpp
This is where all the bluetooth specific functions will live and was built with EmotiBitWifi as a blueprint.
EmotiBit.h/.cpp
EmotiBit_stock_firemware.ino
board_feather_esp32.ini
Setup Flow
Bluetooth Connections Flow
LED Speeds
Testing
All tests were run with Bluefruit Connect or nRF Connect which can be downloaded on the app store.
Results
Testing included the following feature tests. I also ensured that record begin/end would work as well.
Feature Tests
Shared files
Firmware binary:
1.14.3.Feat ble prototype example.0 .zip
Other files.
Checklist to allow merge
masterDIGITAL_WRITE_DEBUG= false (if set true while testing)Screenshots:
Summary by CodeRabbit
Release Notes
New Features