Conversation
nitin710
left a comment
nitin710
left a comment
There was a problem hiding this comment.
I like the new capability to modify the test type. However, I think there might be a bug that might cause some undefined behavior with string literal comparison. Check out the comments for more information.
Highlighted some more stylistic issues.
- replaced const char* with enum - extracted 3 - naming - spacing
nitin710
left a comment
nitin710
left a comment
There was a problem hiding this comment.
See review comments. Otherwise looks good!
Also see the latest PR template, specifically the testing section. Let's conform this PR to that.
produceconsumerobot
left a comment
produceconsumerobot
left a comment
There was a problem hiding this comment.
Generally looks good. Added some small suggestions and queries.
src/EmotiBitPacket.cpp
Outdated
There was a problem hiding this comment.
Code readability might be improved to start the conditional with the testType rather than the testCount
There was a problem hiding this comment.
I'm not 100% sure, but it seems like using <= EmotiBitPacket::maxTestLength might create one extra packet. E.g. if you set maxTestLength = 1, wouldn't it actually create 2 packets?
There was a problem hiding this comment.
Started with conditional, second comment addressed below
| } | ||
|
|
||
| // End case to visually signal end of test | ||
| else if (testCount == EmotiBitPacket::maxTestLength + 1) |
There was a problem hiding this comment.
maxTestLength + 1 also seems to suggest maxTestLength + 1 packets may be created
There was a problem hiding this comment.
The +1 accounts for the "start message"
src/EmotiBitPacket.cpp
Outdated
There was a problem hiding this comment.
Using testCount is fine, it does the job, but I wonder if having payloadLen as an input would have given a little more transparency into how the input translates into a test packet.
There was a problem hiding this comment.
Switched to payloadLength as an input
src/EmotiBitPacket.cpp
Outdated
There was a problem hiding this comment.
It's fine to have a "0" marker at the end and I probably wouldn't recommend changing it now because it might require changing in a few places, but I wonder if it's necessary. Could you have just used PACKET_DELIMITER_CSV as the packet-end marker that you search for in the comparison script?
There was a problem hiding this comment.
I'll add a ToDo
WalkthroughVersion bumped to 1.7.2. Introduced TestType enum and extended createTestDataPacket to accept a test type, adding a fixed-length packet generator. Increased max test length to 512. Updated tests to call the new API, added execution step in test script, and expanded README instructions. Minor formatting tweaks elsewhere. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Main as tests/MockDataTest main
participant Packet as EmotiBitPacket
note over Tester,Main: Test execution (MockDataTest)
Tester->>Main: Run executable
Main->>Packet: createTestDataPacket(dataMessage, testType)
alt testType == SAWTOOTH
Packet->>Packet: createTestSawtoothData(...)
Packet-->>Main: dataMessage (sawtooth)
else testType == FIXED_PACKET_LENGTH
Packet->>Packet: createTestPacketFixedLength(payloadLength)
Packet-->>Main: dataMessage (fixed length)
end
Main-->>Tester: Output CSV/test packet
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
src/EmotiBitPacket.h (2)
364-367: Doc mismatch: note references isRecording which isn’t part of the API.createTestDataPacket() doesn’t use an isRecording flag; it relies on internal static state (firstMessage/testCount). Update the note to describe actual behavior, including that the first call emits a begin marker and the (maxTestLength + 1)-th data call emits an end marker.
Suggested replacement:
- “On first invocation, a begin-of-test packet is emitted. Subsequent invocations emit test packets until maxTestLength is reached. The next invocation emits an end-of-test packet.”
373-377: Param name/docs imply payload length, but implementation targets total packet length.createTestPacketFixedLength(int payloadLength) subtracts headerString.length() and delimiter overhead to compute dash count, i.e., it makes the total packet length equal to the passed value. If the intent is total packet length, rename the parameter and adjust docs; if the intent is payload length, drop the header length subtraction.
Two options:
- If total packet length is intended (current behavior), update docs and param name (source-compatible if only used internally):
-//! @param payloadLength payload length of the packet to create +//! @param packetLength total packet length to create (including header and delimiters) -static String createTestPacketFixedLength(int payloadLength); +static String createTestPacketFixedLength(int packetLength);
- If payload length is intended, adjust implementation to use the number directly and set header.dataLength accordingly.
- int dataLength = payloadLength - (int)headerString.length() - payloadLengthOffset; + int dataLength = payloadLength; // payload length equals requested lengthsrc/EmotiBitPacket.cpp (4)
556-567: Clarity: “payloadLength” is the desired total packet length, not payload bytes.The function computes
dataLengthby subtracting header length and fixed suffix length, which means the input is a target packet size. Consider renaming the parameter or documenting the contract inline to avoid confusion.Example:
-String EmotiBitPacket::createTestPacketFixedLength(int payloadLength) +// desiredTotalLength: target total packet size including header, delimiter, data, and trailing PACKET_DELIMITER_CSV +String EmotiBitPacket::createTestPacketFixedLength(int desiredTotalLength) { - int payloadLengthOffset = (String(PAYLOAD_DELIMITER) + "0" + String(PACKET_DELIMITER_CSV)).length(); + int payloadLengthOffset = (String(PAYLOAD_DELIMITER) + "0" + String(PACKET_DELIMITER_CSV)).length(); ... - int dataLength = payloadLength - (int)headerString.length() - payloadLengthOffset; + int dataLength = desiredTotalLength - (int)headerString.length() - payloadLengthOffset;
518-518: Use a non-sensor tag for the end-of-test marker.Using
TypeTag::EDAfor the terminal “0” looks like a real EDA sample and can pollute downstream analytics. PreferUSER_NOTE(consistent with the start-of-test) or a dedicated control tag.Apply this diff:
- EmotiBitPacket::Header endHeader = EmotiBitPacket::createHeader(EmotiBitPacket::TypeTag::EDA, 0, 0, 1, 0, 0); + EmotiBitPacket::Header endHeader = EmotiBitPacket::createHeader(EmotiBitPacket::TypeTag::USER_NOTE, 0, 0, 1, 0, 0);
484-514: Provide a reset path for repeated runs and test-type switching.
firstMessageandtestCountare static and never reset, so subsequent runs (or switching test types) won’t emit the start/end markers. Recommend adding a smallresetTest()or abool resetargument to reinitialize these.If you share the expected UX (e.g., toggling sendTestData, changing test type from the console), I can wire up a minimal
resetTest()and reference usage locations.
528-541: Edge-case guard: future-proof against numValues == 1.If
numValuesis ever changed to 1,((numValues - 1))hits division by zero. Cheap guard keeps this robust.Apply this diff:
- int numValues = 10; // Number of values to generate + int numValues = 10; // Number of values to generate int minVal = 0; // Minimum value int maxVal = 100; // Maximum value for (uint8_t i = 0; i < numValues; ++i) { if (i > 0) payload += EmotiBitPacket::PAYLOAD_DELIMITER; - int value = minVal + ((maxVal - minVal) * i) / (numValues - 1); + int denom = max(1, numValues - 1); + int value = minVal + ((maxVal - minVal) * i) / denom; payload += value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
library.properties(1 hunks)src/ArduinoString.h(1 hunks)src/EmotiBitPacket.cpp(3 hunks)src/EmotiBitPacket.h(2 hunks)tests/MockDataTest/README.md(1 hunks)tests/MockDataTest/scripts/run_test.sh(1 hunks)tests/MockDataTest/src/main.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/MockDataTest/src/main.cpp (1)
src/EmotiBitPacket.cpp (2)
createTestDataPacket(484-524)createTestDataPacket(484-484)
src/EmotiBitPacket.cpp (2)
src/ArduinoString.h (1)
String(6-25)src/EmotiBitPacket.h (1)
createPacket(345-360)
src/EmotiBitPacket.h (1)
src/EmotiBitPacket.cpp (6)
createTestDataPacket(484-524)createTestDataPacket(484-484)createTestSawtoothData(526-542)createTestSawtoothData(526-526)createTestPacketFixedLength(544-569)createTestPacketFixedLength(544-544)
🪛 Shellcheck (0.10.0)
tests/MockDataTest/scripts/run_test.sh
[warning] 46-46: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 48-48: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🪛 LanguageTool
tests/MockDataTest/README.md
[grammar] ~6-~6: There might be a mistake here.
Context: ...ype. A typical workflow will consist of: 1. Going into debug mode 2. Setting sendTes...
(QB_NEW_EN)
[grammar] ~7-~7: There might be a mistake here.
Context: ...ill consist of: 1. Going into debug mode 2. Setting sendTestData to true 3. Choosing...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...bug mode 2. Setting sendTestData to true 3. Choosing the Sawtooth test '#' 4. Pressi...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...o true 3. Choosing the Sawtooth test '#' 4. Pressing record in the oscilliscope and ...
(QB_NEW_EN)
[grammar] ~10-~10: Ensure spelling is correct
Context: ...ooth test '#' 4. Pressing record in the oscilliscope and waiting for the test to finish 5. C...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~10-~10: There might be a mistake here.
Context: ...scope and waiting for the test to finish 5. Comparing the SD card result with the ba...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...ing the extension | Command | Details | |--------|--------| | < | Sets "sendTest...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ... Command | Details | |--------|--------| | < | Sets "sendTestData" to true| | > |...
(QB_NEW_EN)
🔇 Additional comments (3)
library.properties (1)
2-2: Version bump to 1.7.2 looks consistent with the new test APIs.No issues from this change alone.
src/ArduinoString.h (1)
71-75: No functional change; operator+= (int) remains correct.Indent-only tweak; behavior unchanged.
src/EmotiBitPacket.h (1)
257-261: Convert TestType to a scoped enumAll current references use
TestType::SAWTOOTHandTestType::FIXED_PACKET_LENGTH, and there are no raw uses ofSAWTOOTHorFIXED_PACKET_LENGTHin the codebase. Converting toenum classwill preserve existing usage and eliminate potential name collisions.Locations to update:
- src/EmotiBitPacket.h (around lines 257–261)
Proposed change:
-enum TestType { - FIXED_PACKET_LENGTH, - SAWTOOTH -}; +enum class TestType { + FIXED_PACKET_LENGTH, + SAWTOOTH +};
|
Ready for merge |
3 participants
Description
sendDataRequirements
To run this branch you need:
PR #329
Issues Referenced
None
Documentation update
None
Notes for Reviewer
createPacketFixedLengthTestto facilitate testing the splitting.Testing
Please follow "Mock Data Testing" in the "EmotiBit Feature Test Protocols" document. This has been updated with the new changes
Results
✅Choose between Packet Fixed Length and Sawtooth tests
✅Splits at the desired delimiter
Feature Tests
Shared files
Checklist to allow merge
masterofxEmotiBitVersion.hDIGITAL_WRITE_DEBUG= false (if set true while testing)Screenshots:
Summary by CodeRabbit
New Features
Tests
Documentation
Chores