search

gpstar81 / GPStar-proton-pack

GPStar Proton Pack and Neutrona Wand
https://www.gpstartechnologies.com
GNU General Public License v3.0
39 stars 9 forks source link

Formatting, wand SYSTEM_YEAR fixes #230

Closed nomakewan closed 11 months ago

nomakewan commented 12 months ago

This update adds #pragma once to all header files to make certain they can only be included once per compile, removes the legacy year_mode variable from the Wand's Header.h, removes some trailing whitespaces, and updates some phrasing in code comments.

Adding the #pragma once stops certain overzealous IDEs from complaining about macro types. As an example, Visual Studio will throw errors whenever setting or getting a macro value if there is not some form of header duplication guard in place. So for instance, the line SYSTEM_MODE = MODE_SUPER_HERO; would throw an error about the SYSTEM_MODES types overlapping without header guards present.

This change also fixes several conditional statements in the Neutrona Wand code to call getNeutronaWandYearMode() instead of directly accessing the value of SYSTEM_YEAR if the wand is in standalone mode as SYSTEM_YEAR is only ever updated via serial data commands from the Proton Pack.

Further, it renames ACTION_EEPROM_MENU to ACTION_LED_EEPROM_MENU for clarity of purpose, and changes getNeutronaWandYearMode() to return a SYSTEM_YEARS enum value rather than an unsigned integer.

DustinGrau commented 12 months ago

Do you run a Mac or Windows machine for your development? Just asking as I have a compile script which works under Mac using arduino-cli to build the binaries, but haven't yet worked out a batch script for Windows (though Cygwin might let you run the bash script).

DustinGrau commented 12 months ago

So another question is why the include guards rather than adopting "#pragma once" since we don't really have a complex setup here? I get using the guards where we might have includes including includes, but so far only the Attenuator does something like that for the wireless.h to provide what's needed when the hardware supports it. https://luckyresistor.me/2019/07/13/why-its-time-to-use-pragma-once/

nomakewan commented 12 months ago

Do you run a Mac or Windows machine for your development? Just asking as I have a compile script which works under Mac using arduino-cli to build the binaries, but haven't yet worked out a batch script for Windows (though Cygwin might let you run the bash script).

My dev environment is Windows. The Arduino IDE will happily compile GPStar without guardrails but it's not good practice; Visual Studio however gets very irate if you don't have them in place.

So another question is why the include guards rather than adopting "#pragma once" since we don't really have a complex setup here? I get using the guards where we might have includes including includes, but so far only the Attenuator does something like that for the wireless.h to provide what's needed when the hardware supports it. https://luckyresistor.me/2019/07/13/why-its-time-to-use-pragma-once/

