jetpack_Oli_buttons.h #755
Conversation
Added Jetpack Effects
| EFFECT(targetting); // viewfinder search/finds target sound | ||
| EFFECT(missilelaunch); // missile launch sound | ||
| EFFECT(missilegoesboom); // double explosion in the distance sound | ||
| EFFECT(mandotalk); // "Nice shot! I was aiming for the other one! |
There was a problem hiding this comment.
Nothing wrong with quote, but I am planning to have a quote player added later down the line and this one has a specific use (recreating the missile scene of The Mandalorian S02E06 that I don't want to mix with regular quotes.
If you want me to use quote, I will but I would rather not.
There was a problem hiding this comment.
And I also would like to have a specific animation for this "quote"
props/jetpack_Oli_buttons.h
Outdated
| case EVENTID(BUTTON_POWER, EVENT_FIRST_SAVED_CLICK_SHORT, MODE_OFF): | ||
| case EVENTID(BUTTON_POWER, EVENT_FIRST_SAVED_CLICK_SHORT, MODE_ON): | ||
| if (!mode_volume_) { | ||
| if (idle_) StartFlightMode(); // Transition from idle to flight (rev more up) |
There was a problem hiding this comment.
This is not how you make an if statement easy to read, please remove extra spaces and put the function call on the next line.
There was a problem hiding this comment.
Done, but I personally find it harder to read.
props/jetpack_Oli_buttons.h
Outdated
| else if (flight_) StopFlightMode(); // Transition from flight to idle (rev down) | ||
| else StartIdleMode(); // Jetpack initial start from 0 to idle (rev up) | ||
| // if idle, it revs up / if up, it revs down to idle / if not idle or not up, it needs to rev "up" to idle. | ||
| } else VolumeUp(); |
There was a problem hiding this comment.
If you have a } before the else, you should have one after the else too.
} else {
VolumeUp();
}
props/jetpack_Oli_buttons.h
Outdated
| // Next Preset (1 button): | ||
| #if NUM_BUTTONS == 1 | ||
| if (!mode_volume_) next_preset(); | ||
| return true; |
There was a problem hiding this comment.
Since you have return true at the end of both the #if and the #else, you should just move it out to after the #endif.
props/jetpack_Oli_buttons.h
Outdated
| #else | ||
| // Start or Stop Track (2 and 3 buttons): | ||
| #ifdef JETPACK_TRACK_PLAYER | ||
| if (!istrack_playing_) StartTrackPlayer(); |
There was a problem hiding this comment.
Don't do this:
if (A) B();
else C();
Do this instead:
if (A)
B();
else
C();
There was a problem hiding this comment.
Done, I think I got them all ? Maybe a second "first pass" to make sure ?
props/jetpack_Oli_buttons.h
Outdated
| return true; | ||
| #endif // NUM_BUTTONS == 1 | ||
| case EVENTID(BUTTON_POWER, EVENT_FIRST_CLICK_LONG, MODE_ON): | ||
| // Volume Down (1 button): |
props/jetpack_Oli_buttons.h
Outdated
| #ifndef DISABLE_COLOR_CHANGE | ||
| ToggleColorChangeMode(); | ||
| #endif | ||
| return true; |
There was a problem hiding this comment.
You probably shouldn't return true here if DISABLE_COLOR_CHANGE is defined, since it doesn't actually do anything in that case.
There was a problem hiding this comment.
Done, I think ?
props/jetpack_Oli_buttons.h
Outdated
| #endif | ||
| #else | ||
| // Color Change mode (3 buttons): | ||
| #ifndef DISABLE_COLOR_CHANGE |
There was a problem hiding this comment.
Seems like it would be better to do:
#ifndef DISABLE_COLOR_CHANGE
#if NUM_BUTTONS == 1
#elif NUM_BUTTONS == 2
#elif ...
#endif
#endif
instead of:
#if NUM_BUTTONS==1
#ifndef DISABLE_COLOR_CHANGE
#endif
#elif NUM_BUTTONS==2
#ifndef DISABLE_COLOR_CHANGE
#endif
#ifndef NUM_BUTTONS==3
#ifndef DISABLE_COLOR_CHANGE
#endif
#endif
There was a problem hiding this comment.
Done, yes that looks much better.
props/jetpack_Oli_buttons.h
Outdated
| void EffectHelper (Effect* effect1,const char* msg1) { | ||
| hybrid_font.PlayPolyphonic(effect1); | ||
| PVLOG_STATUS << msg1 << "\n"; | ||
| return; |
There was a problem hiding this comment.
return; at the end of a function is redundant
There was a problem hiding this comment.
Removed.
But then do I need to add return; at the end of each line in the switch (where it was initially before I had the EffectHelper function)?
Suggestions changed
added the lines for #define EXTRA_DISPLAY_CONTROLLER_INCLUDE
I have not yet been able to test it because my Proffie board is still not yet connected but I am hoping for my setup to be ready by end March 2025. I know I said end February but it is taking me a lot longer then I thought to design a chassis that I will be happy with.
I am hoping for a head-start on the code review as I am hoping not to miss the OS8.x launch.
Thank you for your time.
I also have the "jetpackdisplaycontroler" but I will wait until it can be integrated in the prop directly rather than in ssd1306.h