search

vlachoudis / bCNC

GRBL CNC command sender, autoleveler and g-code editor
GNU General Public License v2.0
1.54k stars 528 forks source link

Initial support for G2Core #1772

Closed bgbsww closed 1 year ago

bgbsww commented 1 year ago

Tested against an Arduino Due on a full sheet gantry router.

Harvie commented 1 year ago

Thank you. This is cool! I will be happy to merge this, but since it modifies files used by other controllers... is there someone who can verify that the other controlers are not broken by this?

rschell commented 1 year ago

Thank you. This is cool! I will be happy to merge this, but since it modifies files used by other controllers... is there someone who can verify that the other controlers are not broken by this?

I'll take a look, I have both a grbl 1.1 and a grblhal controller I can check.

bgbsww commented 1 year ago

Exciting! I moved up to testing on an hour long file and fixed a couple of remaining bugs. I also dug up an Uno, flashed GRBL 1.1 and threw it on the mill; it smoke tests cleanly, but I'll work it further. I appreciate rschell getting another set of test eyes on it.

rschell commented 1 year ago

Found nothing running air cuts with a large 20+min gcode file on either the grbl 1.1 or grblhal controllers.

Might consider testing the purgeController routine a little more. In my testing of this section, I found the recording of G43.1 in the following code to be problematic:

    # remember and send all G commands
    G = " ".join([x for x in CNC.vars["G"] if x[0] == "G"])  # remember $G

The issue is that the G43.1 command gets recorded but does not have its corresponding TLO Z value, so the subsequent sendGcode(G) command fails.

I have modified my version to be like:

    # remember and send all G commands except G43.1 which needs z length parameter
    G = " ".join([x for x in CNC.vars["G"] if x[0] == "G" and x != "G43.1"])

In this way, the sendGcode(G) gets accepted and the following sendGcode("G43.1") resets the TLO value correctly.

Spoiler Alert...there are other TLO processing issues to resolve.

Harvie commented 1 year ago

Ok, cool. Just let me know when you feel this PR is ready to be safe for everyday use :-)

bgbsww commented 1 year ago

(This has been superseded, see below)

Good find, rschell. I just copied the routine from _GenericController.py so that I could change the protocol piece.

Unfortunately, G2core does not support G43.1 at all! There's no documentation at https://github.com/synthetos/g2/wiki/Gcodes and even more troubling, the parser in the G2core source code only recognizes G43 and G43.2.

I'm honestly a little confused about how you got a G43.1 in CNC.vars to begin with - I don't have it in my testing - I don't have any G codes to capture and send back. It appears the only way it can be set is if the value is returned through parseBracketSquare, which doesn't exist in the G2Core driver. Is this hungover from running GRBL1 in the same or a prior session, maybe?

The summary is: this code doesn't support TLO in any form, and I can't see a path to making it do so since G2Core does not appear to return the necessary information. Unless there's a mechanism for capturing this info whenever the user sets it, and trusting that the controller got the data.

So, looking for input here: simplify purgeController to avoid trying to reset the GCodes and declare no TLO for G2Core, or implement a solution based on some idea one of you have.

justinclift commented 1 year ago

Interesting. Looks like G43.1 might be something originating from LinuxCNC.

So it's probably generated by something that has support for that.

bgbsww commented 1 year ago

Sorry, talking to myself and making you all listen. Figured out that you can force configuration of tofx,tofy,tofz into the status report, which means it is possible to get the current TOF, which changes the game. More code to come. Still have to deal with no G43.1, which means using the tool table offsets.

I do want to note that I see this in two parts: the modifications that might affect other controllers, and those localized to G2Core.py. I expect that first category to be pretty stable ( haven't changed it at all ), whereas I expect to keep progressing G2Core.py to make it smarter.

bgbsww commented 1 year ago

Okay, I lied. I added another change so that I could set TLO without G43.1, and that changes code used by other controllers, although I think in an entirely safe way.

And based on the helpful nudges above, I think I've worked out how to get TLO going correctly, cleaned up WCS, and generally have this in a state where I'd now call it "Intermediate support for G2Core".

Looking forward to other reviews from folks, although I suspect this might be good to go for this step.

bgbsww commented 1 year ago

Suggest moving 2236 to line 44 in _GenericController.py and remove pass

Agreed, done. I was biased towards minimizing changes as opposed to clean correctness, and this is the better long term approach.

bgbsww commented 1 year ago

Thanks for this. I've corrected the issues in setTLO. The other changes appear to me to be addressing bugs in a different scope than this patch, and I'm not in a position to test them correctly. Do they belong in here, or separate?

Harvie commented 1 year ago

Does not look too scary. Merged and we will see what other people think :-)

Thank you.

Harvie commented 1 year ago

I think we have a first complaint: https://github.com/vlachoudis/bCNC/issues/1801

Any ideas what might be causing this?

rschell commented 1 year ago

The offending code in #1801 predates #1772 and #1714.