Feat splitter refactor by Joseph-Jacobson · Pull Request #329 · EmotiBit/EmotiBit_FeatherWing

Joseph-Jacobson · 2025-07-14T22:04:28Z

Description

This PR moves the packet splitter functionality into sendData, out of EmotiBitWifi and writeSdCardMessage. This sets us up to split packets in a single spot, so when we enable Bluetooth we won't add yet another splitter.

Debugger commands is added to allow for greater control
Added PacketFixedLengthTest to check the new splitting functionality
New error handler if writeSdCardMessage receives a packet over 512. Now a Data Overflow packet is now printed with a payload of the intended payload length.

Requirements

To run this branch you need:

FeatherWing 1.14.1
XPlat 1.7.2

PR #30

Issues Referenced

Fixes processDebugInputs() doesn't process inputs correctly #318
replacing "read" with "peek" allowed me to input the other debugging commands

Documentation update

None

Notes for Reviewer

The data splitter in EmotiBitWiFi sendUDP is moved into EmotiBit sendData. The WiFi splitter was chosen over the splitter in writeSdCardMessage as it splits based on the delimiter.
Added flexibility to testing, letting us choose the tests to run during run time via the debugger. A table for the added modes/changes is provided below.

Command	Details
>	Sets "sendTestData" to true
<	Sets "sendTestData" to false
@	Sets test data to Packet Fixed Length Test
#	Sets test data to Sawtooth (this is also the default)

Testing

Please follow "Packet Fixed Length Test" in the "EmotiBit Feature Test Protocols" document. This has been updated with the new changes

Results

✅PacketFixedLengthTest determines if the error is generated

Detailed results located in PR #30

Feature Tests

Shared files

Firmware binary: [Link to firmware binary]
Other files.

Checklist to allow merge

Screenshots:

Summary by CodeRabbit

New Features
- Improved data processing flow with chunked wireless transmission and SD writes.
- Added overflow detection for SD writes with clear error logging.
- Expanded debug controls to enable/disable test data and select test patterns.
Bug Fixes
- More reliable trigger handling without consuming input bytes.
- Corrected debug logging to show the active data type.
Documentation
- Added guide for the Packet Fixed Length Test.
Tests
- Added script to verify absence of Debug Overloads in recorded data.
Chores
- Bumped version to 1.14.1.

nitin710

Review 01

EmotiBit.h

EmotiBit.cpp

EmotiBit.h

testing/PacketFixedLengthTest/run_test.sh

EmotiBit.cpp

- removed constant print infos in debug mode - spacing - removed splitter indicator

nitin710

Looks good. See review comments in the XPlat PR.

Also, see the latest PR template, specifically the Testing section. Let's conform this PR to that.

nitin710 · 2025-08-12T15:19:45Z

@coderabbitai perform an review

coderabbitai · 2025-08-12T15:20:03Z

@nitin710 I'll perform a comprehensive review of this PR focusing on the packet splitter refactoring, new debugger commands, and error handling improvements.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2025-08-12T15:20:06Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feat-SplitterRefactor

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 7

🔭 Outside diff range comments (1)

EmotiBit.cpp (1)
2421-2421: Fix debug print to use correct variable name.

