MaslowCNC / GroundControl

This is the Ground Control software used to control the Maslow CNC Machine
https://www.MaslowCNC.com
GNU General Public License v3.0
277 stars 123 forks source link

add Run/Pause/Stop/FAKE_SERVO hotkeys #801

Closed blurfl closed 5 years ago

blurfl commented 5 years ago
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...)!

BarbourSmith commented 5 years ago

Nice work! It seems like the robot is snoozing on this repo entirely so we'll probably need to merge this one manually as well

blurfl commented 5 years ago

I don't know json formatting, but I wonder whether

"VoteHours": "48"

should be

"VoteHours": 48

Instead?

jbarchuk commented 5 years ago

Maybe it's accepting quotes, but failing to parse properly. If it's a generic error others would have to have noticed it already.

Tried jsonlint.com? Maybe another error, but the 48 shows up as the symptom.

https://stackoverflow.com/questions/15368231/can-json-numbers-be-quoted/15368259#15368259

On Sun, Aug 25, 2019, 6:40 PM Scott Smith notifications@github.com wrote:

I don't know json formatting, but I wonder whether

"VoteHours": "48"

should be

"VoteHours": 48

Instead?

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

blurfl commented 5 years ago

Thanks for the links. This repo is different from the Firmware repo, having the "VoteHours" key in ROBOT.md
The robot.py script in the Community-Garden-Robot uses that to decide when to close voting. This is a recent change, and wants some massaging. Your helpful comment suggests that you grok reading json with python, does anything stand out to you in that robot.py script?

jbarchuk commented 5 years ago

I know literally 0.1% about json, py, js. I know enough that I get different effects with/without digit quotes. I give up and quote everything because it's easier than thinking. And it's supposed to work.

"...having the "VoteHours" key in ROBOT.md The robot.py https://github.com/MaslowCommunityGarden/Community-Garden-Robot/blob/master/Scripts/robot.py script in the Community-Garden-Robot https://github.com/MaslowCommunityGarden/Community-Garden-Robot usesthat to decide..." Meaning MaslowCommunityGarden https://github.com/MaslowCommunityGarden/ROBOT.md has the hours? Yes I see robotURL in robot.py. But in ROBOT.md I see only one line and it's not about hours.

On Sun, Aug 25, 2019 at 9:47 PM Scott Smith notifications@github.com wrote:

Thanks for the links. This repo is different from the Firmware repo, having the "VoteHours" key in ROBOT.md The robot.py https://github.com/MaslowCommunityGarden/Community-Garden-Robot/blob/master/Scripts/robot.py script in the Community-Garden-Robot https://github.com/MaslowCommunityGarden/Community-Garden-Robot usesthat to decide when to close voting. This is a recent change, and wants some massaging. Your helpful comment suggests that you grok reading json with python, does anything stand out to you in that robot.py script?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MaslowCNC/GroundControl/pull/801?email_source=notifications&email_token=ABNC5RF4VPE6ESOFUKKE3ILQGMY3RA5CNFSM4IPIFHZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5DBINY#issuecomment-524686391, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNC5REJB4XQTWHNGSU44SLQGMY3RANCNFSM4IPIFHZA .

BarbourSmith commented 5 years ago

One should be read as a number, while the other is read as a string. I think we want the number, so that is a mistake in quotes....I think. But because we are using the default value of 48 I think we can safely take that line out entirely. Nice spot. I am going to open a pull request to remove that line...which we will need to merge manually 😁🙄

blurfl commented 5 years ago

data = json.loads(robotText)

That might choke on a number instead of a string, but leaving the string and using

numberOfHoursForVoting = int(data["VoteHours"])

might be a way around it. Just eliminating the key takes care of it as well, in a stroke!

blurfl commented 5 years ago

which we will need to merge manually 😁🙄

I've merged that one, let's see if this one handles voting correctly now. 😁

MaslowCommunityGardenRobot commented 5 years ago

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

BarbourSmith commented 5 years ago

Wooooo 🎉🎉🎉