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(gcode): stricter gcode line number matching #4034

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

barthy-koeln
Copy link

@barthy-koeln barthy-koeln commented Apr 6, 2024

Changes

The current G-CODE syntax is too loose on the "line number" instruction matching.

Example:

N1 M6
N2 SAVE_CONFIG

Rendered as:

<span class="hljs-symbol">N1</span> <span class="hljs-name">M6</span>
<span class="hljs-symbol">N1</span> SAVE_CO<span class="hljs-symbol">NFIG</span>

NFIG is not a line number instruction or symbol. My proposed fix is to strictly match the letter N, followed by one or more digits.


This does not resolve all issues, such as matching N\d+ that is NOT at the start of the line:

SKEW_PROFILE SAVE=GuteN8

Rendered as

SKEW_PROFILE SAVE=Gute<span class="hljs-symbol">N8</span>

I haven't found a straightforward and reliable specification or source listing exactly which optional characters are allowed before line numbers. Unfortunately, I currently don't have the time to dive deeper into ISO specs.

But I think that this a good minimal change that resolves some issues and hopefully doesn't cause others.

Checklist

  • Added markup tests
  • Updated the changelog at CHANGES.md

illegal: '\\W'
}
]
begin: 'N\\d+'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
begin: 'N\\d+'
match: /N\d+/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I change the other 10 regexes with single quotes in that file, too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I found heaps more issues with samples of gcode I could find online. I will submit a PR with substantial changes. Until this, this suggestion was implemented and this PR can stay as small as it is.

@barthy-koeln
Copy link
Author

I suggest closing this in favor of #4040

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.

None yet

2 participants