8000 fix: resolve an issue where Grbl controller cannot recognize certain startup message by cheton · Pull Request #573 · cncjs/cncjs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: resolve an issue where Grbl controller cannot recognize certain startup message #573

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Mar 6, 2020
Merged

fix: resolve an issue where Grbl controller cannot recognize certain startup message #573

merged 11 commits into from
Mar 6, 2020

Conversation

cheton
Copy link
Collaborator
@cheton cheton commented Mar 4, 2020 •
edited
Loading

This issue was reported on FB where some users may experience startup problems when using a custom Grbl firmware with certain startup message (e.g. Grbl 1.1h: LongMill build ['$' for help]):

image

I changed the strict regular expression from

/^([a-zA-Z0-9]+)\s+((?:\d+\.){1,2}\d+[a-zA-Z0-9\-\.]*)\s+(\[[^\]]+\])/

to

/^([a-zA-Z0-9]+)\s+((?:\d+\.){1,2}\d+[a-zA-Z0-9\-\.]*)([^\[]*\[[^\]]+\].*)?/

so it can math the string Grbl 1.1h: LongMill build ['$' for help]

@cheton cheton requested a review from MitchBradley March 4, 2020 03:53
@coveralls
Copy link
coveralls commented Mar 4, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 82.502% when pulling 96f1302 on fix/grbl-startup-message into 7c0caf2 on master.

@cheton cheton self-assigned this Mar 4, 2020
MitchBradley
MitchBradley reviewed Mar 4, 2020
View reviewed changes
Copy link
Contributor
@MitchBradley MitchBradley left a comment

Choose a reason for hiding this comment

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

I reached out to the LongMill team about this issue. One of their customers already filed a bug report, and as a result they changed the startup string so it now reads like

Grbl 1.1f ['$' for help] (Longmill Build Oct 21, 2019)

That does not confuse the CNCjs parser.

For reference, here is the report from the LongMill forum. According to Andy Lee hi@sienci.com, that suggestion has been folded into the most recent firm. So maybe CNCjs does not need to change.

@falviani
Copy link
falviani commented Mar 4, 2020 via email

@MitchBradley
Copy link
Contributor

These instructions tell how to deploy new LongMill firmware: https://sienci.com/dmx-longmill/grbl-firmware/

@cheton
Copy link
Collaborator Author
cheton commented Mar 5, 2020 •
edited
Loading

I made a slight change to the code (a7d9bab) to increase the flexibility:

/^([a-zA-Z0-9]+)\s+((?:\d+\.){1,2}\d+[a-zA-Z0-9\-\.]*)([^\[]*\[[^\]]+\].*)?/

It will get the exact firmware and version from the startup message, and the rest is optional.

MitchBradley
MitchBradley approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor
@MitchBradley MitchBradley left a comment

Choose a reason for hiding this comment

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

Looks good to me

@cheton cheton merged commit 3250c1b into master Mar 6, 2020
@cheton cheton deleted the fix/grbl-startup-message branch March 6, 2020 16:51
@cheton
Copy link
Collaborator Author
cheton commented Mar 7, 2020

Fixed in 1.9.21

@falviani
Copy link
falviani commented Mar 7, 2020 via email

cheton added a commit that referenced this pull request Mar 9, 2020
@cheton
fix: resolve an issue where Grbl controller cannot recognize certain …
2a67619 
…startup message (#573)

* fix: resolve an issue where Grbl controller cannot recognize certain startup message that is used for branding

* chore: downgrade to 1.9.21 to make a patch release

* fix(eslint): fix unexpected indentation

* chore: add more flexibility to the regular expression

* fix: export 'STLLoader' (imported as 'THREE') was not found in 'three'

* fix: incorrect spelling

* build(ci): drop support for Node.js 8 and earlier versions

* chore: bump electron 4.1.4 to 8.0.3, bump serialport 7.1.4 to 7.1.5

* docs: update README.md

* fix: rollback Electron to 4.2.12 with Node.js 10

* chore: install rpmbuild

Co-authored-by: Cheton Wu <chetonu@gmail.com>
@etx
Copy link
etx commented Mar 10, 2020

Fixed my issues on the Millright Mega V controller also. Thanks a million!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MitchBradley MitchBradley MitchBradley approved these changes

Assignees

@cheton cheton

Labels
⭐ feature (enhancement)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cheton @coveralls @falviani @MitchBradley @etx
0