Parking mode idle improved behavior #32
Conversation
| @@ -592,6 +594,7 @@ static bool check_faults(data *d) { | |||
| } | |||
| } else { | |||
| d->fault_switch_timer = d->current_time; | |||
| d->motor.last_erpm_sign = d->motor.erpm_sign; | |||
There was a problem hiding this comment.
Modifying the MotorData structure outside of the motor_data_ functions goes against the concept of the modularization (which is, any work with the module data happens inside the module, outside of it, you only read them).
I'm also not sure there's more than meets the eye to the surgical precision with which you placed the assignment in two places of the check_faults() function 😀. But the fact it's two places is bad in itself, and this function really needs stuff removed, not added (it needs a refactor).
Instead, please set last_erpm_sign inside motor_data_update(). You'll have to pass whatever data you need for the condition. Seems to me you need d->state.state and check it's equal to STATE_RUNNING and keep updating the var for as long as that holds true. But I might have missed a detail in how you placed those assignments, it's not entirely obvious (which is the point I'm trying to make).
I'd call the variable erpm_last_running_sign.
src/motor_control.c
Outdated
| } else { | ||
| mc->parking_brake_active = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
I like this change to the logic (where the ALWAYS/NEVER cases are handled in configure), it's easier to understand this way 👍.
But is there a reason you put this into the "brake logic" branch, meaning it's not evaluated when there's regular current requested? I'm not sure of the implications right now...
|
Forgot to mention, the changelog record:
Will be out of context and not really clear in the changelog. I'd also incline more towards marking it as a feature, as it's not a bug that it's fixing per se. What about something like:
|
|
Reposting the updated PR against the testing branch |
If rider jumps off the board and the board changes direction after jumping off, it is assumed that the parking brake should be on. Feature: Parking Brake: Brake immediately if the board would roll the opposite way (e.g. downhill when one jumps off while going uphill)
surfdado
force-pushed
the
parking_mode_idle
branch
from
ea98b2e to
2200420
Compare
If rider jumps off the board and the board changes direction after jumping off, it is assumed that the parking brake should be on.
This addresses the use case where you're going uphill and you jump off because it got too steep