Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix STATUSPAT and POSPAT for non-6-axes GRBL0 controllers #1525

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

aib
Copy link
Contributor

@aib aib commented Jan 30, 2021

The optional, non-capturing groups introduced with 5abe8cb had capturing groups inside them, causing nulls to be captured, causing:

Traceback (most recent call last):
  File "/usr/lib/python3.8/threading.py", line 932, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.8/threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "/home/aib/proj/OS/bCNC/bCNC/Sender.py", line 787, in serialIO
    elif self.mcontrol.parseLine(line, cline, sline):
  File "/home/aib/proj/OS/bCNC/bCNC/controllers/_GenericController.py", line 213, in parseLine
    self.parseBracketAngle(line, cline)
  File "/home/aib/proj/OS/bCNC/bCNC/controllers/GRBL0.py", line 27, in parseBracketAngle
    CNC.vars["wx"] = float(pat.group(5))
TypeError: float() argument must be a string or a number, not 'NoneType'

with <6-axis GRBL 0 controllers, e.g.:

<Idle,MPos:0.000,0.000,0.000,WPos:0.000,0.000,0.000>

This would in turn cause the program to be stuck at Status "Not connected" and not work at all.

Probably fixes #779

...by removing regex capture groups for the 4th, 5th and 6th fields
@Harvie
Copy link
Collaborator

Harvie commented Feb 16, 2021

Hello, thanks for improvements... Have you tested this to work for 3-axis machines as well as the 6-axis?

@aib
Copy link
Contributor Author

aib commented Feb 17, 2021

I have tested it with a 3-axis machine. I do not have a 6-axis machine to test against.

However, for a line like:

<Idle,MPos:0.000,1.000,2.000,3.000,4.000,5.000,WPos:6.000,7.000,8.000,9.000,10.000,11.000>

The old code produces variables:

'mx': 0.0, 'my': 1.0, 'mz': 2.0
'wx': 3.0, 'wy': 4.0, 'wz': 5.0
'ma': 0.0, 'mb': 0.0, 'mc': 0.0
'wa': 0.0, 'wb': 0.0, 'wc': 0.0

which I suspect are wrong. The new code produces:

'mx': 0.0, 'my': 1.0, 'mz': 2.0
'wx': 6.0, 'wy': 7.0, 'wz': 8.0
'wa': 0.0, 'wb': 0.0, 'wc': 0.0
'ma': 0.0, 'mb': 0.0, 'mc': 0.0

in addition to working properly with 3-axes. (As has been reported, the old code does not produce variables at all with 3 axes.)

This is because the old regex (last changed in the commit I mentioned) made the ma-mb-mc-wa-wb-wc numbers optional without changing the capture groups. Therefore, the groups 4-5-6 and 10-11-12 are optional (i.e. can be None.) Since the code assigning the variables from the groups assumes mx/y/z=1-2-3 and wx/y/z=4-5-6, it fails to work when any of 4-5-6 are None. This is the case when there are less than 6 axes.

It might have been better to fix the code's assumption that wx/y/z lie at 4-5-6 (i.e. leave groups 4-5-6 optional and assign wx/y/z from 7-8-9) but that would have required more time, testing, and knowledge I did not have until I started writing this explanation. And other declared to use STATUSPAT and POSPAT, too (some of which, I see now, are erroneous).

I'd be willing to do some of these other improvements as well, now that I know they are being considered.

@Harvie
Copy link
Collaborator

Harvie commented Feb 17, 2021

Ok, i've noticed there is another PR for the same issue.
Would you mind doing quick review of changes proposed in #1452 please?
To see how they differ and ensure both parties are satisfied with the results?

I've done quick diff between proposed POSPATs:

image

Thank you

@Harvie Harvie mentioned this pull request Feb 17, 2021
@aib
Copy link
Contributor Author

aib commented Feb 18, 2021

Are you sure your modifications targeted to v0 will not break compatibility with v1 protocol?

I am sure as far as that I'm modifying variables used by GRBLv0.py and unused by GRBLv1.py.

Oh i didn't noticed your PR is targeted to GRBLv0. In my opinion it is rather weird to use GRBLv0 nowadays, when v1 is already becoming obsolete...

I just wanted to test this board, which came with v0, before upgrading the firmware. That's when I noticed and quickly fixed the bug. As a matter of principle, I make proper commits out of my bug fixes and share them with the original project.

You are right that using v0 is weird/obsolete. In fact, it's not worth any more of my time—and I've already spent way more than I was planning to. I am closing this request.

@aib aib closed this Feb 18, 2021
@Harvie Harvie reopened this Feb 18, 2021
@Harvie Harvie merged commit 6c0e3d2 into vlachoudis:master Feb 18, 2021
@Harvie
Copy link
Collaborator

Harvie commented Feb 18, 2021

:-D

@Harvie
Copy link
Collaborator

Harvie commented Feb 18, 2021

I am sure as far as that I'm modifying variables used by GRBLv0.py and unused by GRBLv1.py.

Now i see. It was actualy imported in GRBLv1, but not used. I will remove the import.

I make proper commits out of my bug fixes and share them with the original project.

Sorry i was hesitant with merging, your work is certainly appreciated... Actualy there are still people reporting GRBLv0 bugs now and then. But i am kinda busy and regular expressions are not easy to code-review. Especialy when i don't have any GRBLv0 device to test it.

Thank you.

@aib
Copy link
Contributor Author

aib commented Feb 18, 2021

But i am kinda busy and regular expressions are not easy to code-review.

You are right and I defintiely appreciate the effort.

I will try to find a way to keep or re-upload v0 firmware so I can go back and forth for better testing.

Thank you, too.

@Harvie
Copy link
Collaborator

Harvie commented Feb 18, 2021

I've cleaned up the regex code a bit and in the moment of horror i realized that only the following regex is used in the GRBLv1:

SPLITPAT  = re.compile(r"[:,]")

I have no idea how this is sufficient to properly parse everything... I thik i will have to review the GRBLv1 code later...

@rschell
Copy link
Contributor

rschell commented Feb 24, 2021

VARPAT was commented out as not being used but in the generic routine it is used in line 248

Harvie added a commit that referenced this pull request Feb 24, 2021
@Harvie
Copy link
Collaborator

Harvie commented Feb 24, 2021

@rschell Thank you. Good point :-)

rar8000 pushed a commit to rar8000/bCNC that referenced this pull request Jul 21, 2023
rar8000 pushed a commit to rar8000/bCNC that referenced this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serial not working correctly.
3 participants