search

MaslowCNC / Firmware

Maslow Firmware
GNU General Public License v3.0
263 stars 134 forks source link

When (pinCheck >= 6) the board version matches the version strapping #510

Closed blurfl closed 5 years ago

blurfl commented 5 years ago

Add comments to explain the zero-based version number reported from the 1-based gpio pin strapping as suggested in recent discussions: PR#502 Issue#504 PR#505

Beginning with board v1.6, the value from the gpio pins for new boards will be zero-based to match the number reported by the firmware and the silkscreen, as suggested here: Issue#504

Hidden gem: starting with v1.5, pin numbering matches version numbering, we are good at least until v1.15! After that some creativity will be necessary. But for that, return pinCheck<5 ? pinCheck-1 : pinCheck;, in getPCBVersion() to be discussed, of course!

PR#502 PR#505

There is a parallel/competing PR#511 which updates the comments without changing the version strapping - choose one or the other with your vote.

MaslowCommunityGardenRobot commented 5 years ago

Congratulations on the pull request @blurfl

Now we need to decide as a community if we want to integrate these changes. You should vote by giving this comment a thumbs up or a thumbs down. Votes are counted in 48 hours. Ties will not be merged.

I'm just a robot, but I love to see people contributing so I'm going vote thumbs up (but my vote won't count...)!

blurfl commented 5 years ago

PR#510 and PR#511 aren't compatible with each other, one or the other can be merged but not both. I couldn't see a way to be sure that only one would 'win' merging, so I guess I'll watch the votes and close the one with a lower total. Help with the vote count by giving one a 👍🏻and the other a 👎.

BarbourSmith commented 5 years ago

Got it! I saw updating the comments first and thought updating comments is always a good idea...thanks for pointing out that merging both isn't a good idea. I will take my 👍 off of #511 when I next get a stable internet connection.

On Thu, Jun 20, 2019, 10:10 PM Scott Smith notifications@github.com wrote:

PR#510 and PR#511 aren't compatible with each other, one or the other can be merged but not both. I couldn't see a way to be sure that only one would 'win' merging, so I guess I'll watch the votes and close the one with a lower total. Help with the vote count by giving one a 👍🏻and the other a 👎.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/MaslowCNC/Firmware/pull/510?email_source=notifications&email_token=ACHNAV6GKH2MJXGJ6BNFSZ3P3O2YZA5CNFSM4HZMSYF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYGBFJQ#issuecomment-504107686, or mute the thread https://github.com/notifications/unsubscribe-auth/ACHNAV3BONHVLNI7BNRXTYTP3O2YZANCNFSM4HZMSYFQ .

MaslowCommunityGardenRobot commented 5 years ago

Time is up and we're ready to merge this pull request. Great work!