fixed a bug in arduino code #14
Conversation
WalkthroughThe pull request modifies the EMG game controller Arduino code, focusing on restructuring the conditional compilation logic for handling EMG envelope data output. The changes primarily involve reorganizing the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Nitpick comments (1)
12_2CH_EMG_Game_Controller/12_2CH_EMG_Game_Controller.ino (1)
182-184: Fix indentation and use consistent operators in the remaining conditions.The indentation is inconsistent, and the logical operators should be standardized.
Apply this diff:
- else if(envelope1 > 20 and envelope1 < threshold1 and envelope2 < threshold2 and envelope2 > 10) + else if(envelope1 > CH_MID_THRESHOLD && envelope1 < threshold1 && + envelope2 < threshold2 && envelope2 > CH_LOW_THRESHOLD) { - Serial.println("3"); + Serial.println("3"); } else { - Serial.println("0"); + Serial.println("0"); }Also applies to: 187-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
12_2CH_EMG_Game_Controller/12_2CH_EMG_Game_Controller.ino(2 hunks)
🔇 Additional comments (2)
12_2CH_EMG_Game_Controller/12_2CH_EMG_Game_Controller.ino (2)
310-310: LGTM!The closing brace is correctly placed.
153-209: Verify if the original bug is fixed.The PR title mentions a bug fix, but the changes appear to be mostly restructuring. Please clarify:
- What was the original bug?
- How do these changes fix it?
Also, consider adding test cases to verify the fix and prevent regression.
Would you like me to help create test cases to validate the changes?
| if(envelope1 > envelope2 and envelope1 > threshold1 and envelope2 < 20) | ||
| { | ||
| if(envelope2>10) | ||
| { | ||
| Serial.println("4"); | ||
| } | ||
| else | ||
| { | ||
| Serial.println("1"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent comparison operators and define magic numbers as constants.
- Replace 'and' with '&&' for consistency with C++ style
- Define threshold values as constants instead of using magic numbers
- Add missing check for CH2 activation (asymmetric with CH1)
Apply this diff to fix the issues:
+// Define threshold constants at the top with other constants
+#define CH_LOW_THRESHOLD 10
+#define CH_MID_THRESHOLD 20
- if(envelope1 > envelope2 and envelope1 > threshold1 and envelope2 < 20)
+ if(envelope1 > envelope2 && envelope1 > threshold1 && envelope2 < CH_MID_THRESHOLD)
{
- if(envelope2>10)
+ if(envelope2 > CH_LOW_THRESHOLD)
{
Serial.println("4");
}
else
{
Serial.println("1");
}
}
- else if(envelope2 > envelope1 and envelope2 > threshold2)
+ else if(envelope2 > envelope1 && envelope2 > threshold2 && envelope1 < CH_MID_THRESHOLD)
{
- if(envelope1>20)
+ if(envelope1 > CH_MID_THRESHOLD)Also applies to: 171-180
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
02_EMG_Filter/02_EMG_Filter.ino (3)
57-70: Consider validating filter output alongside raw data.
Currently, the new band-stop/high-pass combination is only tested by printing the final filtered signal. It may be helpful to print (or log) the raw data alongside the filtered output in a single line during development/testing, so you can quickly compare both values and ensure filter performance is as intended.
74-78: Documentation is comprehensive; add reference to filter configuration script.
This comment block thoroughly explains the filter characteristics. Consider referencing the location or filename of the script/tool you used (e.g., “filter_gen.py” in your repository) for easy traceability when updating or regenerating filter coefficients in the future.
83-83: Remove the double negative to improve readability.
While correct mathematically, the double negative can be confusing. Consider simplifying as shown in the diff below:- float x = output - -0.82523238*z1 - 0.29463653*z2; + float x = output + 0.82523238*z1 - 0.29463653*z2;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
02_EMG_Filter/02_EMG_Filter.ino(1 hunks)
🔇 Additional comments (1)
02_EMG_Filter/02_EMG_Filter.ino (1)
91-96: Kudos on providing clear filter documentation.
The band-stop filter comment block is concise and mirrors the high-pass filter’s style. No issues found here.
02_EMG_Filter/02_EMG_Filter.ino
Outdated
| float output = input; | ||
| { | ||
| static float z1, z2; // filter section state | ||
| float x = output - 0.47319594*z1 - 0.70744137*z2; | ||
| output = 1.00000000*x + 2.00000000*z1 + 1.00000000*z2; | ||
| float x = output - -1.56858163*z1 - 0.96424138*z2; | ||
| output = 0.96508099*x + -1.56202714*z1 + 0.96508099*z2; | ||
| z2 = z1; | ||
| z1 = x; | ||
| } | ||
| { | ||
| static float z1, z2; // filter section state | ||
| float x = output - -1.00211112*z1 - 0.74520226*z2; | ||
| output = 1.00000000*x + -2.00000000*z1 + 1.00000000*z2; | ||
| float x = output - -1.61100358*z1 - 0.96592171*z2; | ||
| output = 1.00000000*x + -1.61854514*z1 + 1.00000000*z2; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using distinct names for static states in each filter stage.
Repeatedly declaring z1 and z2 for multiple second-order sections within the same function can introduce confusion. Using unique identifiers (e.g., z1_stage1, z1_stage2) or grouping them in a struct can help maintain clarity.
02_EMG_Filter/02_EMG_Filter.ino
Outdated
| // Apply the band-stop filter (48 Hz to 52 Hz) | ||
| float bandstop_filtered = BandStopFilter(sensor_value); | ||
|
|
||
| // Apply the high-pass filter (70 Hz) | ||
| float highpass_filtered = HighPassFilter(bandstop_filtered); | ||
|
|
||
| // Print the final filtered signal | ||
| Serial.print(highpass_filtered); | ||
|
|
||
| // Uncomment below 2 lines to compare with raw data | ||
| // Serial.print(" "); | ||
| // Serial.println(sensor_value); |
There was a problem hiding this comment.
@CIumsy please fix indentation here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
02_EMG_Filter/02_EMG_Filter.ino (2)
97-102: Consider consistent coefficient precisionWhile the filter coefficients are correct, you might want to standardize the number of decimal places for consistency and readability. Additionally, consider whether the high precision is necessary for your microcontroller's floating-point capabilities.
Also applies to: 104-109
57-69: Minor indentation inconsistencyThere appears to be a mix of tabs and spaces in the indentation. The code in the
loop()function uses spaces for indentation while other parts use tabs. Consider standardizing on either tabs or spaces throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
02_EMG_Filter/02_EMG_Filter.ino(1 hunks)
🔇 Additional comments (5)
02_EMG_Filter/02_EMG_Filter.ino (5)
57-68: Well-structured filter implementation approachThe revised EMG signal processing pipeline is now more clear and follows standard DSP practices by applying specific filters in sequence. First removing power line noise (band-stop filter) and then applying high-pass filtering makes sense for EMG processing.
61-62: Good fix for power line noiseAdding a dedicated band-stop filter targeting 48-52 Hz addresses power line noise issues (typically 50Hz in many regions), which was likely the bug being fixed. This is much more effective than trying to handle it with a single filter.
72-87: Well-documented filter implementationThe high-pass filter is well-documented with clear references to the filter generation method. The code structure is clean and the filter parameters are clearly specified.
80-85: Consider using distinct names for static states in each filter stage.Repeatedly declaring
z1andz2for multiple second-order sections within the same function can introduce confusion. Using unique identifiers (e.g.,z1_stage1,z1_stage2) or grouping them in a struct can help maintain clarity.
89-110: Good addition of band-stop filter for noise rejectionThe band-stop filter is properly implemented as two second-order sections, which provides better numerical stability than a single fourth-order filter. The frequency range (48-52 Hz) is appropriate for rejecting power line interference.
|
@lorforlinux Fixed the indentation in 02_EMG_Filter.ino |
Summary by CodeRabbit
BandStopFilterandHighPassFilterfunctions.