search

gpstar81 / GPStar-proton-pack

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

Vibration fixes #324

Closed nomakewan closed 3 months ago

nomakewan commented 3 months ago

This PR fixes situations where a custom pack without a vibration toggle switch would turn off all vibration on both pack and wand even if the EEPROM mode is set to override, fixes a situation where a wand would disable vibration based on initial pack setting even if set to override, fixes incorrect inner cyclotron switch panel LED behavior regarding vibration on/off setting, simplifies several vibration-related checks on the wand, and improves vibration-related code documentation.

nomakewan commented 3 months ago

I have reached out to the two users who reported this bug to have them test it, since they know what steps they took to reproduce the issue.

Toy203 commented 3 months ago

Might be able to test it tomorrow morning (Italy time) if needed before pull.

nomakewan commented 3 months ago

Might be able to test it tomorrow morning (Italy time) if needed before pull.

That would be fantastic if you could!

nomakewan commented 3 months ago

I have updated the variable names to make it clearer what they do.

As for making it an enum, yes, that can be done. The issue with making it an enum is that it would then take the setting out of Configuration.h and put it into Header.h and have the initial setting be done in setup() of the INO instead. With the bools the way they are, they can be set in Configuration.h instead.

While I'm okay with doing that, I'd like to get input from @gpstar81 to see if that would be okay before making that change since it means removing an Advanced Configuration option.

DustinGrau commented 3 months ago

Thank you, that does help with the renaming. And I see that the VIBRATION_MODE_EEPROM at least condenses things down to a single setting for storage, so you're right the only thing stopping a true consolidation is the Configuration.h file. Hopefully that may go away entirely as we get more of the fine-tuning for preferences moved into the menu system. So perhaps that's more of a consideration for cleanup when that particular re-org effort takes place.

Toy203 commented 3 months ago

Let me know if you need some beta testing for the new code. =)

nomakewan commented 3 months ago

Let me know if you need some beta testing for the new code. =)

Yep, would still very much appreciate it!

Toy203 commented 3 months ago

Confirmed, vibration settings are now read correctly from what i can see.

nomakewan commented 3 months ago

The person who reported the bug the first time says they will be able to test it tomorrow. I would still like to hold off on merging this until I hear back from them as well. That said, thank you so much for testing, and awesome to hear it works on your end!

If for some reason I don't hear back tomorrow or they reach out saying they won't be able to test, I'll merge. Otherwise as soon as I hear back from them with success, I'll merge.

nomakewan commented 3 months ago

Just got word back that they tested it and it worked!