Improvements to HV behavior #30
Conversation
Haptic buzz needs to go off instantly Tiltback kicks in later
Also, HV tiltback now delayed by 5 seconds instead of just 0.5
| } | ||
| // setting the state regardless to ensure haptic buzz starts right away | ||
| d->state.sat = SAT_PB_HIGH_VOLTAGE; |
There was a problem hiding this comment.
Not super major, but for awareness: This function is bound for a refactor, I'd like to change it so that only the state.sat is set based on the conditions, and then there'll be a function that sets setpoint_target according to sat (haven't fully thought through some of the cases but that's the rough idea).
This special-case throws a wrench into that plan, and is not pretty as it is. Obviously if this behavior is desired it can be worked out and it should be prettier after the refactor... But it's better to not have special cases.
| strength = strength * fminf(1.5, duration * 0.25 + 0.25); | ||
| } | ||
| } | ||
| VESC_IF->foc_play_tone(0, tone->frequency, strength); |
There was a problem hiding this comment.
This is a second special-case being added 🥲.
Also, this raises the strength above the user-set threshold, and by as much as 50%. Doesn't seem like a good thing to do.
If I was to contrive a disaster scenario, someone configures their haptic feedback and tests it at the configured levels (no way to easily know what this +50% strength will do and even that it can actually happen on HV). Then, months later, they trigger HV on a steep downhill. They don't react in time, the loud haptics mess up the imu and make the nose go higher, they go into taildrag and crash.
| @@ -27,7 +27,8 @@ typedef enum { | |||
| HAPTIC_FEEDBACK_DUTY, | |||
| HAPTIC_FEEDBACK_DUTY_CONTINUOUS, | |||
| HAPTIC_FEEDBACK_ERROR_TEMPERATURE, | |||
| HAPTIC_FEEDBACK_ERROR_VOLTAGE, | |||
| HAPTIC_FEEDBACK_ERROR_LO_VOLTAGE, | |||
| HAPTIC_FEEDBACK_ERROR_HI_VOLTAGE, | |||
There was a problem hiding this comment.
I don't think the one or two extra chars will change anything, so I'd prefer the HIGH and LOW spelling.
| // 500ms have passed or voltage is another volt higher, time for some tiltback | ||
| if ((d->current_time - d->tb_highvoltage_timer) > 5) { | ||
| // It is assumed that haptic feedback is enabled! | ||
| // 5s have passed or voltage is another volt higher, time for some tiltback |
There was a problem hiding this comment.
You've removed the "another volt higher" part of the condition. A case in point with these comments, self-explanatory code is better.
But more importantly, at this stage we can't make that assumption, which in my eyes just invalidates the PR.
In High Voltage cases: