Documentation - #222
Conversation
telemetry comments added
shell folder
hardware docs
logging docs
Adding comments to flight systems docs
|
FSM SILSIM: 6/6 passed ✓ — plots |
mpkarpov-ui
left a comment
There was a problem hiding this comment.
Thanks you guys for taking on this effort! I think it's been a while since we've had a documentation pass over the system.
That being said, I think there's some work to be done on keeping comments terse-- Code should be self-documenting. I also noticed a few things that confused me so I'm adding some comments to the review.
| // Check minimum horizontal velocity if enabled. | ||
| if (vx_min != -1 && cur_vx < vx_min) { return false; } | ||
|
|
||
| // Check maximum horizontal velocity if enabled. |
There was a problem hiding this comment.
vx refers to the x-coordinate in GNC space which is actually vertical velocity
| /// Minimum allowable horizontal velocity. | ||
| float vx_min; | ||
|
|
||
| /// Maximum allowable horizontal velocity. | ||
| float vx_max; |
There was a problem hiding this comment.
see above, vx = vertical velocity
| * @param cur_tilt Current vehicle tilt. | ||
| * @param cur_motor Current motor count or stage. | ||
| * @param cur_time_since_launch Time since launch in seconds. | ||
| * @param cur_vx Current horizontal velocity. |
| // any number of leading slashes is tolerated. | ||
| /** | ||
| * @brief Helper evaluation block verifying if a given directory file targets the live logging output metrics stream. | ||
| * @param arg Active flight vehicle operational class instances. |
| /** | ||
| * @brief Safety sweep utility targeting deletion tasks spanning passive log file fragments across media partitions. | ||
| * @details Retains and isolates the currently running data structure files to guard active recording sessions. | ||
| * @param arg Hardware telemetry core coordination state machine structures. | ||
| * @return true On clean execution sweeps; false if hardware blockades disrupt operations. | ||
| */ |
There was a problem hiding this comment.
Genuinely what are these comments
| * @brief Packages and streams unified high-fidelity launch session artifacts down telemetry pipelines. | ||
| * @details Interleaves structure elements, system metadata tracking logs, and direct checksum validation. |
There was a problem hiding this comment.
please reconsider all of these
| void Telemetry::transmit(RocketData& rocket_data, const MIDASEEPROM& eeprom, LEDController& led) { | ||
| // static_assert(sizeof(TelemetryPacket) == 20); | ||
|
|
||
| // Build the packed structured data frame | ||
| TelemetryPacket packet = makePacket(rocket_data, eeprom); | ||
| // Provide visual indication of data frame transmission | ||
| led.toggle(LED::BLUE); | ||
|
|
||
| // Hand over the payload to the specific RF transceiver driver | ||
| backend.send(packet); |
There was a problem hiding this comment.
these comments seem overly verbose
tjmcmanamen38
left a comment
There was a problem hiding this comment.
Thanks everyone for all the work on this!
I'll largely echo Michael's feedback - just more or less there's some places where comments seem to be trying to describe too much and other places where I think they're trying to oversimplify things a bit much.
Also apologies if I'm mistaken on this but a few of these comments are giving me a bit of an AI vibe which to be completely frank I'm not a huge fan of for Spaceshot work in general and definitely documentation.
Overall a lot of my stuff is just nitpicking lol (as you know I can be quite the nitpicker) so up to you how much you want to change but I would encourage reviewing everything a bit more and making some refinements. And I know Michael and I are always happy to give feedback and please please please reach out to us if you need any help with stuff cause ultimately we're the people most familiar with this codebase and want to make sure that you all understand it too ❤️
|
|
||
|
|
||
| bool FSMPyroAction::conditions_met(FSMState fsm_state, float cur_tilt, uint8_t cur_motor, float cur_time_since_launch, float cur_vx) const { | ||
| // Pyro must be enabled. |
There was a problem hiding this comment.
imo most comments in this function are fairly unnecessary since the if statements are pretty self explanatory
|
|
||
| /** | ||
| * ============================================================ | ||
| * SAFE STATE | ||
| * ============================================================ | ||
| * | ||
| * The vehicle is fully disarmed. The only permitted transitions | ||
| * are into PYRO_TEST mode or ARMED mode through explicit commands. | ||
| */ |
There was a problem hiding this comment.
I appreciate the detail with these but I don't think we need to dedicate so many lines to this, just makes it harder to find the actual code between all the comments so id suggest at minimum removing all the =========. I also think its useful to not remove the detail from the comments in this function about where the commands are coming from (wirelessly in this case)
| case FSMState::STATE_PYRO_TEST: | ||
|
|
||
| // Force transtion to safe if requested + clear all transition flags. | ||
| // Immediately return to SAFE if requested. |
There was a problem hiding this comment.
this is just removing detail imo
| case FSMState::STATE_ARMED: | ||
|
|
||
| // Force transtion to safe if requested + clear all transition flags. | ||
| // Allow immediate disarm if commanded. |
| commands.FSM_should_set_cam_feed_cam1 = true; // Set camera to side cam | ||
| cur_state_lockin = false; // Reset "state lock in" flag for boost detect | ||
| launch_time = current_time; // Record launch time. | ||
| time_entered_cur_state = current_time; // Record BOOST entry time. | ||
| commands.FSM_should_set_cam_feed_cam1 = true; // Switch camera feed. |
There was a problem hiding this comment.
switch camera feed removes detail. what is cam1?
There was a problem hiding this comment.
file should be deleted (autogenerated on build)
There was a problem hiding this comment.
personally I see EEPROM as very distinct from logging and feels weird to categorize them in the same folder (configuration data vs flight data more or less)
| } | ||
|
|
||
| /** | ||
| * @brief Router terminal handle processing sub-demands targeted toward flight profiling rules. |
There was a problem hiding this comment.
sorry this wording just feels really unintuitive
| # Running SILSIM | ||
|
|
||
| To build and execute the Software-In-The-Loop simulator: | ||
|
|
||
| ```bash | ||
| pio run -e mcu_silsim | ||
| ``` | ||
|
|
||
| SILSIM allows flight code to be tested without physical hardware. | ||
|
|
||
| > More documentation describing simulation inputs, outputs, and workflows will be added later. |
There was a problem hiding this comment.
silsim is insanely outdated and does not do anything with the current midas software so I don't think its useful to document how to run it until we having a working version in this repo
Documentation for the MIDAS folder