From 53fcd6fc8f8d9545e6d069e457b70a6a059dae49 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Wed, 23 Jun 2021 21:52:25 +0200 Subject: [PATCH 1/5] Work-around GCC LTO codegen bug in process_commands() When building with GCC 4.9.2 (bundled with PF-build-env-1.0.6.*), -Os and LTO enabled, PID_autotune gets automatically inlined into process_commands(). Sadly, due to the massive size of process_commands(), it results in codegen bug doing a partial stack overwrite in process_commands() itself, manifesting as random behavior depending on the timing of interrupts and the codepath taken inside the merged function. Mark the function as noinline and add a note about the affected compiler version in order to be checked again in the future. --- Firmware/temperature.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Firmware/temperature.cpp b/Firmware/temperature.cpp index 63eb8a36..3765589b 100755 --- a/Firmware/temperature.cpp +++ b/Firmware/temperature.cpp @@ -287,8 +287,10 @@ bool checkAllHotends(void) return(result); } - void PID_autotune(float temp, int extruder, int ncycles) - { +// WARNING: the following function has been marked noinline to avoid a GCC 4.9.2 LTO +// codegen bug causing a stack overwrite issue in process_commands() +void __attribute__((noinline)) PID_autotune(float temp, int extruder, int ncycles) +{ pid_number_of_cycles = ncycles; pid_tuning_finished = false; float input = 0.0; From ecce6f865f5456c8949be106e4df1cee568983a1 Mon Sep 17 00:00:00 2001 From: Voinea Dragos Date: Tue, 22 Jun 2021 12:28:37 +0300 Subject: [PATCH 2/5] write_command() no line number handling --- Firmware/cardreader.cpp | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/Firmware/cardreader.cpp b/Firmware/cardreader.cpp index 6155e4ec..41fee385 100644 --- a/Firmware/cardreader.cpp +++ b/Firmware/cardreader.cpp @@ -596,20 +596,9 @@ void CardReader::getStatus(bool arg_P) } void CardReader::write_command(char *buf) { - char* begin = buf; - char* npos = 0; - char* end = buf + strlen(buf) - 1; - file.writeError = false; - if((npos = strchr(buf, 'N')) != NULL) - { - begin = strchr(npos, ' ') + 1; - end = strchr(npos, '*') - 1; - } - end[1] = '\r'; - end[2] = '\n'; - end[3] = '\0'; - file.write(begin); + file.write(buf); //write command + file.write("\r\n"); //write line termination if (file.writeError) { SERIAL_ERROR_START; From 380e34d481a2526b8d832a30f5c89916c87881dc Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Thu, 24 Jun 2021 17:34:39 +0200 Subject: [PATCH 3/5] Include "Dcodes.h" after "Marlin.h" for configuration This is needed in order to get the function prototypes right according to the actual enabled configuration. --- Firmware/Dcodes.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firmware/Dcodes.cpp b/Firmware/Dcodes.cpp index 5a840ee4..8bd019e6 100644 --- a/Firmware/Dcodes.cpp +++ b/Firmware/Dcodes.cpp @@ -1,5 +1,5 @@ -#include "Dcodes.h" #include "Marlin.h" +#include "Dcodes.h" #include "Configuration.h" #include "language.h" #include "cmdqueue.h" From 56e531d40a1c81aff1004ab3fd3f8b1b8519718c Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Thu, 24 Jun 2021 17:35:55 +0200 Subject: [PATCH 4/5] Improve/fix D23 for M2.5/S printers - Move D23 into it's own function inside Dcodes - Correctly include a break in the switch statement - Show the dumper status (enabled/disabled) after toggling - Allow to generate an immediate dump via g-code using D23 E for symmetry with D20 E --- Firmware/Dcodes.cpp | 12 ++++++++++++ Firmware/Dcodes.h | 3 ++- Firmware/Marlin_main.cpp | 6 ++++-- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/Firmware/Dcodes.cpp b/Firmware/Dcodes.cpp index 8bd019e6..f9f3c589 100644 --- a/Firmware/Dcodes.cpp +++ b/Firmware/Dcodes.cpp @@ -974,6 +974,18 @@ void dcode_22() bool emergency_serial_dump = false; +void dcode_23() +{ + if(code_seen('E')) + serial_dump_and_reset(dump_crash_reason::manual); + else + { + emergency_serial_dump = !code_seen('R'); + SERIAL_ECHOPGM("serial dump "); + SERIAL_ECHOLNRPGM(emergency_serial_dump? _N("enabled"): _N("disabled")); + } +} + void __attribute__((noinline)) serial_dump_and_reset(dump_crash_reason reason) { uint16_t sp; diff --git a/Firmware/Dcodes.h b/Firmware/Dcodes.h index c4b470e4..964f6d74 100644 --- a/Firmware/Dcodes.h +++ b/Firmware/Dcodes.h @@ -37,7 +37,8 @@ extern void dcode_22(); //D22 - Clear crash dump state #ifdef EMERGENCY_SERIAL_DUMP #include "xflash_dump.h" -extern bool emergency_serial_dump; +extern void dcode_23(); //D23 - Request/generate an online serial crash dump +extern bool emergency_serial_dump; //emergency dump enabled flag extern void serial_dump_and_reset(dump_crash_reason); #endif diff --git a/Firmware/Marlin_main.cpp b/Firmware/Marlin_main.cpp index 5aca69bd..d1bc83a1 100755 --- a/Firmware/Marlin_main.cpp +++ b/Firmware/Marlin_main.cpp @@ -9382,12 +9382,14 @@ Sigma_Exit: When online dumps are enabled, the FW will dump memory on the serial before resetting. #### Usage - D23 [R] + D23 [E] [R] #### Parameters + - `E` - Perform an emergency crash dump (resets the printer). - `R` - Disable online dumps. */ case 23: { - emergency_serial_dump = !code_seen('R'); + dcode_23(); + break; }; #endif From 1279a6cf4b852287ba708ea7f76c7ebb49f80c1c Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sun, 27 Jun 2021 19:51:21 +0200 Subject: [PATCH 5/5] Correctly read FW_VERSION_NR array from progmem In PR #3093 the progmem array FW_VERSION_NR was introduced to store the version components, however the code didn't read it properly using the pgm_read_* functions, making version comparisons fail. Fix the existing/unused is_provided_version_newer() and reuse it in show_upgrade_dialog_if_version_newer(). Similarly also read/update correctly the version in the eeprom. --- Firmware/util.cpp | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/Firmware/util.cpp b/Firmware/util.cpp index 97ea9fff..fc305517 100644 --- a/Firmware/util.cpp +++ b/Firmware/util.cpp @@ -146,9 +146,14 @@ inline int8_t is_provided_version_newer(const char *version_string) uint16_t ver_gcode[4]; if (! parse_version(version_string, ver_gcode)) return -1; - for (uint8_t i = 0; i < 3; ++ i) - if (ver_gcode[i] > FW_VERSION_NR[i]) + for (uint8_t i = 0; i < 4; ++ i) + { + uint16_t v = (uint16_t)pgm_read_word(&FW_VERSION_NR[i]); + if (ver_gcode[i] > v) return 1; + else if (ver_gcode[i] < v) + return 0; + } return 0; } @@ -182,19 +187,9 @@ bool force_selftest_if_fw_version() bool show_upgrade_dialog_if_version_newer(const char *version_string) { - uint16_t ver_gcode[4]; - if (! parse_version(version_string, ver_gcode)) { -// SERIAL_PROTOCOLLNPGM("parse_version failed"); + int8_t upgrade = is_provided_version_newer(version_string); + if (upgrade < 0) return false; - } - bool upgrade = false; - for (uint8_t i = 0; i < 4; ++ i) { - if (ver_gcode[i] > FW_VERSION_NR[i]) { - upgrade = true; - break; - } else if (ver_gcode[i] < FW_VERSION_NR[i]) - break; - } if (upgrade) { lcd_display_message_fullscreen_P(_i("New firmware version available:"));////MSG_NEW_FIRMWARE_AVAILABLE c=20 r=2 @@ -220,11 +215,11 @@ void update_current_firmware_version_to_eeprom() for (int8_t i = 0; i < FW_PRUSA3D_MAGIC_LEN; ++ i){ eeprom_update_byte((uint8_t*)(EEPROM_FIRMWARE_PRUSA_MAGIC+i), pgm_read_byte(FW_PRUSA3D_MAGIC_STR+i)); } - eeprom_update_word((uint16_t*)EEPROM_FIRMWARE_VERSION_MAJOR, FW_VERSION_NR[0]); - eeprom_update_word((uint16_t*)EEPROM_FIRMWARE_VERSION_MINOR, FW_VERSION_NR[1]); - eeprom_update_word((uint16_t*)EEPROM_FIRMWARE_VERSION_REVISION, FW_VERSION_NR[2]); + eeprom_update_word((uint16_t*)EEPROM_FIRMWARE_VERSION_MAJOR, (uint16_t)pgm_read_word(&FW_VERSION_NR[0])); + eeprom_update_word((uint16_t*)EEPROM_FIRMWARE_VERSION_MINOR, (uint16_t)pgm_read_word(&FW_VERSION_NR[1])); + eeprom_update_word((uint16_t*)EEPROM_FIRMWARE_VERSION_REVISION, (uint16_t)pgm_read_word(&FW_VERSION_NR[2])); // See FirmwareRevisionFlavorType for the definition of firmware flavors. - eeprom_update_word((uint16_t*)EEPROM_FIRMWARE_VERSION_FLAVOR, FW_VERSION_NR[3]); + eeprom_update_word((uint16_t*)EEPROM_FIRMWARE_VERSION_FLAVOR, (uint16_t)pgm_read_word(&FW_VERSION_NR[3])); }