From c0b5fea525594ce2e34274c7c82bb9bf156932b6 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Thu, 28 Jul 2022 00:40:18 +0200 Subject: [PATCH 1/5] Prevent re-entry in EOF command processing cmdqueue will run commands when EOF is reached without returning to the main loop, which is already incorrect. However, since it needs to ensure the queue is empty, an st_synchronize call can result in a re-entrant call to get_command, which will reprocess EOF again. Even if we removed st_synchronize, another command could be picked by an unsuspecting manage_inactivity() somewhere else. Short-circuit EOF processing by closing the file early and checking for the file state early in get_command. This should fix #3549 --- Firmware/cmdqueue.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Firmware/cmdqueue.cpp b/Firmware/cmdqueue.cpp index 0380c16d1..e53122a4f 100755 --- a/Firmware/cmdqueue.cpp +++ b/Firmware/cmdqueue.cpp @@ -534,7 +534,7 @@ void get_command() } #ifdef SDSUPPORT - if(!card.sdprinting || serial_count!=0){ + if(!card.sdprinting || !card.isFileOpen() || serial_count!=0){ // If there is a half filled buffer from serial line, wait until return before // continuing with the serial line. return; @@ -631,6 +631,10 @@ void get_command() // cleared by printingHasFinished after peforming all remaining moves. if(!cmdqueue_calc_sd_length()) { + // queue is complete, but before we process EOF commands prevent + // re-entry by disabling SD processing from any st_synchronize call + card.closefile(); + SERIAL_PROTOCOLLNRPGM(_n("Done printing file"));////MSG_FILE_PRINTED stoptime=_millis(); char time[30]; From a533ba357467a8932f8351a7f7216022fcbdc8b4 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sat, 6 Aug 2022 22:16:50 +0200 Subject: [PATCH 2/5] Reset sdpos_atomic when starting a new SD print Fix an incorrect SD offset on the first G-Code command when the second SD print is started. --- Firmware/Marlin_main.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Firmware/Marlin_main.cpp b/Firmware/Marlin_main.cpp index e2069d204..6cffa3baf 100644 --- a/Firmware/Marlin_main.cpp +++ b/Firmware/Marlin_main.cpp @@ -5675,6 +5675,7 @@ eeprom_update_word((uint16_t*)EEPROM_NOZZLE_DIAMETER_uM,0xFFFF); { // A new print has started from scratch, reset stats failstats_reset_print(); + sdpos_atomic = 0; #ifndef LA_NOCOMPAT la10c_reset(); #endif @@ -5802,6 +5803,7 @@ eeprom_update_word((uint16_t*)EEPROM_NOZZLE_DIAMETER_uM,0xFFFF); { // A new print has started from scratch, reset stats failstats_reset_print(); + sdpos_atomic = 0; #ifndef LA_NOCOMPAT la10c_reset(); #endif From 68c04ca2f6c360dd99ef9390fbc8e632c1d18290 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sat, 6 Aug 2022 23:08:03 +0200 Subject: [PATCH 3/5] Switch a few pointers that don't manipute strings to const --- Firmware/Dcodes.cpp | 2 +- Firmware/Marlin_main.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Firmware/Dcodes.cpp b/Firmware/Dcodes.cpp index 4a51b4d74..95daaf805 100644 --- a/Firmware/Dcodes.cpp +++ b/Firmware/Dcodes.cpp @@ -44,7 +44,7 @@ void print_hex_word(daddr_t val) print_hex_byte(val & 0xFF); } -int parse_hex(char* hex, uint8_t* data, int count) +int parse_hex(const char* hex, uint8_t* data, int count) { int parsed = 0; while (*hex) diff --git a/Firmware/Marlin_main.cpp b/Firmware/Marlin_main.cpp index 6cffa3baf..6c5c0fef3 100644 --- a/Firmware/Marlin_main.cpp +++ b/Firmware/Marlin_main.cpp @@ -4248,7 +4248,7 @@ void process_commands() */ else if (code_seen_P(PSTR("M0")) || code_seen_P(PSTR("M1 "))) {// M0 and M1 - (Un)conditional stop - Wait for user button press on LCD - char *src = strchr_pointer + 2; + const char *src = strchr_pointer + 2; codenum = 0; bool hasP = false, hasS = false; if (code_seen('P')) { @@ -5774,7 +5774,7 @@ eeprom_update_word((uint16_t*)EEPROM_NOZZLE_DIAMETER_uM,0xFFFF); } starpos = (strchr(strchr_pointer + 4,'*')); - char* namestartpos = (strchr(strchr_pointer + 4,'!')); //find ! to indicate filename string start. + const char* namestartpos = (strchr(strchr_pointer + 4,'!')); //find ! to indicate filename string start. if(namestartpos==NULL) { namestartpos=strchr_pointer + 4; //default name position, 4 letters after the M From 4f22de2333bcd9cfd603c144dd037358fc3c9d5f Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sat, 6 Aug 2022 23:15:46 +0200 Subject: [PATCH 4/5] Do *not* shorten the current command in printer_smodel_check printer_smodel_check was incorrectly substituting the final " with a null in the command to simplify the model string comparison, but in doing so was also corrupting the next pop from the cmdqueue. We can modify the current strchr_pointer as long as we *don't* change it's length. This can cause an incorrect extra read from the queue, resulting in the last command to be completely ignored. --- Firmware/util.cpp | 7 +++---- Firmware/util.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/Firmware/util.cpp b/Firmware/util.cpp index 7b192b8c3..3d789e08f 100644 --- a/Firmware/util.cpp +++ b/Firmware/util.cpp @@ -431,7 +431,7 @@ lcd_update_enable(true); // display / status-line recovery #define GCODE_DELIMITER '"' #define ELLIPSIS "..." -char* code_string(char* pStr,size_t* nLength) +char* code_string(const char* pStr,size_t* nLength) { char* pStrBegin; char* pStrEnd; @@ -444,11 +444,10 @@ pStrEnd=strchr(pStrBegin,GCODE_DELIMITER); if(!pStrEnd) return(NULL); *nLength=pStrEnd-pStrBegin; -pStrBegin[*nLength] = '\0'; return pStrBegin; } -void printer_smodel_check(char* pStrPos) +void printer_smodel_check(const char* pStrPos) { char* pResult; size_t nLength,nPrinterNameLength; @@ -458,7 +457,7 @@ pResult = code_string(pStrPos,&nLength); if(pResult != NULL && nLength == nPrinterNameLength) { // Only compare them if the lengths match - if (strcmp_P(pResult, sPrinterName) == 0) return; + if (strncmp_P(pResult, sPrinterName, nLength) == 0) return; } switch(oCheckModel) diff --git a/Firmware/util.h b/Firmware/util.h index 6d34aa081..7533429e2 100644 --- a/Firmware/util.h +++ b/Firmware/util.h @@ -104,7 +104,7 @@ extern ClCheckGcode oCheckGcode; void fCheckModeInit(); void nozzle_diameter_check(uint16_t nDiameter); void printer_model_check(uint16_t nPrinterModel); -void printer_smodel_check(char* pStrPos); +void printer_smodel_check(const char* pStrPos); void fw_version_check(const char *pVersion); void gcode_level_check(uint16_t nGcodeLevel); From 3a1914f2fb06a95e7a89e26d5751aa8688319c1c Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sat, 6 Aug 2022 23:29:34 +0200 Subject: [PATCH 5/5] Simplify printingHasFinished Unswitch the call to file.close(). Do not call quickStop(): motion has already completed due to st_synchronize. --- Firmware/cardreader.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Firmware/cardreader.cpp b/Firmware/cardreader.cpp index 630b0cbd1..0ff8793dc 100644 --- a/Firmware/cardreader.cpp +++ b/Firmware/cardreader.cpp @@ -1008,9 +1008,10 @@ void CardReader::flush_presort() { void CardReader::printingHasFinished() { st_synchronize(); + file.close(); + if(file_subcall_ctr>0) //heading up to a parent file that called current as a procedure. { - file.close(); file_subcall_ctr--; openFileReadFilteredGcode(filenames[file_subcall_ctr],true); setIndex(filespos[file_subcall_ctr]); @@ -1018,8 +1019,6 @@ void CardReader::printingHasFinished() } else { - quickStop(); - file.close(); sdprinting = false; if(SD_FINISHED_STEPPERRELEASE) {