-
Notifications
You must be signed in to change notification settings - Fork 85
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
New saber_sabersense_buttons.h Prop File Request #717
base: master
Are you sure you want to change the base?
New saber_sabersense_buttons.h Prop File Request #717
Conversation
INTRODUCING THE UPDATED SABERSENSE™ LIGHTSABER SWITCH CONTROL SYSTEM OVERVIEW The Sabersense Button System has been engineered with simplicity in mind. It is designed to be very easy to use, bomb-proof and consistent in terms of ‘rules’. It doesn’t require a ‘knack’ to access certain features, and it avoids tricky combinations like holding a button while simultaneously twisting or making other active gestures which can be difficult to execute. HARMONIZED ONE-BUTTON AND TWO-BUTTON CONTROLS By default, basic features like normal ignition, track playing, force effects, mute ignition, quotes etc. all have the same button controls on both one-button and two-button setups. This means users with large hilt collections which include one-button and two-button sabers don’t need to remember different sets of controls for these core features. CONSISTENT ‘RULES’ This again means the user has less to remember. For example, one click of POWER with blade off always lights the blade - short click for normal, long click for muted; double clicking POWER always plays a sound effect - track, character quote or force effect depending hilt status (see next paragraph). This is all intended to make it easier to remember the various controls. COMPREHENSIVE SOUND FONT NAVIGATION One-button and two-button setups both allow users to move forwards or backwards one font at a time, or to skip to first font, last font or middle font. Two-button setups include additional navigation features which allow the user to skip forwards or backwards five and ten fonts at a time. SEPARATE FORCE EFFECT AND CHARACTER QUOTE PLAYER This allows users to access separate force effects, character quotes and music tracks depending on hilt orientation and status. Tracks can be pIayed with the blade OFF, hilt pointing down, Force effects can be played with blade ON, hilt pointing down, and Character Quotes can be played in both blade ON and OFF states with hilt pointing up. If desired, these orientations can be reversed using #define SABERSENSE_FLIP_AUDIO_PLAYERS. Character Quotes are always played sequentially while Force Effects are played randomly. ON DEMAND BLADE ID AND MANUAL ARRAY SELECTOR This allows users to run Blade ID on demand with a button press, eliminating potential blade ID scan errors that might occur under constant automatic scanning when blades are lit. Alternatively the BladeID engine can be reconfigured with a simple define allowing you to cycle through all blade and preset arrays manually, regardless of the actual BladeID status. This is useful if you have a number of different blades, for instance, that all return the same BladeID values and therefore cannot be differentiated by the system. Alternatively it can be used to divide sound fonts into separate arrays; for instance all light side fonts could be on array 1, all dark side fonts on array 2 and all diagnostic and engineering fonts on array 3. RESTORE 'FACTORY' DEFAULTS If you've ever found yourself stuck with settings changes you don't like, you will appreciate this feature. Using a simple, but unlikely, button press (to prevent accidental triggering) the system will delete all user-modified settings such as volume, blade colour and font selection and revert to the settings that were uploaded onto your saber. OPTIONAL BUTTON ‘CLICKER’ DEFINE For hilts that have buttons with poor tactile feedback (KR’s Scavenger for instance, with the emitter wheel button setup), simply adding a press.wav and release.wav of a clicking sound to the appropriate font folder, and including the #define SABERSENSE_BUTTON_CLICKER in your config will play those effects with their respective button actions to aid in feature navigation. Suitable wav files are included to download from the Sabersense website.
A couple of comments made lines more than 100 characters - now sorted.
A couple of tiny clarification tweaks to user guide comments at top - no changes to actual code.
Sorry - just a few more minor comment tweaks for clarity, plus a couple of rogue tabs I spotted. No changes to actual code.
Apologies - spotted a missing bracket on line 542. Now sorted.
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.
first pass...
props/saber_sabersense_buttons.h
Outdated
#endif | ||
|
||
#ifdef SABERSENSE_SWING_ON | ||
#define SABERSENSE_SWING_GESTURE |
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.
What is the point of an #ifdef that just defines another define?
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.
Ermm, I'm not sure. That bit was essentially copied from the SA22C prop. But I'll run some tests without that section to see if it affects anything - it sounds like it won't - in which case I'll delete it.
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.
Now deleted to no ill effect as far as I can tell. Not sure what they were for under SA22C, but gone now.
props/saber_sabersense_buttons.h
Outdated
EFFECT(array); // for playing array idents | ||
EFFECT(bladeid); // for playing bladeid idents | ||
EFFECT(reset); // for playing factory default reset completed | ||
#ifdef SABERSENSE_OS7_LEGACY_SUPPORT |
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.
I think it would be better to add a define like HAVE_EFFECT_QUOTE in sound/effect.h and then check that define here.
(with #ifndef)
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.
Apologies, I'm a little confused on that. Do you mean EFFECT_QUOTE needs to be added to the sound/effect.h file? If so, I thought OS-8/Github Master already had that, and the only reason I need to mention it in my prop is to make my prop work with OS-7.x (as EFFECT_QUOTE isn't included in OS-7's sound/effect.h file). What am I missing here?
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.
I mean, we could add
#define HAVE_EFFECT_QUOTE to effect.h
Then you don't have to have a special define that the user has to add to the their config file for it to work, it would just work.
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.
But wouldn't that mean that anyone trying to use this prop file with OS-7 would need to edit their own sound/effect.h file for it to work (which obviously we can't do for them as OS-7 is already 'out there')? My understanding is that "quote" is already included in OS-8, and so doesn't need listing in the prop file like this - it only needs listing in the prop when using quotes with this prop under OS-7, doesn't it? And since the prop won't work at all under OS-7 without the SABERSENSE_OS7_LEGACY_SUPPORT define, people would need to add it anyway under OS-7 which will automatically make quotes work? Sorry if I'm getting the wrong end of the stick here.
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.
No.
effect.h has #define HAVE_EFFECT_QUOTE in it (OS8 only)
Then here, we could just do:
#ifndef HAVE_EFFECT_QUOTE
EFFECT(quote);
#endif
This means that if HAVE_EFFECT_QUOTE isn't there, then it gets added here.
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.
Aaahhhhh - sorry - the penny has finally dropped! LOL! I'm with you. Will get that sorted. :)
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.
Sorry that took a while for me to grasp. Now sorted.
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.
Doh! I need to look at this again. It works under OS-7 but throws an error under OS-8. Also I can't see HAVE_EFFECT_QUOTE anywhere in sound/effect.h in GitHub Master. If we're proposing to add it, that feels like more work than just using the SABERSENSE_OS7_LEGACY_SUPPORT define in my prop?
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.
Adding HAVE_EFFECT_QUOTE to sound/effect.h is no harder than adding a line to this file.
props/saber_sabersense_buttons.h
Outdated
const char* name() override { return "SabersenseButtons"; } | ||
|
||
|
||
// MAIN LOOP |
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 comment is useless.
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.
Removed.
props/saber_sabersense_buttons.h
Outdated
Event(BUTTON_NONE, EVENT_SWING); | ||
} | ||
} | ||
// EVENT_THRUST |
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.
fix indentation
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.
Still getting my head round the finer points of indenting best practice, but hopefully it's all correct now.
props/saber_sabersense_buttons.h
Outdated
if (SFX_reset) { // Optional confirmation sound file 'reset'. | ||
hybrid_font.PlayCommon(&SFX_reset); | ||
while(IsResetSoundPlaying()); // Lock system while sound finishes... | ||
STM32.reset(); // ...then reboot saber. |
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.
STM.reset() is the same in both if and else branches, so it can be moved out (and the else branch can then be removed)
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.
In the course of fixing this I found that things were'nt very user-friendly without the reset.wav file. So I've added a confirmation beep where no reset file is available which should hopefully make things work better.
props/saber_sabersense_buttons.h
Outdated
} | ||
// Find all immediate subdirectories of the root and delete files. | ||
LSFS::Iterator dirIterator("/"); // Iterator for the root directory. | ||
while (dirIterator) { |
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.
All save directories are listed in the blades[] array, so it would be better to iterate over that instead of iterating over all directories on the SD card.
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.
Done.
props/saber_sabersense_buttons.h
Outdated
swing_blast_ = false; | ||
SaberBase::DoBeginLockup(); | ||
return true; | ||
break; |
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.
no need for break after return
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.
Three instances deleted.
#if NUM_BUTTONS == 1 | ||
// 1 button lockup | ||
case EVENTID(BUTTON_NONE, EVENT_CLASH, MODE_ON | BUTTON_POWER): | ||
#else |
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.
use #elif defined(SANERSEMSE_NO_LOCKUP_HOLD)
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.
Not used #elif before so I think I've done this right.
props/saber_sabersense_buttons.h
Outdated
swing_blast_ = false; | ||
SaberBase::DoBlast(); | ||
} | ||
return 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.
return should be on the same indentation level as previous line
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.
Fixed.
All above comments now fixed (I hope).
I think there was a save issue just now - but all comments hopefully now sorted.
Sincere apologies - I just realised I left some elements I was using for testing in there. No sorted. Should definitely be finished now. Sorry for the hassle.
Tweaks to save file delete process so that button release doesn't conflict with reboot.
Have reverted to the SABERSENSE_OS7_LEGACY_SUPPORT quotes define for now as we know it works with OS-8.
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.
There are lots and lots and lots of indentation issues, which makes the code hard to read.
I suggest finding something that can format the whole code for you, set it to two spaces and re-format the whole code.
props/saber_sabersense_buttons.h
Outdated
============================================================ | ||
========= BUTTON SYSTEM PRINCIPLES AND LOGIC NOTES ========= | ||
|
||
The Sabersense™ button control system has been engineered |
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.
C++ files are generally supposed to be plain ASCII.
To avoid problems either remove the TM character, or replace it with "(TM)"
props/saber_sabersense_buttons.h
Outdated
*/ | ||
#ifndef PROPS_SABER_SABERSENSE_BUTTONS_H | ||
#define PROPS_SABER_SABERSENSE_BUTTONS_H | ||
|
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.
extra empty line (there is never a need for three newlines in a row)
props/saber_sabersense_buttons.h
Outdated
EFFECT(array); // for playing array idents | ||
EFFECT(bladeid); // for playing bladeid idents | ||
EFFECT(reset); // for playing factory default reset completed | ||
#ifdef SABERSENSE_OS7_LEGACY_SUPPORT |
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.
Adding HAVE_EFFECT_QUOTE to sound/effect.h is no harder than adding a line to this file.
props/saber_sabersense_buttons.h
Outdated
if (millis() - push_begin_millis_ > SABERSENSE_FORCE_PUSH_LENGTH) { | ||
Event(BUTTON_NONE, EVENT_PUSH); | ||
push_begin_millis_ = millis(); | ||
} |
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.
indentation doesn't like up here
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.
I fear the indenting thing may be the thing that defeats this endeavour. I've tried running the code through a couple of different online formatters, but they all seem to do something slightly different with it, and I can't tell which is correct. They all seem also to do things that look wrong to me, but given that my formatting is wrong, maybe they are all correct, even with their respective differences. I've tried to read up on it and tried comparing with other ProffieOS prop files, but I just can't seem to find definitive correct methods. Anyway I've posted a version that, according to the reformatter, is correct. I've also done the bits I CAN do, like losing the TM and sorting double line spaces. I still need to sort out HAVE_EFFECT_QUOTE as I was kind of under the impression that the only file I could change was the prop file so that I wouldn't mess with anything else in the OS. But I'll have a crack at making that work. Although of course if the indenting does indeed prove to be the final impasse, everything else is pretty much irrelevant.
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.
I don't even understand why indentation is hard.
{ means more indentation (2 spaces)
} means less indentation (2 spaces)
Everything else is the same indentation as the previous line.
} generally goes on a line by itself
{ generally goes at the end of a line
Make sure to use spaces for everything, no tabs.
if (SOMETHING) {
A;
B;
}
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.
Shouldn't A; & B; be two spaces to the right ?
On my screen if, A;, B;, & } are all on the same line.
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.
Unfortunately github took away the spaces...
props/saber_sabersense_buttons.h
Outdated
} | ||
if (!swinging_ && fusor.swing_speed() > SABERSENSE_SWING_ON_SPEED) { | ||
swinging_ = true; | ||
Event(BUTTON_NONE, EVENT_SWING); |
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.
indentation
props/saber_sabersense_buttons.h
Outdated
if (millis() - thrust_begin_millis_ > 15) { | ||
Event(BUTTON_NONE, EVENT_THRUST); | ||
thrust_begin_millis_ = millis(); | ||
} |
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.
indentation
props/saber_sabersense_buttons.h
Outdated
Event(BUTTON_NONE, EVENT_THRUST); | ||
thrust_begin_millis_ = millis(); | ||
} | ||
} else { |
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.
hard to tell, because there are earlier problems with indentation
I fear the indenting thing may be the thing that defeats this endeavour. I've tried running the code through a couple of different online formatters, but they all seem to do something slightly different with it, and I can't tell which is correct. They all seem also to do things that look wrong to me, but given that my formatting is wrong, maybe they are all correct, even with their respective differences. I've tried to read up on it and tried comparing with other ProffieOS prop files, but I just can't seem to find definitive correct methods. Anyway I've posted a version that, according to the reformatter, is correct. I've also done the bits I CAN do, like losing the TM and sorting double line spaces. I still need to sort out HAVE_EFFECT_QUOTE as I was kind of under the impression that the only file I could change was the prop file so that I wouldn't mess with anything else in the OS. But I'll have a crack at making that work. Although of course if the indenting does indeed prove to be the final impasse, everything else is pretty much irrelevant.
OK, I've been looking again at the HAVE_EFFECT_QUOTE thing, and aside from not being quite comfortable messing with files other than the prop file, I'm finding that even when I do, I simply can't make it work under both OS-7 and OS-8. So I think my only option is to remove OS-7 backwards-compatibility from my prop completely, which is what I've now done.
Missed some double lines, and one reference to OS-7. Now fixed.
Indentations and formatting hopefully now correct. (Thanks to Olivier who kindly offered to go through and check it, making any necessary tweaks). I've also removed the last reference to the now-omitted legacy support.
INTRODUCING THE UPDATED SABERSENSE™
LIGHTSABER SWITCH CONTROL SYSTEM
OVERVIEW The Sabersense Button System has been engineered with simplicity in mind. It is designed to be very easy to use, bomb-proof and consistent in terms of ‘rules’. It doesn’t require a ‘knack’ to access certain features, and it avoids tricky combinations like holding a button while simultaneously twisting or making other active gestures which can be difficult to execute.
HARMONIZED ONE-BUTTON AND TWO-BUTTON CONTROLS By default, basic features like normal ignition, track playing, force effects, mute ignition, quotes etc. all have the same button controls on both one-button and two-button setups. This means users with large hilt collections which include one-button and two-button sabers don’t need to remember different sets of controls for these core features.
CONSISTENT ‘RULES’ This again means the user has less to remember. For example, one click of POWER with blade off always lights the blade - short click for normal, long click for muted; double clicking POWER always plays a sound effect - track, character quote or force effect depending hilt status (see next paragraph). This is all intended to make it easier to remember the various controls.
COMPREHENSIVE SOUND FONT NAVIGATION One-button and two-button setups both allow users to move forwards or backwards one font at a time, or to skip to first font, last font or middle font. Two-button setups include additional navigation features which allow the user to skip forwards or backwards five and ten fonts at a time.
SEPARATE FORCE EFFECT AND CHARACTER QUOTE PLAYER This allows users to access separate force effects, character quotes and music tracks depending on hilt orientation and status. Tracks can be pIayed with the blade OFF, hilt pointing down, Force effects can be played with blade ON, hilt pointing down, and Character Quotes can be played in both blade ON and OFF states with hilt pointing up. If desired, these orientations can be reversed using #define SABERSENSE_FLIP_AUDIO_PLAYERS. Character Quotes are always played sequentially while Force Effects are played randomly.
ON DEMAND BLADE ID AND MANUAL ARRAY SELECTOR This allows users to run Blade ID on demand with a button press, eliminating potential blade ID scan errors that might occur under constant automatic scanning when blades are lit. Alternatively the BladeID engine can be reconfigured with a simple define allowing you to cycle through all blade and preset arrays manually, regardless of the actual BladeID status. This is useful if you have a number of different blades, for instance, that all return the same BladeID values and therefore cannot be differentiated by the system. Alternatively it can be used to divide sound fonts into separate arrays; for instance all light side fonts could be on array 1, all dark side fonts on array 2 and all diagnostic and engineering fonts on array 3.
RESTORE 'FACTORY' DEFAULTS
If you've ever found yourself stuck with settings changes you don't like, you will appreciate this feature. Using a simple, but unlikely, button press (to prevent accidental triggering) the system will delete all user-modified settings such as volume, blade colour and font selection and revert to the settings that were uploaded onto your saber.
OPTIONAL BUTTON ‘CLICKER’ DEFINE For hilts that have buttons with poor tactile feedback (KR’s Scavenger for instance, with the emitter wheel button setup), simply adding a press.wav and release.wav of a clicking sound to the appropriate font folder, and including the #define SABERSENSE_BUTTON_CLICKER in your config will play those effects with their respective button actions to aid in feature navigation. Suitable wav files are included to download from the Sabersense website.