From bc2cedbb6a68df8e3035fd1ab569c06605b29444 Mon Sep 17 00:00:00 2001 From: Lukas Lendvorsky Date: Mon, 6 Nov 2023 16:24:53 +0100 Subject: [PATCH 001/164] Fit thermal problem redscreens Instead of redscreen, printer would show BSOD because it failed to write error to usb serial. So writes to USB-CDC are skipped from ISR. Its not possible to acquire mutex to write to USB from ISR, and even if it would be somehow implemented, it wouldn't probably flush the message in time anyway. --- lib/Arduino_Core_Buddy/cores/arduino/USBSerial.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/Arduino_Core_Buddy/cores/arduino/USBSerial.cpp b/lib/Arduino_Core_Buddy/cores/arduino/USBSerial.cpp index 6a0fa82dbc..1584ca7578 100644 --- a/lib/Arduino_Core_Buddy/cores/arduino/USBSerial.cpp +++ b/lib/Arduino_Core_Buddy/cores/arduino/USBSerial.cpp @@ -66,6 +66,10 @@ void USBSerial::LineBufferAppend(char character) { } size_t USBSerial::write(uint8_t ch) { + // its not possible to write to USB-CDC from ISR, so skip the write alltogether + if (xPortIsInsideInterrupt()) + return 0; + if (enabled) { while (tud_cdc_write_char(ch) != 1) { // TX is full, yield to lower-priority (which usb is part of) threads until ready @@ -94,6 +98,10 @@ static void cdc_write_sync(const uint8_t *buffer, size_t size) { } size_t USBSerial::write(const uint8_t *buffer, size_t size) { + // its not possible to write to USB-CDC from ISR, so skip the write alltogether + if (xPortIsInsideInterrupt()) + return 0; + size_t beg = 0; size_t end = 0; From 3ff389055e0071909f775133740237e08767dada Mon Sep 17 00:00:00 2001 From: Lukas Lendvorsky Date: Thu, 2 Nov 2023 17:46:04 +0100 Subject: [PATCH 002/164] Ensure user has enough room to clean nozzles and install sheet in nozzle offset calibration --- src/common/client_response.hpp | 2 +- src/common/selftest/selftest_tool_offsets.cpp | 14 +++++++++++--- src/common/selftest/selftest_tool_offsets.hpp | 1 + .../selftest/selftest_tool_offsets_interface.cpp | 2 ++ src/common/selftest_state_names.hpp | 4 ++-- src/gui/wizard/selftest_frame_tool_offsets.cpp | 2 +- 6 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/common/client_response.hpp b/src/common/client_response.hpp index f5bea3b433..6a6b0a5fc7 100644 --- a/src/common/client_response.hpp +++ b/src/common/client_response.hpp @@ -274,7 +274,7 @@ enum class PhasesSelftest : uint16_t { ToolOffsets_wait_user_install_pin, ToolOffsets_wait_stable_temp, ToolOffsets_wait_calibrate, - ToolOffsets_wait_final_park, + ToolOffsets_wait_move_away, ToolOffsets_wait_user_remove_pin, _last_Tool_Offsets = ToolOffsets_wait_user_remove_pin, diff --git a/src/common/selftest/selftest_tool_offsets.cpp b/src/common/selftest/selftest_tool_offsets.cpp index c1b03ca978..915dc0215d 100644 --- a/src/common/selftest/selftest_tool_offsets.cpp +++ b/src/common/selftest/selftest_tool_offsets.cpp @@ -105,9 +105,17 @@ LoopResult CSelftestPart_ToolOffsets::state_ask_user_confirm_start() { } LoopResult CSelftestPart_ToolOffsets::state_clean_nozzle_start() { - IPartHandler::SetFsmPhase(PhasesSelftest::ToolOffsets_wait_user_clean_nozzle_cold); - set_nozzle_temps(0); + IPartHandler::SetFsmPhase(PhasesSelftest::ToolOffsets_wait_move_away); disable_all_steppers(); // Let the user operate tools, pull out the filament if required + set_nozzle_temps(0); + + return LoopResult::RunNext; +} + +LoopResult CSelftestPart_ToolOffsets::state_move_away() { + IPartHandler::SetFsmPhase(PhasesSelftest::ToolOffsets_wait_user_clean_nozzle_cold); + // we'll ask user to clean nozzle and put on sheet - so give him some space + do_z_clearance(100); return LoopResult::RunNext; } @@ -203,7 +211,7 @@ LoopResult CSelftestPart_ToolOffsets::state_calibrate() { } LoopResult CSelftestPart_ToolOffsets::state_final_park() { - IPartHandler::SetFsmPhase(PhasesSelftest::ToolOffsets_wait_final_park); + IPartHandler::SetFsmPhase(PhasesSelftest::ToolOffsets_wait_move_away); // Let user uninstall the pin marlin_server::enqueue_gcode("P0 S1"); // Park tool marlin_server::enqueue_gcode("G27"); // Park head diff --git a/src/common/selftest/selftest_tool_offsets.hpp b/src/common/selftest/selftest_tool_offsets.hpp index 4d49bbb178..cc781b4164 100644 --- a/src/common/selftest/selftest_tool_offsets.hpp +++ b/src/common/selftest/selftest_tool_offsets.hpp @@ -12,6 +12,7 @@ class CSelftestPart_ToolOffsets { LoopResult state_ask_user_confirm_start(); LoopResult state_clean_nozzle_start(); + LoopResult state_move_away(); LoopResult state_clean_nozzle(); LoopResult state_ask_user_install_sheet(); LoopResult state_wait_user(); diff --git a/src/common/selftest/selftest_tool_offsets_interface.cpp b/src/common/selftest/selftest_tool_offsets_interface.cpp index 186470f838..1415be8c33 100644 --- a/src/common/selftest/selftest_tool_offsets_interface.cpp +++ b/src/common/selftest/selftest_tool_offsets_interface.cpp @@ -17,6 +17,8 @@ TestReturn phaseToolOffsets([[maybe_unused]] const uint8_t tool_mask, IPartHandl &CSelftestPart_ToolOffsets::state_ask_user_confirm_start, &CSelftestPart_ToolOffsets::state_wait_user, &CSelftestPart_ToolOffsets::state_clean_nozzle_start, + &CSelftestPart_ToolOffsets::state_move_away, + &CSelftestPart_ToolOffsets::state_wait_moves_done, &CSelftestPart_ToolOffsets::state_clean_nozzle, &CSelftestPart_ToolOffsets::state_ask_user_install_sheet, &CSelftestPart_ToolOffsets::state_wait_user, diff --git a/src/common/selftest_state_names.hpp b/src/common/selftest_state_names.hpp index b5f4e77eb7..57a1aa9c22 100644 --- a/src/common/selftest_state_names.hpp +++ b/src/common/selftest_state_names.hpp @@ -162,8 +162,8 @@ constexpr const char *get_selftest_state_name(PhasesSelftest state) { return "ToolOffsets_wait_user_clean_nozzle_hot"; case PhasesSelftest::ToolOffsets_wait_user_install_sheet: return "ToolOffsets_wait_user_install_sheet"; - case PhasesSelftest::ToolOffsets_wait_final_park: - return "ToolOffsets_wait_final_park"; + case PhasesSelftest::ToolOffsets_wait_move_away: + return "ToolOffsets_wait_move_away"; case PhasesSelftest::ToolOffsets_wait_user_remove_pin: return "ToolOffsets_wait_user_remove_pin"; } diff --git a/src/gui/wizard/selftest_frame_tool_offsets.cpp b/src/gui/wizard/selftest_frame_tool_offsets.cpp index 85780723ae..c50800ac02 100644 --- a/src/gui/wizard/selftest_frame_tool_offsets.cpp +++ b/src/gui/wizard/selftest_frame_tool_offsets.cpp @@ -72,7 +72,7 @@ void SelftestFrameToolOffsets::change() { text_detail.Hide(); break; - case PhasesSelftest::ToolOffsets_wait_final_park: + case PhasesSelftest::ToolOffsets_wait_move_away: text_phase.SetText(_("Moving away.")); text_detail.Hide(); break; From 9e0252ded6fe1a0a669dca10042a75cee87149b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20=C4=8Cejchan?= Date: Mon, 6 Nov 2023 12:42:06 +0100 Subject: [PATCH 003/164] Hotfix: Window file list updating fccused index --- src/gui/window_file_list.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/gui/window_file_list.cpp b/src/gui/window_file_list.cpp index 93f57c869f..1e2ac7fba4 100644 --- a/src/gui/window_file_list.cpp +++ b/src/gui/window_file_list.cpp @@ -136,6 +136,8 @@ void window_file_list_t::Load(WF_Sort_t sort, const char *sfnAtCursor, const cha } } + // Force focused index update + focused_index_ = std::nullopt; move_focus_to_index(new_focused_index); } From eb6c6ccccff28855502c0d2d1a8486a417cb462a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Jakub=C3=ADk?= Date: Mon, 6 Nov 2023 16:37:09 +0100 Subject: [PATCH 004/164] xl: Split PID values for inidividual dwarves to fix nozzle heater test If one dwarf finished much earlier than another, it would reset the PID and the other would fail the test. BFW-4607 --- lib/Marlin/Marlin/src/module/temperature.h | 4 ++-- src/common/marlin_server.cpp | 9 ++++++--- src/common/selftest/selftest_XL.cpp | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/Marlin/Marlin/src/module/temperature.h b/lib/Marlin/Marlin/src/module/temperature.h index cb15de645f..45814c512f 100644 --- a/lib/Marlin/Marlin/src/module/temperature.h +++ b/lib/Marlin/Marlin/src/module/temperature.h @@ -934,9 +934,9 @@ class Temperature { last_e_position = 0; #endif #if ENABLED(PRUSA_TOOLCHANGER) - // Set all dwarfs with PID parameters of the first hotend + // Set PID parameters to all dwarves HOTEND_LOOP() { - buddy::puppies::dwarfs[e].set_pid(Temperature::temp_hotend[0].pid.Kp, Temperature::temp_hotend[0].pid.Ki, Temperature::temp_hotend[0].pid.Kd); + buddy::puppies::dwarfs[e].set_pid(Temperature::temp_hotend[e].pid.Kp, Temperature::temp_hotend[e].pid.Ki, Temperature::temp_hotend[e].pid.Kd); } #endif /*HAS_DWARF()*/ } diff --git a/src/common/marlin_server.cpp b/src/common/marlin_server.cpp index 34fc68189b..ee5a581e2d 100644 --- a/src/common/marlin_server.cpp +++ b/src/common/marlin_server.cpp @@ -635,6 +635,7 @@ void settings_save(void) { config_store().pid_bed_d.set(Temperature::temp_bed.pid.Kd); #endif #if ENABLED(PIDTEMP) + // Save only first nozzle PID config_store().pid_nozzle_p.set(Temperature::temp_hotend[0].pid.Kp); config_store().pid_nozzle_i.set(Temperature::temp_hotend[0].pid.Ki); config_store().pid_nozzle_d.set(Temperature::temp_hotend[0].pid.Kd); @@ -652,9 +653,11 @@ void settings_load(void) { Temperature::temp_bed.pid.Kd = config_store().pid_bed_d.get(); #endif #if ENABLED(PIDTEMP) - Temperature::temp_hotend[0].pid.Kp = config_store().pid_nozzle_p.get(); - Temperature::temp_hotend[0].pid.Ki = config_store().pid_nozzle_i.get(); - Temperature::temp_hotend[0].pid.Kd = config_store().pid_nozzle_d.get(); + HOTEND_LOOP() { + Temperature::temp_hotend[e].pid.Kp = config_store().pid_nozzle_p.get(); + Temperature::temp_hotend[e].pid.Ki = config_store().pid_nozzle_i.get(); + Temperature::temp_hotend[e].pid.Kd = config_store().pid_nozzle_d.get(); + } thermalManager.updatePID(); #endif diff --git a/src/common/selftest/selftest_XL.cpp b/src/common/selftest/selftest_XL.cpp index 05f77f0f67..a3e3c7f67f 100644 --- a/src/common/selftest/selftest_XL.cpp +++ b/src/common/selftest/selftest_XL.cpp @@ -153,7 +153,7 @@ static consteval HeaterConfig_t make_nozzle_config(const char *name) { .heater_full_load_min_W = 20, // 35 W +- 43% .heater_full_load_max_W = 50, .pwm_100percent_equivalent_value = 127, - .min_pwm_to_measure = 26 + .min_pwm_to_measure = 127 // Check power only when fully on }; } From 17c35af015fcdcb3fb50a568a5c4363bc9d6b7c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Jakub=C3=ADk?= Date: Mon, 6 Nov 2023 16:38:40 +0100 Subject: [PATCH 005/164] xl: Do not migrate PID values from old EEPROM It was migrating p=17 which was default at that time, but never used. Now the default is 14 and will follow the default if we want change. --- .../config_store/old_eeprom/last_migration.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/persistent_stores/store_instances/config_store/old_eeprom/last_migration.cpp b/src/persistent_stores/store_instances/config_store/old_eeprom/last_migration.cpp index 00107f6f8a..131ea0c1df 100644 --- a/src/persistent_stores/store_instances/config_store/old_eeprom/last_migration.cpp +++ b/src/persistent_stores/store_instances/config_store/old_eeprom/last_migration.cpp @@ -154,13 +154,18 @@ void migrate(old_eeprom::current::vars_body_t &body, journal::Backend &backend) migrate_one(journal::hash("FSensor Enabled"), body.FSENSOR_ENABLED); + // Do not migrate PID on XL, these values were never used on XL +#if ENABLED(PIDTEMP) && !PRINTER_IS_PRUSA_XL migrate_one(journal::hash("PID Nozzle P"), body.PID_NOZ_P); migrate_one(journal::hash("PID Nozzle I"), body.PID_NOZ_I); migrate_one(journal::hash("PID Nozzle D"), body.PID_NOZ_D); +#endif /* ENABLED(PIDTEMP) */ +#if ENABLED(PIDTEMPBED) migrate_one(journal::hash("PID Bed P"), body.PID_BED_P); migrate_one(journal::hash("PID Bed I"), body.PID_BED_I); migrate_one(journal::hash("PID Bed D"), body.PID_BED_D); +#endif /* ENABLED(PIDTEMPBED) */ migrate_one(journal::hash("LAN Flag"), body.LAN_FLAG); migrate_one(journal::hash("LAN IP4 Address"), body.LAN_IP4_ADDR); From b1d43b2d75c8f00f6af3b546c5b986aeaed06002 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Jakub=C3=ADk?= Date: Wed, 1 Nov 2023 15:07:38 +0100 Subject: [PATCH 006/164] Fix race in crash_s that ends with "reentrant recovery" If ISR would happen during that function, the crash_s.loop would stay true during the loop when crash_s.set_state(Crash_s::RECOVERY) is set. --- src/common/marlin_server.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/common/marlin_server.cpp b/src/common/marlin_server.cpp index ee5a581e2d..08a7ad33ce 100644 --- a/src/common/marlin_server.cpp +++ b/src/common/marlin_server.cpp @@ -507,6 +507,7 @@ static void check_crash() { || (crash_s.get_state() == Crash_s::TRIGGERED_TOOLFALL) || (crash_s.get_state() == Crash_s::TRIGGERED_TOOLCRASH) || (crash_s.get_state() == Crash_s::TRIGGERED_HOMEFAIL))) { + crash_s.loop = false; // Set again to prevent race when ISR happens during this function server.print_state = State::CrashRecovery_Begin; return; } From e3e14623ee3e0db8d7c8c3e95fd99545405908d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Jakub=C3=ADk?= Date: Wed, 8 Nov 2023 07:24:13 +0100 Subject: [PATCH 007/164] Revert "CRASH_RECOVERY: Set XY home bumps and move back to 10mm." This reverts commit fb4e117b941d4c0398abd5c1eb27dfb9dccb5a42. --- include/marlin/Configuration_MINI.h | 2 +- include/marlin/Configuration_MINI_adv.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/marlin/Configuration_MINI.h b/include/marlin/Configuration_MINI.h index 1a2f9fb184..957b534e70 100644 --- a/include/marlin/Configuration_MINI.h +++ b/include/marlin/Configuration_MINI.h @@ -565,7 +565,7 @@ //! implemented only for Cartesian kinematics #define MOVE_BACK_BEFORE_HOMING #if ENABLED(MOVE_BACK_BEFORE_HOMING) - #define MOVE_BACK_BEFORE_HOMING_DISTANCE 10.0f + #define MOVE_BACK_BEFORE_HOMING_DISTANCE 1.92f #endif // Specify here all the endstop connectors that are connected to any endstop or probe. diff --git a/include/marlin/Configuration_MINI_adv.h b/include/marlin/Configuration_MINI_adv.h index 138ce40210..ce9c072eec 100644 --- a/include/marlin/Configuration_MINI_adv.h +++ b/include/marlin/Configuration_MINI_adv.h @@ -482,8 +482,8 @@ #define HOMING_MAX_ATTEMPTS 10 // Homing hits each endstop, retracts by these distances, then does a slower bump. -#define X_HOME_BUMP_MM 10 -#define Y_HOME_BUMP_MM 10 +#define X_HOME_BUMP_MM 0 +#define Y_HOME_BUMP_MM 0 #define Z_HOME_BUMP_MM 2 #define HOMING_BUMP_DIVISOR \ { 1, 1, 4 } // Re-Bump Speed Divisor (Divides the Homing Feedrate) From c03e15531dd1303a15cb602c28bc0abcff50569a Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Fri, 3 Nov 2023 14:58:21 +0100 Subject: [PATCH 008/164] Gcode upload: Better errors around dirs Handle unexpected dirs (blocking the uploaded file) and missing destination dirs in a better way than just "Unknown error". BFW-4543. --- lib/WUI/nhttp/gcode_upload.cpp | 17 ++++++++++++++--- lib/WUI/nhttp/status_renderer.cpp | 10 +++++----- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/WUI/nhttp/gcode_upload.cpp b/lib/WUI/nhttp/gcode_upload.cpp index 3c123f1b39..1bd481cc4c 100644 --- a/lib/WUI/nhttp/gcode_upload.cpp +++ b/lib/WUI/nhttp/gcode_upload.cpp @@ -291,6 +291,8 @@ namespace { switch (errno) { case EBUSY: return make_tuple(Status::Conflict, "File is busy"); + case EISDIR: + return make_tuple(Status::Conflict, "Is a directory or ongoing transfer"); default: return make_tuple(Status::InternalServerError, "Unknown error"); } @@ -303,10 +305,19 @@ namespace { int result = rename(src, fn); if (result != 0) { + // Backup errno before calling other things + int old_errno = errno; remove(src); // No check - no way to handle errors anyway. - // we already checked for existence of the file, so we guess - // it is weird file name/forbidden chars that contain (422 Unprocessable Entity). - return make_tuple(Status::UnprocessableEntity, "Filename contains invalid characters"); + switch (old_errno) { + case EISDIR: + return make_tuple(Status::Conflict, "Is a directory or ongoing transfer"); + case ENOTDIR: + return make_tuple(Status::NotFound, "Destination directory does not exist"); + default: + // we already checked for existence of the file, so we guess + // it is weird file name/forbidden chars that contain (422 Unprocessable Entity). + return make_tuple(Status::UnprocessableEntity, "Filename contains invalid characters"); + } } else { return f(fn); } diff --git a/lib/WUI/nhttp/status_renderer.cpp b/lib/WUI/nhttp/status_renderer.cpp index 43e3b06404..2c95f2aba3 100644 --- a/lib/WUI/nhttp/status_renderer.cpp +++ b/lib/WUI/nhttp/status_renderer.cpp @@ -42,11 +42,11 @@ json::JsonResult StatusRenderer::renderState(size_t resume_point, json::JsonOutp JSON_FIELD_INT("time_printing", marlin_vars()->print_duration); JSON_OBJ_END JSON_COMMA; } - JSON_FIELD_OBJ("storage"); - JSON_FIELD_STR("path", "/usb/") JSON_COMMA; - JSON_FIELD_STR("name", "usb") JSON_COMMA; - JSON_FIELD_BOOL("read_only", false); - JSON_OBJ_END JSON_COMMA; + JSON_FIELD_OBJ("storage"); + JSON_FIELD_STR("path", "/usb/") JSON_COMMA; + JSON_FIELD_STR("name", "usb") JSON_COMMA; + JSON_FIELD_BOOL("read_only", false); + JSON_OBJ_END JSON_COMMA; if (state.transfer_id.has_value()) { JSON_FIELD_OBJ("transfer"); JSON_FIELD_INT_G(transfer_status.has_value(), "id", transfer_status->id) JSON_COMMA; From ec75f49c2a211b2182edd6bf256b3e84ec3d6076 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Fri, 3 Nov 2023 11:14:16 +0100 Subject: [PATCH 009/164] transfers: Tune retries * Use 5 retries in a row. * Reset back to 5 when we make some progress. BFW-4547. --- src/transfers/transfer.cpp | 10 +++++++++- src/transfers/transfer.hpp | 4 +++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/transfers/transfer.cpp b/src/transfers/transfer.cpp index 258c4aa69d..1dfded1cc5 100644 --- a/src/transfers/transfer.cpp +++ b/src/transfers/transfer.cpp @@ -385,7 +385,15 @@ Transfer::State Transfer::step(bool is_printing) { } else if (download.has_value()) { auto step_result = download->step(); bool has_issues = step_result != DownloadStep::Continue && step_result != DownloadStep::Finished; - slot.progress(partial_file->get_state(), has_issues); + auto state = partial_file->get_state(); + auto downloaded_size = state.get_valid_size(); + if (downloaded_size != last_downloaded_size) { + // If we make any progress, reset the retries. + // (progress is updated whenever the USB submits a new block). + last_downloaded_size = downloaded_size; + retries_left = MAX_RETRIES; + } + slot.progress(state, has_issues); switch (step_result) { case DownloadStep::Continue: { update_backup(/*force=*/false); diff --git a/src/transfers/transfer.hpp b/src/transfers/transfer.hpp index bafd11ab59..cff7fc3672 100644 --- a/src/transfers/transfer.hpp +++ b/src/transfers/transfer.hpp @@ -14,7 +14,7 @@ namespace transfers { -inline constexpr size_t MAX_RETRIES = 50; +inline constexpr size_t MAX_RETRIES = 5; struct NoTransferSlot {}; @@ -245,6 +245,8 @@ class Transfer { /// Allow at most this many retries for network errors. size_t retries_left = MAX_RETRIES; + /// Reset download retries when this changes... when we make some progres. + size_t last_downloaded_size = 0; /// Current state of the transfer State state; From 0a956f89a0e17d961c1149e638293dd2866f8351 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Mon, 6 Nov 2023 13:50:53 +0100 Subject: [PATCH 010/164] upload: Handle conflicts with dirs better Apparently, our remove & stuff didn't report EISDIR properly. We go with stat instead of fopen to figure stuff out. BFW-4147. --- lib/WUI/nhttp/gcode_upload.cpp | 38 ++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/lib/WUI/nhttp/gcode_upload.cpp b/lib/WUI/nhttp/gcode_upload.cpp index 1bd481cc4c..26701ebd31 100644 --- a/lib/WUI/nhttp/gcode_upload.cpp +++ b/lib/WUI/nhttp/gcode_upload.cpp @@ -6,6 +6,7 @@ #include "../../src/common/filename_type.hpp" #include "../wui_api.h" +#include #include #include @@ -281,26 +282,27 @@ namespace { return error; } - FILE *attempt = fopen(fn, "r"); - if (attempt) { - fclose(attempt); - if (overwrite) { - int result = remove(fn); - if (result == -1) { - remove(src); - switch (errno) { - case EBUSY: - return make_tuple(Status::Conflict, "File is busy"); - case EISDIR: - return make_tuple(Status::Conflict, "Is a directory or ongoing transfer"); - default: - return make_tuple(Status::InternalServerError, "Unknown error"); - } - } - } else { + struct stat st = {}; + int stat_result = stat_retry(fn, &st); + if (stat_result == 0 && S_ISREG(st.st_mode) && overwrite) { + int result = remove(fn); + if (result == -1) { remove(src); - return make_tuple(Status::Conflict, "File already exists"); + switch (errno) { + case EBUSY: + return make_tuple(Status::Conflict, "File is busy"); + case EISDIR: + // Shouldn't happen due to st_mode already checked? + return make_tuple(Status::Conflict, "Is a directory or ongoing transfer"); + default: + return make_tuple(Status::InternalServerError, "Unknown error"); + } } + } else if (stat_result == 0 && S_ISDIR(st.st_mode) && overwrite) { + return make_tuple(Status::Conflict, "Is a directory or ongoing transfer"); + } else if (stat_result == 0) { + remove(src); + return make_tuple(Status::Conflict, "File already exists"); } int result = rename(src, fn); From 414881a7c7928974259bc5cd8f6ef2c41891f903 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 7 Nov 2023 17:48:02 +0100 Subject: [PATCH 011/164] transfers: Back up after failure When we have a download failure, back up the position before retrying so we don't lose any data unnecessarily. Slightly related to BFW-4647 (that one made it more observable for some reason). --- src/transfers/transfer.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/transfers/transfer.cpp b/src/transfers/transfer.cpp index 1dfded1cc5..68bc424833 100644 --- a/src/transfers/transfer.cpp +++ b/src/transfers/transfer.cpp @@ -394,11 +394,12 @@ Transfer::State Transfer::step(bool is_printing) { retries_left = MAX_RETRIES; } slot.progress(state, has_issues); - switch (step_result) { - case DownloadStep::Continue: { - update_backup(/*force=*/false); + if (has_issues) { + update_backup(/*force=*/true); + } else { init_download_order_if_needed(); Transfer::Action next_step = std::visit([&](auto &&arg) { return arg.step(*partial_file); }, *order); + switch (next_step) { case Transfer::Action::Continue: if (is_printable && !already_notified) { @@ -415,6 +416,10 @@ Transfer::State Transfer::step(bool is_printing) { done(State::Finished, Monitor::Outcome::Finished); break; } + } + switch (step_result) { + case DownloadStep::Continue: { + update_backup(/*force=*/false); break; } case DownloadStep::FailedNetwork: From 515b5b1f70d2780bc234e3bce032196c26fca4e0 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 7 Nov 2023 11:50:02 +0100 Subject: [PATCH 012/164] transfers: Postpone initiating download to retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code unification & simplification ‒ create a transfer in a "failed" state and "retry" it later. Apart from making the code shorter and less duplicate, it also makes sure the download order is initialized before the first request (needed for future commits). (The code was duplicated for historical reasons ‒ originally, Download could fail at startup; this is no longer possible) BFW-4647. --- src/transfers/partial_file.hpp | 7 +-- src/transfers/transfer.cpp | 80 +++++++++++++++++----------------- src/transfers/transfer.hpp | 2 +- 3 files changed, 46 insertions(+), 43 deletions(-) diff --git a/src/transfers/partial_file.hpp b/src/transfers/partial_file.hpp index f7a6efb6ca..4a7baed75a 100644 --- a/src/transfers/partial_file.hpp +++ b/src/transfers/partial_file.hpp @@ -195,9 +195,10 @@ class PartialFile { PartialFile(UsbhMscRequest::LunNbr drive, UsbhMscRequest::SectorNbr first_sector, State state, int file_lock); ~PartialFile(); using Ptr = std::shared_ptr; + using Result = std::variant; /// Try to create a new partial file of preallocated size - static std::variant create(const char *path, size_t size); + static Result create(const char *path, size_t size); /// Open existing partial file /// @@ -205,12 +206,12 @@ class PartialFile { /// /// * ignore_opened: If set to true, it'll open the file (for writing) even /// if there's a reader somewhere else. - static std::variant open(const char *path, State state, bool ignore_opened); + static Result open(const char *path, State state, bool ignore_opened); /// Convert an open FILE * into this. /// /// state.total_size is updated according to what is found on the disk and overwritten. - static std::variant convert(const char *path, unique_file_ptr file, State state); + static Result convert(const char *path, unique_file_ptr file, State state); /// Seek to a given offset within the file bool seek(size_t offset); diff --git a/src/transfers/transfer.cpp b/src/transfers/transfer.cpp index 68bc424833..84bd16cf13 100644 --- a/src/transfers/transfer.cpp +++ b/src/transfers/transfer.cpp @@ -128,54 +128,58 @@ Transfer::BeginResult Transfer::begin(const char *destination_path, const Downlo return AlreadyExists {}; } + auto path = Path(destination_path); + unique_file_ptr backup; + PartialFile::Result preallocated; + PartialFile::Ptr partial_file; + + auto cleanup = [&]() { + // Close all potentially opened files + backup.reset(); + preallocated = ""; + partial_file.reset(); + // And remove everything we may have created + remove(path.as_partial()); + remove(path.as_backup()); + rmdir(path.as_destination()); + }; + // make a directory there if (mkdir(destination_path, 0777) != 0) { log_error(transfers, "Failed to create directory %s", destination_path); + cleanup(); return Storage { "Failed to create directory" }; } if (!store_transfer_index(destination_path)) { log_error(transfers, "Failed to store path to index"); + cleanup(); return Storage { "Failed to store path to index" }; } // make the request - auto path = Path(destination_path); - // Create the backup file first to avoid a race condition (if we create the - // partial file first, lose power, we would then think the file full of - // garbage is _complete_). - // - // Then just close it and leave it empty until we have something to write into it. - auto backup = unique_file_ptr(fopen(path.as_backup(), "w")); + backup = unique_file_ptr(fopen(path.as_backup(), "w+")); + if (backup.get() == nullptr) { + cleanup(); + return Storage { "Failed to create backup file" }; + } + size_t file_size = request.encryption->orig_size; + preallocated = move(PartialFile::create(path.as_partial(), file_size)); + if (const char **err = get_if(&preallocated); err != nullptr) { + const char *e = *err; // Backup, cleanup resets preallocated state + cleanup(); + return Storage { e }; + }; + partial_file = get(move(preallocated)); + if (Transfer::make_backup(backup.get(), request, partial_file->get_state(), *slot) == false) { + cleanup(); + return Storage { "Failed to fill the backup file" }; + } backup.reset(); - auto &&download = Download::begin(request, path.as_partial()); - log_info(transfers, "Download request initiated"); - return std::visit([&](auto &&arg) -> Transfer::BeginResult { - using T = std::decay_t; - if constexpr (is_same_v) { - slot->update_expected_size(arg.file_size()); - // we got a valid response and can start downloading - // so lets make a backup file for recovery - auto backup_file = [&]() { - auto transfer_path = Path(destination_path); - return unique_file_ptr(fopen(transfer_path.as_backup(), "w+")); - }(); - if (backup_file.get() == nullptr || Transfer::make_backup(backup_file.get(), request, arg.get_partial_file()->get_state(), *slot) == false) { - return Storage { "Failed to create backup file" }; - } - auto partial_file = arg.get_partial_file(); // get the partial file before we std::move the download away - return Transfer(State::Downloading, std::move(arg), std::move(*slot), std::nullopt, partial_file); - } else { - log_error(transfers, "Failed to initiate download"); - // remove all the files we might have created - remove(path.as_partial()); - remove(path.as_backup()); - rmdir(path.as_destination()); - return arg; - } - }, - download); + Transfer transfer(std::move(*slot), partial_file); + transfer.restart_download(); + return transfer; } bool Transfer::restart_download() { @@ -362,15 +366,13 @@ Transfer::RecoverResult Transfer::recover(const char *destination_path) { slot->progress(partial_file_state, false); - return Transfer(Transfer::State::Retrying, std::nullopt, std::move(*slot), std::nullopt, partial_file); + return Transfer(std::move(*slot), partial_file); } -Transfer::Transfer(State state, std::optional &&download, Monitor::Slot &&slot, std::optional &&order, PartialFile::Ptr partial_file) +Transfer::Transfer(Monitor::Slot &&slot, PartialFile::Ptr partial_file) : slot(std::move(slot)) - , download(std::move(download)) , path(slot.destination()) - , order(order) - , state(state) + , state(State::Retrying) , partial_file(partial_file) , is_printable(filename_is_printable(slot.destination())) { assert(partial_file.get() != nullptr); diff --git a/src/transfers/transfer.hpp b/src/transfers/transfer.hpp index cff7fc3672..99c8325982 100644 --- a/src/transfers/transfer.hpp +++ b/src/transfers/transfer.hpp @@ -270,7 +270,7 @@ class Transfer { /// into place before that. bool is_printable = false; - Transfer(State state, std::optional &&download, Monitor::Slot &&slot, std::optional &&order, PartialFile::Ptr partial_file); + Transfer(Monitor::Slot &&slot, PartialFile::Ptr partial_file); /// Initiates a new download() request based on current state. Returns true on succes. /// TODO: We have to better handle errors here (distinguish between recoverable and non-recoverable ones) From f569483ebb3f8d061f16dc86f339f8b77c2801b6 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 7 Nov 2023 12:40:37 +0100 Subject: [PATCH 013/164] transfers: Download code simplification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The variant of construction with a destination path is no longer used, remove from the code. As a result, the initialization can't fail ‒ it can become a constructor directly. BFW-4647. --- src/transfers/download.cpp | 29 ++++------------------------- src/transfers/download.hpp | 14 +++++--------- src/transfers/transfer.cpp | 30 +++--------------------------- 3 files changed, 12 insertions(+), 61 deletions(-) diff --git a/src/transfers/download.cpp b/src/transfers/download.cpp index dbed5fc878..58b0183471 100644 --- a/src/transfers/download.cpp +++ b/src/transfers/download.cpp @@ -456,37 +456,16 @@ void Download::AsyncDeleter::operator()(Async *a) { } } -Download::Download(AsyncPtr &&async) - : async(move(async)) {} - -Download::BeginResult Download::begin(const Request &request, DestinationPath destination, uint32_t start_range, optional end_range) { +Download::Download(const Request &request, PartialFile::Ptr destination, uint32_t start_range, optional end_range) { // Plain downloads are no longer supported, need encryption info assert(request.encryption); size_t file_size = request.encryption->orig_size; auto decryptor = make_unique(request.encryption->key, request.encryption->nonce, start_range, file_size - start_range); + assert(destination); - PartialFile::Ptr file; - if (auto f = get_if(&destination); f != nullptr) { - file = *f; - } else if (auto f = get_if(&destination); f != nullptr) { - auto preallocated = PartialFile::create(*f, file_size); - if (auto *err = get_if(&preallocated); err != nullptr) { - // TODO: We've lost the AlreadyExists variant somewhere on our way - return Storage { *err }; - } else { - file = get(preallocated); - } - } else { - // No other possibilities - assert(0); - } - assert(file); - - file->seek(start_range); - AsyncPtr async(new Async(request.host, request.port, request.url_path, move(file), move(decryptor), start_range, end_range)); + destination->seek(start_range); + async.reset(new Async(request.host, request.port, request.url_path, move(destination), move(decryptor), start_range, end_range)); tcpip_callback_nofail(Async::start_wrapped, async.get()); - - return Download(move(async)); } DownloadStep Download::step() { diff --git a/src/transfers/download.hpp b/src/transfers/download.hpp index 940891b332..6cb3a87351 100644 --- a/src/transfers/download.hpp +++ b/src/transfers/download.hpp @@ -104,23 +104,19 @@ class Download { }; using AsyncPtr = std::unique_ptr; AsyncPtr async; - Download(AsyncPtr &&async); public: - Download(Download &&other) = default; - Download(const Download &other) = delete; - Download &operator=(Download &&other) = default; - Download &operator=(const Download &other) = delete; - - using DestinationPath = std::variant; - using BeginResult = std::variant; /// Makes an HTTP request. /// /// \param request The request to make. /// \param destination The destination file. If PartialFile is provided, it has to match the final file size. If a string is provided, the PartialFile will be created with the same name. /// \param offset The offset to start the download from. /// \return A Download object if the request was successful and the caller is expected to call step() in a loop to continue with the download. - static BeginResult begin(const Request &request, DestinationPath destination, uint32_t start_range = 0, std::optional end_range = std::nullopt); + Download(const Request &request, PartialFile::Ptr destination, uint32_t start_range = 0, std::optional end_range = std::nullopt); + Download(Download &&other) = default; + Download(const Download &other) = delete; + Download &operator=(Download &&other) = default; + Download &operator=(const Download &other) = delete; /// Continue the download. DownloadStep step(); diff --git a/src/transfers/transfer.cpp b/src/transfers/transfer.cpp index 84bd16cf13..26e0fb5885 100644 --- a/src/transfers/transfer.cpp +++ b/src/transfers/transfer.cpp @@ -176,6 +176,7 @@ Transfer::BeginResult Transfer::begin(const char *destination_path, const Downlo return Storage { "Failed to fill the backup file" }; } backup.reset(); + slot->update_expected_size(file_size); Transfer transfer(std::move(*slot), partial_file); transfer.restart_download(); @@ -241,34 +242,9 @@ bool Transfer::restart_download() { } } - auto &&download = Download::begin(*request, partial_file, position, end_range); + download.emplace(*request, partial_file, position, end_range); - log_info(transfers, "Download request initiated, position: %u", position); - - return std::visit([&](auto &&arg) { - using T = std::decay_t; - if constexpr (is_same_v) { - this->download = std::move(arg); - return true; - } else if constexpr (is_same_v) { - log_error(transfers, "Destination path %s already exists", slot.destination()); - last_connection_error_ms = ticks_ms(); - return false; - } else if constexpr (is_same_v) { - log_error(transfers, "Download request refused"); - last_connection_error_ms = ticks_ms(); - return false; - } else if constexpr (is_same_v) { - log_error(transfers, "Failed to download; storage: %s", arg.msg); - last_connection_error_ms = ticks_ms(); - return false; - } else { - log_error(transfers, "Failed to restart download"); - last_connection_error_ms = ticks_ms(); - return false; - } - }, - download); + return true; } void Transfer::init_download_order_if_needed() { From 664477d5ad3a04c9f7871cdbd763b4f4834905aa Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 7 Nov 2023 13:12:28 +0100 Subject: [PATCH 014/164] transfers: Save one request on text gcodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change the order of downloading textual gcodes from header -> tail -> body to tail -> header + body (the latter one being in one chunk). We need both the header and tail before we start the print, but we don't care in which order we get them. This way we save one request (one seek) ‒ cheaper for us, cheaper for the server and likely few seconds faster before the print starts. BFW-4647. --- src/transfers/transfer.cpp | 34 +++++----------------------------- src/transfers/transfer.hpp | 5 +---- 2 files changed, 6 insertions(+), 33 deletions(-) diff --git a/src/transfers/transfer.cpp b/src/transfers/transfer.cpp index 26e0fb5885..e339a65a16 100644 --- a/src/transfers/transfer.cpp +++ b/src/transfers/transfer.cpp @@ -32,47 +32,27 @@ using std::is_same_v; using std::optional; Transfer::PlainGcodeDownloadOrder::PlainGcodeDownloadOrder(const PartialFile &file) { - if (file.has_valid_head(HeadSize)) { - if (file.has_valid_tail(TailSize)) { - if (file.get_state().get_valid_size() == file.final_size()) { - state = State::Finished; - } else { - state = State::DownloadedBase; - } - } else { - state = State::DownloadingTail; - } + if (file.has_valid_tail(TailSize)) { + state = State::DownloadingBody; } else { - state = State::DownloadingHeader; + state = State::DownloadingTail; } } Transfer::Action Transfer::PlainGcodeDownloadOrder::step(const PartialFile &file) { switch (state) { - case State::DownloadingHeader: - if (file.has_valid_head(HeadSize)) { - state = State::DownloadingTail; - return Action::RangeJump; - } - return Action::Continue; case State::DownloadingTail: if (file.has_valid_tail(TailSize)) { - state = State::DownloadedBase; + state = State::DownloadingBody; return Action::RangeJump; } return Action::Continue; - case State::DownloadedBase: - state = State::DownloadingBody; - return Action::Continue; case State::DownloadingBody: if (file.final_size() == file.get_state().get_valid_size()) { - state = State::Finished; return Action::Finished; } else { return Action::Continue; } - case State::Finished: - return Action::Finished; default: fatal_error("unhandled state", "download"); } @@ -80,7 +60,7 @@ Transfer::Action Transfer::PlainGcodeDownloadOrder::step(const PartialFile &file size_t Transfer::PlainGcodeDownloadOrder::get_next_offset(const PartialFile &file) const { switch (state) { - case State::DownloadingHeader: { + case State::DownloadingBody: { auto head = file.get_valid_head(); return head.has_value() ? head->end : 0; } @@ -89,10 +69,6 @@ size_t Transfer::PlainGcodeDownloadOrder::get_next_offset(const PartialFile &fil log_info(transfers, "returning offset for tail: %i, %u, %u", tail.has_value(), tail->start, tail->end); return tail.has_value() ? tail->end : file.final_size() - TailSize; } - case State::DownloadingBody: - case State::DownloadedBase: - case State::Finished: - return file.get_valid_head()->end; default: fatal_error("unhandled state", "download"); } diff --git a/src/transfers/transfer.hpp b/src/transfers/transfer.hpp index 99c8325982..30d264d8fa 100644 --- a/src/transfers/transfer.hpp +++ b/src/transfers/transfer.hpp @@ -58,14 +58,11 @@ class Transfer { static constexpr size_t TailSize = 50000; enum class State { - DownloadingHeader, DownloadingTail, - DownloadedBase, DownloadingBody, - Finished, }; - State state = State::DownloadingHeader; + State state = State::DownloadingTail; public: static constexpr size_t MinimalFileSize = 512 * 1024; From ee74432407f8c0a13c8fa281118517e1e90965a8 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 7 Nov 2023 18:55:29 +0100 Subject: [PATCH 015/164] simplification: Introduce a ScopeGuard To postpone a closure until the end of a scope / turn a closure into RAII. --- src/common/scope_guard.hpp | 29 +++++++++++++++++++++++++++++ src/transfers/transfer.cpp | 12 +++++------- 2 files changed, 34 insertions(+), 7 deletions(-) create mode 100644 src/common/scope_guard.hpp diff --git a/src/common/scope_guard.hpp b/src/common/scope_guard.hpp new file mode 100644 index 0000000000..52d758a61a --- /dev/null +++ b/src/common/scope_guard.hpp @@ -0,0 +1,29 @@ +#pragma once + +#include + +/// A RAII adaptor for a clean-up closure. +/// +/// Captures a closure to be called at the end of scope. If more are used, they +/// get called in reverse order. +template +class [[nodiscard]] ScopeGuard { +private: + Cback cback; + bool armed = true; + +public: + ScopeGuard(Cback &&cback) + : cback(std::move(cback)) {} + ScopeGuard(const ScopeGuard &other) = delete; + ScopeGuard(ScopeGuard &&other) = delete; + ScopeGuard &operator=(const ScopeGuard &other) = delete; + ScopeGuard &operator=(ScopeGuard &&other) = delete; + ~ScopeGuard() { + if (armed) { + cback(); + } + } + /// Cancel calling of the closure. + void disarm() { armed = false; } +}; diff --git a/src/transfers/transfer.cpp b/src/transfers/transfer.cpp index e339a65a16..08c3ad8148 100644 --- a/src/transfers/transfer.cpp +++ b/src/transfers/transfer.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include