From a54672fb54a3671e451c28a852a64540d385f335 Mon Sep 17 00:00:00 2001 From: Vojtech Kral Date: Fri, 18 May 2018 15:38:33 +0200 Subject: [PATCH] Firmware updater: Make the GUI less scary --- xs/src/avrdude/avrdude-slic3r.cpp | 62 +++++++++++---- xs/src/avrdude/avrdude-slic3r.hpp | 30 ++++++- xs/src/avrdude/avrdude.h | 8 +- xs/src/avrdude/main.c | 64 +++++++++++---- xs/src/slic3r/GUI/FirmwareDialog.cpp | 115 +++++++++++++++++++-------- 5 files changed, 211 insertions(+), 68 deletions(-) diff --git a/xs/src/avrdude/avrdude-slic3r.cpp b/xs/src/avrdude/avrdude-slic3r.cpp index d47469e84c..8e00c73dc2 100644 --- a/xs/src/avrdude/avrdude-slic3r.cpp +++ b/xs/src/avrdude/avrdude-slic3r.cpp @@ -8,37 +8,67 @@ extern "C" { namespace Slic3r { -namespace AvrDude { - - // Used by our custom code in avrdude to receive messages that avrdude normally outputs on stdout (see avrdude_message()) static void avrdude_message_handler_closure(const char *msg, unsigned size, void *user_p) { - auto *message_fn = reinterpret_cast(user_p); + auto *message_fn = reinterpret_cast(user_p); (*message_fn)(msg, size); } -int main(std::vector args, std::string sys_config, MessageFn message_fn) +// Used by our custom code in avrdude to report progress in the GUI +static void avrdude_progress_handler_closure(const char *task, unsigned progress, void *user_p) +{ + auto *progress_fn = reinterpret_cast(user_p); + (*progress_fn)(task, progress); +} + + +AvrDude::AvrDude() {} +AvrDude::~AvrDude() {} + +AvrDude& AvrDude::sys_config(std::string sys_config) +{ + m_sys_config = std::move(sys_config); + return *this; +} + +AvrDude& AvrDude::on_message(MessageFn fn) +{ + m_message_fn = std::move(fn); + return *this; +} + +AvrDude& AvrDude::on_progress(MessageFn fn) +{ + m_progress_fn = std::move(fn); + return *this; +} + +int AvrDude::run(std::vector args) { std::vector c_args {{ const_cast(PACKAGE_NAME) }}; for (const auto &arg : args) { c_args.push_back(const_cast(arg.data())); } - ::avrdude_message_handler_set(avrdude_message_handler_closure, reinterpret_cast(&message_fn)); - const auto res = ::avrdude_main(static_cast(c_args.size()), c_args.data(), sys_config.c_str()); + if (m_message_fn) { + ::avrdude_message_handler_set(avrdude_message_handler_closure, reinterpret_cast(&m_message_fn)); + } else { + ::avrdude_message_handler_set(nullptr, nullptr); + } + + if (m_progress_fn) { + ::avrdude_progress_handler_set(avrdude_progress_handler_closure, reinterpret_cast(&m_progress_fn)); + } else { + ::avrdude_progress_handler_set(nullptr, nullptr); + } + + const auto res = ::avrdude_main(static_cast(c_args.size()), c_args.data(), m_sys_config.c_str()); + ::avrdude_message_handler_set(nullptr, nullptr); + ::avrdude_progress_handler_set(nullptr, nullptr); return res; } -int main(std::vector args, std::string sys_config, std::ostream &os) -{ - return main(std::move(args), std::move(sys_config), std::move([&os](const char *msg, unsigned /* size */) { - os << msg; - })); -} - - -} } diff --git a/xs/src/avrdude/avrdude-slic3r.hpp b/xs/src/avrdude/avrdude-slic3r.hpp index b6143b5080..ecae654b24 100644 --- a/xs/src/avrdude/avrdude-slic3r.hpp +++ b/xs/src/avrdude/avrdude-slic3r.hpp @@ -8,12 +8,34 @@ namespace Slic3r { -namespace AvrDude { +class AvrDude +{ +public: typedef std::function MessageFn; + typedef std::function ProgressFn; + + AvrDude(); + AvrDude(AvrDude &&) = delete; + AvrDude(const AvrDude &) = delete; + AvrDude &operator=(AvrDude &&) = delete; + AvrDude &operator=(const AvrDude &) = delete; + ~AvrDude(); + + // Set location of avrdude's main configuration file + AvrDude& sys_config(std::string sys_config); + // Set message output callback + AvrDude& on_message(MessageFn fn); + // Set progress report callback + // Progress is reported per each task (reading / writing), progress is reported in percents. + AvrDude& on_progress(MessageFn fn); + + int run(std::vector args); +private: + std::string m_sys_config; + MessageFn m_message_fn; + ProgressFn m_progress_fn; +}; - int main(std::vector args, std::string sys_config, MessageFn message_fn); - int main(std::vector args, std::string sys_config, std::ostream &os); -} } diff --git a/xs/src/avrdude/avrdude.h b/xs/src/avrdude/avrdude.h index 7763d1464d..7941699210 100644 --- a/xs/src/avrdude/avrdude.h +++ b/xs/src/avrdude/avrdude.h @@ -29,9 +29,15 @@ extern int verbose; /* verbosity level (-v, -vv, ...) */ extern int quell_progress; /* quiteness level (-q, -qq) */ typedef void (*avrdude_message_handler_t)(const char *msg, unsigned size, void *user_p); -avrdude_message_handler_t avrdude_message_handler_set(avrdude_message_handler_t newhandler, void *user_p); +void avrdude_message_handler_set(avrdude_message_handler_t newhandler, void *user_p); int avrdude_message(const int msglvl, const char *format, ...); +// Progress reporting callback +// `progress` is in range 0 ~ 100 percent +typedef void (*avrdude_progress_handler_t)(const char *task, unsigned progress, void *user_p); +void avrdude_progress_handler_set(avrdude_progress_handler_t newhandler, void *user_p); +void avrdude_progress_external(const char *task, unsigned progress); + #define MSG_INFO (0) /* no -v option, can be supressed with -qq */ #define MSG_NOTICE (1) /* displayed with -v */ #define MSG_NOTICE2 (2) /* displayed with -vv, used rarely */ diff --git a/xs/src/avrdude/main.c b/xs/src/avrdude/main.c index 02feb079b8..727166ec12 100644 --- a/xs/src/avrdude/main.c +++ b/xs/src/avrdude/main.c @@ -68,26 +68,24 @@ char msgbuffer[MSGBUFFER_SIZE]; static void avrdude_message_handler_null(const char *msg, unsigned size, void *user_p) { + // Output to stderr by default (void)size; + (void)user_p; fputs(msg, stderr); } static void *avrdude_message_handler_user_p = NULL; static avrdude_message_handler_t avrdude_message_handler = avrdude_message_handler_null; -avrdude_message_handler_t avrdude_message_handler_set(avrdude_message_handler_t newhandler, void *user_p) +void avrdude_message_handler_set(avrdude_message_handler_t newhandler, void *user_p) { - avrdude_message_handler_t previous = avrdude_message_handler == avrdude_message_handler_null ? NULL : avrdude_message_handler; - if (newhandler != NULL) { avrdude_message_handler = newhandler; avrdude_message_handler_user_p = user_p; } else { - avrdude_message_handler = NULL; + avrdude_message_handler = avrdude_message_handler_null; avrdude_message_handler_user_p = NULL; } - - return previous; } int avrdude_message(const int msglvl, const char *format, ...) @@ -117,6 +115,34 @@ int avrdude_message(const int msglvl, const char *format, ...) } +static void avrdude_progress_handler_null(const char *task, unsigned progress, void *user_p) +{ + // By default do nothing + (void)task; + (void)progress; + (void)user_p; +} + +static void *avrdude_progress_handler_user_p = NULL; +static avrdude_progress_handler_t avrdude_progress_handler = avrdude_progress_handler_null; + +void avrdude_progress_handler_set(avrdude_progress_handler_t newhandler, void *user_p) +{ + if (newhandler != NULL) { + avrdude_progress_handler = newhandler; + avrdude_progress_handler_user_p = user_p; + } else { + avrdude_progress_handler = avrdude_progress_handler_null; + avrdude_progress_handler_user_p = NULL; + } +} + +void avrdude_progress_external(const char *task, unsigned progress) +{ + avrdude_progress_handler(task, progress, avrdude_progress_handler_user_p); +} + + struct list_walk_cookie { FILE *f; @@ -176,7 +202,7 @@ static void usage(void) " -Y Initialize erase cycle # in EEPROM.\n" " -v Verbose output. -v -v for more.\n" " -q Quell progress output. -q -q for less.\n" - " -l logfile Use logfile rather than stderr for diagnostics.\n" +// " -l logfile Use logfile rather than stderr for diagnostics.\n" " -? Display this usage.\n" "\navrdude version %s, URL: \n" ,progname, version); @@ -222,6 +248,7 @@ static void update_progress_no_tty (int percent, double etime, char *hdr) { static int done = 0; static int last = 0; + static char *header = NULL; int cnt = (percent>>1)*2; // setvbuf(stderr, (char*)NULL, _IONBF, 0); @@ -230,16 +257,23 @@ static void update_progress_no_tty (int percent, double etime, char *hdr) avrdude_message(MSG_INFO, "\n%s | ", hdr); last = 0; done = 0; + header = hdr; + avrdude_progress_external(header, 0); } else { while ((cnt > last) && (done == 0)) { avrdude_message(MSG_INFO, "#"); cnt -= 2; } + + if (done == 0) { + avrdude_progress_external(header, percent > 99 ? 99 : percent); + } } if ((percent == 100) && (done == 0)) { avrdude_message(MSG_INFO, " | 100%% %0.2fs\n\n", etime); + avrdude_progress_external(header, 100); last = 0; done = 1; } @@ -386,7 +420,7 @@ int avrdude_main(int argc, char * argv [], const char *sys_config) int silentsafe; /* Don't ask about fuses, 1=silent, 0=normal */ int init_ok; /* Device initialization worked well */ int is_open; /* Device open succeeded */ - char * logfile; /* Use logfile rather than stderr for diagnostics */ + // char * logfile; /* Use logfile rather than stderr for diagnostics */ enum updateflags uflags = UF_AUTO_ERASE; /* Flags for do_op() */ unsigned char safemode_lfuse = 0xff; unsigned char safemode_hfuse = 0xff; @@ -396,9 +430,9 @@ int avrdude_main(int argc, char * argv [], const char *sys_config) char * safemode_response; int fuses_specified = 0; int fuses_updated = 0; -#if !defined(WIN32NATIVE) - char * homedir; -#endif +// #if !defined(WIN32NATIVE) +// char * homedir; +// #endif /* * Set line buffering for file descriptors so we see stdout and stderr @@ -465,7 +499,7 @@ int avrdude_main(int argc, char * argv [], const char *sys_config) safemode = 1; /* Safemode on by default */ silentsafe = 0; /* Ask by default */ is_open = 0; - logfile = NULL; + // logfile = NULL; // #if defined(WIN32NATIVE) @@ -611,9 +645,9 @@ int avrdude_main(int argc, char * argv [], const char *sys_config) ovsigck = 1; break; - case 'l': - logfile = optarg; - break; + // case 'l': + // logfile = optarg; + // break; case 'n': uflags |= UF_NOWRITE; diff --git a/xs/src/slic3r/GUI/FirmwareDialog.cpp b/xs/src/slic3r/GUI/FirmwareDialog.cpp index a6eaa265ce..414e158868 100644 --- a/xs/src/slic3r/GUI/FirmwareDialog.cpp +++ b/xs/src/slic3r/GUI/FirmwareDialog.cpp @@ -17,6 +17,8 @@ #include #include #include +#include +#include #include "libslic3r/Utils.hpp" #include "avrdude/avrdude-slic3r.hpp" @@ -36,16 +38,20 @@ struct FirmwareDialog::priv wxComboBox *port_picker; wxFilePickerCtrl *hex_picker; - wxStaticText *status; + wxStaticText *txt_status; + wxStaticText *txt_progress; + wxGauge *progressbar; wxTextCtrl *txt_stdout; wxButton *btn_close; wxButton *btn_flash; - - bool flashing; - priv() : + bool flashing; + unsigned progress_tasks_done; + + priv() : avrdude_config((fs::path(::Slic3r::resources_dir()) / "avrdude" / "avrdude.conf").string()), - flashing(false) + flashing(false), + progress_tasks_done(0) {} void find_serial_ports(); @@ -68,22 +74,25 @@ void FirmwareDialog::priv::find_serial_ports() void FirmwareDialog::priv::set_flashing(bool value, int res) { flashing = value; - + if (value) { txt_stdout->SetValue(wxEmptyString); - status->SetLabel(_(L("Flashing in progress. Please do not disconnect the printer!"))); - // auto status_color_orig = status->GetForegroundColour(); - status->SetForegroundColour(GUI::get_label_clr_modified()); + txt_status->SetLabel(_(L("Flashing in progress. Please do not disconnect the printer!"))); + txt_status->SetForegroundColour(GUI::get_label_clr_modified()); btn_close->Disable(); btn_flash->Disable(); + progressbar->SetRange(200); // See progress callback below + progressbar->SetValue(0); + progress_tasks_done = 0; } else { auto text_color = wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOWTEXT); btn_close->Enable(); btn_flash->Enable(); - status->SetForegroundColour(text_color); - status->SetLabel( + txt_status->SetForegroundColour(text_color); + txt_status->SetLabel( res == 0 ? _(L("Flashing succeeded!")) : _(L("Flashing failed. Please see the avrdude log below.")) ); + progressbar->SetValue(200); } } @@ -105,6 +114,25 @@ void FirmwareDialog::priv::perform_upload() wxTheApp->Yield(); }; + auto progress_fn = [this](const char *, unsigned progress) { + // We try to track overall progress here. + // When uploading the firmware, avrdude first reas a littlebit of status data, + // then performs write, then reading (verification). + // We Pulse() during the first read and combine progress of the latter two tasks. + + if (this->progress_tasks_done == 0) { + this->progressbar->Pulse(); + } else { + this->progressbar->SetValue(this->progress_tasks_done - 100 + progress); + } + + if (progress == 100) { + this->progress_tasks_done += 100; + } + + wxTheApp->Yield(); + }; + std::vector args {{ "-v", "-p", "atmega2560", @@ -120,7 +148,11 @@ void FirmwareDialog::priv::perform_upload() return a + ' ' + b; }); - auto res = AvrDude::main(std::move(args), avrdude_config, std::move(message_fn)); + auto res = AvrDude() + .sys_config(avrdude_config) + .on_message(std::move(message_fn)) + .on_progress(std::move(progress_fn)) + .run(std::move(args)); BOOST_LOG_TRIVIAL(info) << "avrdude exit code: " << res; @@ -137,10 +169,13 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) : enum { DIALOG_MARGIN = 15, SPACING = 10, + MIN_WIDTH = 600, + MIN_HEIGHT = 100, + LOG_MIN_HEIGHT = 200, }; - wxFont bold_font = wxSystemSettings::GetFont(wxSYS_DEFAULT_GUI_FONT); - bold_font.MakeBold(); + wxFont status_font = wxSystemSettings::GetFont(wxSYS_DEFAULT_GUI_FONT); + status_font.MakeBold(); wxFont mono_font(wxFontInfo().Family(wxFONTFAMILY_TELETYPE)); mono_font.MakeSmaller(); @@ -148,33 +183,50 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) : wxBoxSizer *vsizer = new wxBoxSizer(wxVERTICAL); panel->SetSizer(vsizer); - auto *txt_port_picker = new wxStaticText(panel, wxID_ANY, _(L("Serial port:"))); + auto *label_port_picker = new wxStaticText(panel, wxID_ANY, _(L("Serial port:"))); p->port_picker = new wxComboBox(panel, wxID_ANY); auto *btn_rescan = new wxButton(panel, wxID_ANY, _(L("Rescan"))); auto *port_sizer = new wxBoxSizer(wxHORIZONTAL); port_sizer->Add(p->port_picker, 1, wxEXPAND | wxRIGHT, SPACING); port_sizer->Add(btn_rescan, 0); - auto *txt_hex_picker = new wxStaticText(panel, wxID_ANY, _(L("Firmware image:"))); + auto *label_hex_picker = new wxStaticText(panel, wxID_ANY, _(L("Firmware image:"))); p->hex_picker = new wxFilePickerCtrl(panel, wxID_ANY); - auto *txt_status = new wxStaticText(panel, wxID_ANY, _(L("Status:"))); - p->status = new wxStaticText(panel, wxID_ANY, _(L("Ready"))); - p->status->SetFont(bold_font); + auto *label_status = new wxStaticText(panel, wxID_ANY, _(L("Status:"))); + p->txt_status = new wxStaticText(panel, wxID_ANY, _(L("Ready"))); + p->txt_status->SetFont(status_font); - auto *sizer_pickers = new wxFlexGridSizer(2, SPACING, SPACING); - sizer_pickers->AddGrowableCol(1); - sizer_pickers->Add(txt_port_picker, 0, wxALIGN_CENTER_VERTICAL); - sizer_pickers->Add(port_sizer, 0, wxEXPAND); - sizer_pickers->Add(txt_hex_picker, 0, wxALIGN_CENTER_VERTICAL); - sizer_pickers->Add(p->hex_picker, 0, wxEXPAND); - sizer_pickers->Add(txt_status, 0, wxALIGN_CENTER_VERTICAL); - sizer_pickers->Add(p->status, 0, wxEXPAND); - vsizer->Add(sizer_pickers, 0, wxEXPAND | wxBOTTOM, SPACING); + auto *label_progress = new wxStaticText(panel, wxID_ANY, _(L("Progress:"))); + p->progressbar = new wxGauge(panel, wxID_ANY, 1, wxDefaultPosition, wxDefaultSize, wxGA_HORIZONTAL | wxGA_SMOOTH); - p->txt_stdout = new wxTextCtrl(panel, wxID_ANY, wxEmptyString, wxDefaultPosition, wxDefaultSize, wxTE_MULTILINE | wxTE_READONLY); + auto *grid = new wxFlexGridSizer(2, SPACING, SPACING); + grid->AddGrowableCol(1); + grid->Add(label_port_picker, 0, wxALIGN_CENTER_VERTICAL); + grid->Add(port_sizer, 0, wxEXPAND); + + grid->Add(label_hex_picker, 0, wxALIGN_CENTER_VERTICAL); + grid->Add(p->hex_picker, 0, wxEXPAND); + + grid->Add(label_status, 0, wxALIGN_CENTER_VERTICAL); + grid->Add(p->txt_status, 0, wxEXPAND); + + grid->Add(label_progress, 0, wxALIGN_CENTER_VERTICAL); + grid->Add(p->progressbar, 1, wxEXPAND | wxALIGN_CENTER_VERTICAL); + + vsizer->Add(grid, 0, wxEXPAND | wxTOP | wxBOTTOM, SPACING); + + // Unfortunatelly wxCollapsiblePane seems to resize parent in weird ways. + // Sometimes it disrespects min size. + // The only combo that seems to work well is setting size in its c-tor and a min size on the window. + auto *spoiler = new wxCollapsiblePane(panel, wxID_ANY, _(L("Advanced: avrdude output log")), wxDefaultPosition, wxSize(MIN_WIDTH, MIN_HEIGHT)); + auto *spoiler_pane = spoiler->GetPane(); + auto *spoiler_sizer = new wxBoxSizer(wxVERTICAL); + p->txt_stdout = new wxTextCtrl(spoiler_pane, wxID_ANY, wxEmptyString, wxDefaultPosition, wxSize(0, LOG_MIN_HEIGHT), wxTE_MULTILINE | wxTE_READONLY); p->txt_stdout->SetFont(mono_font); - vsizer->Add(p->txt_stdout, 1, wxEXPAND | wxBOTTOM, SPACING); + spoiler_sizer->Add(p->txt_stdout, 1, wxEXPAND); + spoiler_pane->SetSizer(spoiler_sizer); + vsizer->Add(spoiler, 1, wxEXPAND | wxBOTTOM, SPACING); p->btn_close = new wxButton(panel, wxID_CLOSE); p->btn_flash = new wxButton(panel, wxID_ANY, _(L("Flash!"))); @@ -187,8 +239,7 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) : auto *topsizer = new wxBoxSizer(wxVERTICAL); topsizer->Add(panel, 1, wxEXPAND | wxALL, DIALOG_MARGIN); SetSizerAndFit(topsizer); - SetMinSize(wxSize(400, 400)); - SetSize(wxSize(800, 800)); + SetMinSize(wxSize(MIN_WIDTH, MIN_HEIGHT)); p->find_serial_ports();