MarlinSerial emergency-command parser (with M108) #4226

thinkyhead · 2016-07-06T18:18:54Z

Add a emergency-command parser in MarlinSerial located in MarlinSerial's RX interrupt.

The parser tries to find and execute M112,M108,M410 before the commands disappear in the RX-buffer.

~~To avoid false positives M117, comments and commands followed by filenames (M23, M28, M30, M32, M33) are recognized.~~

This enables Marlin to receive and respond to the Emergency command at all times - regardless of whether command buffers are full or not. It remains to convince host software to send the commands. To inform hosts about the new feature a new entry in the M115-report was made. "EMERGENCY_CODES:M112,M108,M410;".

The parser is fast. It only needs two switch decisions and one assignment of the new state for every character.

One problem remains. If the host has sent an incomplete line before sending an emergency command the emergency command could be omitted when the parser is in state_IGNORE. In that case the host should send "\ncommand\n"

Also introducing M108 to break the waiting for the heaters in M109, M190 and M303.

Rename cancel_heatup to wait_for_heatup to better see the purpose.

thinkyhead · 2016-07-06T19:29:49Z

@AnHardt Looking at the emergency parser, it seems like in some cases, it might be able to go into state_IGNORE instead of state_RESET and save on some processing.

I see that it doesn't account for the N character ahead of the command, but it could do this and then be a little smarter about how it proceeds from there.

For example, when in state_RESET if it encounters a space, it should stay in state_RESET, but if it encounters anything other than a space, N, or M, it should just go to state_IGNORE right away. This avoids simply staying in state_RESET when it doesn't see N, M, or ;. This should ensure that only the initial command is parsed but nothing after it.

Alternate Code Sample

if (c == '\n') state = state_IGNORE; // IGNORE -> RESET

switch (state) {
  case state_RESET:
    switch (c) {
      case ' ': break;
      case 'N': state = state_N;      break;
      case 'M': state = state_M;      break;
      default: state = state_IGNORE;
    }
    break;

  case state_N:
    if (c == ' ')
      state = state_RESET;
    else if (!NUMERIC_SIGNED(c))
      state = state_IGNORE;
    break;

  case state_M:
    switch (c) {
      case '1': state = state_M1;     break;
      case '4': state = state_M4;     break;
      default: state = state_IGNORE;
    }
    break;

  case state_M1:
    switch (c) {
      case '0': state = state_M10;    break;
      case '1': state = state_M11;    break;
      default: state = state_IGNORE;
    }
    break;

  case state_M10:
    if (c == '8') wait_for_heatup = false; // M108
    state = state_IGNORE;
    break;

  case state_M11:
    if (c == '2') kill(PSTR(MSG_KILLED));  // M112
    state = state_IGNORE;
    break;

  case state_M4:
    state = (c == '1') ? state_M41 : state_IGNORE;
    break;

  case state_M41:
    if (c == '0') quickstop_stepper();     // M410
    state = state_IGNORE;
    break;

  case state_IGNORE:
    if (c == '\n') state = state_RESET;
    break;

  default:
    state = state_RESET;
}

AnHardt · 2016-07-06T21:28:16Z

Marlin/MarlinSerial.cpp

+
+      case state_N:
+        if (!NUMERIC_SIGNED(c)) state = state_RESET;
+        break;


I can't see what the state_N is for.
Just ignore everything else than a 'M' in state_RESET and stay in state_RESET.

The main thing I want to ensure is that we are at the start of the command and not anyplace else. If (for some reason) we get a command like "G12 M108" we want to ignore the "M108" in this case. So in state_RESET (at the beginning of a new line) if we encounter "N" that's okay. We can skip past it and then go back to state_RESET. If there's a space just skip. If we see "M", then we go to state_M. With the added screening for "N", every other character can jump to state_IGNORE and stay there for the rest of the line.

Hosts will most likely not send these commands with N line numbers, but it's something to watch out for, to ensure we know we're still at the start of the command.

