-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements to HV behavior #30
base: v1.1-preview2-devel1
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the one or two extra chars will change anything, so I'd prefer the |
||
} HapticFeedbackType; | ||
|
||
typedef struct { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -755,20 +755,20 @@ static void calculate_setpoint_target(data *d) { | |
} else if (d->motor.duty_cycle > 0.05 && input_voltage > d->float_conf.tiltback_hv) { | ||
d->beep_reason = BEEP_HV; | ||
beep_alert(d, 3, false); | ||
if (((d->current_time - d->tb_highvoltage_timer) > .5) || | ||
(input_voltage > d->float_conf.tiltback_hv + 1)) { | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if (d->motor.erpm > 0) { | ||
d->setpoint_target = d->float_conf.tiltback_hv_angle; | ||
} else { | ||
d->setpoint_target = -d->float_conf.tiltback_hv_angle; | ||
} | ||
|
||
d->state.sat = SAT_PB_HIGH_VOLTAGE; | ||
} else { | ||
// The rider has 500ms to react to the triple-beep, or maybe it was just a short spike | ||
d->state.sat = SAT_NONE; | ||
d->setpoint_target = 0; | ||
} | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not super major, but for awareness: This function is bound for a refactor, I'd like to change it so that only the 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. |
||
} else if (VESC_IF->mc_temp_fet_filtered() > d->mc_max_temp_fet) { | ||
// Use the angle from Low-Voltage tiltback, but slower speed from High-Voltage tiltback | ||
beep_alert(d, 3, true); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.