[Path] Replace old grbl_post.py with new grbl_G81_post.py.

[fra589]

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:

• Branch rebased on latest master git pull --rebase upstream master
• Unit tests confirmed to pass by running ./bin/FreeCAD --run-test 0
• Commit message is well-written
• Commit message includes issue #<id> or fixes #<id> where <id> is the associated MantisBT issue id if one exists

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.19 Changelog Forum Thread.

---

[sliptonic]

For the most part, this looks great. Thank you! It's definitely an improvement over the old post.

Since we're touching it this heavily, we might as well do a full cleanup on it.

For one thing, there's a mix of spaces and tabs in the file. This is a real pain for maintenance. Also the indentation depth isn't consistent. Sometimes 2 spaces, sometimes 4.

I ran a PEP8 de-linter on it to clean up that stuff.
I also replaced the bare Except clause with an except Exception. Bare excepts are discouraged.

I didn't touch the copyright notice but I'd suggest that you should. This is now a derivative work so preserving the original copyright and adding additional authors and/or history is cleaner.

For an example, take a look at how Russ did Path Surface in this file:
https://github.com/FreeCAD/FreeCAD/blob/master/src/Mod/Path/PathScripts/PathSurface.py

There were a few warnings in the lgtm report for your old version but I think they're all addressed. Might want to double check here:
https://lgtm.com/projects/g/FreeCAD/FreeCAD/latest/files/src/Mod/Path/PathScripts/post/grbl_G81_post.py?sort=name&dir=ASC&mode=heatmap

I haven't made the changes directly but here's a pastebin after the pep8 cleanup:
https://pastebin.com/vMzDUYf7

[mlampert]

if we replace the default grbl post processor then the default should be
TRANSLATE_DRILL_CYCLES = False
changing existing behaviour is generally not a good idea.

Other than that - love it!

[sliptonic]

@mlampert My understanding is that no version of grbl can process canned cycles. I know it changes existing behavior but does it make sense for the default condition to produce something that doesn't work...ever?

[mlampert]

grbl requires a g-code feeder and any g-code feeder dealing with grbl knows about its limitations. In other words, anybody already using grbl has configured their g-code feeder properly and however they did that works for them (I use bCNC and never had a problem with drill cycles).

Now if we change the default behaviour we change the behaviour for everybody who's already using grbl. grbl by itself is useless, it's the combination of grbl with a feeder that makes it work which is what we have to consider here.

[sliptonic]

Ok. I concur.

[fra589]

Hi @sliptonic,

Thanks for your reply,

Oups, sorry for the copyright notice, I took back yours and have simply added my name on it.

For one thing, there's a mix of spaces and tabs in the file.

??? I dont found any tabs in my file, I use geany as IDE and it is configured by default with space indents and with 2 space by indent...

Also the indentation depth isn't consistent. Sometimes 2 spaces, sometimes 4.

The joys of copying / pasting :( => corrected...

This is a real pain for maintenance.

I agree 😟

Bare excepts are discouraged.

Oh! I did not know that... => corrected...

There were a few warnings in the lgtm report for your old version

I registered on lgtm and i am fighting to understand how it works ... but i think now it's good :)

Your pastebin link doesn't point to the grbl_post.py file, but I installed and ran pycodestyle (the pep8 successor) on my system and do a full cleanup on my new grbl_post.py. So, I think I have a good new version.

Hi @mlampert,
I do not quite agree with you about the ability of all gcode loaders to translate the drilling cycles.
I know many who do not, and I think it's not their role to do it.
In my opinion, the role of a gcode loader is to send the file as it is, without any blind modification. This allows control between the contents of the file and what is actually done by the firmware of the machine.
However, I accept your argument about the current post-processor usage, and I changed the default option with TRANSLATE_DRILL_CYCLES = False

Unfortunately, I do not know how to correct this pull request ... Should I cancel it and make a new one, or is there a way to make it evolve?

@++;
Gauthier.

[sliptonic]

You can just push your revised code to the same branch on your repo and the pr will update. Thanks!

…

On Wed, Jun 12, 2019, 3:49 AM Gauthier Brière ***@***.***> wrote: Hi @sliptonic <https://github.com/sliptonic>, Thanks for your reply, Oups, sorry for the copyright notice, I took back yours and have simply added my name on it. For one thing, there's a mix of spaces and tabs in the file. ??? I dont found any tabs in my file, I use geany as IDE and it is configured by default with space indents and with 2 space by indent... Also the indentation depth isn't consistent. Sometimes 2 spaces, sometimes 4. The joys of copying / pasting :( => corrected... This is a real pain for maintenance. I agree 😟 Bare excepts are discouraged. Oh! I did not know that... => corrected... There were a few warnings in the lgtm report for your old version I registered on lgtm and i am fighting to understand how it works ... but i think now it's good :) Your pastebin link doesn't point to the grbl_post.py file, but I installed and ran pycodestyle (the pep8 successor) on my system and do a full cleanup on my new grbl_post.py. So, I think I have a good new version. Hi @mlampert <https://github.com/mlampert>, I do not quite agree with you about the ability of all gcode loaders to translate the drilling cycles. I know many who do not, and I think it's not their role to do it. In my opinion, the role of a gcode loader is to send the file as it is, without any blind modification. This allows control between the contents of the file and what is actually done by the firmware of the machine. However, I accept your argument about the current post-processor usage, and I changed the default option with TRANSLATE_DRILL_CYCLES = False Unfortunately, I do not know how to correct this pull request ... Should I cancel it and make a new one, or is there a way to make it evolve? @++; Gauthier. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2255?email_source=notifications&email_token=AAEDLSIRRG6RA3BQ2O4PFIDP2C2C5A5CNFSM4HW44T7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXPWQRA#issuecomment-501180484>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEDLSJGW76IS45F7GNN76DP2C2C5ANCNFSM4HW44T7A> .

[fra589]

Hi @sliptonic,
Ah, OK 😄
... pushed.
@++;
Gauthier.

[sliptonic]

I think this looks fine and I'll merge it. It's failing the travis build because it isn't rebased but I can update it here in github.

In the future, please use 4 spaces for indentation. It's not a big thing but it's pep8 standard so all the delinters complain if it's not used. Also, it's what we use everywhere. It's one of those personal preference things that you get used to it so it makes spotting things in the code easier.

Also, Thanks for doing this!

[CleisonManriqueAguirre]

Thanks , after trying different options I found it and works