Firmware updater: Make the GUI less scary

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<MessageFn*>(user_p);
+	auto *message_fn = reinterpret_cast<AvrDude::MessageFn*>(user_p);
 	(*message_fn)(msg, size);
 }
 
-int main(std::vector<std::string> 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<AvrDude::ProgressFn*>(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<std::string> args)
 {
 	std::vector<char *> c_args {{ const_cast<char*>(PACKAGE_NAME) }};
 	for (const auto &arg : args) {
 		c_args.push_back(const_cast<char*>(arg.data()));
 	}
 
-	::avrdude_message_handler_set(avrdude_message_handler_closure, reinterpret_cast<void*>(&message_fn));
+	if (m_message_fn) {
-	const auto res = ::avrdude_main(static_cast<int>(c_args.size()), c_args.data(), sys_config.c_str());
+		::avrdude_message_handler_set(avrdude_message_handler_closure, reinterpret_cast<void*>(&m_message_fn));
+	} else {
+		::avrdude_message_handler_set(nullptr, nullptr);
+	}
+	
+	if (m_progress_fn) {
+		::avrdude_progress_handler_set(avrdude_progress_handler_closure, reinterpret_cast<void*>(&m_progress_fn));
+	} else {
+		::avrdude_progress_handler_set(nullptr, nullptr);
+	}
+	
+	const auto res = ::avrdude_main(static_cast<int>(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<std::string> 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;
-	}));
-}
-
-
-}
 
 }

xs/src/avrdude/avrdude-slic3r.hpp

@@ -8,12 +8,34 @@
 
 namespace Slic3r {
 
-namespace AvrDude {
+class AvrDude
+{
+public:
 	typedef std::function<void(const char * /* msg */, unsigned /* size */)> MessageFn;
+	typedef std::function<void(const char * /* task */, unsigned /* progress */)> 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<std::string> args);
+private:
+	std::string m_sys_config;
+	MessageFn   m_message_fn;
+	ProgressFn  m_progress_fn;
+};
 
-	int main(std::vector<std::string> args, std::string sys_config, MessageFn message_fn);
-	int main(std::vector<std::string> args, std::string sys_config, std::ostream &os);
-}
 
 }
 

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 */

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 <number>                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: <http://savannah.nongnu.org/projects/avrdude/>\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)
+// #if !defined(WIN32NATIVE)
-  char  * homedir;
+//   char  * homedir;
-#endif
+// #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':
+  //     case 'l':
-	logfile = optarg;
+	// logfile = optarg;
-	break;
+	// break;
 
       case 'n':
         uflags |= UF_NOWRITE;

xs/src/slic3r/GUI/FirmwareDialog.cpp

@@ -17,6 +17,8 @@
 #include <wx/textctrl.h>
 #include <wx/stattext.h>
 #include <wx/combobox.h>
+#include <wx/gauge.h>
+#include <wx/collpane.h>
 
 #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!")));
+		txt_status->SetLabel(_(L("Flashing in progress. Please do not disconnect the printer!")));
-		// auto status_color_orig = status->GetForegroundColour();
+		txt_status->SetForegroundColour(GUI::get_label_clr_modified());
-		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);
+		txt_status->SetForegroundColour(text_color);
-		status->SetLabel(
+		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<std::string> 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);
+	wxFont status_font = wxSystemSettings::GetFont(wxSYS_DEFAULT_GUI_FONT);
-	bold_font.MakeBold();
+	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:")));
+	auto *label_status = new wxStaticText(panel, wxID_ANY, _(L("Status:")));
-	p->status = new wxStaticText(panel, wxID_ANY, _(L("Ready")));
+	p->txt_status = new wxStaticText(panel, wxID_ANY, _(L("Ready")));
-	p->status->SetFont(bold_font);
+	p->txt_status->SetFont(status_font);
 
-	auto *sizer_pickers = new wxFlexGridSizer(2, SPACING, SPACING);
+	auto *label_progress = new wxStaticText(panel, wxID_ANY, _(L("Progress:")));
-	sizer_pickers->AddGrowableCol(1);
+	p->progressbar = new wxGauge(panel, wxID_ANY, 1, wxDefaultPosition, wxDefaultSize, wxGA_HORIZONTAL | wxGA_SMOOTH);
-	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);
 
-	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));
+	SetMinSize(wxSize(MIN_WIDTH, MIN_HEIGHT));
-	SetSize(wxSize(800, 800));
 
 	p->find_serial_ports();