-
Notifications
You must be signed in to change notification settings - Fork 10
Feat spo2 #333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feat spo2 #333
Conversation
@coderabbitai perform a code review and suggest any other details that should be added to the PR |
🧩 Analysis chainI'll perform a comprehensive code review of this SpO2 feature addition. Let me first examine the code changes and key files to provide thorough feedback. 🏁 Script executed:
Length of output: 639 🏁 Script executed:
Length of output: 4775 🏁 Script executed:
Length of output: 4492 🏁 Script executed:
Length of output: 3329 🏁 Script executed:
Length of output: 1978 🏁 Script executed:
Length of output: 2587 🏁 Script executed:
Length of output: 4896 @jaketheduque I've performed a comprehensive code review of the SpO2 feature addition. Here's my detailed analysis and recommendations: Code Review Findings✅ What's Working Well
🔧 Code Quality Issues1. Missing Documentation// EmotiBit.h - Missing doxygen documentation
void processSpO2(); // ❌ No documentation
// Should be:
/**
* @brief Processes SpO2 calculation from PPG data buffers
* @details Accumulates IR and RED PPG data in static buffers and calculates
* SpO2 when sufficient data is available using Brainflow algorithm
*/
void processSpO2(); 2. Buffer Management Issues// EmotiBit.cpp - Current implementation has potential issues
if (irDataIndex >= SPO2_PPG_BUFFER_SIZE-15 && redDataIndex >= SPO2_PPG_BUFFER_SIZE-15) {
get_oxygen_level(irDataBuffer, redDataBuffer, min(irDataIndex, redDataIndex), &spo2); Issues:
Suggested improvement: // Add constants for better maintainability
#define SPO2_MIN_SAMPLES_REQUIRED 49 // SPO2_PPG_BUFFER_SIZE - 15
#define SPO2_ALGORITHM_OVERLAP 15 // Buffer overlap for algorithm stability
// Improve buffer management
if (irDataIndex >= SPO2_MIN_SAMPLES_REQUIRED && redDataIndex >= SPO2_MIN_SAMPLES_REQUIRED) {
// Add error handling for the algorithm call
int result = get_oxygen_level(irDataBuffer, redDataBuffer,
min(irDataIndex, redDataIndex), &spo2);
if (result == 0) { // Assuming 0 indicates success
addPacket(timestamp, EmotiBitPacket::TypeTag::SPO2, &spo2, APERIODIC_DATA_LEN);
}
// Reset indices after successful processing
irDataIndex = 0;
redDataIndex = 0;
} 3. Missing Error HandlingThe 4. Buffer Overflow Protection// Current code has potential buffer overflow
if (irDataIndex + irDataSize < SPO2_PPG_BUFFER_SIZE) {
// What happens if this condition fails? Data is silently dropped
} 📝 Missing PR Details1. Performance Impact Documentation
2. Configuration Options
3. Error Handling Strategy
4. Testing Evidence
🧪 Testing Recommendations1. Unit Tests Needed// Suggested test cases:
- Test buffer overflow conditions
- Test with insufficient data
- Test algorithm failure scenarios
- Test flag enable/disable functionality
- Test memory usage and buffer resets 2. Integration Tests
📋 Additional PR Details to Add1. Algorithm Details## SpO2 Algorithm Information
- **Source**: Brainflow SpO2 Algorithm v[version]
- **Method**: [Brief description of algorithm approach]
- **Accuracy**: ±[accuracy]% (specify test conditions)
- **Range**: [min-max]% SpO2 detection range
- **Sampling Requirements**: [requirements for reliable operation] 2. Configuration Section## Configuration Options
- `SPO2_PPG_BUFFER_SIZE`: Buffer size for algorithm (default: 64)
- `acquireData.spo2`: Enable/disable SpO2 processing (default: true)
- Minimum PPG data quality requirements 3. Performance Impact## Performance Impact
- **Memory**: +512 bytes static RAM for buffers
- **CPU**: ~[X]ms processing time per calculation
- **Output Rate**: SpO2 values every ~[X] seconds 4. Validation Results## Validation
- [ ] Tested against clinical-grade pulse oximeter
- [ ] Accuracy: ±[X]% over [range]% SpO2
- [ ] Tested with different skin tones/conditions
- [ ] Motion artifact handling verified 🔒 Security/Robustness Concerns
📖 Documentation Updates Needed
🚀 Recommendations for Future PRs
Would you like me to elaborate on any of these points or help you implement specific improvements? ✅ Actions performedReview triggered.
|
WalkthroughAdds SpO2 processing: new processSpO2 method, buffering IR/Red PPG and emitting SPO2 packets in processData when enabled. Updates header (API, buffers, firmware version, acquireData.spo2), build configs (library dependency, PlatformIO partitions and script), and minor logging/include order adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Sensor as MAX30101 (PPG)
participant EmotiBit as EmotiBit::processData
participant SpO2 as processSpO2
participant Algo as get_oxygen_level
participant Packets as addPacket(SPO2)
Sensor->>EmotiBit: Provide IR/Red PPG samples + timestamp
EmotiBit->>SpO2: If AcquireData.spo2, pass samples
SpO2->>SpO2: Buffer IR/Red until threshold
SpO2->>Algo: Compute SpO2 when buffers ready
Algo-->>SpO2: SpO2 value
SpO2->>Packets: Emit SPO2 packet (APERIODIC_DATA_LEN, timestamp)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (3)
EmotiBit.h (1)
59-59
: Consider using semantic versioning for feature branches.The version string "1.14.0-feat-spo2.0" deviates from standard semantic versioning. Consider using a pre-release identifier format like "1.14.0-alpha.1" or "1.14.0-spo2.1" for better compatibility with version parsing tools.
EmotiBit.cpp (2)
3399-3407
: Replace unexplained magic number with a named constant or a clear condition
SPO2_PPG_BUFFER_SIZE - 15
is opaque. Either:
- Require a full buffer (
minLen >= SPO2_PPG_BUFFER_SIZE
), or- Introduce a named margin constant with an inline rationale.
Minimal tweak within current design:
- if (irDataIndex >= SPO2_PPG_BUFFER_SIZE-15 && redDataIndex >= SPO2_PPG_BUFFER_SIZE-15) { + constexpr size_t SPO2_TAIL_MARGIN = 15; // Margin required by algorithm; avoids partial-window bias + if (irDataIndex >= SPO2_PPG_BUFFER_SIZE - SPO2_TAIL_MARGIN && + redDataIndex >= SPO2_PPG_BUFFER_SIZE - SPO2_TAIL_MARGIN) {Please add a brief comment explaining why this margin is needed (e.g., filter group delay, algorithm warm-up).
3366-3409
: Add doxygen-style docs to the new API for consistencyAdd a brief doxygen block above processSpO2 detailing:
- Purpose
- Input streams used (PPG_INFRARED, PPG_RED)
- Window length and timing assumptions
- Output packet type and rate
This keeps it aligned with the rest of the codebase’s documentation practices.Would you like me to draft the doxygen block with parameter/return details?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
EmotiBit_stock_firmware/partitions.csv
is excluded by!**/*.csv
📒 Files selected for processing (6)
EmotiBit.cpp
(3 hunks)EmotiBit.h
(5 hunks)EmotiBit_stock_firmware/EmotiBit_stock_firmware.ino
(2 hunks)EmotiBit_stock_firmware/platformio.ini
(1 hunks)library.properties
(1 hunks)pio_scripts/removeBindingCPP.py
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
EmotiBit.h (1)
EmotiBit.cpp (2)
processSpO2
(3366-3408)processSpO2
(3366-3366)
🪛 Ruff (0.12.2)
pio_scripts/removeBindingCPP.py
1-1: Undefined name Import
(F821)
6-6: Undefined name env
(F821)
🔇 Additional comments (7)
pio_scripts/removeBindingCPP.py (1)
1-6
: Fix the PlatformIO-specific import syntax and add documentation.The static analysis warnings about undefined
Import
andenv
are false positives - these are PlatformIO build system globals. However, the script lacks documentation explaining its purpose and the middleware pattern.Consider adding a module docstring to clarify the script's purpose:
+""" +PlatformIO build middleware to exclude bindings.cpp files from compilation. +This prevents compilation of Brainflow binding files that are not needed +for the EmotiBit firmware build. +""" Import("env") def skip_bindings_cpp(env, node): + """Return None to skip building files matching the pattern.""" return None env.AddBuildMiddleware(skip_bindings_cpp, "**/bindings.cpp")EmotiBit.h (1)
38-38
: Verify availability ofEmotibit_Brainflow_SpO2_Algorithm.h
I wasn’t able to locate
Emotibit_Brainflow_SpO2_Algorithm.h
anywhere in the repository. Please confirm that:
- The header file is included in your project (e.g., placed under
src/
orinclude/
, or provided by an external “Emotibit” library folder).- Your
library.properties
declares the dependency supplying this header.- The include path in your build setup actually points to the directory containing this file.
If the file is missing, add it to your repo or adjust the library/dependency configuration so the compiler can find it.
EmotiBit_stock_firmware/EmotiBit_stock_firmware.ino (2)
1-2
: Header include order change has no functional impact.The reordering of
Arduino.h
afterEmotiBit.h
doesn't affect functionality but deviates from the common Arduino convention of includingArduino.h
first.
5-5
: Removal of comment improves code cleanliness.Removing the redundant
//115200
comment that didn't match the actual baud rate value improves code clarity.EmotiBit.cpp (3)
2437-2437
: LGTM: debug log now prints the passed-in typeThe change to print the function argument’s type is correct and improves the usefulness of the debug message.
3459-3462
: LGTM: gating and placement of SpO2 processing is correctSpO2 processing is enabled behind acquireData.spo2 and is invoked after buffer swaps, which ensures you read OUT buffers consistently and do not race the ISR. This aligns with HeartRate placement.
3366-3409
: Improve processSpO2 buffer safety and index handlingEmotiBit.cpp → processSpO2 (≈lines 3366–3409):
• Change
irDataIndex
/redDataIndex
fromuint8_t
tosize_t
to avoid truncation.
• Clamp each incoming chunk to the remaining capacity instead of skipping the copy and still advancing the index.
• Replace the magic-15
tail margin with a named constant, e.g.constexpr size_t SPO2_TAIL_MARGIN = 15; constexpr size_t MIN_WINDOW = SPO2_PPG_BUFFER_SIZE - SPO2_TAIL_MARGIN;• Remove or actually use the unused
timePeriod
variable—ideally to compute an end-of-window timestamp.
• After callingget_oxygen_level(...)
, validatespo2
(must be finite and in [0,100]) and skipaddPacket()
if invalid.
• Use the later of the two channel timestamps plus(window_length–1) * timePeriodMs
for the packet timestamp.
• Verifyget_oxygen_level
’s declaration/definition and its return or error-status semantics—if it returns an error code, branch on it before emitting a packet.
@@ -331,6 +332,7 @@ class EmotiBit { | |||
#define IMU_SAMPLING_DIV 2 | |||
#define BATTERY_SAMPLING_DIV 50 | |||
#define DUMMY_ISR_DIV 10 | |||
#define SPO2_PPG_BUFFER_SIZE 64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Document the rationale for the SpO2 buffer size.
The buffer size of 64 samples appears to be arbitrary. Please add documentation explaining:
- How this size relates to the SpO2 algorithm requirements
- The relationship to PPG sampling rate and time window needed for accurate SpO2 calculation
- Memory impact of these static buffers
+// SpO2 calculation requires a minimum window of PPG samples.
+// At 100Hz PPG rate, 64 samples = 640ms window
+// This size balances algorithm accuracy requirements with memory constraints
#define SPO2_PPG_BUFFER_SIZE 64
🤖 Prompt for AI Agents
In EmotiBit.h around line 335, the macro defining SPO2_PPG_BUFFER_SIZE as 64
lacks documentation; add a concise comment above the define that explains why 64
was chosen by linking it to the SpO2 algorithm's required sample window (e.g.,
required number of PPG cycles or seconds), show the calculation using the PPG
sampling rate (samples/sec × time window = buffer length) so reviewers can
verify/adjust, note the memory footprint of the static buffer (bytes = buffer
length × bytes per sample × number of channels) and mention it can be tuned via
the macro if a different sampling rate or time window is used.
@@ -7,4 +7,4 @@ paragraph= Requires dependent libraries as shown in the getting started document | |||
category=Sensors | |||
url=https://github.com/EmotiBit/EmotiBit_FeatherWing | |||
architectures=* | |||
depends=EmotiBit BMI160, EmotiBit MAX30101, EmotiBit MLX90632, EmotiBit NCP5623, EmotiBit SI7013, EmotiBit XPlat Utils, EmotiBit ADS1X15, EmotiBit External EEPROM, EmotiBit EmojiLib, EmotiBit ArduinoFilters, EmotiBit SimpleFTPServer, EmotiBit KTD2026 | |||
depends=EmotiBit BMI160, EmotiBit MAX30101, EmotiBit MLX90632, EmotiBit NCP5623, EmotiBit SI7013, EmotiBit XPlat Utils, EmotiBit ADS1X15, EmotiBit External EEPROM, EmotiBit EmojiLib, EmotiBit ArduinoFilters, EmotiBit SimpleFTPServer, EmotiBit KTD2026, EmotiBit Brainflow SpO2 Algorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify the availability of the EmotiBit Brainflow SpO2 Algorithm library.
The dependency on "EmotiBit Brainflow SpO2 Algorithm" needs to be available in the Arduino Library Registry. According to the PR description, this requires PR #1 in the EmotiBit_Brainflow_SpO2_Algorithm repository to be merged first.
🏁 Script executed:
#!/bin/bash
# Check if the library dependency is properly documented
echo "Checking for library documentation or references:"
rg -A 3 "EmotiBit.*Brainflow.*SpO2" --type-add 'doc:*.{md,txt,rst}' -t doc
# Check for any library.json or similar configuration files
echo -e "\nSearching for library configuration files:"
fd -e json -e properties | xargs grep -l "Brainflow\|SpO2" 2>/dev/null || true
Length of output: 366
🌐 Web query:
Is the "EmotiBit Brainflow SpO2 Algorithm" library available in the Arduino Library Registry?
💡 Result:
Short answer: No — I couldn't find a library named "EmotiBit Brainflow SpO2 Algorithm" in the Arduino Library Registry.
Details and sources:
- The Arduino Library Registry does include several EmotiBit libraries (for example EmotiBit MAX30101, BMI160, FeatherWing), but there is no entry with the exact name "EmotiBit Brainflow SpO2 Algorithm". (arduino.cc)
- EmotiBit’s MAX30101 library (which contains a spo2_algorithm.h implementation) is available on GitHub and is the place to look for SpO2 code for EmotiBit hardware; that library is also installable from the Arduino Library Manager. (github.com)
- BrainFlow provides host-side support for EmotiBit (for streaming/processing on the PC), but I did not find a separately-registered Arduino library called “EmotiBit Brainflow SpO2 Algorithm.” (brainflow.org, github.com)
If you want, I can:
- Re-run a live search in the Arduino Library Registry right now to double-check, or
- Show how to install/use the EmotiBit_MAX30101 library (and its spo2 algorithm) from the Arduino Library Manager or from GitHub, or
- Help integrate EmotiBit with BrainFlow on the host to compute SpO2 off-board. Which would you like?
Citations:
- 1: https://www.arduino.cc/reference/en/libraries/emotibit-max30101/?utm_source=chatgpt.com
- 2: https://github.com/EmotiBit/EmotiBit_MAX30101/blob/master/src/spo2_algorithm.h?utm_source=chatgpt.com
- 3: https://brainflow.org/?utm_source=chatgpt.com
- 4: https://github.com/artoo-git/Emotibit-Brainflow?utm_source=chatgpt.com
Dependency missing: “EmotiBit Brainflow SpO2 Algorithm” is not published
The library “EmotiBit Brainflow SpO2 Algorithm” referenced on line 10 of library.properties
isn’t available in the Arduino Library Registry. You’ll need to:
- Merge and publish PR Fix units of thermistor #1 in the EmotiBit_Brainflow_SpO2_Algorithm repo so it appears in the Library Manager, then update this dependency.
- Or, if you just need the SpO₂ algorithm on-device today, replace it with the existing
EmotiBit_MAX30101
library (which includesspo2_algorithm.h
and is already on the Library Manager).
File and location:
library.properties
— line 10 (depends)
🤖 Prompt for AI Agents
library.properties around line 10: the depends entry references "EmotiBit
Brainflow SpO2 Algorithm" which is not published to the Arduino Library Manager;
either merge and publish PR #1 in the EmotiBit_Brainflow_SpO2_Algorithm repo so
that the package name becomes available and then update this depends line to the
exact published library name, version and spelling, or replace that dependency
immediately with the existing published library "EmotiBit_MAX30101" (which
contains spo2_algorithm.h) by changing the depends entry to include
EmotiBit_MAX30101 instead; ensure the final depends line only lists libraries
available in the Arduino registry and matches their exact package names.
Description
The Brainflow SpO2 algorithm has been added into the base EmotiBit firmware.
Requirements
Notes for Reviewer
processSpO2
that calculates SpO2 from PPG data buffers and sends an SpO2 data packetspo2
flag toacquireData
that can enable/disable SpO2 calculationEmotiBit_stock_firmware
for PlatformIOremoveBindingCPP.py
underpio_scripts
so thatbinding.cpp
is not compiled during PlatformIO buildEmotiBit Brainflow SpO2 Algorithm
tolibrary.properties
Checklist to allow merge
master
ofxEmotiBitVersion.h
DIGITAL_WRITE_DEBUG
= false (if set true while testing)Screenshots:
Summary by CodeRabbit
New Features
Bug Fixes
Chores