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

Add POSPAT tests #1452

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add POSPAT tests #1452

wants to merge 1 commit into from

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Sep 1, 2020

No description provided.

@sttts
Copy link
Contributor Author

sttts commented Sep 1, 2020

@Harvie I think travis build with 2.7 is broken independently from this PR.

@Harvie
Copy link
Collaborator

Harvie commented Dec 27, 2020

Hello,
can you please explain the changes to me? how are we supposed to use the included test controler?

Also it does not start:

$ python -m bCNC
Traceback (most recent call last):
  File "/usr/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/harvie/Temp/bCNC-sttts/bCNC/__main__.py", line 50, in <module>
    import Utils
  File "/home/harvie/Temp/bCNC-sttts/bCNC/Utils.py", line 41, in <module>
    if getattr( sys, 'frozen', False ):
NameError: name 'sys' is not defined

@Harvie
Copy link
Collaborator

Harvie commented Dec 28, 2020

Also there is already some similar change done by you merged in the past... Are you aware of this?

5abe8cb#diff-4857d30b496749a712eae2d5bc42b2b5f8473e34daed6fffb46515519ca30ff4

@Harvie
Copy link
Collaborator

Harvie commented Feb 17, 2021

Also what do you think about changes proposed in #1525 ?

@aib
Copy link
Contributor

aib commented Feb 17, 2021

This makes the 8th and 9th capture groups of the regex non-capture, i.e.:

[PRB:-356.0,-5.28,-118.3,4.0,5.0,6.0:3]

The :3 and 3 (overlapping) parts just before the ] in the example above, provided in the excellent tests.

As there is no code currently using groups beyond the 4th one (apart from the tests here), this should not affect working code.

@Harvie
Copy link
Collaborator

Harvie commented Feb 17, 2021

As there is no code currently using groups beyond the 4th one (apart from the tests here), this should not affect working code.

For me it is important for code to match the following requirements:

  • Be robust enough not to fail when some new grbl fork adds more data to the list (just read what we understand and ignore the rest)
  • Allow for multiaxis setups, no matter if 3,4,5,6 or more axes. There is already basic 6-axis support in bCNC (have to be enabled in config to show the GUI).

So right now i am not sure which one of these two PRs will yield better results.

@aib
Copy link
Contributor

aib commented Feb 17, 2021

This PR will not change any code. It's just tests for POSPAT.

The other one allows 3-axis controllers to be used.
The other one is a fix for 5abe8cb which did not work as intended.

They can be combined, though it might have to be done manually.

@Harvie
Copy link
Collaborator

Harvie commented Feb 17, 2021

This PR will not change any code. It's just tests for POSPAT.

What do you mean? Have you clicked on the "changed files" ?

image

@Harvie
Copy link
Collaborator

Harvie commented Feb 17, 2021

The other one allows 3-axis controllers to be used.

I still don't fully understand the problem... 3-axis GRBL seems to work with current bCNC code...

They can be combined, though it might have to be done manually.

Are there any benefits from doing so except for the tests?

@aib
Copy link
Contributor

aib commented Feb 17, 2021

This PR will not change any code. It's just tests for POSPAT.

What do you mean? Have you clicked on the "changed files" ?

I mean it will not change how the program behaves. It's a test, code intended for developers, not users. (And if you mean POSPAT I've explained the change and why it shouldn't matter above)

I still don't fully understand the problem... 3-axis GRBL seems to work with current bCNC code...

GRBLv0? Not mine. Can you (or someone else) post an example status or position line? Is it reporting 6 axes perchance?

They can be combined, though it might have to be done manually.

Are there any benefits from doing so except for the tests?

Seeing as how the whole point of this one is tests, no :)

@Harvie
Copy link
Collaborator

Harvie commented Feb 17, 2021

GRBLv0? Not mine.

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...

That just brings another question... Are you sure your modifications targeted to v0 will not break compatibility with v1 protocol?

@Harvie
Copy link
Collaborator

Harvie commented Sep 25, 2023

Maybe we should strictly separate patterns for individual GRBL versions, so we don't have to worry about this.

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.

3 participants