One thing that will fail here is something like "N123M108" without spaces. Unless…

if (!NUMERIC_SIGNED(c)) state = (c == 'M') ? state_M : state_RESET;

Crap. Now i see.
No i don't. All dangerous commands, followed by text, begin with 'M'

thinkyhead · 2016-07-06T22:28:29Z

Another tweak could just wait until a CR before taking any action:

switch (state) {
  case state_RESET:
    switch (c) {
      case ' ': break;
      case 'N': state = state_N;      break;
      case 'M': state = state_M;      break;
      default: state = state_IGNORE;
    }
    break;

  case state_N:
    switch (c) {
      case '0': case '1': case '2':
      case '3': case '4': case '5':
      case '6': case '7': case '8':
      case '9': case '-': case ' ':   break;
      case 'M': state = state_M;      break;
      default:  state = state_IGNORE;
    }
    break;

  case state_M:
    switch (c) {
      case ' ': break;
      case '1': state = state_M1;     break;
      case '4': state = state_M4;     break;
      default: state = state_IGNORE;
    }
    break;

  case state_M1:
    switch (c) {
      case '0': state = state_M10;    break;
      case '1': state = state_M11;    break;
      default: state = state_IGNORE;
    }
    break;

  case state_M4:
    state = (c == '1') ? state_M41 : state_IGNORE;
    break;

  case state_M10:
    state = (c == '8') ? state_M108 : state_IGNORE;
    break;

  case state_M11:
    state = (c == '2') ? state_M112 : state_IGNORE;
    break;

  case state_M41:
    state = (c == '0') ? state_M410 : state_IGNORE;
    break;

  case state_IGNORE:
    if (c == '\n') state = state_RESET;
    break;

  default:
    if (c == '\n') {
      switch (state) {
        case state_M108:
          wait_for_heatup = false;
          break;
        case state_M112:
          kill(PSTR(MSG_KILLED));
          break;
        case state_M410:
          quickstop_stepper();
          break;
      }
      state = state_RESET;
    }
}

AnHardt · 2016-07-06T22:29:50Z

Marlin/MarlinSerial.cpp

+          case 'M': state = state_M;      break;
+          default: state = state_IGNORE;
+        }
+        break;


How about:

case state_RESET: switch (c) { case 'M': state = state_M; break; case 'G': case 'T': state = state_IGNORE; break; default: state = state_RESET; } break;

to omit state_N

EDITED 2 times

Why only ignore G and T? What about a command (as artificial as this may be) like "Hey there M108!". Although it doesn't start with G or T it would still proceed to state_M108. We know N and are ok beforeM. While everything else can go tostate_IGNORE. This should result in our being instate_IGNORE a lot more.

I'm attempted to say: "Who sends crap - gets crap."

AnHardt · 2016-07-06T23:38:11Z

Marlin/MarlinSerial.cpp

+      case state_N:
+        if (!NUMERIC_SIGNED(c) && c != ' ')
+          state = (c == 'M') ? state_M : state_RESET;
+        break;


I'm quite unlucky about state_N. Now we have 6 decisions and a assignment. In my original version there have been always 2 decisions and one assignment. The beauty got lost.

