Feature: Implement Haptic Feedback 6.05 #12
Conversation
Relys
force-pushed
the
haptic-feedback-6.05
branch
2 times, most recently
from
80bb0c9
to
4fc90ea
Compare
Relys
force-pushed
the
haptic-feedback-6.05
branch
25 times, most recently
from
10bb08a
to
17f592a
Compare
Relys
force-pushed
the
haptic-feedback-6.05
branch
from
17f592a
to
3693eeb
Compare
| float haptic_error_volt; | ||
| int haptic_error_freq; | ||
| float haptic_vibrate_volt; | ||
| int haptic_vibrate_freq; |
There was a problem hiding this comment.
Can you make a struct member haptic_feedback the same way e.g. the LEDs have it's own config structs? Then you can easily make a copy of it in your HapticFeedback struct.
| if (!VESC_IF->foc_play_tone) { | ||
| return; | ||
| } | ||
| if ((volt > 0) && (freq > 0.0)) { |
There was a problem hiding this comment.
The braces around the > expressions are redundant (and this is a very common expression composition). Can you remove them? This repeats multiple times throughout the PR.
When in doubt, refer to C Operator Precedence
| } | ||
| __attribute__((fallthrough)); | ||
| case SAT_PB_LOW_VOLTAGE: | ||
| __attribute__((fallthrough)); |
There was a problem hiding this comment.
The attribute isn't needed if the case labels follow directly after each other.
| } | ||
|
|
||
| if (!haptic_feedback->tone_in_progress) { | ||
| haptic_feedback_play_two_tones(haptic_feedback); |
There was a problem hiding this comment.
In a clean design this call shouldn't be here. This Function should just set up the tones or the pattern, and the haptic_feedback_update() function take care of the playing by itself. I understand the logic you have relies on this, see my comment on the haptic_feedback_update() function.
There was a problem hiding this comment.
If you don't immediately start playing a tone when called, then by the time you get to the haptic_feedback_update function timer and alt_timer will already be out of date.
| if (!(VESC_IF->foc_play_tone && VESC_IF->foc_stop_audio)) { | ||
| return; | ||
| } | ||
|
|
||
| if (haptic_feedback->tone_in_progress) { | ||
| if ((fabsf(haptic_feedback->timer - current_time) > haptic_feedback->note_duration) || | ||
| (haptic_feedback->haptic_feedback_state.type == HAPTIC_FEEDBACK_BEEP && | ||
| haptic_feedback->beep_num == 0)) { | ||
| haptic_feedback_reset(haptic_feedback, current_time); | ||
| } else if ((haptic_feedback->haptic_feedback_state.peroid > 0) && | ||
| (fabsf(haptic_feedback->alt_timer - current_time) > | ||
| haptic_feedback->haptic_feedback_state.peroid / 1000.0)) { | ||
| haptic_feedback->alt_timer = current_time; | ||
| // update for special modes | ||
| haptic_feedback->counter++; | ||
| if (haptic_feedback->counter % 2 == 0) { | ||
| if (haptic_feedback->haptic_feedback_state.type == HAPTIC_FEEDBACK_BEEP) { | ||
| haptic_feedback_play_single_tone( | ||
| haptic_feedback->haptic_feedback_state.freq, | ||
| haptic_feedback->haptic_feedback_state.volt, | ||
| 0 | ||
| ); | ||
| haptic_feedback->beep_num--; | ||
| } else { | ||
| haptic_feedback_play_two_tones(haptic_feedback); | ||
| } | ||
| } else { | ||
| VESC_IF->foc_stop_audio(false); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic is very complex and opaque. You also repeatedly call the play functions all the time, which is not very clean. What I'd do instead is take a similar approach to when you implement an animation. I would store a bool that says whether you are currently playing a tone. Then, depending on the time, determine whether you're supposed to be playing a tone, and if that differs from the current state, start or stop playing the tone.
For a duty tone pattern, tone-gap-tone-gap-..., you can do e.g.:
const float tone_length = 0.2f;
const float period = tone_length * 2; // note: according to convention I call period the whole segment that repeats
time = fmodf(time, period); // now time is your time within a single period
bool should_be_playing = time < tone_length;For lets say three beeps and a pause:
const float tone_length = 0.2f;
const float period = tone_length * 8; // tone-gap-tone-gap-tone-gap-gap-gap = 8 beats
time = fmodf(time, period);
int beat = floorf(time / tone_length);
bool should_be_playing = (beat % 2) == 0 && beat != 6;I hope this makes sense to you, if not let me know and I'll try to come up with a PoC to demonstrate better.
There was a problem hiding this comment.
So I don't call any of the functions redundantly, only every period when alt_timer expires. Regarding the example for duty tone pattern, I'm not sure that this logic would blend smoothly with how I'm implementing creating a solid tone for when we've reached the duty cycle threshold (by continue to update alt_timer so it never reaches its next period). If you can provide an alternative that's fine, but I'm not sure I want to spend more cycles here rewriting this.
|
I would suggest these revisions. GitHub isn't letting me "review", so I'll just post the commit: Will give this some actual ride testing this week. |
All function calls should be < 6.05 safe as they explicitly check if the play tone pointers aren't null to determine if the API is available.
Features:
Duty Cycle Solid Tone: Duty Cycle threshold where solid tone is played. Tiltback will pulse at a period of 200ms and then go solid once this value is set. Set <= tiltback to always have it solid. Set >=95 to disable (just like tiltback)
Duty Cycle Strength: Zero disables. Will pulse at 200ms when duty cycle tiltback is present.
Duty Cycle Frequency:
Error Strength: Zero Disables. Will pulse at 200ms with a 400ms pause every 2 beeps for hv/lv. For temp (and bms in future) it will pulse at half rate. i.e. 100ms with 200ms pause.
Error Frequency:
Vibrate Strength: Zero disables. Plays in separate channel
Vibrate Frequency: