Braking system improvements#17
Conversation
| loop { | ||
| let update = nav_rx.receive().await; | ||
| latest = latest.merge(update); | ||
| brake_solenoid_pin.set_high(); // Unclamp brakes (Ensure brakes start at known state) |
There was a problem hiding this comment.
You could go above and beyond and macro this so that it's even clearer to read, something like:
macro_rules! unclamp_brakes {
() => {
brake_solenoid_pin.set_high();
};
}
macro_rules! clamp_brakes {
() => {
brake_solenoid_pin.set_low();
};
}then you can use unclamp_brakes!() and clamp_brakes!() in your code instead.
There was a problem hiding this comment.
Actually, you seem to have engage_brake_solenoid and release_brake_solenoid defined in this crate and imported in this file which you could use too?
| continue; | ||
| } | ||
| // Distance travelled during reaction time (d = v * t) | ||
| let reaction_dist = velocity * reaction_time_s; |
There was a problem hiding this comment.
Where is reaction_time_s defined? It should probably be read from the config file, or if it's a constant in the code it should be REACTION_TIME_S.
| continue; | ||
| } | ||
| // Distance travelled during reaction time (d = v * t) | ||
| let reaction_dist = velocity * reaction_time_s; |
There was a problem hiding this comment.
| let reaction_dist = velocity * reaction_time_s; | |
| let reaction_distance = velocity * reaction_time_s; |
| let mut brakes_engaged = false; | ||
|
|
||
| // Safety Parameters | ||
| let max_brake_decel = MAX_BRAKE_FORCE_N / POD_MASS_KG; // m/s^2 |
There was a problem hiding this comment.
| let max_brake_decel = MAX_BRAKE_FORCE_N / POD_MASS_KG; // m/s^2 | |
| let max_brake_deceleration = MAX_BRAKE_FORCE_N / POD_MASS_KG; // m/s^2 |
There was a problem hiding this comment.
Actually, MAX_BRAKE_FORCE_N and POD_MASS_KG also sound like they should be in the config file and not constants.
| if velocity > 0.1 && distance_to_target <= stopping_distance { | ||
| // Total required buffer distance to fully brake | ||
| let required_stop_buffer_distance = reaction_dist + physical_braking_distance + SAFETY_MARGIN_M; | ||
| // SAFETY_MARGIN_M from line 57 (to be implemented) |
There was a problem hiding this comment.
Is this still to be implemented? Also sounds like something that could go in the config file
| if velocity > 0.1 && projected_stop_point >= target { | ||
| if !brakes_engaged { | ||
| defmt::info!( | ||
| "Engaging brakes: pos={} m, target={} m, dist={} m, vel={} m/s, stop_dist={} m", | ||
| defmt::warn!( | ||
| "BRAKING POINT REACHED: Pos={:.2} m, Vel={:.2}, Target={:.2}, Proj_stop={:.2}", | ||
| position, | ||
| target, | ||
| distance_to_target, | ||
| velocity, | ||
| stopping_distance | ||
| target, | ||
| projected_stop_point | ||
| ); | ||
| // Tell motor controller to stop drive before clamping. | ||
| enqueue_canopen(Messages::QuickStop).await; | ||
| enqueue_canopen(Messages::Shutdown).await; | ||
| // Brake solenoid clamps when driven low (per pneumatics::engage_brakes comments). | ||
| engage_brake_solenoid().await; | ||
| brake_solenoid_pin.set_low(); | ||
| brakes_engaged = true; | ||
| } | ||
| } else { | ||
| // Keep brakes released until within stopping distance. | ||
| if brakes_engaged { | ||
| defmt::info!("Brakes remain engaged (no auto-release implemented)"); | ||
| } else { | ||
| release_brake_solenoid().await; | ||
| brake_solenoid_pin.set_high(); | ||
| } |
There was a problem hiding this comment.
I think the logic could be tidied up a bit, e.g. checking if the brakes are engaged and using continue instead of nesting ifs
| defmt::info!("Braking system active (mechanical brakes via GPIO)"); | ||
|
|
||
| let mut latest = NavKinematics::default(); | ||
| let mut brakes_engaged = false; |
There was a problem hiding this comment.
I'm not sure this is required if you're expecting it to be the same as the value on the pin, because you can access that with brake_solenoid_pin.is_set_high() or brake_solenoid_pin.get_output_level() (docs)
| (_, _, Some(v)) if v > 1.0 => { | ||
| if !brakes_engaged { | ||
| defmt::error!("CRITICAL: Lost Position/Target while moving (v={})! Engaging emergency brakes.", v) | ||
| enqueue_canopen(Messages::QuickStop).await; | ||
| brake_solenoid_pin.set_low(); | ||
| brakes_engaged = true; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
What's the intention behind handling this separately? Would be good to pop a comment above it with the rationale
2 participants
Upgraded the braking loop from a passive monitor to an active fail-safe system. This PR introduces communication timeouts and accounts for mechanical latency in stopping distance calculations.
Key changes:
DATA_TIMEOUTcan be modified but is set to a default value of 100ms.The previous loop could hang indefinitely if it doesnt receive data from navigation. These changes ensure that the pod defaults to a safe state under any data loss.