Conversation
gautamdayal
left a comment
gautamdayal
left a comment
There was a problem hiding this comment.
Mostly nitpicks and I think we'll need to test a lot in hardware before this is approved. I have a software defined radio (SDR) that can help us look at whether packets are being transmitted or not that you can use while debugging, just let me know when you need it.
Good stuff!
| TelemetryPacket packet = makePacket(dataLogger.read()); | ||
| Packets packet; | ||
|
|
||
| auto rocket_state = dataLogger.read().rocketState_data.rocketStates[0]; |
There was a problem hiding this comment.
Also I might be wrong, but I think you can also use the getActiveFSM() function to get the state, it would make this line a little more clear. See how it's obtained in Controller::ActiveControl_ON() in ActiveControl.cpp
| auto rocket_state = dataLogger.read().rocketState_data.rocketStates[0]; | ||
| if (rocket_state == FSM_State::STATE_INIT || rocket_state == FSM_State::STATE_IDLE) { | ||
| packet.compact_packet = makeCompactPacket(dataLogger.read()); | ||
| packet.type = Packets::COMPACT; |
There was a problem hiding this comment.
Would appreciate if this section were commented
| int16_t barometer_temp; //[-128, 128] | ||
| }; | ||
|
|
||
| struct Packets { |
There was a problem hiding this comment.
Clarify the reasoning for having both possible messages defined in this singular struct in a comment. Another possibility would just be to separate the structs completely, but this does seem nice so a comment describing the design would help.
|
|
||
| struct Packets { | ||
| enum { | ||
| COMPACT, |
There was a problem hiding this comment.
nit: Since in the future we may define more packet types, let's make the names a bit more descriptive. For example, COMPACT could be named PAD_LANDED and DEFAULT could be named MAIN_FLIGHT etc.
2 participants
No description provided.