Two reasons, neither of which are technical. First, because of personal habit (all the libraries I've worked with use header guards). Second, because the WAV Trigger in this very project used header guards already. I hold no allegiance to them, however, so let me switch them over to #pragma once and let's make sure the compile script handles it cleanly. I'll do that as soon as I'm done with breakfast.

DustinGrau commented 12 months ago

Ah, good info, I was not aware that other IDE's would consider that a problem, and glad you found that pattern in use elsewhere. If the pragma line will work, that seems like fewer files to change. Either way, if you could update the PR description to add the explanation about the IDE wanting to enforce the safeguards for include files that would be of benefit later to know why the fix was made.

nomakewan commented 12 months ago

Sorry for the mess there for a bit; I had noticed on my bench that Cross The Streams was acting a little odd in 1984 (it sounded like Afterlife), so I dug into it and saw the mismatched checks for the system year versus wand year. That issue should now be resolved!

DustinGrau commented 12 months ago

Oh, that's right, the pack may send an update that the SYSTEM_YEAR has been changed, but if the user selected a distinct mode for the wand that should take precedence. It would appear the only way you would use the "system" or pack-selected year is if the wand is using YEAR_DEFAULT in which case getNeutronaWandYearMode returns the SYSTEM_YEAR.

DustinGrau commented 12 months ago

The code looks good, but I'd like to run a bench test regardless. If I can get to that today I will certainly try.

nomakewan commented 12 months ago

Oh, that's right, the pack may send an update that the SYSTEM_YEAR has been changed, but if the user selected a distinct mode for the wand that should take precedence. It would appear the only way you would use the "system" or pack-selected year is if the wand is using YEAR_DEFAULT in which case getNeutronaWandYearMode returns the SYSTEM_YEAR.

Correct; the user can also select "Default" in the EEPROM menu, which will drop the wand back to defaults (that is, whatever the pack selects, or Afterlife if the wand is in standalone mode).

The code looks good, but I'd like to run a bench test regardless. If I can get to that today I will certainly try.

Absolutely! More testing is absolutely appreciated since I don't have a pack to test with, only a wand. My standalone wand tests seem to be all good, but I'd really appreciate a full test of the equipment to make sure my changes didn't mess something up with the serial data comms.

nomakewan commented 12 months ago

Decided to re-read the code around some of the serial data and found some bits where it was actually intentional to use SYSTEM_YEAR, as those sections were directly referring to the Pack's year selection rather than the Wand's. I've reverted those sections back so that they will continue to refer to the Pack only (and will only affect the Wand if the Wand is set to YEAR_DEFAULT).

DustinGrau commented 12 months ago

After the reversions, it appears the primary changes are to the firing/stopping actions to ensure the right effects are used/halted for the wand mode, correct?

When in the benchtest mode, I presume the mode changes work as expected, while the EEPROM options let you select the default for boot up? I need to revisit the operation guide to remember how to set the pack and wand to different themes.

gpstar81 commented 12 months ago

After the reversions, it appears the primary changes are to the firing/stopping actions to ensure the right effects are used/halted for the wand mode, correct?

When in the benchtest mode, I presume the mode changes work as expected, while the EEPROM options let you select the default for boot up? I need to revisit the operation guide to remember how to set the pack and wand to different themes.

EEPROM Config Menu 3 setting #5 for changing the wand years. -> WAND_YEAR_MODE

SYSTEM_YEAR is more for the sound effects for firing and this will always match what year the Proton Pack is set to or if a standalone wand, then whatever year the wand is set to.

WAND_YEAR_MODE is for idle effects and beeps of the wand. It basically comes down between 1984/1989 (no ramps, no beep loops) and Afterlife/Frozen Empire (ramp sounds, beep looping).

I haven't had a chance to test this pull request yet, as the new audio updates are keeping me super busy.

nomakewan commented 12 months ago

SYSTEM_YEAR is more for the sound effects for firing and this will always match what year the Proton Pack is set to or if a standalone wand, then whatever year the wand is set to.

WAND_YEAR_MODE is for idle effects and beeps of the wand. It basically comes down between 1984/1989 (no ramps, no beep loops) and Afterlife/Frozen Empire (ramp sounds, beep looping).

Aha, this explains the issue I saw when testing on my bench, then. Since the Proton Pack's EEPROM is what stores SYSTEM_YEAR and not the Wand, but the Wand's EEPROM can store WAND_YEAR_MODE, a standalone Wand can get out of sync when power cycled.

So the actual solution would not be the changes I posted--since they would change the firing/idle sounds for the wand based on WAND_YEAR_MODE for a Wand+Pack setup and that is not desired--but rather entirely separate operation if in standalone mode (that is, creating another conditional to check b_no_pack and using getNeutronaWandYearMode() in lieu of SYSTEM_YEAR only when in standalone mode). Got it.

I'll work on those changes after breakfast.

DustinGrau commented 12 months ago

Glad you caught this! I am now away on vacation through Christmas so I will take a look after the holiday, assuming Michael doesn't get to it first once you complete the changes. I assume this isn't too high priority and the compiler options will help you keep moving on any other updates you were planning to make.

nomakewan commented 12 months ago

Glad you caught this! I am now away on vacation through Christmas so I will take a look after the holiday, assuming Michael doesn't get to it first once you complete the changes. I assume this isn't too high priority and the compiler options will help you keep moving on any other updates you were planning to make.

Yeah, this only affects people who don't have a Pack and are still trying to make things work anyway (which is me and....me? Hahaha), so it's not high priority at all. I've just pushed the update which should now operate as originally intended, and provides some clarifications in the documentation and code comments as to what's going on.

I too will be leaving for the holidays tomorrow and won't be back until around new years (assuming a certain airline which shall not be named does not have a complete system meltdown again). I'll have access to my phone and e-mail but not my dev environment or equipment.

Happy holidays to you all!