search

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
321 stars 101 forks source link

Grbl: Cycle Start/Resume command generates buffer overflow #44

Closed setaperlacloche closed 6 years ago

setaperlacloche commented 6 years ago

To sum up : Each a time a Cycle Start/Resume command ('~') is processed by SPJS, a new G-Code line is transmitted to Grbl controller even if buffer free space don't allow it.

How to reproduce : Context : http://chilipeppr.com/jpadie, SPJS v.1.95, Grbl 1.1f Put in JPadie workspace the following G-Code :

N1G21
N2G1X30Y0Z0F500
N3G1X00Y0Z0F1000
N4G1X01Y0Z0F1000
N5G1X00Y0Z0F1000
N6G1X01Y0Z0F1000
N7G1X00Y0Z0F1000
N8G1X01Y0Z0F1000
N9G1X00Y0Z0F1000
N10G1X01Y0Z0F1000
N11G1X00Y0Z0F1000
N12G1X01Y0Z0F1000
N13G1X00Y0Z0F1000
N14G1X01Y0Z0F1000
N15G1X00Y0Z0F1000
N16G1X01Y0Z0F1000
N17G1X00Y0Z0F1000
N18G1X01Y0Z0F1000
N19G1X00Y0Z0F1000
N20G1X01Y0Z0F1000
N21G1X00Y0Z0F1000
N22G1X01Y0Z0F1000
N23G1X00Y0Z0000000000000000000000000000000000000000000F1000
N24G1X01Y0Z0F1000
N25G1X00Y0Z0F1000
N26G1X01Y0Z0F1000
N27G1X00Y0Z0F1000
N28G1X01Y0Z0F1000
N29G1X00Y0Z0F1000

Run it. Everythink should be ok. (about 25s to run) Run it again, but this time, press Hold button in the first seconds when the milling head is moving right (executing line N2). The controller enters in Hold state and SPJS Queue label indicates 7. Press Resume button, immediately SPJS Queue label decreases to 6. Wait the job to complete. Line N24 should be in error. The issue here is : Line N23 is transmitted to controller immediately after Resume action but no line has been popped during the long execution of N2 ==> N24 raises an error:25 (i.e. A G-code word was repeated in the block) due to buffer overflow.

Why : The answer is in SPJS code. When processed by SPJS, the realtime command '~' is detected as an 'UnpauseBuffer' command (Function SeeIfSpecificCommandsShouldUnpauseBuffer bufferflow_grbl.go:316). The caller of this function is located in serial.go:399. If 'UnpauseBuffer' command is validated, Unpause (bufferflow_grbl.go:287) function is called, which calls SetPaused (bufferflow_grbl.go:418) which release a semaphore named sem. This action unlocks function BlockUntilReady (bufferflow_grbl.go:67) which return true, true, "". The caller writerBuffered (serialport.go:217) interprets the return as a goodToGo. So a new command is pushed in sendNoBuf chan.

Correction : To correct this issue we need to answer the question : 'When transmission to Grbl controller should be paused ?' The answer is quite simple :

And that's all. So transmission is only driven by flow control. Nothing in specification denies transmission when the controller is in HOLD state. HOLD state only impacts (freeze) mouvement execution. For example the command !G0X10\n~ is perfectly legal, and is played as expected (HOLD state, registers a move to X10, RUN state, moves to X10).

That being said, the patch is easy, no realtime command is able to pause transmission. So I unconditionally return false in functions SeeIfSpecificCommandsShouldPauseBuffer (bufferflow_grbl.go:305) and SeeIfSpecificCommandsShouldUnpauseBuffer (bufferflow_grbl.go:316). I tested with success the modification using my Grbl controller.

What do you think ?

Note : A quick review of TinyG* buffer code makes me think that the same issue is present, but I can't check it.

chilipeppr commented 6 years ago

Well, it sounds reasonable what you did. I think that could be worthy of a pull request. If you've tested it thoroughly and feel good on the change, I'd be up for integrating it.

On Thu, Feb 8, 2018 at 12:55 PM, setaperlacloche notifications@github.com wrote:

To sum up : Each a time a Cycle Start/Resume command ('~') is processed by SPJS, a new G-Code line is transmitted to Grbl controller even if buffer free space don't allow it.

How to reproduce : Context : http://chilipeppr.com/jpadie, SPJS v.1.95, Grbl 1.1f Put in JPadie workspace the following G-Code :

N1G21 N2G1X30Y0Z0F500 N3G1X00Y0Z0F1000 N4G1X01Y0Z0F1000 N5G1X00Y0Z0F1000 N6G1X01Y0Z0F1000 N7G1X00Y0Z0F1000 N8G1X01Y0Z0F1000 N9G1X00Y0Z0F1000 N10G1X01Y0Z0F1000 N11G1X00Y0Z0F1000 N12G1X01Y0Z0F1000 N13G1X00Y0Z0F1000 N14G1X01Y0Z0F1000 N15G1X00Y0Z0F1000 N16G1X01Y0Z0F1000 N17G1X00Y0Z0F1000 N18G1X01Y0Z0F1000 N19G1X00Y0Z0F1000 N20G1X01Y0Z0F1000 N21G1X00Y0Z0F1000 N22G1X01Y0Z0F1000 N23G1X00Y0Z0000000000000000000000000000000000000000000F1000 N24G1X01Y0Z0F1000 N25G1X00Y0Z0F1000 N26G1X01Y0Z0F1000 N27G1X00Y0Z0F1000 N28G1X01Y0Z0F1000 N29G1X00Y0Z0F1000

