search

gpstar81 / GPStar-proton-pack

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

Fix compilation errors #153

Closed nomakewan closed 1 year ago

nomakewan commented 1 year ago

This PR resolves the compilation errors displayed in the Arduino IDE by more strictly enforcing datatypes rather than leaving conversions and comparisons up to the compiler.

DustinGrau commented 1 year ago

While I'm good with the changes to the true "gpstar" code I would be hesitant to approve the 3rd-party code that was added more of convenience but not written explicitly for this project. That said, I do understand the changes: I can see there's removal of a variable that's written but never read, and casting of a value explicitly rather than implicitly.

nomakewan commented 1 year ago

That's understandable; I noticed that when it was added to this project, the headers were modified so that they would work specifically with this project (that is, the user no longer has to edit the header in order to make it work for the gpstar/Mega/Nano setups).

I'd be more than happy to open a PR for those changes on the source repo as well. In fact, I think I'll do just that.

EDIT: Done.

EDIT EDIT: Additionally, I see that this repo's version of WavTrigger already implemented the fix for AltSoftSerial which was not present in the upstream library. I only noticed that after fixing it for a PR there. So implementing fixes to the gpstar version of WavTrigger should be a-ok.

DustinGrau commented 1 year ago

For reference, is this the upstream repo you mentioned? https://github.com/robertsonics/WAV-Trigger-Arduino-Serial-Library

nomakewan commented 1 year ago

Yep! That's the one. I have an active PR there now to fix several issues, which I have also replicated here.

nomakewan commented 1 year ago

Looks like the latest commit added a unique serialFlush() function to the Proton Pack copy of WavTrigger, so just to be sure I've updated the branch of this PR. Looks like it's still good to go.

DustinGrau commented 1 year ago

Michael was experimenting with some additional changes which may or may not move forward. I think he plans some cleanup. We both have some travel/plans lined up so the software will go on a brief break before that occurs.

nomakewan commented 1 year ago

I've merged main into this so that the newly-instituted compile checks would run on the PR so that it could more easily be approved for merging. The other PR has likewise been updated to account for the new checks. Apologies for the delay in updating, I was away from my lab.

DustinGrau commented 1 year ago

No apologies needed, I had not expected Michael to approve my changes early. I had just returned from my trip overseas and he is still on vacation most of this week. This was something we had spoken about implementing, though ideally we want to pre-build the binary firmware files for easy uploading as the end result. I happened to incorporate some of your changes as part of my testing the compilation process--I'm holding out on the WavTrigger updates until Michael returns and implements some items he's been thinking over.

nomakewan commented 1 year ago

Gotcha. I'll keep an eye out for the changes and update as necessary.