case state_N: switch (c) { case ' ': case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9': case '-': break; case 'M': state = state_M; break; default: state_RESET; }

?

A switch-case will actually generate several "decisions." A non-numeric comparison like if (c > '9' || c < '0') will fail (most of the time) on the first comparison, resulting in only a single test. But let me compare the ASM output…

Sure enough, the switch() approach does generate smarter test ordering than the current logic.

case state_N: switch (c) { case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9': case '-': case ' ': break; case 'M': state = state_M; break; default: state = state_IGNORE; } break;

case state_N: switch (c) { 1ad8: 80 33 cpi r24, 0x30 ; 48 1ada: 08 f0 brcs .+2 ; 0x1ade <_Z16emergency_parserh+0x4c> 1adc: 50 c0 rjmp .+160 ; 0x1b7e <_Z16emergency_parserh+0xec> 1ade: 80 32 cpi r24, 0x20 ; 32 1ae0: 09 f4 brne .+2 ; 0x1ae4 <_Z16emergency_parserh+0x52> 1ae2: 4d c0 rjmp .+154 ; 0x1b7e <_Z16emergency_parserh+0xec> 1ae4: 8d 32 cpi r24, 0x2D ; 45 1ae6: 61 f5 brne .+88 ; 0x1b40 <_Z16emergency_parserh+0xae> 1ae8: 08 95 ret default: state = state_IGNORE; } break;

thinkyhead · 2016-07-07T23:13:16Z

@AnHardt Should we disable EMERGENCY_PARSER by default, or do you think this is pretty low-impact?

AnHardt · 2016-07-07T23:24:09Z

I'd suggest to disable for now, to give us some more time to make some more tests with the freshly changed code. If we are golden i have no doubts it can be activated by default.

Add an emergency-command parser to MarlinSerial's RX interrupt. The parser tries to find and execute M108,M112,M410 before the commands disappear in the RX-buffer. To avoid false positives for M117, comments and commands followed by filenames (M23, M28, M30, M32, M33) are filtered. This enables Marlin to receive and react on the Emergency command at all times - regardless of whether the buffers are full or not. It remains to convince hosts to send the commands. To inform the hosts about the new feature a new entry in the M115-report was made. "`EMERGENCY_CODES:M112,M108,M410;`". The parser is fast. It only ever needs two switch decisions and one assignment of the new state for every character. One problem remains. If the host has sent an incomplete line before sending an emergency command the emergency command could be omitted when the parser is in `state_IGNORE`. In that case the host should send "\ncommand\n" Also introduces M108 to break the waiting for the heaters in M109, M190 and M303. Rename `cancel_heatup` to `wait_for_heatup` to better see the purpose.

…top()

AnHardt · 2016-07-07T23:43:55Z

The time impact of the parser is as low as possible to do its job.
But compared to the original runtime of the rx-interrupt considerable.

On the other side, compared to the time we waste while busy waiting for sending, this is nothing.
My TX-buffer patch is ready since a while but has some merge conflicts now. Interested?

thinkyhead · 2016-07-07T23:50:57Z

time we waste while busy waiting for sending
My TX-buffer patch is ready … Interested?

Sure, as long as it remains truly compatible with the hardware, it would be good!

Add E3D Revo variant

thinkyhead added PR: Improvement C: Hosts & Protocols labels Jul 6, 2016

thinkyhead force-pushed the rc_emergency_command_parser branch 3 times, most recently from 5ccb52e to 929e36a Compare July 6, 2016 19:00

thinkyhead force-pushed the rc_emergency_command_parser branch 5 times, most recently from a81efce to 7c6433c Compare July 6, 2016 20:56

AnHardt reviewed Jul 6, 2016
View reviewed changes

thinkyhead force-pushed the rc_emergency_command_parser branch from 7c6433c to 7d0e93a Compare July 6, 2016 22:09

AnHardt reviewed Jul 6, 2016
View reviewed changes

thinkyhead force-pushed the rc_emergency_command_parser branch from 7d0e93a to 5a98a34 Compare July 6, 2016 23:04

AnHardt reviewed Jul 6, 2016
View reviewed changes

thinkyhead force-pushed the rc_emergency_command_parser branch from 5a98a34 to a0e37b5 Compare July 7, 2016 00:41

thinkyhead force-pushed the rc_emergency_command_parser branch from cae2ba7 to dd05919 Compare July 7, 2016 23:28

AnHardt and others added 6 commits July 7, 2016 16:37

Group EMERGENCY_PARSER commands

ecffe92

Add quickstop_stepper to update current position with stepper.quick_s…

834ad14

…top()

Simplify emergency parser (only parse initial command)

ea47803

Wait for end of line before invoking action

bd5a825

Make wait_for_heatup volatile

2ee4e4f

thinkyhead force-pushed the rc_emergency_command_parser branch from dd05919 to 2ee4e4f Compare July 7, 2016 23:37

thinkyhead merged commit 98d0167 into MarlinFirmware:RCBugFix Jul 7, 2016

thinkyhead deleted the rc_emergency_command_parser branch July 7, 2016 23:53

thinkyhead mentioned this pull request Jul 7, 2016

M190 and M109 may lead to zombie state #4198

Closed

jbrazio modified the milestone: 1.1.0 Jul 18, 2016

scalez mentioned this pull request Jul 18, 2016

Introduce the reprobe and rewipe features #3553

Closed

thinkyhead mentioned this pull request Jul 25, 2016

Marlin 1.1.0-RC7 #4237

Closed

drewmoseley pushed a commit to drewmoseley/Marlin that referenced this pull request Nov 8, 2023

Merge pull request MarlinFirmware#4226 from 3d-gussner/MK3_REVO

9ac5436

Add E3D Revo variant

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MarlinSerial emergency-command parser (with M108) #4226

MarlinSerial emergency-command parser (with M108) #4226

thinkyhead commented Jul 6, 2016 •

edited

Loading

thinkyhead commented Jul 6, 2016 •

edited

Loading

AnHardt Jul 6, 2016

thinkyhead Jul 6, 2016 •

edited

Loading

AnHardt Jul 6, 2016 •

edited

Loading

thinkyhead commented Jul 6, 2016 •

edited

Loading

AnHardt Jul 6, 2016 •

edited

Loading

thinkyhead Jul 6, 2016 •

edited

Loading

Blue-Marlin Jul 6, 2016

thinkyhead Jul 7, 2016 •

edited

Loading

AnHardt Jul 6, 2016 •

edited

Loading

AnHardt Jul 6, 2016

thinkyhead Jul 7, 2016

thinkyhead Jul 7, 2016

thinkyhead commented Jul 7, 2016

AnHardt commented Jul 7, 2016

AnHardt commented Jul 7, 2016 •

edited by thinkyhead

Loading

thinkyhead commented Jul 7, 2016

MarlinSerial emergency-command parser (with M108) #4226

MarlinSerial emergency-command parser (with M108) #4226

Conversation

thinkyhead commented Jul 6, 2016 • edited Loading

thinkyhead commented Jul 6, 2016 • edited Loading

AnHardt Jul 6, 2016

Choose a reason for hiding this comment

thinkyhead Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

AnHardt Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

thinkyhead commented Jul 6, 2016 • edited Loading

AnHardt Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

thinkyhead Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

Blue-Marlin Jul 6, 2016

Choose a reason for hiding this comment

thinkyhead Jul 7, 2016 • edited Loading

Choose a reason for hiding this comment

AnHardt Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

AnHardt Jul 6, 2016

Choose a reason for hiding this comment

thinkyhead Jul 7, 2016

Choose a reason for hiding this comment

thinkyhead Jul 7, 2016

Choose a reason for hiding this comment

thinkyhead commented Jul 7, 2016

AnHardt commented Jul 7, 2016

AnHardt commented Jul 7, 2016 • edited by thinkyhead Loading

thinkyhead commented Jul 7, 2016

thinkyhead commented Jul 6, 2016 •

edited

Loading

thinkyhead commented Jul 6, 2016 •

edited

Loading

thinkyhead Jul 6, 2016 •

edited

Loading

AnHardt Jul 6, 2016 •

edited

Loading

thinkyhead commented Jul 6, 2016 •

edited

Loading

AnHardt Jul 6, 2016 •

edited

Loading

thinkyhead Jul 6, 2016 •

edited

Loading

thinkyhead Jul 7, 2016 •

edited

Loading

AnHardt Jul 6, 2016 •

edited

Loading

AnHardt commented Jul 7, 2016 •

edited by thinkyhead

Loading