The debug print references an undefined variable t instead of the parameter type.
 size_t EmotiBit::getData(DataType type, float** data, uint32_t * timestamp) {
 #ifdef DEBUG
 	Serial.print("getData: type=");
-	Serial.println((uint8_t) t);
+	Serial.println((uint8_t) type);
 #endif // DEBUG

🧹 Nitpick comments (2)

testing/PacketFixedLengthTest/README.md (2)
2-2: Grammar fixes: “This tests verifies” → “This test verifies.”

Minor but visible in the first sentence.

Apply this diff (already included in a previous suggestion but calling it out explicitly due to prominence):
- This tests verifies
+ This test verifies
10-10: Spelling: “oscilliscope” → “oscilloscope.”

Caught by static analysis; fix for professionalism.

Apply this diff (already included in a previous suggestion but calling it out explicitly for traceability):
-4. Pressing record in the oscilliscope and waiting for the test to finish
+4. Pressing record in the oscilloscope and waiting for the test to finish

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f48eeaa and ef762a0.

📒 Files selected for processing (6)

EmotiBit.cpp (8 hunks)
EmotiBit.h (3 hunks)
EmotiBitWiFi.cpp (1 hunks)
library.properties (1 hunks)
testing/PacketFixedLengthTest/README.md (1 hunks)
testing/PacketFixedLengthTest/run_test.sh (1 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (1)

EmotiBit.cpp (1)

EmotiBitWiFi.cpp (2)

sendData (407-416)

sendData (407-407)

🪛 LanguageTool

testing/PacketFixedLengthTest/README.md

[grammar] ~10-~10: Ensure spelling is correct
Context: ...tter test '@' 4. Pressing record in the oscilliscope and waiting for the test to finish 5. C...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🔇 Additional comments (10)

library.properties (1)

2-2: LGTM! Version bump is appropriate for the refactor.

The version increment from 1.14.0 to 1.14.1 correctly follows semantic versioning for a patch release containing the packet splitter refactor and bug fixes.

EmotiBit.h (2)

301-301: Good rename to better reflect the constant's broader usage.

The rename from MAX_SD_WRITE_LEN to MAX_SEND_LEN better reflects that this constant is now used for both SD card writes and network sends.

428-428: LGTM! Well-structured test data type addition.

The addition of _testDataType with a sensible default of SAWTOOTH provides good flexibility for different test data patterns.

EmotiBit.cpp (7)

1651-1651: Good fix for the debug input processing issue.

Using Serial.peek() instead of Serial.read() correctly addresses the issue from #318 where the 'F' character was being consumed prematurely, preventing FTP mode from being triggered.

1749-1753: LGTM! Cleaner main loop data flow.

Moving processData() and sendData() out of conditional logic ensures data is always processed consistently, regardless of test mode.

3409-3488: Well-implemented centralized packet splitting logic.

The refactored sendData() function successfully centralizes packet splitting from WiFi and SD card write functions. The implementation correctly:

Handles test data injection when enabled

Splits messages into chunks respecting MAX_SEND_LEN boundaries

Preserves packet integrity by splitting at delimiter boundaries

Handles both the main buffer and remainder consistently

3665-3678: Good error handling for oversized SD card writes.

The new overflow detection correctly identifies when a message exceeds MAX_SEND_LEN and logs a DATA_OVERFLOW packet with the attempted size. This provides valuable debugging information.

3876-3876: Appropriate use of new test data API.

The updated call to EmotiBitPacket::createTestDataPacket correctly passes the _testDataType parameter to support different test patterns.

3983-3986: Well-designed debug commands for test data control.

The new debug commands provide good runtime control over test data:

> and < for enabling/disabling test data

@ and # for selecting test type
Clear user prompts make the functionality discoverable.

Also applies to: 4333-4351

3411-3422: Consider edge case: empty test data when not recording.

The logic only adds test data when _sdWrite is true (line 3875), but test data mode can be enabled without recording. This might confuse users who enable test mode but don't see any test data because recording isn't active.

Consider whether test data should be generated regardless of recording state, or if the current behavior should be documented more clearly in the debug help text.

EmotiBitWiFi.cpp

testing/PacketFixedLengthTest/README.md

testing/PacketFixedLengthTest/run_test.sh

produceconsumerobot

Some minor comments, but overall looks great.

EmotiBit.cpp

produceconsumerobot · 2025-08-20T13:53:05Z

testing/PacketFixedLengthTest/README.md

From this description, I'm not sure I can understand what this test is doing. Perhaps improving the clarity would be helpful.

Edited for clarity

Joseph-Jacobson · 2025-08-25T23:43:22Z

Ready for merge

…cript to run

Joseph-Jacobson added 2 commits July 2, 2025 10:13

initial changes

f06d910

Splitting Logic with debugging pipeline

1667d14

Joseph-Jacobson mentioned this pull request Jul 14, 2025

Feat splitter refactor EmotiBit/EmotiBit_XPlat_Utils#30

Merged

12 tasks

Bumped Library Version

a6f4444

nitin710 changed the base branch from 1.12.1.feat-blePrototype-Example to dev July 14, 2025 23:44

Joseph-Jacobson requested a review from nitin710 July 15, 2025 00:23

Joseph-Jacobson added 5 commits July 15, 2025 13:50

new error message for sd card

b404014

Moved splitter test into featherwing

87cc88a

Renamed Folder

7851676

Review edits

076a412

minor changes

189d1de

Joseph-Jacobson marked this pull request as ready for review July 16, 2025 16:01

Joseph-Jacobson requested a review from produceconsumerobot July 16, 2025 16:02

nitin710 requested changes Jul 24, 2025

View reviewed changes

Joseph-Jacobson added 3 commits July 28, 2025 13:48

PR changes

d7dc259

- removed constant print infos in debug mode - spacing - removed splitter indicator

Removed unnecessary instructions

7262ae3

ToDo added

d2748ec

Joseph-Jacobson requested a review from nitin710 July 29, 2025 22:33

nitin710 reviewed Aug 5, 2025

View reviewed changes

Joseph-Jacobson added 2 commits August 5, 2025 10:31

Renaming

03246b5

Updated comment

ef762a0

coderabbitai bot reviewed Aug 12, 2025

View reviewed changes

Minor readme changes

0404b1f

produceconsumerobot requested changes Aug 20, 2025

View reviewed changes

PR edits

e6fda40

pulled latest master into SplitterRefactor

aa6862e

nitin710 added 4 commits September 11, 2025 15:46

updating workflow to delete all libraries after workflow completes

cdcffe2

fix:url file naming and dependency in library.properties

b6d035e

fix: check for tag with and without prefix "v"

199e413

fix: does not delete the FeatherWing checkout to allow for post run s…

431ac53

…cript to run

nitin710 merged commit 831b4f8 into dev Sep 11, 2025
2 checks passed

nitin710 deleted the feat-SplitterRefactor branch September 11, 2025 20:11

coderabbitai bot mentioned this pull request Oct 23, 2025

Feat ble prototype example #331

Draft

7 tasks

Conversation

Joseph-Jacobson commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Requirements

Issues Referenced

Documentation update

Notes for Reviewer

Testing

Results

Feature Tests

Shared files

Checklist to allow merge

Screenshots:

Summary by CodeRabbit

Uh oh!

nitin710 left a comment

Choose a reason for hiding this comment

Review 01

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nitin710 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitin710 commented Aug 12, 2025

Uh oh!

coderabbitai bot commented Aug 12, 2025

Uh oh!

coderabbitai bot commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

produceconsumerobot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

produceconsumerobot Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Joseph-Jacobson Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Joseph-Jacobson commented Aug 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Joseph-Jacobson commented Jul 14, 2025 •

edited

Loading

nitin710 left a comment •

edited

Loading

coderabbitai bot commented Aug 12, 2025 •

edited

Loading