Run it. Everythink should be ok. (about 25s to run) Run it again, but this time, press Hold button in the first seconds when the milling head is moving right (executing line N2). The controller enters in Hold state and SPJS Queue label indicates 7. Press Resume button, immediately SPJS Queue label decreases to 6. Wait the job to complete. Line N24 should be in error. The issue here is : Line N23 is transmitted to controller immediately after Resume action but no line has been popped during the long execution of N2 ==> N24 raises an error:25 (i.e. A G-code word was repeated in the block) due to buffer overflow.

Why : The answer is in SPJS code. When processed by SPJS, the realtime command '~' is detected as an 'UnpauseBuffer' command (Function SeeIfSpecificCommandsShouldUnpauseBuffer bufferflow_grbl.go:316). The caller of this function is located in serial.go:399. If 'UnpauseBuffer' command is validated, Unpause (bufferflow_grbl.go:287) function is called, which calls SetPaused (bufferflow_grbl.go:418) which release a semaphore named sem. This action unlocks function BlockUntilReady (bufferflow_grbl.go:67) which return true, true, "". The caller writerBuffered (serialport.go:217) interprets the return as a goodToGo. So a new command is pushed in sendNoBuf chan.

Correction : To correct this issue we need to answer the question : 'When transmission to Grbl controller should be paused ?' The answer is quite simple :

  • When free space on reception buffer is not large enough to hold next G-Code line.

And that's all. So transmission is only driven by flow control. Nothing in specification denies transmission when the controller is in HOLD state. HOLD state only impacts (freeze) mouvement execution. For example the command !G0X10\n~ is perfectly legal, and is played as expected (HOLD state, registers a move to X10, RUN state, moves to X10).

That being said, the patch is easy, no realtime command is able to pause transmission. So I unconditionally return false in functions SeeIfSpecificCommandsShouldPauseBuffer (bufferflow_grbl.go:305) and SeeIfSpecificCommandsShouldUnpauseBuffer (bufferflow_grbl.go:316). I tested with success the modification using my Grbl controller.

What do you think ?

Note : A quick review of TinyG* buffer code makes me think that the same issue is present, but I can't check it.

— 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/44, or mute the thread https://github.com/notifications/unsubscribe-auth/AHidbVjRbQxpzpdljajWgyDLoRUGuCBXks5tS19OgaJpZM4R_Ate .

setaperlacloche commented 6 years ago

Thanks! I will do it this week-end.

I would like to insist on the fact that others buffer flows (TinyG*, marlin) use the same logic. (ie '~' unlocks sending, codes are very very similar with the Grbl one).

Here a file Issue44.txt suitable to test this issue on all platforms :

Note : If SPJS queue length is not directly accessible in ChiliPeppr workspace, a connection to http://ip-address-of-SPJS:8989 permits to catch the information. Example of capture :

{"Cmd":"Queued","QCnt":566,"Ids":[""],"D":["~\n"],"Port":"/dev/ttyUSB2"}
{"Cmd":"Write","QCnt":565,"Id":"","P":"/dev/ttyUSB2"}
{"Cmd":"CompleteFake","Id":"","P":"/dev/ttyUSB2"}
{"Cmd":"Write","QCnt":564,"Id":"g25","P":"/dev/ttyUSB2"}

Here SPJS queue length decrease from 565 to 564 (Id g25 is sent immediately after transmission of '~' done on second line).

People of goodwill having the associated hardware will are welcome !

chilipeppr commented 6 years ago

I see your point in other buffers having same logic problem, but I suppose this is a bug that hasn't been seen out there on TinyG because it processes its serial port input fast and even if the device is in a hold state it keeps processing. Most holds are done manually, so there's plenty of time to process any serial port chars. So sending a line after an unpause probably always has headroom.

On Fri, Feb 9, 2018 at 2:12 AM, setaperlacloche notifications@github.com wrote:

Thanks! I will do it this week-end.

I would like to insist on the fact that others buffer flows (TinyG*, marlin) use the same logic. (ie '~' unlocks sending, codes are very very similar with the Grbl one).

Here a file Issue44.txt https://github.com/chilipeppr/serial-port-json-server/files/1710316/Issue44.txt suitable to test this issue on all platforms :

  • Load it in ChiliPeppr workspace.
  • Run it.
  • Press Hold button while executing first milling operation (1 minute long). Check SPJS queue length.
  • Press Resume Button, If SPJS queue length decrease of 1 unit this issue is present.

Note : If SPJS queue length is not directly accessible in ChiliPeppr workspace, a connection to http://ip-address-of-SPJS:8989 permits to catch the information. Example of capture :

{"Cmd":"Queued","QCnt":566,"Ids":[""],"D":["~\n"],"Port":"/dev/ttyUSB2"} {"Cmd":"Write","QCnt":565,"Id":"","P":"/dev/ttyUSB2"} {"Cmd":"CompleteFake","Id":"","P":"/dev/ttyUSB2"} {"Cmd":"Write","QCnt":564,"Id":"g25","P":"/dev/ttyUSB2"}

Here SPJS queue length decrease from 565 to 564 (Id g25 is sent immediately after transmission of '~' done on second line).

People of goodwill having the associated hardware will are welcome !

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