Conversation
nitin710
left a comment
There was a problem hiding this comment.
Left some comments. Let me know if you want to talk about anything.
EmotiBit.cpp
Outdated
| @@ -3839,25 +3839,8 @@ void EmotiBit::updateBatteryIndication(float battPercent) | |||
|
|
|||
| void EmotiBit::appendTestData(String &dataMessage, uint16_t &packetNumber) | |||
There was a problem hiding this comment.
If we are not using the second parameter packetNumber, do we need that in this function signature?
There was a problem hiding this comment.
Also, not that the new appendTestData is no longer "appending". It is setting dataMessage to "" and filling in the new test data.
Can you think of a reason why we might not want to do that? I'm thinking, if there is any reason to preserve the data in dataMessage, at the time appendTestData is being called. If there is a case, we want to pass down the packetNumber to appendTestData?
There was a problem hiding this comment.
Edited with relevent changes
EmotiBit.h
Outdated
|
|
||
| void updateBatteryIndication(float battPercent); | ||
| void appendTestData(String &dataMessage, uint16_t &packetNumber); | ||
| void setTestData(String &dataMessage); |
There was a problem hiding this comment.
Why not call the function sendTestData?
There was a problem hiding this comment.
Add doxygen comments too?
There was a problem hiding this comment.
_sendTestData is a bool flag so that naming could be confusing. I named it setTestData because we are setting dataMessage with setTestData from createTestDataPacket in EmotiBit Packet. The current setTestData function doesn't actually send the data either, that comes from sendData.
There was a problem hiding this comment.
I don't like set. It usually connotes "setting something", more like in the class of "getters" and "setters". It almost feels like we *should be* setting a bool or something.
Also, by that logic, this function may be more accurately called getTestData? because we are getting test Data for dataMessage?
How about addTestData? It tries to replicate the addPacket in the sendData function.
Key point to note that, processTestData works on the buffers and send test data empties the buffers and creates a packet+adds that to outDataMessage.
What we are doing is essentially the same, create a packet and add to outDataMessage.
There was a problem hiding this comment.
I like addTestData, will be making that change
EmotiBit.h
Outdated
| void updateBatteryIndication(float battPercent); | ||
| void appendTestData(String &dataMessage, uint16_t &packetNumber); | ||
| /** | ||
| * @brief Sets dataMessage to the test packet string |
There was a problem hiding this comment.
Should this now be "Adds test packet string to dataMessage"?
Description
Requirements
XPlat_Utils pull#28
Notes for Reviewer
This change removes the previous sawtooth logic, replacing it with a function call to "createTestDataPacket" in EmotiBitPacket which will be building the test packets for EmotiBit.cpp. It is also wrapped in a conditional to check if recording has begun, ensuring our SD card files follow the determined results from EmotiBitPacket
Testing
Results
###MockDataTest
ESP32
M0
Feature Tests
None
Shared files
None
None
Checklist to allow merge
masterofxEmotiBitVersion.hDIGITAL_WRITE_DEBUG= false (if set true while testing)Screenshots: