Modify ')' parsing in GCode.cpp

esspe2 commented 5 years ago

As discussed before, this condition seemed useless or incomplete. In the latter case I put a pertinent warning following what I have understood.

MaslowCommunityGardenRobot commented 5 years ago

Congratulations on the pull request @esspe2

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

Have you had a chance to test this? I haven't found a way to cause the print statement to occur. I've tried:

GroundControl strips off comments, but doesn't handle incorrectly bracketed comments properly - it ignores the line. The firmware runs the lines that GC does send without ever seeing the comments. There is an opportunity for a contribution in the GC repo right there...
Using a serial terminal (cycle DTR on/off/on to reset the firmware) to send gcodes with comments, the firmware freezes on receiving any line with a comment, correct or not.

Finally, would one want the firmware to stop or pause execution when a comment issue is detected? Look at the flag ALARM_GCODE_PARAM_ERROR, which posts the message: There is a parameter error in this line of Gcode - make a note of the line number. Cutting will be put on hold until you choose whether to 'Resume Cut' (skipping this line) or 'Stop'. and handles the response.

esspe2 commented 5 years ago

Well seen: I didn't make any test, that just seemed logical and better than simply dropping the condition.

I have looked at the ALARM_GCODE_PARAM_ERROR flag and its message; even after hours of thinking I don't know which behavior is best.

ignore (actual)
warn and continue (my proposal)
stop and ask for reply to resume/stop (as with the flag)

You wouldn't find a way to cause its statement to print either :wink: (it isn't used anywhere)

Either way, if the firmware freeze appeared with this change, I'll close the PR; tell me what you see.

blurfl commented 5 years ago

The firmware freezes even without this change, so for that it doesn't matter. I would suggest that stopping a cut to tell the user that a line is about to be skipped is very important. A path can be very distorted if one of its steps is eliminated. Stopping gives the operator a chance to fix things or cut their losses before expensive material is damaged. For that reason I lean toward a 👎vote. I'm not in a position to do anything about the firmware freeze issue though.

davidelang commented 5 years ago

the problem is that you may freeze with the bit in contact with the wood, that can lead to burning the wood, if not setting a fire.

if you are going to stop and complain, move the bit to the safe height first, and then move it back into the wood after the fault is acknowledged.

It would be safer to have GC check the g-code when it loads the file.

David Lang

On Thu, 2 May 2019, Scott Smith wrote:

Date: Thu, 02 May 2019 20:44:23 -0700 From: Scott Smith notifications@github.com Reply-To: MaslowCNC/Firmware reply@reply.github.com To: MaslowCNC/Firmware Firmware@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [MaslowCNC/Firmware] Update GCode.cpp (#496)

The firmware freezes even without this change, so for that it doesn't matter. I would suggest that stopping a cut to tell the user that a line is about to be skipped is very important. A path can be very distorted if one of its steps is eliminated. Stopping gives the operator a chance to fix things or cut their losses before expensive material is damaged. For that reason I lean toward a 👎vote. I'm not in a position to do anything about the firmware freeze issue though.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/MaslowCNC/Firmware/pull/496#issuecomment-488918250

blurfl commented 5 years ago

if you are going to stop and complain, move the bit to the safe height first, and then move it back into the wood after the fault is acknowledged.

The stop alert pops up for other faults besides this one. 'Stop' behavior would make a good issue on its own.

It would be safer to have GC check the g-code when it loads the file.

Agreed, presently GC swallows the lines with faulty comments and sends a gcode stream missing those lines. This too is a separate issue. What other gcode senders are you familiar with - do they eat comments or send them on?

esspe2 commented 5 years ago

Other firmwares don't seem to bother with comments in gcode, if I have searched well: Grbl, Repetier-DaVinci and Marlin gave me no clues about that, but I didn't look thoroughly of course.

So comments cleaning would be the responsibility of GC?

Then 'ignore' would be the best choice, just to be fool-proof.

And that PR set aside, some new issues could be filed after this discusssion.

esspe2 commented 5 years ago

OK we may abandon this PR for now: my change is useless before fixing the freeze behavior pointed by blrufl.

Btw this freeze seems to happen if there is at least one character after the opening parenthesis.

MaslowCNC / Firmware

Modify ')' parsing in GCode.cpp #496