Added NPG-Lite examples: Server/Client, FFT, IR AC control and some other BCI examples. #2
Conversation
WalkthroughMultiple new Arduino/ESP32 sketches have been introduced, each targeting specific hardware functionalities: BLE client/server communication, EEG signal processing with FFT and bandpower analysis, IR remote control for LG air conditioners, and NeoPixel LED/buzzer effects. The programs utilize various libraries for BLE, IR, FFT, and NeoPixel control, and include real-time signal processing, visual/audio feedback, and hardware interfacing. Changes
Sequence Diagram(s)BLE Client-Server Connection and Notification FlowsequenceDiagram
participant BLE_Client
participant BLE_Server
participant User
User->>BLE_Client: Starts scan
BLE_Client->>BLE_Server: Scans for advertised service UUID
BLE_Server->>BLE_Client: Advertises service/characteristics
BLE_Client->>BLE_Server: Connects to server
BLE_Client->>BLE_Server: Discovers service and characteristics
BLE_Client->>BLE_Server: Subscribes to notifications
User->>BLE_Server: Presses button (toggles LED)
BLE_Server->>BLE_Client: Sends notification with LED state
BLE_Client->>BLE_Client: Processes notification, updates local LED
EEG Signal Processing and IR/BLE/LED ControlsequenceDiagram
participant MCU
participant EEG_Sensor
participant NeoPixel
participant IR_Device
participant BLE_Client
loop Every sample interval
EEG_Sensor->>MCU: Analog EEG sample
MCU->>MCU: Filter, buffer, FFT, bandpower calculation
MCU->>NeoPixel: Update LEDs based on beta power
alt Beta power > threshold for 1s
MCU->>IR_Device: Send IR ON/OFF command (BCI-IR-Send)
MCU->>BLE_Client: Notify/toggle BLE characteristic (BLE-BCI-Server-Toggle)
end
end
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
NPG-Server/server.ino (1)
80-87: Button logic triggers on release, not press – verify intention.Because
buttonPinis configuredINPUT_PULLUP, the line is HIGH while idle and LOW when pressed.
The code toggles the LED whenbuttonState == HIGH, i.e. on button release.
If you want the toggle on actual press, change the test toLOW.NPG-Client/client.ino (1)
176-189: Scanning never stops after a successful connection – wastes power.
doScanis set totrueon discovery and you start a forever scan (start(0)) every second inloop().
Onceconnectedbecomestrue, consider callingBLEDevice::getScan()->stop();and resettingdoScan = false;.NPG-FFT/NPG-FFT.ino (1)
38-56: Filter design comment disagrees with configured sample rate.
SAMPLE_RATEis 512 Hz but the filter comments state 500 Hz. That mismatch shifts the notch/EEG band edges by ≈2.4 %.
Either regenerate the coefficients for 512 Hz or setSAMPLE_RATE 500to avoid spectral bias.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
IR-ON-OFF/IR-ON-OFF.ino(1 hunks)NPG-Client/client.ino(1 hunks)NPG-FFT/NPG-FFT.ino(1 hunks)NPG-Server/server.ino(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
NPG-FFT/NPG-FFT.ino (1)
207-208:⚠️ Potential issueGPIO 7 is a flash line – choose a different output pin.
Driving GPIO 7 on ESP32 will crash the MCU at boot. Use a safe GPIO pin instead.
- pinMode(7, OUTPUT); + const uint8_t alertPin = 25; // Define a constant for the pin + pinMode(alertPin, OUTPUT);Also update the digitalWrite calls:
- digitalWrite(7, HIGH); + digitalWrite(alertPin, HIGH);- digitalWrite(7, LOW); + digitalWrite(alertPin, LOW);
🧹 Nitpick comments (6)
NPG-Server/NPG-Server.ino (1)
23-23: Fix typo in comment.There's a typo in "examle" which should be "example".
- This examle is adapted from the client and server code provided by MoThunderz + This example is adapted from the client and server code provided by MoThunderzNPG-Client/NPG-Client.ino (3)
23-23: Fix typo in comment.There's a typo in "examle" which should be "example".
- This examle is adapted from the client and server code provided by MoThunderz + This example is adapted from the client and server code provided by MoThunderz
29-29: Remove commented-out include directive.The commented-out include appears to be unnecessary. Either uncomment it if needed or remove it to keep the code clean.
-//#include "BLEScan.h"
170-190: Consider reducing the delay time in the main loop.The 1-second delay in the main loop might reduce responsiveness. For BLE applications requiring quicker responses, consider using a shorter delay.
- delay(1000); + delay(100); // Shorter delay for more responsive operationNPG-FFT/NPG-FFT.ino (2)
97-100: Incorrect filter description in comments.The comment for the EEG filter incorrectly describes it as a band-stop filter when it appears to be a bandpass filter based on the implementation.
-// Band-Stop Butterworth IIR digital filter, generated using filter_gen.py. -// Sampling rate: 500.0 Hz, frequency: [48.0, 52.0] Hz. -// Filter is order 2, implemented as second-order sections (biquads). -// Reference: https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.butter.html +// Bandpass Butterworth IIR digital filter, generated using filter_gen.py. +// Sampling rate: 500.0 Hz, frequency: [0.5, 30.0] Hz for EEG signals. +// Filter is order 2, implemented as second-order sections (biquads). +// Reference: https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.butter.html
186-186: Add explicit float suffix for numeric constant.For consistency with other float comparisons in the code, add an 'f' suffix to the numeric constant.
- if(((smoothedPowers.beta/ T)*100)>20.0) + if(((smoothedPowers.beta/ T)*100)>20.0f)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
IR-ON-OFF/IR-ON-OFF.ino(1 hunks)NPG-Client/NPG-Client.ino(1 hunks)NPG-FFT/NPG-FFT.ino(1 hunks)NPG-Server/NPG-Server.ino(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- IR-ON-OFF/IR-ON-OFF.ino
🔇 Additional comments (10)
NPG-Server/NPG-Server.ino (5)
28-54: Well-structured BLE initialization with proper variable declaration.The code correctly initializes all necessary BLE components with clear naming conventions and appropriate comments.
69-78: Good implementation of BLE server callbacks.The server callback implementation properly handles connection and disconnection events, updating the deviceConnected flag appropriately.
80-134: Setup function is well-organized and follows BLE best practices.The setup function properly initializes all components in a logical sequence, including GPIO configuration, BLE server creation, service and characteristic setup, and advertising configuration.
136-186: Loop function implements proper button debouncing and state management.The main loop effectively handles button debouncing, LED state toggling, and BLE connection state management. The debounce implementation follows best practices for handling mechanical button input.
35-35:❓ Verification inconclusive
GPIO 7 is connected to SPI flash on most ESP32 boards.
Using GPIO 7 for the LED may cause system instability as it's typically connected to the SPI flash on ESP32 boards.
- const int ledPin = 7; // the number of the LED pin + const int ledPin = 21; // the number of the LED pin (use a safer GPIO like 21)
🏁 Script executed:
#!/bin/bash # Check documentation on safe GPIO pins for ESP32 rg -A 10 "GPIO pinout|GPIO restrictions|SPI flash|Strapping pins" --glob "*.md"Length of output: 81
Verify ESP32 GPIO Usage for LED Pin
I couldn’t find any in-repo documentation confirming that GPIO7 is tied to the SPI flash on your target ESP32 board. Please check the official Espressif datasheet or your board’s schematic to be sure. If GPIO7 is indeed reserved for SPI flash, switch to a safer pin (e.g., GPIO21):Affected location:
- NPG-Server/NPG-Server.ino, line 35
Suggested change:
- const int ledPin = 7; // the number of the LED pin + const int ledPin = 21; // safer GPIO (not used by SPI flash)NPG-Client/NPG-Client.ino (2)
46-65: Well-implemented notification callback with proper data conversion.The notification callback correctly identifies the characteristic, converts the received bytes to an integer, and updates the GPIO output accordingly.
78-116: Robust server connection function with proper error handling.The
connectToServerfunction properly establishes a connection to the BLE server, discovers services and characteristics, and includes appropriate error handling.NPG-FFT/NPG-FFT.ino (3)
212-231: Well-implemented timing mechanism for precise sampling rate.The loop function uses an elegant timing approach to maintain the desired sampling rate regardless of processing time variations. This is a robust implementation for real-time signal processing.
138-146: Good error handling in FFT initialization.The FFT initialization function properly checks for errors and halts execution if the initialization fails, which is a good practice for robust embedded systems.
113-136: Well-designed bandpower calculation with frequency band separation.The bandpower calculation functions efficiently separate signal power into standard EEG frequency bands and implement appropriate smoothing using exponential moving average, which is ideal for real-time signal analysis.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (19)
BLE-Server/BLE-Server.ino (2)
56-60: Unused variable detected.The variable
valueon line 60 is initialized but never used in the code.-// Variable that will continuously be increased and written to the client -uint32_t value = 0;
95-108: BLE Service and Characteristics creation.Service and characteristics are properly created with appropriate properties. Note that pCharacteristic_2 is defined with read, write, and notify properties but is not used in the main code logic.
Consider either removing the unused characteristic or implementing its functionality in the loop.
BLE-Client/BLE-Client.ino (6)
28-30: Unnecessary commented code.The BLEScan include is commented out but not needed since BLEDevice.h already includes it.
-//#include "BLEScan.h"
47-65: Notification callback function implementation.The callback properly handles incoming notifications, checks the characteristic UUID, and controls the output pin based on received data.
There's a potential issue with bit shifting in the counter conversion logic that may not handle multi-byte data correctly.
Consider using a more robust method for multi-byte data conversion:
- uint32_t counter = pData[0]; - for(int i = 1; i<length; i++) { - counter = counter | (pData[i] << i*8); - } + uint32_t counter = 0; + for(int i = 0; i < length && i < sizeof(counter); i++) { + counter |= (uint32_t)pData[i] << (i * 8); + }
62-63: Hardcoded pin control without pinMode configuration.Pin 6 is controlled here but its pinMode is set much later in the setup function. This is a logical ordering issue.
Ensure all pins have their modes set before they are used in callbacks that might execute out of sequence.
68-76: Client callback class implementation.The callback class correctly handles connection and disconnection events, but the onConnect method is empty.
Consider adding connection status logging in the onConnect method for symmetry with onDisconnect:
void onConnect(BLEClient* pclient) { + Serial.println("Connected to server"); }
79-116: connectToServer function implementation.The function correctly handles server connection, service discovery, and characteristic verification. However, there's a typo in the error message on line 180.
- Serial.println("We have failed to connect to the server; there is nothin more we will do."); + Serial.println("We have failed to connect to the server; there is nothing more we will do.");
170-189: Main loop implementation.The loop function correctly handles connection attempts and scan management, but the 1-second delay seems excessive and may affect responsiveness.
Consider reducing the delay time or implementing a more responsive approach:
- delay(1000); + delay(100); // Reduced delay for better responsivenessIR-Send/IR-Send.ino (2)
32-33: Non-descriptive variable name.The variable
cis used as a counter but has a non-descriptive name.-int c=0; +int toggleCounter=0;
51-70: Main loop with button detection and IR command sending.The loop function properly detects button presses and sends IR commands. However, it uses a simple delay for debouncing instead of proper debounce logic.
Consider implementing proper debounce logic similar to the BLE-Server example:
-void loop() { - bool pressed = digitalRead(BOOT_BUTTON) == LOW; - if (pressed && !lastPressed) - { // LOW edge detected - if (c % 2 == 0) - { - Serial.println("→ Sending AC ON"); - irsend.sendLG(kAcOn, 28); - delay(250); - } - else - { - Serial.println("→ Sending AC OFF"); - irsend.sendLG(kAcOff, 28); - delay(250); - } - c=c+1; - } - lastPressed = pressed; -} +unsigned long lastDebounceTime = 0; +unsigned long debounceDelay = 50; + +void loop() { + bool reading = digitalRead(BOOT_BUTTON) == LOW; + + if (reading != lastPressed) { + lastDebounceTime = millis(); + } + + if ((millis() - lastDebounceTime) > debounceDelay) { + if (reading && !lastPressed) { // LOW edge detected and debounced + if (toggleCounter % 2 == 0) { + Serial.println("→ Sending AC ON"); + irsend.sendLG(kAcOn, 28); + } else { + Serial.println("→ Sending AC OFF"); + irsend.sendLG(kAcOff, 28); + } + toggleCounter++; + } + } + + lastPressed = reading; +}Serial-FFT/Serial-FFT.ino (5)
73-94: Notch filter implementation.The 48-52 Hz notch filter is implemented as a cascade of second-order sections with fixed coefficients.
Consider adding comments about the filter's expected performance characteristics and limitations for future maintenance.
96-110: Low-pass filter implementation.The 45 Hz low-pass filter is implemented correctly, but there's an inconsistency in coefficient formatting.
- float x = output - -1.22465158 * z1 - 0.45044543 * z2; + float x = output - (-1.22465158 * z1) - (0.45044543 * z2);
185-190: Beta threshold detection and pin control.The beta band power threshold detection is implemented correctly, but uses a hardcoded threshold value.
- if(((smoothedPowers.beta/ T)*100)>20.0) + #define BETA_THRESHOLD 20.0 // Beta power threshold as percentage of total power + if(((smoothedPowers.beta/ T)*100) > BETA_THRESHOLD)
192-199: Serial output formatting.The code outputs relative bandpower values and peak frequency, but doesn't include column headers.
Consider adding a periodic header row or documenting the output format in comments.
211-230: Main loop with precise timing.The main loop implements a precise sampling timer using micros() and processes FFT when the buffer is full.
However, the code doesn't handle the case where processing takes longer than sampling, which could lead to buffer overruns.
Consider adding a check for processing time to ensure timing accuracy:
if(idx >= FFT_SIZE){ + unsigned long processStart = micros(); processFFT(); + unsigned long processTime = micros() - processStart; + // Log if processing is taking too long + if (processTime > (1000000 / SAMPLE_RATE) * 10) { + Serial.print("Warning: Processing time too long: "); + Serial.println(processTime); + } idx = 0; }BCI-IR-Send/BCI-IR-Send.ino (4)
221-228: Comment says “4 s” but code waits 1 s → update one or the other
The inline comment on line 227 references a 4-second hold, yet the condition is>= 1000UL(1 s). This mismatch can confuse maintainers and leads to wrong expectations during neuro-feedback testing.- // 2) already timing → has 4 s elapsed? + // 2) already timing → has 1 s elapsed?(or change the constant to
4000UL).
219-228: Magic numbers: 20 % threshold & 1 s dwell deserve#defines
Hard-coding behavioural parameters makes future tuning harder and obscures intent. Condense them into named constants near the user-config block:+#define BETA_TRIGGER_PCT 20.0f // % +#define TRIGGER_DWELL_MS 1000UL // ms ... - if (((smoothedPowers.beta/ T)*100)>20.0) + if (((smoothedPowers.beta / T) * 100) > BETA_TRIGGER_PCT) ... - else if ((millis() - timer1Start) >= 1000UL) + else if ((millis() - timer1Start) >= TRIGGER_DWELL_MS)Improves readability and avoids scattering constants through DSP code.
276-279:timervariable name collides with timer-related globals – rename for clarity
The staticlong timer(sampling scheduler) is unrelated totimer1Start/timerRunning. Consider renaming tosampleTickeror similar to prevent mental collisions when scanning the code.
45-50: Use a boolean for AC power state instead of integer counterc
c % 2toggles power but grows unbounded over runtime (risking overflow on very long sessions). A simplebool acOnexpresses intent and removes the modulo:-int c = 0; +bool acOn = false; ... - if (c%2==0) { + if (!acOn) { Serial.println("→ Sending AC ON"); irsend.sendLG(kAcOn, 28); - } - else { + acOn = true; + } else { Serial.println("→ Sending AC OFF"); irsend.sendLG(kAcOff, 28); + acOn = false; } - c=c+1;Improves readability and avoids unnecessary arithmetic.
Also applies to: 229-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
BCI-IR-Send/BCI-IR-Send.ino(1 hunks)BLE-Client/BLE-Client.ino(1 hunks)BLE-Server/BLE-Server.ino(1 hunks)IR-Send/IR-Send.ino(1 hunks)Serial-FFT/Serial-FFT.ino(1 hunks)
🔇 Additional comments (39)
BLE-Server/BLE-Server.ino (16)
1-21: Appropriate GPL v3 licensing and project attribution.The licensing information and project attribution are well-structured, providing clear copyright information and project mission statement.
22-26: Good practice: Source attribution included.Proper attribution to the original source code is provided, including GitHub repository link and YouTube reference, which is excellent for open-source projects.
28-31: Required BLE libraries correctly imported.All necessary BLE libraries for ESP32 BLE server implementation are properly included.
33-36: GPIO pin assignments for button and LED.Pin 9 for button and pin 7 for LED are assigned as constants, following good programming practices.
37-46: Well-structured variable declarations with comments.Variables for state tracking and debounce logic are properly defined with appropriate data types and clear comments.
47-54: BLE object pointers correctly initialized.All BLE-related pointers are properly declared for later initialization in the setup function.
62-68: Well-defined UUIDs with documentation.Service and characteristic UUIDs are properly defined with reference to UUID generation source.
69-78: BLE server callbacks correctly implemented.The callback class properly implements connection and disconnection event handling.
80-87: Proper initialization in setup function.Serial communication, pin modes, and initial LED state are correctly configured.
88-93: BLE device and server initialization.BLE device and server are properly initialized with appropriate callbacks.
110-123: BLE descriptors properly configured.Descriptors and BLE2902 notification enablers are correctly added to the characteristics.
124-134: BLE service start and advertising configuration.Service is properly started and advertising is correctly configured with appropriate parameters.
136-169: Button debounce logic well implemented.The button reading and debounce logic are well-implemented, with proper timing and state tracking.
170-171: LED state update.LED state is properly updated based on the button logic.
173-183: BLE reconnection handling.The code properly handles disconnection and reconnection by restarting advertising when needed.
184-186: Button state tracking for next iteration.The current button state is properly saved for comparison in the next loop iteration.
BLE-Client/BLE-Client.ino (6)
22-26: Source attribution included.Proper attribution to the original source code, maintaining consistency with the server file.
32-34: UUIDs match server configuration.The UUIDs correctly match those defined in the BLE-Server file, ensuring compatibility.
36-45: BLE connection state variables and pointers.Connection state tracking variables and BLE pointers are properly defined.
119-131: Connect characteristic helper function.The function properly verifies characteristic existence and sets up notification callbacks.
134-150: Advertised device callbacks implementation.The callback properly filters for devices advertising the target service and sets up connection flags.
152-168: Setup function implementation.The setup function correctly initializes serial communication, pin modes, and BLE scanning with appropriate parameters.
IR-Send/IR-Send.ino (7)
1-23: Well-structured license and attribution information.The GPL licensing and attribution information is properly included with multiple authors credited.
24-27: Required IR libraries imported.All necessary libraries for IR transmission on ESP32 are properly included.
28-31: Pin assignments clearly defined.IR LED and boot button pins are clearly defined as constants.
34-34: IRsend object initialization.IRsend object is properly initialized with the specified pin.
36-39: IR codes defined as constants.LG AC IR codes are properly defined as constants with helpful comments.
41-47: Setup function implementation.The setup function properly initializes pins, serial communication, and the IR sender.
49-49: Static variable for button state tracking.A static variable is correctly used to track the last button state.
Serial-FFT/Serial-FFT.ino (10)
1-33: Thorough licensing and attribution information.Proper GPL licensing with multiple authors credited and references to the original ESP-DSP examples.
35-57: Well-defined configuration constants and EEG bands.Sample rate, FFT size, and EEG frequency bands are clearly defined as constants.
59-71: Buffer allocation and data structures.Input and FFT buffers are properly allocated with appropriate data structures for EEG band powers.
112-126: Bandpower calculation function.The function correctly calculates power in different EEG frequency bands from the FFT results.
128-135: Smoothing function for bandpower values.Exponential smoothing is properly implemented for all bandpower values.
137-145: FFT initialization with error handling.The FFT initialization includes proper error handling with clear user feedback.
148-166: FFT processing implementation.The signal packing and FFT processing are implemented correctly using the two-real FFT trick.
168-178: Peak frequency detection logic.The code correctly identifies the peak frequency in the power spectrum.
180-183: Bandpower calculation and smoothing.The bandpower calculation and smoothing are correctly applied to the FFT results.
201-209: Setup function implementation.The setup function correctly initializes serial, pins, and FFT.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
BCI-IR-Send/BCI-IR-Send.ino (1)
90-130: Properly tuned filters for 512 Hz sampling rateGood job updating the filter coefficients to be properly tuned for the 512 Hz sampling rate as mentioned in the previous review comments. The notch filter targets 48-52 Hz and the low-pass filter has a 45 Hz cutoff.
🧹 Nitpick comments (10)
Serial-FFT/Serial-FFT.ino (6)
38-58: Consider defining threshold constants and documenting value choicesThe code has well-defined constants for sampling rate, FFT size, and EEG bands. Consider also defining the beta threshold (20.0%) used on line 188 as a named constant for better maintainability.
+ // Threshold settings + #define BETA_THRESHOLD 20.0f // Beta threshold as percentage of total power
56-57: Document the rationale behind SMOOTHING_FACTOR and EPS valuesThe chosen values for SMOOTHING_FACTOR (0.63f) and EPS (1e-7f) would benefit from brief comments explaining their selection criteria and impact on signal processing.
-#define SMOOTHING_FACTOR 0.63f -#define EPS 1e-7f +// Smoothing factor of 0.63 balances responsiveness and stability +#define SMOOTHING_FACTOR 0.63f +// Small epsilon to prevent division by zero +#define EPS 1e-7f
151-202: Consider adding comments to clarify the FFT mathematicsWhile the FFT implementation correctly follows ESP-DSP examples, adding brief comments explaining the mathematical principles behind key steps like the two-real FFT trick would help future maintainers.
// FFT dsps_fft2r_fc32(y_cf, FFT_SIZE); + // Perform bit reversal to get correct output order dsps_bit_rev_fc32(y_cf, FFT_SIZE); + // Convert complex output to two real outputs using the two-real FFT trick dsps_cplx2reC_fc32(y_cf, FFT_SIZE);
183-193: Consider improving the beta power threshold documentation and implementationThe threshold for beta power is hardcoded without explanation. Consider adding a comment explaining the significance of 20% threshold and turning this into a named constant.
- if(((smoothedPowers.beta/ T)*100)>20.0) + // Beta power > 20% of total power indicates cognitive engagement/focus + if(((smoothedPowers.beta/ T)*100) > BETA_THRESHOLD)
214-233: Consider adding overflow protection for micros() timerWhile the microcontroller will run for hours before micros() overflows, a robust implementation would handle potential overflow to prevent timing issues in long-running applications.
static unsigned long lastMicros = micros(); unsigned long now = micros(), dt = now - lastMicros; + // Handle potential overflow + if (now < lastMicros) { + dt = (0xFFFFFFFF - lastMicros) + now + 1; + } lastMicros = now;
225-226: Consider centering ADC readings before filteringWhile not strictly necessary, centering the ADC readings (subtracting 2048 for 12-bit ADC) before filtering can improve filter performance by removing DC offset:
int raw = analogRead(INPUT_PIN); -float filt = EEGFilter(Notch(raw)); +// Center ADC readings around zero +float centered = (float)raw - 2048.0f; +float filt = EEGFilter(Notch(centered));BCI-IR-Send/BCI-IR-Send.ino (4)
42-54: Improve variable naming and IR code documentationThe counter variable
cis not descriptive. Consider renaming it to something more meaningful liketoggleCount. Also, enhance the IR code comments to explain their source and meaning more clearly.-// Counter for On/Off -int c=0; +// Counter to toggle between AC On/Off commands +int toggleCount = 0; // LG‐AC codes are 28 bits long. Replace these with YOUR captured values: const uint32_t kAcOff = 0x88C0051; -//– ON (whatever your "power on" capture reversed to) +// Power ON code - captured from LG AC remote control const uint32_t kAcOn = 0x8800B4F;
222-251: Consider extracting IR control logic into a separate functionThe IR control logic embedded in processFFT() would be clearer if extracted into a dedicated function. Also, consider using a named constant for the threshold duration (1000ms).
+// Duration in milliseconds that beta power must exceed threshold before triggering IR +#define BETA_THRESHOLD_DURATION 1000UL +// Send IR command based on current toggle state +void sendIRCommand() { + if (toggleCount % 2 == 0) { + Serial.println("→ Sending AC ON"); + irsend.sendLG(kAcOn, 28); + } else { + Serial.println("→ Sending AC OFF"); + irsend.sendLG(kAcOff, 28); + } + toggleCount++; +} // In processFFT(): - else if ((millis() - timer1Start) >= 1000UL) + else if ((millis() - timer1Start) >= BETA_THRESHOLD_DURATION) { - if (c%2==0) { - Serial.println("→ Sending AC ON"); - irsend.sendLG(kAcOn, 28); - } - else { - Serial.println("→ Sending AC OFF"); - irsend.sendLG(kAcOff, 28); - } - c=c+1; + sendIRCommand(); stopTimer(); // reset for next detection
262-269: Remove or explain commented out codeThere's a commented out pinMode call for pin 7. Either remove it if it's unnecessary or add a comment explaining why it's disabled.
- // pinMode(7, OUTPUT); + // pinMode(7, OUTPUT); // Uncomment to enable LED indicator for debuggingor
- // pinMode(7, OUTPUT);
273-286: Consider implementing timeout for long-running operationsThe current loop implementation doesn't handle potential blocking scenarios. Adding a simple timeout mechanism could make the system more robust.
void loop() { static uint16_t idx = 0; static unsigned long lastMicros = micros(); + static unsigned long operationStartTime = millis(); unsigned long now = micros(), dt = now - lastMicros; lastMicros = now; + // Safety timeout check - reset if stuck for too long + if (millis() - operationStartTime > 5000) { // 5 second timeout + Serial.println("Operation timeout - resetting"); + idx = 0; + operationStartTime = millis(); + } static long timer = 0; timer -= dt; if(timer <= 0){ timer += 1000000L / SAMPLE_RATE; int raw = analogRead(INPUT_PIN); float filt = EEGFilter(Notch(raw)); inputBuffer[idx++] = filt; } if(idx >= FFT_SIZE){ processFFT(); idx = 0; + operationStartTime = millis(); // Reset timer after successful operation } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
BCI-IR-Send/BCI-IR-Send.ino(1 hunks)Serial-FFT/Serial-FFT.ino(1 hunks)
🔇 Additional comments (9)
Serial-FFT/Serial-FFT.ino (7)
1-33: Well-structured license and attribution headerGood job including proper GPL license headers, copyright notices, and clear attribution to the ESP-DSP examples that the FFT routines are based on. This ensures proper compliance and gives credit where due.
74-96: Properly tuned Notch filter coefficients for 512 Hz sampling rateThe notch filter is correctly implemented as a Butterworth IIR with coefficients tuned specifically for the 512 Hz sampling rate, targeting 48-52 Hz rejection (power line noise).
98-113: Properly tuned Low-pass filter for 45 Hz cutoffThe low-pass filter implementation is appropriate for EEG signal conditioning, with coefficients correctly designed for the 512 Hz sampling rate and 45 Hz cutoff.
116-129: Well-implemented bandpower calculationThe function correctly calculates power across standard EEG frequency bands (delta, theta, alpha, beta, gamma) and total power by iterating through frequency bins.
131-138: Effective exponential smoothing implementationThe smoothing function correctly applies exponential smoothing to each frequency band, creating a good balance between responsiveness and stability.
141-148: Good FFT initialization with error handlingThe FFT initialization includes appropriate error handling that halts execution if initialization fails, preventing unpredictable behavior.
204-212: Standard Arduino setup with proper initializationThe setup function correctly initializes serial communication, input/output pins, and FFT processing.
BCI-IR-Send/BCI-IR-Send.ino (2)
1-35: Well-structured license and attribution headerThe file includes proper GPL license headers, copyright notices, and clear attribution to both ESP-DSP examples and IRremote reference code.
167-182: Well-implemented timer functionality for IR trigger controlThe timer implementation is clean and encapsulated in dedicated functions. This makes the threshold-based IR triggering logic easier to understand and maintain.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
NPG-Default/NPG-Default.ino (3)
14-16: Update copyright year to current date.The copyright notices specify the year 2025, which is in the future. Consider updating to the current year or using a range (e.g., "2023-2025") to reflect when the work began.
-// Copyright (c) 2025 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech -// Copyright (c) 2025 Deepak Khatri - deepak@upsidedownlabs.tech -// Copyright (c) 2025 Upside Down Labs - contact@upsidedownlabs.tech +// Copyright (c) 2023 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech +// Copyright (c) 2023 Deepak Khatri - deepak@upsidedownlabs.tech +// Copyright (c) 2023 Upside Down Labs - contact@upsidedownlabs.tech
31-40: Include power management considerations.Since this is for the NPG-Lite firmware which may operate on battery power, consider adding power management. The NeoPixel strip can consume significant power during continuous operation.
Consider adding options to:
- Adjust brightness dynamically
- Add a sleep/power-saving mode
- Include a timeout for animations
For example, you could add:
#define BATTERY_VOLTAGE_PIN A0 #define AUTO_SLEEP_TIMEOUT 60000 // Auto sleep after 60 seconds // In setup(): pinMode(BATTERY_VOLTAGE_PIN, INPUT); // Function to adjust brightness based on battery level void adjustBrightnessForBattery() { int batteryLevel = analogRead(BATTERY_VOLTAGE_PIN); // Map battery level to brightness (adjust values based on your battery) int brightness = map(batteryLevel, 2800, 4200, 20, 50); brightness = constrain(brightness, 20, 50); strip.setBrightness(brightness); }
114-149: Consider adding optional error handling in setup().The setup function initializes hardware but doesn't check if the initialization was successful. Consider adding some simple diagnostics to verify the hardware is connected and responding.
Add basic error checking/reporting:
void setup() { + Serial.begin(115200); + Serial.println("NPG-Default initializing..."); pinMode(LED_MOTOR_PIN, OUTPUT); strip.begin(); // INITIALIZE NeoPixel strip object (REQUIRED) strip.show(); // Turn OFF all pixels ASAP strip.setBrightness(50); // Set BRIGHTNESS to about 1/5 (max = 255) + + // Simple test to verify NeoPixel strip is connected + boolean pixelTest = false; + for(int i=0; i<strip.numPixels(); i++) { + strip.setPixelColor(i, strip.Color(255, 0, 0)); + strip.show(); + pixelTest = true; + delay(50); + strip.setPixelColor(i, strip.Color(0, 0, 0)); + strip.show(); + } + + if (!pixelTest) { + Serial.println("Warning: NeoPixel strip may not be connected!"); + } else { + Serial.println("NeoPixel strip initialized successfully."); + } // Rest of the setup function...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
NPG-Default/layout.pngis excluded by!**/*.pngNPG-Default/schematic.pngis excluded by!**/*.png
📒 Files selected for processing (3)
NPG-Default/NPG-Default.ino(1 hunks)NPG-Default/pitches.h(1 hunks)NPG-Default/toneMelody.txt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- NPG-Default/toneMelody.txt
- NPG-Default/pitches.h
🔇 Additional comments (1)
NPG-Default/NPG-Default.ino (1)
31-32:✅ Verification successful
Verify dependency file is included in the PR.
This sketch depends on "pitches.h" which should be included in the PR. Make sure this file is present alongside the sketch to ensure it works properly.
🏁 Script executed:
#!/bin/bash # Check if pitches.h exists in the same directory if [ -f "NPG-Default/pitches.h" ]; then echo "Found pitches.h in the NPG-Default directory." echo "Content preview:" head -n 10 "NPG-Default/pitches.h" else echo "ERROR: pitches.h not found in NPG-Default directory!" fiLength of output: 453
pitches.h Dependency Confirmed
The file
NPG-Default/pitches.his present in the PR and contains the expected note definitions. No further action is required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
NPG-Default/NPG-Default.ino (1)
1-29: Clarify NPG-Default's role in the BCI ecosystem.This sketch appears to be a default animation/sound demo, but the PR objectives mention BCI (Brain-Computer Interface) examples. Consider adding comments explaining how this fits into the NPG-Lite BCI ecosystem.
/* Based on: - Arduino ToneMelody example (public domain) - Adafruit strandtest example (BSD license) Requires Adafruit NeoPixel library (install via Arduino Library Manager) + + This sketch serves as the default animation & sound demo for NPG-Lite. + For BCI-focused examples, see other sketches in this repository: + - BCI-IR-Send/BCI-IR-Send.ino - IR-based BCI example + - Serial-FFT/Serial-FFT.ino - EEG data FFT example + - [Other BCI examples in this repository] */
🧹 Nitpick comments (3)
NPG-Default/NPG-Default.ino (3)
14-16: Update the copyright year to current year.The copyright notices are set to 2025, which is in the future. Please update them to reflect the current year or the year when the code was actually written.
-// Copyright (c) 2025 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech -// Copyright (c) 2025 Deepak Khatri - deepak@upsidedownlabs.tech -// Copyright (c) 2025 Upside Down Labs - contact@upsidedownlabs.tech +// Copyright (c) 2023 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech +// Copyright (c) 2023 Deepak Khatri - deepak@upsidedownlabs.tech +// Copyright (c) 2023 Upside Down Labs - contact@upsidedownlabs.tech
163-163: Fix comment remnant in line 163.There appears to be an incomplete comment at the end of line 163. This looks like a remnant from the previously reported code duplication issue.
- theaterChaseRainbow(50); // Rainbow-enhanced theaterChase variant// Fill along the length of the strip in various colors... + theaterChaseRainbow(50); // Rainbow-enhanced theaterChase variant
114-149: Consider breaking down the setup function for better readability.The setup function combines multiple initialization tasks. For better maintainability, consider breaking it down into smaller functions.
+void initializeHardware() { + pinMode(LED_MOTOR_PIN, OUTPUT); + strip.begin(); // INITIALIZE NeoPixel strip object (REQUIRED) + strip.show(); // Turn OFF all pixels ASAP + strip.setBrightness(50); // Set BRIGHTNESS to about 1/5 (max = 255) +} + +void setStaticRainbow() { + // Set static rainbow colors on each pixel individually + strip.setPixelColor(0, strip.ColorHSV(0, 255, 255)); // Red + strip.setPixelColor(1, strip.ColorHSV(10922, 255, 255)); // Orange + strip.setPixelColor(2, strip.ColorHSV(21845, 255, 255)); // Yellow + strip.setPixelColor(3, strip.ColorHSV(32768, 255, 255)); // Green + strip.setPixelColor(4, strip.ColorHSV(43690, 255, 255)); // Blue + strip.setPixelColor(5, strip.ColorHSV(54613, 255, 255)); // Purple + + strip.show(); // Update the strip with the new colors +} + +void playStartupMelody() { + // iterate over the notes of the melody: + for (int thisNote = 0; thisNote < 8; thisNote++) { + // to calculate the note duration, take one second divided by the note type. + //e.g. quarter note = 1000 / 4, eighth note = 1000/8, etc. + int noteDuration = 1000 / noteDurations[thisNote]; + tone(BUZZER_PIN, melody[thisNote], noteDuration); + + // to distinguish the notes, set a minimum time between them. + // the note's duration + 30% seems to work well: + int pauseBetweenNotes = noteDuration * 1.30; + digitalWrite(LED_MOTOR_PIN, HIGH); + delay(pauseBetweenNotes/2); + digitalWrite(LED_MOTOR_PIN, LOW); + delay(pauseBetweenNotes/2); + + // stop the tone playing: + noTone(BUZZER_PIN); + } +} + void setup() { - pinMode(LED_MOTOR_PIN, OUTPUT); - strip.begin(); // INITIALIZE NeoPixel strip object (REQUIRED) - strip.show(); // Turn OFF all pixels ASAP - strip.setBrightness(50); // Set BRIGHTNESS to about 1/5 (max = 255) - - // Set static rainbow colors on each pixel individually - strip.setPixelColor(0, strip.ColorHSV(0, 255, 255)); // Red - strip.setPixelColor(1, strip.ColorHSV(10922, 255, 255)); // Orange - strip.setPixelColor(2, strip.ColorHSV(21845, 255, 255)); // Yellow - strip.setPixelColor(3, strip.ColorHSV(32768, 255, 255)); // Green - strip.setPixelColor(4, strip.ColorHSV(43690, 255, 255)); // Blue - strip.setPixelColor(5, strip.ColorHSV(54613, 255, 255)); // Purple - - strip.show(); // Update the strip with the new colors - - // iterate over the notes of the melody: - for (int thisNote = 0; thisNote < 8; thisNote++) { - - // to calculate the note duration, take one second divided by the note type. - //e.g. quarter note = 1000 / 4, eighth note = 1000/8, etc. - int noteDuration = 1000 / noteDurations[thisNote]; - tone(BUZZER_PIN, melody[thisNote], noteDuration); - - // to distinguish the notes, set a minimum time between them. - // the note's duration + 30% seems to work well: - int pauseBetweenNotes = noteDuration * 1.30; - digitalWrite(LED_MOTOR_PIN, HIGH); - delay(pauseBetweenNotes/2); - digitalWrite(LED_MOTOR_PIN, LOW); - delay(pauseBetweenNotes/2); - - // stop the tone playing: - noTone(BUZZER_PIN); - } + initializeHardware(); + setStaticRainbow(); + playStartupMelody(); }
| void loop() { | ||
| // Fill along the length of the strip in various colors... | ||
| colorWipe(strip.Color(255, 0, 0), 50); // Red | ||
| colorWipe(strip.Color( 0, 255, 0), 50); // Green | ||
| colorWipe(strip.Color( 0, 0, 255), 50); // Blue | ||
|
|
||
| // Do a theater marquee effect in various colors... | ||
| theaterChase(strip.Color(127, 127, 127), 50); // White, half brightness | ||
| theaterChase(strip.Color(127, 0, 0), 50); // Red, half brightness | ||
| theaterChase(strip.Color( 0, 0, 127), 50); // Blue, half brightness | ||
|
|
||
| rainbow(10); // Flowing rainbow cycle along the whole strip | ||
| theaterChaseRainbow(50); // Rainbow-enhanced theaterChase variant// Fill along the length of the strip in various colors... | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make animations more interactive for future BCI integration.
Since this is part of a BCI project, consider structuring the animation sequence to allow for user interaction or integration with BCI components in the future.
+unsigned long lastAnimationChange = 0;
+int currentAnimation = 0;
+
+// Function to be called when a BCI event is detected
+void onBciEvent(int eventType) {
+ // Change animation based on BCI event
+ currentAnimation = eventType % 4; // Cycle through 4 animation types
+}
+
void loop() {
- // Fill along the length of the strip in various colors...
- colorWipe(strip.Color(255, 0, 0), 50); // Red
- colorWipe(strip.Color( 0, 255, 0), 50); // Green
- colorWipe(strip.Color( 0, 0, 255), 50); // Blue
-
- // Do a theater marquee effect in various colors...
- theaterChase(strip.Color(127, 127, 127), 50); // White, half brightness
- theaterChase(strip.Color(127, 0, 0), 50); // Red, half brightness
- theaterChase(strip.Color( 0, 0, 127), 50); // Blue, half brightness
-
- rainbow(10); // Flowing rainbow cycle along the whole strip
- theaterChaseRainbow(50); // Rainbow-enhanced theaterChase variant// Fill along the length of the strip in various colors...
+ // Switch animations every 10 seconds if no BCI input
+ unsigned long currentMillis = millis();
+ if (currentMillis - lastAnimationChange > 10000) {
+ currentAnimation = (currentAnimation + 1) % 4;
+ lastAnimationChange = currentMillis;
+ }
+
+ // Run the current animation
+ switch (currentAnimation) {
+ case 0:
+ // Red, Green, Blue color wipe
+ colorWipe(strip.Color(255, 0, 0), 50); // Red
+ colorWipe(strip.Color(0, 255, 0), 50); // Green
+ colorWipe(strip.Color(0, 0, 255), 50); // Blue
+ break;
+ case 1:
+ // Theater chase effects
+ theaterChase(strip.Color(127, 127, 127), 50); // White
+ theaterChase(strip.Color(127, 0, 0), 50); // Red
+ theaterChase(strip.Color(0, 0, 127), 50); // Blue
+ break;
+ case 2:
+ // Rainbow cycle
+ rainbow(10);
+ break;
+ case 3:
+ // Rainbow theater chase
+ theaterChaseRainbow(50);
+ break;
+ }
+
+ // This is where you would check for BCI input
+ // Example: if (serialAvailable()) { processBciInput(); }
}📝 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.
| void loop() { | |
| // Fill along the length of the strip in various colors... | |
| colorWipe(strip.Color(255, 0, 0), 50); // Red | |
| colorWipe(strip.Color( 0, 255, 0), 50); // Green | |
| colorWipe(strip.Color( 0, 0, 255), 50); // Blue | |
| // Do a theater marquee effect in various colors... | |
| theaterChase(strip.Color(127, 127, 127), 50); // White, half brightness | |
| theaterChase(strip.Color(127, 0, 0), 50); // Red, half brightness | |
| theaterChase(strip.Color( 0, 0, 127), 50); // Blue, half brightness | |
| rainbow(10); // Flowing rainbow cycle along the whole strip | |
| theaterChaseRainbow(50); // Rainbow-enhanced theaterChase variant// Fill along the length of the strip in various colors... | |
| } | |
| // Add at top of file (after existing includes/globals) | |
| unsigned long lastAnimationChange = 0; | |
| int currentAnimation = 0; | |
| // Function to be called when a BCI event is detected | |
| void onBciEvent(int eventType) { | |
| // Change animation based on BCI event | |
| currentAnimation = eventType % 4; // Cycle through 4 animation types | |
| } | |
| void loop() { | |
| // Switch animations every 10 seconds if no BCI input | |
| unsigned long currentMillis = millis(); | |
| if (currentMillis - lastAnimationChange > 10000) { | |
| currentAnimation = (currentAnimation + 1) % 4; | |
| lastAnimationChange = currentMillis; | |
| } | |
| // Run the current animation | |
| switch (currentAnimation) { | |
| case 0: | |
| // Red, Green, Blue color wipe | |
| colorWipe(strip.Color(255, 0, 0), 50); | |
| colorWipe(strip.Color(0, 255, 0), 50); | |
| colorWipe(strip.Color(0, 0, 255), 50); | |
| break; | |
| case 1: | |
| // Theater chase effects | |
| theaterChase(strip.Color(127, 127, 127), 50); | |
| theaterChase(strip.Color(127, 0, 0), 50); | |
| theaterChase(strip.Color(0, 0, 127), 50); | |
| break; | |
| case 2: | |
| // Rainbow cycle | |
| rainbow(10); | |
| break; | |
| case 3: | |
| // Rainbow theater chase | |
| theaterChaseRainbow(50); | |
| break; | |
| } | |
| // Placeholder for BCI input polling | |
| // e.g. if (serialAvailable()) { processBciInput(); } | |
| } |
There was a problem hiding this comment.
This is not required for the default program.
There was a problem hiding this comment.
@lorforlinux I understand your point - the default program should remain simple and focused on basic hardware demonstration without adding BCI-specific functionality. Makes sense to keep dedicated BCI features in the separate example files you're adding in this PR rather than complicating the default sketch. Thanks for the clarification.
✏️ Learnings added
Learnt from: lorforlinux
PR: upsidedownlabs/npg-lite-firmware#2
File: NPG-Default/NPG-Default.ino:151-164
Timestamp: 2025-05-10T11:44:39.422Z
Learning: The NPG-Default program should remain simple as a basic hardware demonstration without BCI functionality, as specialized features belong in dedicated example files.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| #define BUZZER_PIN 8 | ||
| #define LED_MOTOR_PIN 7 | ||
| #define PIXEL_PIN 15 | ||
| // How many NeoPixels are attached to the Arduino? | ||
| #define PIXEL_COUNT 6 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document pin assignments and verify for conflicts.
It's important to document why these specific pin assignments were chosen and verify they don't conflict with other NPG-Lite functionality, especially for BCI applications.
#define BUZZER_PIN 8 // Pin connected to the piezo buzzer
#define LED_MOTOR_PIN 7 // Pin connected to the motor indicator LED
#define PIXEL_PIN 15 // Pin connected to the NeoPixel data input
// How many NeoPixels are attached to the Arduino?
#define PIXEL_COUNT 6 // Number of NeoPixels in the stripRun the following to check for potential pin conflicts in other sketches:
🏁 Script executed:
#!/bin/bash
# Check for potential pin conflicts across sketches
echo "Checking for pin usage across sketches..."
echo "Pins used in NPG-Default: 7, 8, 15"
echo "Searching for pins in other files:"
rg -n "PIN.*[78]|PIN.*15" --type inoLength of output: 317
🏁 Script executed:
#!/bin/bash
# Check for potential pin conflicts across all .ino sketches
echo "Searching for pin usage across all .ino sketches..."
rg -n --glob '*.ino' "#define\s+\w+_PIN\s+(7|8|15)" || echo "No matching pin definitions found."Length of output: 325
No pin conflicts detected across sketches
Only NPG-Default/NPG-Default.ino defines BUZZER_PIN (8), LED_MOTOR_PIN (7) and PIXEL_PIN (15). Please add inline comments explaining why these pins were chosen (e.g. hardware pinout constraints, BCI signal isolation) and confirm they don’t conflict with any other hardware or future extensions.
• File: NPG-Default/NPG-Default.ino (lines 33–37)
#define BUZZER_PIN 8 // Pin connected to the piezo buzzer
#define LED_MOTOR_PIN 7 // Pin connected to the motor indicator LED
#define PIXEL_PIN 15 // Pin connected to the NeoPixel data input
// How many NeoPixels are attached to the Arduino?
#define PIXEL_COUNT 6 // Number of NeoPixels in the strip📝 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.
| #define BUZZER_PIN 8 | |
| #define LED_MOTOR_PIN 7 | |
| #define PIXEL_PIN 15 | |
| // How many NeoPixels are attached to the Arduino? | |
| #define PIXEL_COUNT 6 | |
| #define BUZZER_PIN 8 // Pin connected to the piezo buzzer | |
| #define LED_MOTOR_PIN 7 // Pin connected to the motor indicator LED | |
| #define PIXEL_PIN 15 // Pin connected to the NeoPixel data input | |
| // How many NeoPixels are attached to the Arduino? | |
| #define PIXEL_COUNT 6 // Number of NeoPixels in the strip |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
IR-LG-Send/IR-LG-Send.ino (3)
36-40: Consider using an enum or boolean for state tracking.Using a counter and modulo operation to track ON/OFF state is functional but not semantically clear. A boolean or enum would better communicate the intent.
-int c = 0; // Counter for maintaining ON/OFF state +bool acIsOn = false; // Boolean to track if AC is ON or OFF // The full 28-bit MSB-first codes captured by the receiver sketch static const uint32_t FULL_ON = 0x8800B0B; static const uint32_t FULL_OFF = 0x88C0051;Then in the loop function:
- if (c%2==0) { + if (!acIsOn) { Serial.println("→ Sending AC ON"); IrSender.sendLG(FULL_ON, 28); Serial.println(" [Done]\n"); delay(500); + acIsOn = true; } else { Serial.println("→ Sending AC OFF"); IrSender.sendLG(FULL_OFF, 28); Serial.println(" [Done]\n"); delay(500); + acIsOn = false; } - c=c+1;
33-34: Add comments explaining pin functions.The pin definitions could use more explicit comments about what each pin does, especially for hardware-specific pins like the IR_SEND_PIN.
-#define IR_SEND_PIN 22 // your IR-LED pin (via resistor/transistor) -#define BOOT_BUTTON 9 // NPG-Lite BOOT button (active LOW) +#define IR_SEND_PIN 22 // NPG-Lite IR-LED pin (using RMT peripheral via resistor/transistor) +#define BOOT_BUTTON 9 // NPG-Lite BOOT button (active LOW, triggers IR code transmission)
39-40: Add references to how the IR codes were obtained.The IR codes are defined but there's no mention of how they were captured/determined. Adding this information would be helpful for future maintenance.
// The full 28-bit MSB-first codes captured by the receiver sketch -static const uint32_t FULL_ON = 0x8800B0B; -static const uint32_t FULL_OFF = 0x88C0051; +// The full 28-bit MSB-first codes captured by the IR-LG-Receive sketch +static const uint32_t FULL_ON = 0x8800B0B; // Power ON for LG AC +static const uint32_t FULL_OFF = 0x88C0051; // Power OFF for LG AC
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
IR-LG-Receive/IR-LG-Receive.ino(1 hunks)IR-LG-Send/IR-LG-Send.ino(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- IR-LG-Receive/IR-LG-Receive.ino
🔇 Additional comments (1)
IR-LG-Send/IR-LG-Send.ino (1)
14-15:⚠️ Potential issueUpdate the copyright year to the current year.
The copyright notices show the year 2025, which is in the future. This should be updated to the current year (2023 or 2024) for accuracy.
-// Copyright (c) 2025 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech -// Copyright (c) 2025 Upside Down Labs - contact@upsidedownlabs.tech +// Copyright (c) 2024 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech +// Copyright (c) 2024 Upside Down Labs - contact@upsidedownlabs.techLikely an incorrect or invalid review comment.
| void loop() { | ||
| // When Boot button is pressed | ||
| if(digitalRead(BOOT_BUTTON) == LOW) | ||
| { | ||
| if (c%2==0) { | ||
| Serial.println("→ Sending AC ON"); | ||
| // sendLG automatically uses the correct 38 kHz carrier and timing for LG | ||
| IrSender.sendLG(FULL_ON, 28); | ||
| Serial.println(" [Done]\n"); | ||
| delay(500); | ||
| } | ||
| else { | ||
| Serial.println("→ Sending AC OFF"); | ||
| IrSender.sendLG(FULL_OFF, 28); | ||
| Serial.println(" [Done]\n"); | ||
| delay(500); | ||
| } | ||
| c=c+1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve button debouncing implementation.
The current implementation uses a simple delay for debouncing, which is not optimal. A time-based debounce approach would be more reliable.
+unsigned long lastButtonPressTime = 0;
+const unsigned long debounceDelay = 500; // 500ms debounce time
+bool buttonState = HIGH;
+bool lastButtonState = HIGH;
void loop() {
+ // Read the current button state
+ bool reading = digitalRead(BOOT_BUTTON);
+
+ // Check if the button state has changed
+ if (reading != lastButtonState) {
+ lastButtonPressTime = millis();
+ }
+
+ // If enough time has passed, check for a stable button press
+ if ((millis() - lastButtonPressTime) > debounceDelay) {
+ // If the button state has changed and is now stable
+ if (reading != buttonState) {
+ buttonState = reading;
+
+ // Only trigger on button press (LOW)
+ if (buttonState == LOW) {
+ if (c%2==0) {
+ Serial.println("→ Sending AC ON");
+ IrSender.sendLG(FULL_ON, 28);
+ } else {
+ Serial.println("→ Sending AC OFF");
+ IrSender.sendLG(FULL_OFF, 28);
+ }
+ Serial.println(" [Done]\n");
+ c=c+1;
+ }
+ }
+ }
+
+ lastButtonState = reading;
- // When Boot button is pressed
- if(digitalRead(BOOT_BUTTON) == LOW)
- {
- if (c%2==0) {
- Serial.println("→ Sending AC ON");
- // sendLG automatically uses the correct 38 kHz carrier and timing for LG
- IrSender.sendLG(FULL_ON, 28);
- Serial.println(" [Done]\n");
- delay(500);
- }
- else {
- Serial.println("→ Sending AC OFF");
- IrSender.sendLG(FULL_OFF, 28);
- Serial.println(" [Done]\n");
- delay(500);
- }
- c=c+1;
- }
}📝 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.
| void loop() { | |
| // When Boot button is pressed | |
| if(digitalRead(BOOT_BUTTON) == LOW) | |
| { | |
| if (c%2==0) { | |
| Serial.println("→ Sending AC ON"); | |
| // sendLG automatically uses the correct 38 kHz carrier and timing for LG | |
| IrSender.sendLG(FULL_ON, 28); | |
| Serial.println(" [Done]\n"); | |
| delay(500); | |
| } | |
| else { | |
| Serial.println("→ Sending AC OFF"); | |
| IrSender.sendLG(FULL_OFF, 28); | |
| Serial.println(" [Done]\n"); | |
| delay(500); | |
| } | |
| c=c+1; | |
| } | |
| } | |
| unsigned long lastButtonPressTime = 0; | |
| const unsigned long debounceDelay = 500; // 500ms debounce time | |
| bool buttonState = HIGH; | |
| bool lastButtonState = HIGH; | |
| void loop() { | |
| // Read the current button state | |
| bool reading = digitalRead(BOOT_BUTTON); | |
| // Check if the button state has changed | |
| if (reading != lastButtonState) { | |
| lastButtonPressTime = millis(); | |
| } | |
| // If enough time has passed, check for a stable button press | |
| if ((millis() - lastButtonPressTime) > debounceDelay) { | |
| // If the button state has changed and is now stable | |
| if (reading != buttonState) { | |
| buttonState = reading; | |
| // Only trigger on button press (LOW) | |
| if (buttonState == LOW) { | |
| if (c % 2 == 0) { | |
| Serial.println("→ Sending AC ON"); | |
| IrSender.sendLG(FULL_ON, 28); | |
| } else { | |
| Serial.println("→ Sending AC OFF"); | |
| IrSender.sendLG(FULL_OFF, 28); | |
| } | |
| Serial.println(" [Done]\n"); | |
| c = c + 1; | |
| } | |
| } | |
| } | |
| lastButtonState = reading; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
BCI-Server-Toggle/BCI-Server-Toggle.ino (1)
218-222: Avoid magic numbers – use the named pin constant
toggleSwitch()writes to GPIO 7 directly, while the rest of the sketch uses theLED_PINmacro.
Using the same symbolic name everywhere reduces the risk of wiring/port-change bugs.- digitalWrite(7, bci_val); + digitalWrite(LED_PIN, bci_val);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
BCI-Server-Toggle/BCI-Server-Toggle.ino(1 hunks)
🔇 Additional comments (1)
BCI-Server-Toggle/BCI-Server-Toggle.ino (1)
218-221: Send the characteristic value using explicit length to avoid type-size ambiguity
BLECharacteristic::setValue(uint32_t)is not available in all ESP32 BLE variants; some only provide overloads foruint8_t,uint16_t, or raw buffers.
Casting explicitly makes the code portable and future-proof:- pCharacteristic_1->setValue(bci_val); + pCharacteristic_1->setValue(reinterpret_cast<uint8_t*>(&bci_val), + sizeof(bci_val));
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
BCI-IR-Send/BCI-IR-Send.ino (1)
281-298: Comment still says “4 s” while the code checks 1 s – please align the twoA similar mismatch was flagged earlier in another sketch. Here the
millis()comparison is>= 1000UL(1 s) but line 297 claims “4 s”. This can mislead future maintainers.- // dropped below before 4 s → cancel timing + // dropped below before 1 s → cancel timing(Alternatively introduce
constexpr unsigned long HOLD_MS = 1000;and reference the constant in both code and comments.)BCI-Server-Toggle/BCI-Server-Toggle.ino (1)
297-318: “4 s” vs 1 s hold-time – comment/code mismatch persistsLine 305 checks for 1 000 ms yet line 313 still references “4 s”.
Please update or remove the stale comment to avoid confusion.- // dropped below before 4 s → cancel timing + // dropped below before 1 s → cancel timing
🧹 Nitpick comments (4)
BCI-IR-Send/BCI-IR-Send.ino (2)
47-56: Replace magic pin numbers with the already-defined constants for consistency
LED_PINis declared (line 54) but later you still hard-code7inpinMode()anddigitalWrite(). Mixing literals and constants easily drifts out of sync during refactors.- pinMode(7, OUTPUT); + pinMode(LED_PIN, OUTPUT); ... - digitalWrite(7, HIGH); + digitalWrite(LED_PIN, HIGH); ... - digitalWrite(7, LOW); + digitalWrite(LED_PIN, LOW);This keeps the pin assignment single-sourced and self-documenting.
190-206: Use a boolean for the ON/OFF state to avoid unbounded counter growthThe
ccounter increments forever and relies onc % 2to decide ON/OFF.
While overflow is unlikely in practice, it’s conceptually simpler (and cheaper) to store abool:-bool acState = false; // false = OFF, true = ON ... - if (c%2==0) { + if (!acState) { ... - } else { + } else { ... } - c = c + 1; + acState = !acState;Cleaner semantics and one less modulo per trigger.
BCI-Server-Toggle/BCI-Server-Toggle.ino (2)
330-345: Consistently use theLED_PINmacro instead of hard-coding7You already declared
#define LED_PIN 7; however,pinMode(7, …)anddigitalWrite(7, …)bypass the macro. This weakens self-documentation and increases the chance of errors if the pin ever changes.- pinMode(7, OUTPUT); + pinMode(LED_PIN, OUTPUT); ... - digitalWrite(7, bci_val); + digitalWrite(LED_PIN, bci_val);Apply the same replacement in
toggleSwitch()as well.
215-222: Prefer a boolean to represent the toggle stateLike the IR example,
bci_valalternates between 0 / 1 but is stored in auint32_t.
Switching toboolclarifies intent and removes needless arithmetic:-bci_val = (bci_val == 0) ? 1 : 0; // flip state +bci_val = !bci_val; // flip state pCharacteristic_1->setValue(static_cast<uint32_t>(bci_val));(No functional change – just clearer.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
BCI-IR-Send/BCI-IR-Send.ino(1 hunks)BCI-Server-Toggle/BCI-Server-Toggle.ino(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
BCI-Server-Toggle/BCI-Server-Toggle.ino (1)
Learnt from: CIumsy
PR: upsidedownlabs/npg-lite-firmware#2
File: BCI-Server-Toggle/BCI-Server-Toggle.ino:403-408
Timestamp: 2025-05-10T07:29:42.734Z
Learning: In the NPG-Lite firmware, raw ADC values from the ESP32 (0-4095 range) are directly passed to the IIR filter functions (Notch and EEGFilter) without explicit normalization or centering. The filter coefficients are designed to work with these un-scaled values.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
BCI-IR-Send/BCI-IR-Send.ino (1)
344-346: Consider centering analog readings for improved filter performanceThe ADC readings from analogRead() are in the range 0-4095 (for 12-bit resolution), but the filters are designed to work with zero-mean signals. This could affect filter performance and spectral analysis.
I see from the past review comments that you've decided this isn't necessary. While centering signals before filtering is generally a best practice in signal processing, I'll respect your implementation decision.
🧹 Nitpick comments (9)
Serial-FFT/Serial-FFT.ino (4)
14-16: Update copyright year to current yearThe copyright notices use the year 2025, which is in the future. Consider updating to the current year or the year the code was written.
-// Copyright (c) 2025 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech -// Copyright (c) 2025 Deepak Khatri - deepak@upsidedownlabs.tech -// Copyright (c) 2025 Upside Down Labs - contact@upsidedownlabs.tech +// Copyright (c) 2023 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech +// Copyright (c) 2023 Deepak Khatri - deepak@upsidedownlabs.tech +// Copyright (c) 2023 Upside Down Labs - contact@upsidedownlabs.tech
39-43: Define pin numbers as constants for readabilityPin 7 is used in the code but not defined as a constant here. Consider adding a constant for all pins used in the sketch.
#define SAMPLE_RATE 512 // samples per second #define FFT_SIZE 512 // must be a power of two #define BAUD_RATE 115200 #define INPUT_PIN A0 +#define OUTPUT_PIN 7 // pin used for beta threshold detection
188-193: Define the beta threshold as a constantThe 20.0% threshold for beta power is hardcoded. Consider defining it as a configuration constant at the top of the file for better maintainability.
// Add to the configuration section: +#define BETA_THRESHOLD 20.0f // beta power percentage to trigger output // Then modify this code: - if(((smoothedPowers.beta/ T)*100)>20.0) + if(((smoothedPowers.beta/ T)*100) > BETA_THRESHOLD)
209-210: Use the defined OUTPUT_PIN constantFor consistency, use the OUTPUT_PIN constant (which you should add as suggested earlier) instead of hardcoding pin 7.
void setup() { Serial.begin(BAUD_RATE); while(!Serial) { delay(1); } pinMode(INPUT_PIN, INPUT); - pinMode(7, OUTPUT); + pinMode(OUTPUT_PIN, OUTPUT); initFFT(); }BCI-IR-Send/BCI-IR-Send.ino (5)
14-16: Update copyright year to current yearThe copyright notices use the year 2025, which is in the future. Consider updating to the current year or the year the code was written.
-// Copyright (c) 2025 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech -// Copyright (c) 2025 Deepak Khatri - deepak@upsidedownlabs.tech -// Copyright (c) 2025 Upside Down Labs - contact@upsidedownlabs.tech +// Copyright (c) 2023 Krishnanshu Mittal - krishnanshu@upsidedownlabs.tech +// Copyright (c) 2023 Deepak Khatri - deepak@upsidedownlabs.tech +// Copyright (c) 2023 Upside Down Labs - contact@upsidedownlabs.tech
281-302: Comment and code timing mismatchThe comment on line 297 mentions "4 s" but the code on line 289 checks for 1000 milliseconds (1 second). Update either the comment or the code for consistency.
else { - // dropped below before 4 s → cancel timing + // dropped below before 1 s → cancel timing if (timerRunning) { stopTimer(); } }
256-279: Optimize NeoPixel updatesThe code repeatedly clears and sets the NeoPixels in every FFT cycle, which is inefficient and can cause flickering. Instead, only update the pixels when the band power changes enough to affect the display.
+ // Keep track of the previous LED count to avoid unnecessary updates + static int prevLedCount = 0; + int ledCount = 0; - pixels.clear(); // Sets all pixels to 0 (off) - pixels.show(); // Apply changes if (betaPerc >= 0 && betaPerc <= 2) { - showPixels(1); + ledCount = 1; } else if (betaPerc > 2 && betaPerc <= 5) { - showPixels(2); + ledCount = 2; } else if (betaPerc > 5 && betaPerc <= 9) { - showPixels(3); + ledCount = 3; } else if (betaPerc > 9 && betaPerc <= 12) { - showPixels(4); + ledCount = 4; } else if (betaPerc > 12 && betaPerc <= 15) { - showPixels(5); + ledCount = 5; } else if (betaPerc > 15) { - showPixels(6); + ledCount = 6; } - // Update LEDs once at the end (instead of after each change) - pixels.show(); + // Only update if the LED count changed + if (ledCount != prevLedCount) { + pixels.clear(); + showPixels(ledCount); + pixels.show(); + prevLedCount = ledCount; + }
259-276: Extract threshold values as constantsThe beta percentage thresholds for the NeoPixel levels are hardcoded. Define these as constants for better readability and maintainability.
// Add to the configuration section: +#define LEVEL1_THRESHOLD 0.0f // min beta percentage for level 1 +#define LEVEL2_THRESHOLD 2.0f // min beta percentage for level 2 +#define LEVEL3_THRESHOLD 5.0f // min beta percentage for level 3 +#define LEVEL4_THRESHOLD 9.0f // min beta percentage for level 4 +#define LEVEL5_THRESHOLD 12.0f // min beta percentage for level 5 +#define LEVEL6_THRESHOLD 15.0f // min beta percentage for level 6 +#define IR_TRIGGER_THRESHOLD 16.0f // beta percentage to trigger IR command // Then use these constants in the code: - if (betaPerc >= 0 && betaPerc <= 2) { + if (betaPerc >= LEVEL1_THRESHOLD && betaPerc <= LEVEL2_THRESHOLD) { showPixels(1); } - else if (betaPerc > 2 && betaPerc <= 5) { + else if (betaPerc > LEVEL2_THRESHOLD && betaPerc <= LEVEL3_THRESHOLD) { showPixels(2); } // Similarly update all the other if/else blocks
281-293: Define IR trigger threshold as a constantThe beta power threshold for triggering IR commands (16.0%) is hardcoded. Define this as a constant for better readability and maintainability.
// Add to the configuration section if not already added: +#define IR_TRIGGER_THRESHOLD 16.0f // beta percentage to trigger IR command // Then use this constant in the code: - if(betaPerc>16.0) + if(betaPerc > IR_TRIGGER_THRESHOLD)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
BCI-IR-Send/BCI-IR-Send.ino(1 hunks)BLE-Client/BLE-Client.ino(1 hunks)BLE-Server/BLE-Server.ino(1 hunks)Serial-FFT/Serial-FFT.ino(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- BLE-Server/BLE-Server.ino
- BLE-Client/BLE-Client.ino
| delay(250); | ||
| } | ||
| else { | ||
| Serial.println("→ Sending AC OFF"); | ||
| IrSender.sendLG(FULL_OFF, 28); | ||
| Serial.println(" [Done]\n"); | ||
| delay(250); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid blocking delays in IR sending function
Using delay() in the toggleSwitch() function can disrupt timing of the main EEG sampling loop. Consider using a non-blocking approach with millis().
void toggleSwitch()
{
if (c%2==0) {
Serial.println("→ Sending AC ON");
// sendLG automatically uses the correct 38 kHz carrier and timing for LG
IrSender.sendLG(FULL_ON, 28);
Serial.println(" [Done]\n");
- delay(250);
}
else {
Serial.println("→ Sending AC OFF");
IrSender.sendLG(FULL_OFF, 28);
Serial.println(" [Done]\n");
- delay(250);
}
c=c+1;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
BLE-BCI-Server-Toggle/BLE-BCI-Server-Toggle.ino (3)
44-52: Remove dead-code variables to keep the global state minimal
buttonState,lastButtonState,lastDebounceTime,debounceDelay, and the never-used countervalueare declared but never referenced.
In embedded work every byte of RAM matters and unused globals also make maintenance harder because future readers wonder whether they are part of the control logic.-// Variables will change: -uint32_t buttonState; // the current reading from the input pin -int lastButtonState = LOW; // the previous reading from the input pin -uint32_t bci_val = 0; -// the following variables are unsigned longs because the time, measured in -// milliseconds, will quickly become a bigger number than can be stored in an int. -unsigned long lastDebounceTime = 0; // the last time the output pin was toggled -unsigned long debounceDelay = 50; // the debounce time; increase if the output flickers +uint32_t bci_val = 0; // current switch state (0 / 1) ... -// Variable that will continuously be increased and written to the client -uint32_t value = 0;If you intend to add a physical button later, keep the variables but guard them with
// TODO:so the intention is explicit.Also applies to: 65-67
225-231: GuardshowPixels()against out-of-range indicesIf a future change accidentally calls
showPixels()withledCount > PIXEL_COUNT,pixels.setPixelColor()will write past the strip length, leading to undefined NeoPixel behaviour.-for(int i=1;i<=ledCount;i++) -{ - pixels.setPixelColor(i-1, pixels.Color(i*10, 0, 0)); // Level 1 to 6 -} +ledCount = constrain(ledCount, 0, PIXEL_COUNT); +for (uint8_t i = 0; i < ledCount; ++i) { + pixels.setPixelColor(i, pixels.Color((i + 1) * 10, 0, 0)); +}This tiny guard prevents hard-to-trace visual glitches.
395-409: Sampling viamicros()in theloop()can drift under high BLE loadThe tight timing loop achieves ~512 Hz only when the main
loop()finishes within ≈2 ms.
When BLE notifications or NeoPixel DMA stretch the cycle time,dt = micros() – lastMicrosspikes and causes sample-jitter. Over time this produces spectral leakage and reduces beta-band accuracy.Consider:
• Using an
hw_timer_tinterrupt to push ADC data into a ring buffer, or
• Leveraging the ESP32 ADC DMA (I2S0) peripheral, as done inesp32-fftexamples.Either option decouples sampling from application-layer delays and yields much cleaner FFTs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
BLE-BCI-Server-Toggle/BLE-BCI-Server-Toggle.ino(1 hunks)BLE-Client/BLE-Client.ino(1 hunks)BLE-Server/BLE-Server.ino(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- BLE-Server/BLE-Server.ino
- BLE-Client/BLE-Client.ino
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
BLE-BCI-Server-Toggle/BLE-BCI-Server-Toggle.ino (2)
334-340: Use theLED_PINmacro consistently
pinMode(7, OUTPUT);hard-codes the pin number and re-introduces the maintenance risk called out in the previous review. Stick to the symbolic constant so that only one definition must change if the wiring changes.- pinMode(7, OUTPUT); + pinMode(LED_PIN, OUTPUT);
218-221:⚠️ Potential issueAvoid implicit narrowing when updating the BLE characteristic
BLECharacteristic::setValue()is overloaded. Passing auint32_tdirectly selects theuint8_toverload, silently truncatingbci_valto one byte (0or1).
Transmit the full 32-bit value explicitly:- pCharacteristic_1->setValue(bci_val); + pCharacteristic_1->setValue(reinterpret_cast<uint8_t*>(&bci_val), + sizeof(bci_val));This keeps the on-air payload and the local
bci_valin sync and avoids undefined behaviour on little-/big-endian targets.
🧹 Nitpick comments (2)
BLE-BCI-Server-Toggle/BLE-BCI-Server-Toggle.ino (2)
64-67: Remove or use the unusedvaluecounter
uint32_t value = 0;is never read after initialisation, wasting RAM on a memory-constrained MCU.
Either delete it or implement its intended purpose (e.g. heartbeat packets).
224-231: Bound-checkshowPixels()to prevent out-of-range writesIf a future change passes
ledCount > PIXEL_COUNT,setPixelColor()will write past the strip buffer.void showPixels(int ledCount) { - for(int i=1;i<=ledCount;i++) + ledCount = constrain(ledCount, 0, PIXEL_COUNT); + for(int i = 1; i <= ledCount; ++i) { pixels.setPixelColor(i-1, pixels.Color(i*10, 0, 0)); } }
|
@CIumsy Thank you so much for your contribution, This is amazing! I am merging this now |
Added Arduino sketches for bidirectional TCP communication between 2 NPG-Lite (Server & Client), a real-time FFT example processing EEG signals on-device, and an IR-ON-OFF sketch that generates IR codes to toggle an AC unit’s power. Some more BCI examples to be added.
Summary by CodeRabbit