From 3a4c3ab76e131ed91eec532e842243af8f032e58 Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Sat, 16 May 2015 23:14:02 -0700 Subject: [PATCH 1/5] Pre-sanitize the command before handling - Use a global pointer for the current sanitized command - Pre-sanitize the current command to bypass `N` and nullify `*`, removing the need for handlers to bypass, ignore, or nullify these parts, and reducing overhead for `code_seen`, etc. - Pre-skip leading whitespace. - Only look for G, M, T codes at the start of the command. - Verify that G, M, T codes are followed by command codes. --- Marlin/Marlin_main.cpp | 90 +++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 49 deletions(-) diff --git a/Marlin/Marlin_main.cpp b/Marlin/Marlin_main.cpp index efc14ea42..52571fc8e 100644 --- a/Marlin/Marlin_main.cpp +++ b/Marlin/Marlin_main.cpp @@ -236,6 +236,7 @@ bool axis_known_position[3] = { false }; static long gcode_N, gcode_LastN, Stopped_gcode_LastN = 0; +static char *current_command; static int cmd_queue_index_r = 0; static int cmd_queue_index_w = 0; static int commands_in_queue = 0; @@ -940,7 +941,7 @@ long code_value_long() { return strtol(strchr_pointer + 1, NULL, 10); } int16_t code_value_short() { return (int16_t)strtol(strchr_pointer + 1, NULL, 10); } bool code_seen(char code) { - strchr_pointer = strchr(command_queue[cmd_queue_index_r], code); + strchr_pointer = strchr(current_command, code); return (strchr_pointer != NULL); //Return True if a character was found } @@ -2843,7 +2844,7 @@ inline void gcode_G92() { * M1: // M1 - Conditional stop - Wait for user button press on LCD */ inline void gcode_M0_M1() { - char *src = strchr_pointer + 2; + char *args = strchr_pointer + 3; millis_t codenum = 0; bool hasP = false, hasS = false; @@ -2855,11 +2856,9 @@ inline void gcode_G92() { codenum = code_value() * 1000; // seconds to wait hasS = codenum > 0; } - char* starpos = strchr(src, '*'); - if (starpos != NULL) *(starpos) = '\0'; - while (*src == ' ') ++src; - if (!hasP && !hasS && *src != '\0') - lcd_setstatus(src, true); + + if (!hasP && !hasS && *args != '\0') + lcd_setstatus(args, true); else { LCD_MESSAGEPGM(MSG_USERWAIT); #if defined(LCD_PROGRESS_BAR) && PROGRESS_MSG_EXPIRE > 0 @@ -2932,10 +2931,7 @@ inline void gcode_M17() { * M23: Select a file */ inline void gcode_M23() { - char* codepos = strchr_pointer + 4; - char* starpos = strchr(codepos, '*'); - if (starpos) *starpos = '\0'; - card.openFile(codepos, true); + card.openFile(strchr_pointer + 4, true); } /** @@ -2972,14 +2968,7 @@ inline void gcode_M17() { * M28: Start SD Write */ inline void gcode_M28() { - char* codepos = strchr_pointer + 4; - char* starpos = strchr(codepos, '*'); - if (starpos) { - char* npos = strchr(command_queue[cmd_queue_index_r], 'N'); - strchr_pointer = strchr(npos, ' ') + 1; - *(starpos) = '\0'; - } - card.openFile(codepos, false); + card.openFile(strchr_pointer + 4, false); } /** @@ -2996,12 +2985,6 @@ inline void gcode_M17() { inline void gcode_M30() { if (card.cardOK) { card.closefile(); - char* starpos = strchr(strchr_pointer + 4, '*'); - if (starpos) { - char* npos = strchr(command_queue[cmd_queue_index_r], 'N'); - strchr_pointer = strchr(npos, ' ') + 1; - *(starpos) = '\0'; - } card.removeFile(strchr_pointer + 4); } } @@ -3032,17 +3015,14 @@ inline void gcode_M31() { if (card.sdprinting) st_synchronize(); - char* codepos = strchr_pointer + 4; + char* args = strchr_pointer + 4; - char* namestartpos = strchr(codepos, '!'); //find ! to indicate filename string start. - if (! namestartpos) - namestartpos = codepos; //default name position, 4 letters after the M + char* namestartpos = strchr(args, '!'); // Find ! to indicate filename string start. + if (!namestartpos) + namestartpos = args; // Default name position, 4 letters after the M else namestartpos++; //to skip the '!' - char* starpos = strchr(codepos, '*'); - if (starpos) *(starpos) = '\0'; - bool call_procedure = code_seen('P') && (strchr_pointer < namestartpos); if (card.cardOK) { @@ -3061,12 +3041,6 @@ inline void gcode_M31() { * M928: Start SD Write */ inline void gcode_M928() { - char* starpos = strchr(strchr_pointer + 5, '*'); - if (starpos) { - char* npos = strchr(command_queue[cmd_queue_index_r], 'N'); - strchr_pointer = strchr(npos, ' ') + 1; - *(starpos) = '\0'; - } card.openLogFile(strchr_pointer + 5); } @@ -4163,7 +4137,7 @@ inline void gcode_M206() { default: SERIAL_ECHO_START; SERIAL_ECHOPGM(MSG_UNKNOWN_COMMAND); - SERIAL_ECHO(command_queue[cmd_queue_index_r]); + SERIAL_ECHO(current_command); SERIAL_ECHOLNPGM("\""); return; } @@ -5084,8 +5058,7 @@ inline void gcode_M999() { * * F[mm/min] Set the movement feedrate */ -inline void gcode_T() { - uint16_t tmp_extruder = code_value_short(); +inline void gcode_T(uint8_t tmp_extruder) { if (tmp_extruder >= EXTRUDERS) { SERIAL_ECHO_START; SERIAL_CHAR('T'); @@ -5188,17 +5161,36 @@ inline void gcode_T() { } /** - * Process Commands and dispatch them to handlers + * Process a single command and dispatch it to its handler * This is called from the main loop() */ void process_next_command() { + current_command = command_queue[cmd_queue_index_r]; if ((marlin_debug_flags & DEBUG_ECHO)) { SERIAL_ECHO_START; - SERIAL_ECHOLN(command_queue[cmd_queue_index_r]); + SERIAL_ECHOLN(current_command); } - if (code_seen('G')) { + // Sanitize the current command: + // - Skip leading spaces + // - Bypass N... + // - Overwrite * with nul to mark the end + while (*current_command == ' ') ++current_command; + if (*current_command == 'N' && current_command[1] >= '0' && current_command[1] <= '9') { + while (*current_command != ' ') ++current_command; + while (*current_command == ' ') ++current_command; + } + char *starpos = strchr(current_command, '*'); // * should always be the last parameter + if (starpos) *starpos = '\0'; + + // Get the command code as a character + char command_code = *current_command; + + // code_value routines look at strchr_pointer + 1 + strchr_pointer = current_command; + + if (command_code == 'G' && code_has_value()) { int codenum = code_value_short(); @@ -5274,8 +5266,8 @@ void process_next_command() { } } - else if (code_seen('M')) { - switch(code_value_short()) { + else if (command_code == 'M' && code_has_value()) { + switch (code_value_short()) { #ifdef ULTIPANEL case 0: // M0 - Unconditional stop - Wait for user button press on LCD case 1: // M1 - Conditional stop - Wait for user button press on LCD @@ -5704,14 +5696,14 @@ void process_next_command() { } } - else if (code_seen('T')) { - gcode_T(); + else if (command_code == 'T' && code_has_value()) { + gcode_T(code_value_short()); } else { SERIAL_ECHO_START; SERIAL_ECHOPGM(MSG_UNKNOWN_COMMAND); - SERIAL_ECHO(command_queue[cmd_queue_index_r]); + SERIAL_ECHO(current_command); SERIAL_ECHOLNPGM("\""); } From 1116e13f5a5c153a2b4e1995a9154434992695cd Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Sun, 17 May 2015 05:00:09 -0700 Subject: [PATCH 2/5] Further optimization of command parser --- Marlin/Marlin_main.cpp | 173 ++++++++++++++++++++--------------------- 1 file changed, 84 insertions(+), 89 deletions(-) diff --git a/Marlin/Marlin_main.cpp b/Marlin/Marlin_main.cpp index 52571fc8e..80e865f0e 100644 --- a/Marlin/Marlin_main.cpp +++ b/Marlin/Marlin_main.cpp @@ -141,7 +141,7 @@ * M112 - Emergency stop * M114 - Output current position to serial port * M115 - Capabilities string - * M117 - display message + * M117 - Display a message on the controller screen * M119 - Output Endstop status to serial port * M120 - Enable endstop detection * M121 - Disable endstop detection @@ -266,7 +266,7 @@ static bool relative_mode = false; //Determines Absolute or Relative Coordinate static char serial_char; static int serial_count = 0; static boolean comment_mode = false; -static char *strchr_pointer; ///< A pointer to find chars in the command string (X, Y, Z, E, etc.) +static char *seen_pointer; ///< A pointer to find chars in the command string (X, Y, Z, E, etc.) const char* queued_commands_P= NULL; /* pointer to the current line in the active sequence of commands, or NULL when none */ const int sensitive_pins[] = SENSITIVE_PINS; ///< Sensitive pin list for M42 // Inactivity shutdown @@ -786,21 +786,20 @@ void get_command() { fromsd[cmd_queue_index_w] = false; #endif - if (strchr(command, 'N') != NULL) { - strchr_pointer = strchr(command, 'N'); - gcode_N = (strtol(strchr_pointer + 1, NULL, 10)); + char *npos = strchr(command, 'N'); + char *apos = strchr(command, '*'); + if (npos) { + gcode_N = strtol(npos + 1, NULL, 10); if (gcode_N != gcode_LastN + 1 && strstr_P(command, PSTR("M110")) == NULL) { gcode_line_error(PSTR(MSG_ERR_LINE_NO)); return; } - if (strchr(command, '*') != NULL) { - byte checksum = 0; - byte count = 0; + if (apos) { + byte checksum = 0, count = 0; while (command[count] != '*') checksum ^= command[count++]; - strchr_pointer = strchr(command, '*'); - if (strtol(strchr_pointer + 1, NULL, 10) != checksum) { + if (strtol(apos + 1, NULL, 10) != checksum) { gcode_line_error(PSTR(MSG_ERR_CHECKSUM_MISMATCH)); return; } @@ -814,28 +813,26 @@ void get_command() { gcode_LastN = gcode_N; // if no errors, continue parsing } - else { // if we don't receive 'N' but still see '*' - if ((strchr(command, '*') != NULL)) { - gcode_line_error(PSTR(MSG_ERR_NO_LINENUMBER_WITH_CHECKSUM), false); - return; - } + else if (apos) { // No '*' without 'N' + gcode_line_error(PSTR(MSG_ERR_NO_LINENUMBER_WITH_CHECKSUM), false); + return; } - if (strchr(command, 'G') != NULL) { - strchr_pointer = strchr(command, 'G'); - switch (strtol(strchr_pointer + 1, NULL, 10)) { - case 0: - case 1: - case 2: - case 3: - if (IsStopped()) { + // Movement commands alert when stopped + if (IsStopped()) { + char *gpos = strchr(command, 'G'); + if (gpos) { + int codenum = strtol(gpos + 1, NULL, 10); + switch (codenum) { + case 0: + case 1: + case 2: + case 3: SERIAL_ERRORLNPGM(MSG_ERR_STOPPED); LCD_MESSAGEPGM(MSG_STOPPED); - } - break; - default: - break; - } + break; + } + } } // If command was e-stop process now @@ -917,32 +914,32 @@ void get_command() { bool code_has_value() { int i = 1; - char c = strchr_pointer[i]; - if (c == '-' || c == '+') c = strchr_pointer[++i]; - if (c == '.') c = strchr_pointer[++i]; + char c = seen_pointer[i]; + if (c == '-' || c == '+') c = seen_pointer[++i]; + if (c == '.') c = seen_pointer[++i]; return (c >= '0' && c <= '9'); } float code_value() { float ret; - char *e = strchr(strchr_pointer, 'E'); + char *e = strchr(seen_pointer, 'E'); if (e) { *e = 0; - ret = strtod(strchr_pointer+1, NULL); + ret = strtod(seen_pointer+1, NULL); *e = 'E'; } else - ret = strtod(strchr_pointer+1, NULL); + ret = strtod(seen_pointer+1, NULL); return ret; } -long code_value_long() { return strtol(strchr_pointer + 1, NULL, 10); } +long code_value_long() { return strtol(seen_pointer + 1, NULL, 10); } -int16_t code_value_short() { return (int16_t)strtol(strchr_pointer + 1, NULL, 10); } +int16_t code_value_short() { return (int16_t)strtol(seen_pointer + 1, NULL, 10); } bool code_seen(char code) { - strchr_pointer = strchr(current_command, code); - return (strchr_pointer != NULL); //Return True if a character was found + seen_pointer = strchr(current_command, code); + return (seen_pointer != NULL); //Return True if a character was found } #define DEFINE_PGM_READ_ANY(type, reader) \ @@ -1793,6 +1790,13 @@ void gcode_get_destination() { } } +void unknown_command_error() { + SERIAL_ECHO_START; + SERIAL_ECHOPGM(MSG_UNKNOWN_COMMAND); + SERIAL_ECHO(current_command); + SERIAL_ECHOPGM("\"\n"); +} + /** * G0, G1: Coordinated movement of X Y Z E axes */ @@ -2844,7 +2848,7 @@ inline void gcode_G92() { * M1: // M1 - Conditional stop - Wait for user button press on LCD */ inline void gcode_M0_M1() { - char *args = strchr_pointer + 3; + char *args = current_command + 3; millis_t codenum = 0; bool hasP = false, hasS = false; @@ -2931,7 +2935,7 @@ inline void gcode_M17() { * M23: Select a file */ inline void gcode_M23() { - card.openFile(strchr_pointer + 4, true); + card.openFile(current_command + 4, true); } /** @@ -2968,7 +2972,7 @@ inline void gcode_M17() { * M28: Start SD Write */ inline void gcode_M28() { - card.openFile(strchr_pointer + 4, false); + card.openFile(current_command + 4, false); } /** @@ -2985,7 +2989,7 @@ inline void gcode_M17() { inline void gcode_M30() { if (card.cardOK) { card.closefile(); - card.removeFile(strchr_pointer + 4); + card.removeFile(current_command + 4); } } @@ -3015,7 +3019,7 @@ inline void gcode_M31() { if (card.sdprinting) st_synchronize(); - char* args = strchr_pointer + 4; + char* args = current_command + 4; char* namestartpos = strchr(args, '!'); // Find ! to indicate filename string start. if (!namestartpos) @@ -3023,12 +3027,12 @@ inline void gcode_M31() { else namestartpos++; //to skip the '!' - bool call_procedure = code_seen('P') && (strchr_pointer < namestartpos); + bool call_procedure = code_seen('P') && (seen_pointer < namestartpos); if (card.cardOK) { card.openFile(namestartpos, true, !call_procedure); - if (code_seen('S') && strchr_pointer < namestartpos) // "S" (must occur _before_ the filename!) + if (code_seen('S') && seen_pointer < namestartpos) // "S" (must occur _before_ the filename!) card.setIndex(code_value_short()); card.startFileprint(); @@ -3041,7 +3045,7 @@ inline void gcode_M31() { * M928: Start SD Write */ inline void gcode_M928() { - card.openLogFile(strchr_pointer + 5); + card.openLogFile(current_command + 5); } #endif // SDSUPPORT @@ -3838,16 +3842,12 @@ inline void gcode_M115() { SERIAL_PROTOCOLPGM(MSG_M115_REPORT); } -#ifdef ULTIPANEL - - /** - * M117: Set LCD Status Message - */ - inline void gcode_M117() { - lcd_setstatus(strchr_pointer + 5); - } - -#endif +/** + * M117: Set LCD Status Message + */ +inline void gcode_M117() { + lcd_setstatus(current_command + 5); +} /** * M119: Output endstop states to serial output @@ -4135,10 +4135,7 @@ inline void gcode_M206() { autoretract_enabled = true; break; default: - SERIAL_ECHO_START; - SERIAL_ECHOPGM(MSG_UNKNOWN_COMMAND); - SERIAL_ECHO(current_command); - SERIAL_ECHOLNPGM("\""); + unknown_command_error(); return; } for (int i=0; i Date: Sun, 17 May 2015 05:07:51 -0700 Subject: [PATCH 3/5] use fetched value --- Marlin/Marlin_main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Marlin/Marlin_main.cpp b/Marlin/Marlin_main.cpp index 80e865f0e..8ddaa6de3 100644 --- a/Marlin/Marlin_main.cpp +++ b/Marlin/Marlin_main.cpp @@ -5696,7 +5696,7 @@ void process_next_command() { break; case 'T': - gcode_T(code_value_short()); + gcode_T(codenum); break; } From adc8fcb77fcef99e77c0d2b3c88fcd26a791b0c0 Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Sun, 17 May 2015 05:42:04 -0700 Subject: [PATCH 4/5] More parser comments, optimize code_seen, save with goto --- Marlin/Marlin_main.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/Marlin/Marlin_main.cpp b/Marlin/Marlin_main.cpp index 8ddaa6de3..4f222dc9a 100644 --- a/Marlin/Marlin_main.cpp +++ b/Marlin/Marlin_main.cpp @@ -938,7 +938,7 @@ long code_value_long() { return strtol(seen_pointer + 1, NULL, 10); } int16_t code_value_short() { return (int16_t)strtol(seen_pointer + 1, NULL, 10); } bool code_seen(char code) { - seen_pointer = strchr(current_command, code); + seen_pointer = strchr(current_command + 3, code); // +3 since "G0 " is the shortest prefix return (seen_pointer != NULL); //Return True if a character was found } @@ -5184,16 +5184,18 @@ void process_next_command() { // Get the command code, which must be G, M, or T char command_code = *current_command; - bool code_is_good = code_has_value(); + // The code must have a numeric value + bool code_is_good = (current_command[1] >= '0' && current_command[1] <= '9'); - if (!code_is_good) { - unknown_command_error(); - ok_to_send(); - return; - } + int codenum; // define ahead of goto - int codenum = code_value_short(); + // Bail early if there's no code + if (!code_is_good) goto ExitUnknownCommand; + // Interpret the code int + codenum = code_value_short(); + + // Handle a known G, M, or T switch(command_code) { case 'G': switch (codenum) { @@ -5700,6 +5702,9 @@ void process_next_command() { break; } +ExitUnknownCommand: + + // Still unknown command? Throw an error if (!code_is_good) unknown_command_error(); ok_to_send(); From a0f362c735401ebbcd95de3f6f8e3c2f17ecc770 Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Sun, 17 May 2015 16:58:37 -0700 Subject: [PATCH 5/5] Simplify & optimize with current_command_args --- Marlin/Marlin_main.cpp | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/Marlin/Marlin_main.cpp b/Marlin/Marlin_main.cpp index 4f222dc9a..6f1f15482 100644 --- a/Marlin/Marlin_main.cpp +++ b/Marlin/Marlin_main.cpp @@ -236,7 +236,7 @@ bool axis_known_position[3] = { false }; static long gcode_N, gcode_LastN, Stopped_gcode_LastN = 0; -static char *current_command; +static char *current_command, *current_command_args; static int cmd_queue_index_r = 0; static int cmd_queue_index_w = 0; static int commands_in_queue = 0; @@ -938,7 +938,7 @@ long code_value_long() { return strtol(seen_pointer + 1, NULL, 10); } int16_t code_value_short() { return (int16_t)strtol(seen_pointer + 1, NULL, 10); } bool code_seen(char code) { - seen_pointer = strchr(current_command + 3, code); // +3 since "G0 " is the shortest prefix + seen_pointer = strchr(current_command_args, code); // +3 since "G0 " is the shortest prefix return (seen_pointer != NULL); //Return True if a character was found } @@ -2848,7 +2848,7 @@ inline void gcode_G92() { * M1: // M1 - Conditional stop - Wait for user button press on LCD */ inline void gcode_M0_M1() { - char *args = current_command + 3; + char *args = current_command_args; millis_t codenum = 0; bool hasP = false, hasS = false; @@ -2935,7 +2935,7 @@ inline void gcode_M17() { * M23: Select a file */ inline void gcode_M23() { - card.openFile(current_command + 4, true); + card.openFile(current_command_args, true); } /** @@ -2972,7 +2972,7 @@ inline void gcode_M17() { * M28: Start SD Write */ inline void gcode_M28() { - card.openFile(current_command + 4, false); + card.openFile(current_command_args, false); } /** @@ -2989,7 +2989,7 @@ inline void gcode_M17() { inline void gcode_M30() { if (card.cardOK) { card.closefile(); - card.removeFile(current_command + 4); + card.removeFile(current_command_args); } } @@ -3019,11 +3019,9 @@ inline void gcode_M31() { if (card.sdprinting) st_synchronize(); - char* args = current_command + 4; - - char* namestartpos = strchr(args, '!'); // Find ! to indicate filename string start. + char* namestartpos = strchr(current_command_args, '!'); // Find ! to indicate filename string start. if (!namestartpos) - namestartpos = args; // Default name position, 4 letters after the M + namestartpos = current_command_args; // Default name position, 4 letters after the M else namestartpos++; //to skip the '!' @@ -3045,7 +3043,7 @@ inline void gcode_M31() { * M928: Start SD Write */ inline void gcode_M928() { - card.openLogFile(current_command + 5); + card.openLogFile(current_command_args); } #endif // SDSUPPORT @@ -3846,7 +3844,7 @@ inline void gcode_M115() { * M117: Set LCD Status Message */ inline void gcode_M117() { - lcd_setstatus(current_command + 5); + lcd_setstatus(current_command_args); } /** @@ -5192,6 +5190,12 @@ void process_next_command() { // Bail early if there's no code if (!code_is_good) goto ExitUnknownCommand; + // Args pointer optimizes code_seen, especially those taking XYZEF + // This wastes a little cpu on commands that expect no arguments. + current_command_args = current_command; + while (*current_command_args != ' ') ++current_command_args; + while (*current_command_args == ' ') ++current_command_args; + // Interpret the code int codenum = code_value_short();