chilipeppr / serial-port-json-server

Serial Port JSON Server is a websocket server for your serial devices. It compiles to a binary for Windows, Mac, Linux, Raspberry Pi, or BeagleBone Black that lets you communicate with your serial port from a web application. This enables web apps to be written that can communicate with your local serial device such as an Arduino, CNC controller, or any device that communicates over the serial port.
http://chilipeppr.com
GNU General Public License v2.0
322 stars 101 forks source link

Grbl : '\n' added to realtime commands generates buffer overflow #42

Closed setaperlacloche closed 6 years ago

setaperlacloche commented 6 years ago

(Context SPJS 1.95, Grbl 1.1f on Arduino Nano)

When I run a GCode file and do a lot of hold/resume commands, I get errors (mainly 25 et 33).

In order to guess the reason, I put a logical probe on RX/TX on my Arduino. And discovered that '!' (Feed Hold command) and '~' (Cycle Start / Resume) are followed by '\n'. In fact all realtime commands (including jog, fro, Spindle Speed Overrides,..) except '?' are followed by a '\n'. This is a violation of the protocole about realtime commands (from the documentation) :

When a "!/n" is sent, SPJS considers this command skip the internal serial port buffering (serial.go:412), but on Grbl side, char '!' skip the buffer but not the '\n' ==> At this point bytes counting in buffer are unsynchronize between SPJS and Grbl. Grbl hold one more character than SPJS thinking.

The extra '\n' is added in bufferflow_grbl.go:269 So I replaced the line that detects '?' as the only realtime command (bufferflow_grbl.go:260) from :

    } else if item == "?" {

to : } else if b.SeeIfSpecificCommandsShouldSkipBuffer(item) { With this modification, it seems that buffer overflow is away.

What do you think ?

chilipeppr commented 6 years ago

I think your code change looks reasonable and if you've tested it on a couple of jobs then you're probably good to go and a pull request would be welcomed.

If you look at the TinyG buffer, I think I remove the \n in there on a feed hold, resume, or wipe. The thing about \n is that SPJS is very much a line-centric approach, like if you send in a bunch of text with \n's in the middle, it splits and turns those into lines that are sent in that order to the serial device. Generally CNC firmware is line-centric, but then you have the non-line centric stuff like you point out such as !, ~, and %. So, it does make sense to drop the \n in those cases.

On Mon, Feb 5, 2018 at 10:15 AM, setaperlacloche notifications@github.com wrote:

(Context SPJS 1.95, Grbl 1.1f on Arduino Nano)

When I run a GCode file and do a lot of hold/resume commands, I get errors (mainly 25 et 33).

In order to guess the reason, I put a logical probe on RX/TX on my Arduino. And discovered that '!' (Feed Hold command) and '~' (Cycle Start / Resume) are followed by '\n'. In fact all realtime commands (including jog, fro, Spindle Speed Overrides,..) except '?' are followed by a '\n'. This is a violation of the protocole about realtime commands (from the documentation) :

  • Is a single character that may be sent to Grbl at any time.
  • Does not require a line feed or carriage return after them.
  • Is not considered a part of the streaming protocol.

When a "!/n" is sent, SPJS considers this command skip the internal serial port buffering (serial.go:412), but on Grbl side, char '!' skip the buffer but not the '\n' ==> At this point bytes counting in buffer are unsynchronize between SPJS and Grbl. Grbl hold one more character than SPJS thinking.

The extra '\n' is added in bufferflow_grbl.go:269 So I replaced the line that detects '?' as the only realtime command (bufferflow_grbl.go:260) from :

} else if item == "?" {

to :

} else if b.SeeIfSpecificCommandsShouldSkipBuffer(item) {

With this modification, it seems that buffer overflow is away.

What do you think ?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/chilipeppr/serial-port-json-server/issues/42, or mute the thread https://github.com/notifications/unsubscribe-auth/AHidbYzSpTVdDpTaIYOQ3I6hdg4EAvW3ks5tR0UwgaJpZM4R536r .

setaperlacloche commented 6 years ago

About testing : At this time I don't have a CNC (I'm working hard to get one ;-)). Sadly, I can only test Grbl protocole compliance. Ok to pull a request of someone can validate in true condition.