FirmwareDialog: UI improvements, bugfixes

xs/src/avrdude/avrdude-slic3r.cpp

@@ -35,8 +35,9 @@ struct AvrDude::priv
 {
 	std::string sys_config;
 	std::deque<std::vector<std::string>> args;
-	size_t current_args_set = 0;
 	bool cancelled = false;
+	int exit_code = 0;
+	size_t current_args_set = 0;
 	RunFn run_fn;
 	MessageFn message_fn;
 	ProgressFn progress_fn;
@@ -146,15 +147,15 @@ AvrDude::Ptr AvrDude::run()
 			int res = -1;
 
 			if (self->p->run_fn) {
-				self->p->run_fn(*self);
+				self->p->run_fn();
 			}
 
 			if (! self->p->cancelled) {
-				res = self->p->run();
+				self->p->exit_code = self->p->run();
 			}
 
 			if (self->p->complete_fn) {
-				self->p->complete_fn(res, self->p->current_args_set);
+				self->p->complete_fn();
 			}
 		});
 
@@ -179,5 +180,20 @@ void AvrDude::join()
 	}
 }
 
+bool AvrDude::cancelled()
+{
+	return p ? p->cancelled : false;
+}
+
+int AvrDude::exit_code()
+{
+	return p ? p->exit_code : 0;
+}
+
+size_t AvrDude::last_args_set()
+{
+	return p ? p->current_args_set : 0;
+}
+
 
 }

xs/src/avrdude/avrdude-slic3r.hpp

@@ -12,10 +12,10 @@ class AvrDude
 {
 public:
 	typedef std::shared_ptr<AvrDude> Ptr;
-	typedef std::function<void(AvrDude&)> RunFn;
+	typedef std::function<void()> RunFn;
 	typedef std::function<void(const char * /* msg */, unsigned /* size */)> MessageFn;
 	typedef std::function<void(const char * /* task */, unsigned /* progress */)> ProgressFn;
-	typedef std::function<void(int /* exit status */, size_t /* args_id */)> CompleteFn;
+	typedef std::function<void()> CompleteFn;
 
 	// Main c-tor, sys_config is the location of avrdude's main configuration file
 	AvrDude(std::string sys_config);
@@ -54,6 +54,10 @@ public:
 
 	void cancel();
 	void join();
+
+	bool cancelled();          // Whether avrdude run was cancelled
+	int exit_code();           // The exit code of the last invocation
+	size_t last_args_set();    // Index of the last argument set that was processsed
 private:
 	struct priv;
 	std::unique_ptr<priv> p;

xs/src/slic3r/GUI/FirmwareDialog.cpp

@@ -1,12 +1,14 @@
 #include <numeric>
 #include <algorithm>
 #include <thread>
+#include <condition_variable>
 #include <stdexcept>
 #include <boost/format.hpp>
 #include <boost/asio.hpp>
 #include <boost/filesystem/path.hpp>
 #include <boost/filesystem/fstream.hpp>
 #include <boost/log/trivial.hpp>
+#include <boost/optional.hpp>
 
 #include "libslic3r/Utils.hpp"
 #include "avrdude/avrdude-slic3r.hpp"
@@ -32,11 +34,13 @@
 #include <wx/gauge.h>
 #include <wx/collpane.h>
 #include <wx/msgdlg.h>
+#include <wx/filefn.h>
 
 
 namespace fs = boost::filesystem;
 namespace asio = boost::asio;
 using boost::system::error_code;
+using boost::optional;
 
 
 namespace Slic3r {
@@ -46,19 +50,31 @@ using Utils::SerialPortInfo;
 using Utils::Serial;
 
 
+// USB IDs used to perform device lookup
+enum {
+	USB_VID_PRUSA    = 0x2c99,
+	USB_PID_MK2      = 1,
+	USB_PID_MK3      = 2,
+	USB_PID_MMU_BOOT = 3,
+	USB_PID_MMU_APP  = 4,
+};
+
 // This enum discriminates the kind of information in EVT_AVRDUDE,
 // it's stored in the ExtraLong field of wxCommandEvent.
 enum AvrdudeEvent
 {
 	AE_MESSAGE,
 	AE_PROGRESS,
+	AE_STATUS,
 	AE_EXIT,
-	AE_ERROR,
 };
 
 wxDECLARE_EVENT(EVT_AVRDUDE, wxCommandEvent);
 wxDEFINE_EVENT(EVT_AVRDUDE, wxCommandEvent);
 
+wxDECLARE_EVENT(EVT_ASYNC_DIALOG, wxCommandEvent);
+wxDEFINE_EVENT(EVT_ASYNC_DIALOG, wxCommandEvent);
+
 
 // Private
 
@@ -66,15 +82,17 @@ struct FirmwareDialog::priv
 {
 	enum AvrDudeComplete
 	{
+		AC_NONE,
 		AC_SUCCESS,
 		AC_FAILURE,
-		AC_CANCEL,
+		AC_USER_CANCELLED,
 	};
 
 	FirmwareDialog *q;      // PIMPL back pointer ("Q-Pointer")
 
+	// GUI elements
 	wxComboBox *port_picker;
-	std::vector<SerialPortInfo> ports;
+	wxStaticText *port_autodetect;
 	wxFilePickerCtrl *hex_picker;
 	wxStaticText *txt_status;
 	wxGauge *progressbar;
@@ -85,43 +103,66 @@ struct FirmwareDialog::priv
 	wxButton *btn_flash;
 	wxString btn_flash_label_ready;
 	wxString btn_flash_label_flashing;
+	wxString label_status_flashing;
 
 	wxTimer timer_pulse;
 
+	// Async modal dialog during flashing
+	std::mutex mutex;
+	int modal_response;
+	std::condition_variable response_cv;
+
+	// Data
+	std::vector<SerialPortInfo> ports;
+	optional<SerialPortInfo> port;
+	HexFile hex_file;
+
 	// This is a shared pointer holding the background AvrDude task
 	// also serves as a status indication (it is set _iff_ the background task is running, otherwise it is reset).
 	AvrDude::Ptr avrdude;
 	std::string avrdude_config;
 	unsigned progress_tasks_done;
 	unsigned progress_tasks_bar;
-	bool cancelled;
+	bool user_cancelled;
 	const bool extra_verbose;   // For debugging
 
 	priv(FirmwareDialog *q) :
 		q(q),
 		btn_flash_label_ready(_(L("Flash!"))),
 		btn_flash_label_flashing(_(L("Cancel"))),
+		label_status_flashing(_(L("Flashing in progress. Please do not disconnect the printer!"))),
 		timer_pulse(q),
 		avrdude_config((fs::path(::Slic3r::resources_dir()) / "avrdude" / "avrdude.conf").string()),
 		progress_tasks_done(0),
 		progress_tasks_bar(0),
-		cancelled(false),
+		user_cancelled(false),
 		extra_verbose(false)
 	{}
 
 	void find_serial_ports();
+	void fit_no_shrink();
+	void set_txt_status(const wxString &label);
 	void flashing_start(unsigned tasks);
 	void flashing_done(AvrDudeComplete complete);
-	void check_model_id(const HexFile &metadata, const SerialPortInfo &port);
+	void enable_port_picker(bool enable);
+	void load_hex_file(const wxString &path);
+	void queue_status(wxString message);
+	void queue_error(const wxString &message);
 
-	void prepare_common(AvrDude &, const SerialPortInfo &port, const std::string &filename);
-	void prepare_mk2(AvrDude &, const SerialPortInfo &port, const std::string &filename);
-	void prepare_mk3(AvrDude &, const SerialPortInfo &port, const std::string &filename);
-	void prepare_mm_control(AvrDude &, const SerialPortInfo &port, const std::string &filename);
+	bool ask_model_id_mismatch(const std::string &printer_model);
+	bool check_model_id();
+	void wait_for_mmu_bootloader(unsigned retries);
+	void mmu_reboot(const SerialPortInfo &port);
+	void lookup_port_mmu();
+	void prepare_common();
+	void prepare_mk2();
+	void prepare_mk3();
+	void prepare_mm_control();
 	void perform_upload();
 
-	void cancel();
+	void user_cancel();
 	void on_avrdude(const wxCommandEvent &evt);
+	void on_async_dialog(const wxCommandEvent &evt);
 	void ensure_joined();
 };
 
@@ -146,10 +187,30 @@ void FirmwareDialog::priv::find_serial_ports()
 	}
 }
 
+void FirmwareDialog::priv::fit_no_shrink()
+{
+	// Ensure content fits into window and window is not shrinked
+	auto old_size = q->GetSize();
+	q->Layout();
+	q->Fit();
+	auto new_size = q->GetSize();
+	q->SetSize(std::max(old_size.GetWidth(), new_size.GetWidth()), std::max(old_size.GetHeight(), new_size.GetHeight()));
+}
+
+void FirmwareDialog::priv::set_txt_status(const wxString &label)
+{
+	const auto width = txt_status->GetSize().GetWidth();
+	txt_status->SetLabel(label);
+	txt_status->Wrap(width);
+
+	fit_no_shrink();
+}
+
 void FirmwareDialog::priv::flashing_start(unsigned tasks)
 {
+	modal_response = wxID_NONE;
 	txt_stdout->Clear();
-	txt_status->SetLabel(_(L("Flashing in progress. Please do not disconnect the printer!")));
+	set_txt_status(label_status_flashing);
 	txt_status->SetForegroundColour(GUI::get_label_clr_modified());
 	port_picker->Disable();
 	btn_rescan->Disable();
@@ -160,7 +221,7 @@ void FirmwareDialog::priv::flashing_start(unsigned tasks)
 	progressbar->SetValue(0);
 	progress_tasks_done = 0;
 	progress_tasks_bar = 0;
-	cancelled = false;
+	user_cancelled = false;
 	timer_pulse.Start(50);
 }
 
@@ -177,54 +238,190 @@ void FirmwareDialog::priv::flashing_done(AvrDudeComplete complete)
 	progressbar->SetValue(progressbar->GetRange());
 
 	switch (complete) {
-	case AC_SUCCESS: txt_status->SetLabel(_(L("Flashing succeeded!"))); break;
-	case AC_FAILURE: txt_status->SetLabel(_(L("Flashing failed. Please see the avrdude log below."))); break;
-	case AC_CANCEL: txt_status->SetLabel(_(L("Flashing cancelled."))); break;
+	case AC_SUCCESS: set_txt_status(_(L("Flashing succeeded!"))); break;
+	case AC_FAILURE: set_txt_status(_(L("Flashing failed. Please see the avrdude log below."))); break;
+	case AC_USER_CANCELLED: set_txt_status(_(L("Flashing cancelled."))); break;
+	default: break;
 	}
 }
 
-void FirmwareDialog::priv::check_model_id(const HexFile &metadata, const SerialPortInfo &port)
+void FirmwareDialog::priv::enable_port_picker(bool enable)
 {
-	if (metadata.model_id.empty()) {
-		// No data to check against
-		return;
+	port_picker->Show(enable);
+	btn_rescan->Show(enable);
+	port_autodetect->Show(! enable);
+	q->Layout();
+	fit_no_shrink();
+}
+
+void FirmwareDialog::priv::load_hex_file(const wxString &path)
+{
+	hex_file = HexFile(path.wx_str());
+	enable_port_picker(hex_file.device != HexFile::DEV_MM_CONTROL);
+}
+
+void FirmwareDialog::priv::queue_status(wxString message)
+{
+	auto evt = new wxCommandEvent(EVT_AVRDUDE, this->q->GetId());
+	evt->SetExtraLong(AE_STATUS);
+	evt->SetString(std::move(message));
+	wxQueueEvent(this->q, evt);
+}
+
+void FirmwareDialog::priv::queue_error(const wxString &message)
+{
+	auto evt = new wxCommandEvent(EVT_AVRDUDE, this->q->GetId());
+	evt->SetExtraLong(AE_STATUS);
+	evt->SetString(wxString::Format(_(L("Flashing failed: %s")), message));
+
+	wxQueueEvent(this->q, evt);	avrdude->cancel();
+}
+
+bool FirmwareDialog::priv::ask_model_id_mismatch(const std::string &printer_model)
+{
+	// model_id in the hex file doesn't match what the printer repoted.
+	// Ask the user if it should be flashed anyway.
+
+	std::unique_lock<std::mutex> lock(mutex);
+
+	auto evt = new wxCommandEvent(EVT_ASYNC_DIALOG, this->q->GetId());
+	evt->SetString(wxString::Format(_(L(
+		"This firmware hex file does not match the printer model.\n"
+		"The hex file is intended for: %s\n"
+		"Printer reported: %s\n\n"
+		"Do you want to continue and flash this hex file anyway?\n"
+		"Please only continue if you are sure this is the right thing to do.")),
+		hex_file.model_id, printer_model
+	));
+	wxQueueEvent(this->q, evt);
+
+	response_cv.wait(lock, [this]() { return this->modal_response != wxID_NONE; });
+
+	if (modal_response == wxID_YES) { 
+		return true;
+	} else {
+		user_cancel();
+		return false;
 	}
+}
 
-	asio::io_service io;
-	Serial serial(io, port.port, 115200);
-	serial.printer_setup();
+bool FirmwareDialog::priv::check_model_id()
+{
+	// XXX: The implementation in Serial doesn't currently work reliably enough to be used.
+	// Therefore, regretably, so far the check cannot be used and we just return true here.
+	// TODO: Rewrite Serial using more platform-native code.
+	return true;
+	
+	// if (hex_file.model_id.empty()) {
+	// 	// No data to check against, assume it's ok
+	// 	return true;
+	// }
 
+	// asio::io_service io;
+	// Serial serial(io, port->port, 115200);
+	// serial.printer_setup();
+
+	// enum {
+	// 	TIMEOUT = 2000,
+	// 	RETREIES = 5,
+	// };
+
+	// if (! serial.printer_ready_wait(RETREIES, TIMEOUT)) {
+	// 	queue_error(wxString::Format(_(L("Could not connect to the printer at %s")), port->port));
+	// 	return false;
+	// }
+
+	// std::string line;
+	// error_code ec;
+	// serial.printer_write_line("PRUSA Rev");
+	// while (serial.read_line(TIMEOUT, line, ec)) {
+	// 	if (ec) {
+	// 		queue_error(wxString::Format(_(L("Could not connect to the printer at %s")), port->port));
+	// 		return false;
+	// 	}
+
+	// 	if (line == "ok") { continue; }
+
+	// 	if (line == hex_file.model_id) {
+	// 		return true;
+	// 	} else {
+	// 		return ask_model_id_mismatch(line);
+	// 	}
+
+	// 	line.clear();
+	// }
+
+	// return false;
+}
+
+void FirmwareDialog::priv::wait_for_mmu_bootloader(unsigned retries)
+{
 	enum {
-		TIMEOUT = 1000,
-		RETREIES = 3,
+		SLEEP_MS = 500,
 	};
 
-	if (! serial.printer_ready_wait(RETREIES, TIMEOUT)) {
-		throw wxString::Format(_(L("Could not connect to the printer at %s")), port.port);
-	}
+	for (unsigned i = 0; i < retries && !user_cancelled; i++) {
+		std::this_thread::sleep_for(std::chrono::milliseconds(SLEEP_MS));
 
-	std::string line;
-	error_code ec;
-	serial.printer_write_line("PRUSA Rev");
-	while (serial.read_line(TIMEOUT, line, ec)) {
-		if (ec) { throw wxString::Format(_(L("Could not connect to the printer at %s")), port.port); }
-		if (line == "ok") { continue; }
+		auto ports = Utils::scan_serial_ports_extended();
+		ports.erase(std::remove_if(ports.begin(), ports.end(), [=](const SerialPortInfo &port ) {
+			return port.id_vendor != USB_VID_PRUSA && port.id_product != USB_PID_MMU_BOOT;
+		}), ports.end());
 
-		if (line == metadata.model_id) {
+		if (ports.size() == 1) {
+			port = ports[0];
+			return;
+		} else if (ports.size() > 1) {
+			BOOST_LOG_TRIVIAL(error) << "Several VID/PID 0x2c99/3 devices found";
+			queue_error(_(L("Multiple Original Prusa i3 MMU 2.0 devices found. Please only connect one at a time for flashing.")));
 			return;
-		} else {
-			throw wxString::Format(_(L(
-				"The firmware hex file does not match the printer model.\n"
-				"The hex file is intended for:\n  %s\n"
-				"Printer reports:\n  %s"
-			)), metadata.model_id, line);
 		}
-
-		line.clear();
 	}
 }
 
-void FirmwareDialog::priv::prepare_common(AvrDude &avrdude, const SerialPortInfo &port, const std::string &filename)
+void FirmwareDialog::priv::mmu_reboot(const SerialPortInfo &port)
+{
+	asio::io_service io;
+	Serial serial(io, port.port, 1200);
+	std::this_thread::sleep_for(std::chrono::milliseconds(50));
+}
+
+void FirmwareDialog::priv::lookup_port_mmu()
+{
+	BOOST_LOG_TRIVIAL(info) << "Flashing MMU 2.0, looking for VID/PID 0x2c99/3 or 0x2c99/4 ...";
+
+	auto ports = Utils::scan_serial_ports_extended();
+	ports.erase(std::remove_if(ports.begin(), ports.end(), [=](const SerialPortInfo &port ) {
+		return port.id_vendor != USB_VID_PRUSA &&
+			port.id_product != USB_PID_MMU_BOOT &&
+			port.id_product != USB_PID_MMU_APP;
+	}), ports.end());
+
+	if (ports.size() == 0) {
+		BOOST_LOG_TRIVIAL(info) << "MMU 2.0 device not found, asking the user to press Reset and waiting for the device to show up ...";
+
+		queue_status(_(L(
+			"The Multi Material Control device was not found.\n"
+			"If the device is connected, please press the Reset button next to the USB connector ..."
+		)));
+
+		wait_for_mmu_bootloader(30);
+	} else if (ports.size() > 1) {
+		BOOST_LOG_TRIVIAL(error) << "Several VID/PID 0x2c99/3 devices found";
+		queue_error(_(L("Multiple Original Prusa i3 MMU 2.0 devices found. Please only connect one at a time for flashing.")));
+	} else {
+		if (ports[0].id_product == USB_PID_MMU_APP) {
+			// The device needs to be rebooted into the bootloader mode
+			BOOST_LOG_TRIVIAL(info) << boost::format("Found VID/PID 0x2c99/4 at `%1%`, rebooting the device ...") % ports[0].port;
+			mmu_reboot(ports[0]);
+			wait_for_mmu_bootloader(10);
+		} else {
+			port = ports[0];
+		}
+	}
+}
+
+void FirmwareDialog::priv::prepare_common()
 {
 	std::vector<std::string> args {{
 		extra_verbose ? "-vvvvv" : "-v",
@@ -233,10 +430,10 @@ void FirmwareDialog::priv::prepare_common(AvrDude &avrdude, const SerialPortInfo
 		// The Prusa's avrdude is patched to never send semicolons inside the data packets, as the USB to serial chip
 		// is flashed with a buggy firmware.
 		"-c", "wiring",
-		"-P", port.port,
-		"-b", "115200",   // TODO: Allow other rates? Ditto below.
+		"-P", port->port,
+		"-b", "115200",   // TODO: Allow other rates? Ditto elsewhere.
 		"-D",
-		"-U", (boost::format("flash:w:0:%1%:i") % filename).str(),
+		"-U", (boost::format("flash:w:0:%1%:i") % hex_file.path.string()).str(),
 	}};
 
 	BOOST_LOG_TRIVIAL(info) << "Invoking avrdude, arguments: "
@@ -244,97 +441,75 @@ void FirmwareDialog::priv::prepare_common(AvrDude &avrdude, const SerialPortInfo
 			return a + ' ' + b;
 		});
 
-	avrdude.push_args(std::move(args));
+	avrdude->push_args(std::move(args));
 }
 
-void FirmwareDialog::priv::prepare_mk2(AvrDude &avrdude, const SerialPortInfo &port, const std::string &filename)
+void FirmwareDialog::priv::prepare_mk2()
 {
-	prepare_common(avrdude, port, filename);
+	if (! port) { return; }
+
+	if (! check_model_id()) {
+		avrdude->cancel();
+		return;
+	}
+
+	prepare_common();
 }
 
-void FirmwareDialog::priv::prepare_mk3(AvrDude &avrdude, const SerialPortInfo &port, const std::string &filename)
+void FirmwareDialog::priv::prepare_mk3()
 {
-	prepare_common(avrdude, port, filename);
+	if (! port) { return; }
+
+	if (! check_model_id()) {
+		avrdude->cancel();
+		return;
+	}
+
+	prepare_common();
 
 	// The hex file also contains another section with l10n data to be flashed into the external flash on MK3 (Einsy)
 	// This is done via another avrdude invocation, here we build arg list for that:
-	std::vector<std::string> args_l10n {{
+	std::vector<std::string> args {{
 		extra_verbose ? "-vvvvv" : "-v",
 		"-p", "atmega2560",
 		// Using the "Arduino" mode to program Einsy's external flash with languages, using the STK500 protocol (not the STK500v2).
 		// The Prusa's avrdude is patched again to never send semicolons inside the data packets.
 		"-c", "arduino",
-		"-P", port.port,
+		"-P", port->port,
 		"-b", "115200",
 		"-D",
 		"-u", // disable safe mode
-		"-U", (boost::format("flash:w:1:%1%:i") % filename).str(),
+		"-U", (boost::format("flash:w:1:%1%:i") % hex_file.path.string()).str(),
 	}};
 
 	BOOST_LOG_TRIVIAL(info) << "Invoking avrdude for external flash flashing, arguments: "
-		<< std::accumulate(std::next(args_l10n.begin()), args_l10n.end(), args_l10n[0], [](std::string a, const std::string &b) {
+		<< std::accumulate(std::next(args.begin()), args.end(), args[0], [](std::string a, const std::string &b) {
 			return a + ' ' + b;
 		});
 
-	avrdude.push_args(std::move(args_l10n));
+	avrdude->push_args(std::move(args));
 }
 
-void FirmwareDialog::priv::prepare_mm_control(AvrDude &avrdude, const SerialPortInfo &port_in, const std::string &filename)
+void FirmwareDialog::priv::prepare_mm_control()
 {
-	// Check if the port has the PID/VID of 0x2c99/3
-	// If not, check if it is the MMU (0x2c99/4) and reboot the by opening @ 1200 bauds
-	BOOST_LOG_TRIVIAL(info) << "Flashing MMU 2.0, looking for VID/PID 0x2c99/3 or 0x2c99/4 ...";
-	SerialPortInfo port = port_in;
-	if (! port.id_match(0x2c99, 3)) {
-		if (! port.id_match(0x2c99, 4)) {
-			// This is not a Prusa MMU 2.0 device
-			BOOST_LOG_TRIVIAL(error) << boost::format("Not a Prusa MMU 2.0 device: `%1%`") % port.port;
-			throw wxString::Format(_(L("The device at `%s` is not am Original Prusa i3 MMU 2.0 device")), port.port);
-		}
-
-		BOOST_LOG_TRIVIAL(info) << boost::format("Found VID/PID 0x2c99/4 at `%1%`, rebooting the device ...") % port.port;
-
-		{
-			asio::io_service io;
-			Serial serial(io, port.port, 1200);
-			std::this_thread::sleep_for(std::chrono::milliseconds(50));
-		}
-
-		// Wait for the bootloader to show up
-		std::this_thread::sleep_for(std::chrono::milliseconds(2000));
-
-		// Look for the rebooted device
-		BOOST_LOG_TRIVIAL(info) << "... looking for VID/PID 0x2c99/3 ...";
-		auto new_ports = Utils::scan_serial_ports_extended();
-		unsigned hits = 0;
-		for (auto &&new_port : new_ports) {
-			if (new_port.id_match(0x2c99, 3)) {
-				hits++;
-				port = std::move(new_port);
-			}
-		}
-
-		if (hits == 0) {
-			BOOST_LOG_TRIVIAL(error) << "No VID/PID 0x2c99/3 device found after rebooting the MMU 2.0";
-			throw wxString::Format(_(L("Failed to reboot the device at `%s` for programming")), port.port);
-		} else if (hits > 1) {
-			// We found multiple 0x2c99/3 devices, this is bad, because there's no way to find out
-			// which one is the one user wants to flash.
-			BOOST_LOG_TRIVIAL(error) << "Several VID/PID 0x2c99/3 devices found after rebooting the MMU 2.0";
-			throw wxString::Format(_(L("Multiple Original Prusa i3 MMU 2.0 devices found. Please only connect one at a time for flashing.")), port.port);
-		}
+	port = boost::none;
+	lookup_port_mmu();
+	if (! port) {
+		queue_error(_(L("The device could not have been found")));
+		return;
 	}
 
-	BOOST_LOG_TRIVIAL(info) << boost::format("Found VID/PID 0x2c99/3 at `%1%`, flashing ...") % port.port;
+	BOOST_LOG_TRIVIAL(info) << boost::format("Found VID/PID 0x2c99/3 at `%1%`, flashing ...") % port->port;
+	queue_status(label_status_flashing);
 
 	std::vector<std::string> args {{
 		extra_verbose ? "-vvvvv" : "-v",
 		"-p", "atmega32u4",
 		"-c", "avr109",
-		"-P", port.port,
+		"-P", port->port,
 		"-b", "57600",
 		"-D",
-		"-U", (boost::format("flash:w:0:%1%:i") % filename).str(),
+		"-U", (boost::format("flash:w:0:%1%:i") % hex_file.path.string()).str(),
 	}};
 
 	BOOST_LOG_TRIVIAL(info) << "Invoking avrdude, arguments: "
@@ -342,7 +517,7 @@ void FirmwareDialog::priv::prepare_mm_control(AvrDude &avrdude, const SerialPort
 			return a + ' ' + b;
 		});
 
-	avrdude.push_args(std::move(args));
+	avrdude->push_args(std::move(args));
 }
 
 
@@ -351,20 +526,21 @@ void FirmwareDialog::priv::perform_upload()
 	auto filename = hex_picker->GetPath();
 	if (filename.IsEmpty()) { return; }
 
-	int selection = port_picker->GetSelection();
-	if (selection == wxNOT_FOUND) { return; }
+	load_hex_file(filename);  // Might already be loaded, but we want to make sure it's fresh
 
-	const SerialPortInfo &port = this->ports[selection];
-	// Verify whether the combo box list selection equals to the combo box edit value.
-	if (wxString::FromUTF8(this->ports[selection].friendly_name.data()) != port_picker->GetValue()) { 
-		return; 
+	int selection = port_picker->GetSelection();
+	if (selection != wxNOT_FOUND) {
+		port = this->ports[selection];
+
+		// Verify whether the combo box list selection equals to the combo box edit value.
+		if (wxString::FromUTF8(port->friendly_name.data()) != port_picker->GetValue()) {
+			return;
+		}
 	}
 
 	const bool extra_verbose = false;   // For debugging
-	HexFile metadata(filename.wx_str());
-	const std::string filename_utf8(filename.utf8_str().data());
 
-	flashing_start(metadata.device == HexFile::DEV_MK3 ? 2 : 1);
+	flashing_start(hex_file.device == HexFile::DEV_MK3 ? 2 : 1);
 
 	// Init the avrdude object
 	AvrDude avrdude(avrdude_config);
@@ -374,36 +550,23 @@ void FirmwareDialog::priv::perform_upload()
 	auto q = this->q;
 
 	this->avrdude = avrdude
-		.on_run([=](AvrDude &avrdude) {
-			auto queue_error = [&](wxString message) {
-				avrdude.cancel();
-
-				auto evt = new wxCommandEvent(EVT_AVRDUDE, this->q->GetId());
-				evt->SetExtraLong(AE_ERROR);
-				evt->SetString(std::move(message));
-				wxQueueEvent(this->q, evt);
-			};
-
+		.on_run([this]() {
 			try {
-				switch (metadata.device) {
+				switch (this->hex_file.device) {
 				case HexFile::DEV_MK3:
-					this->check_model_id(metadata, port);
-					this->prepare_mk3(avrdude, port, filename_utf8);
+					this->prepare_mk3();
 					break;
 
 				case HexFile::DEV_MM_CONTROL:
-					this->check_model_id(metadata, port);
-					this->prepare_mm_control(avrdude, port, filename_utf8);
+					this->prepare_mm_control();
 					break;
 
 				default:
-					this->prepare_mk2(avrdude, port, filename_utf8);
+					this->prepare_mk2();
 					break;
 				}
-			} catch (const wxString &message) {
-				queue_error(message);
 			} catch (const std::exception &ex) {
-				queue_error(wxString::Format(_(L("Error accessing port at %s: %s")), port.port, ex.what()));
+				queue_error(wxString::Format(_(L("Error accessing port at %s: %s")), port->port, ex.what()));
 			}
 		})
 		.on_message(std::move([q, extra_verbose](const char *msg, unsigned /* size */) {
@@ -423,20 +586,19 @@ void FirmwareDialog::priv::perform_upload()
 			evt->SetInt(progress);
 			wxQueueEvent(q, evt);
 		}))
-		.on_complete(std::move([q](int status, size_t /* args_id */) {
-			auto evt = new wxCommandEvent(EVT_AVRDUDE, q->GetId());
+		.on_complete(std::move([this]() {
+			auto evt = new wxCommandEvent(EVT_AVRDUDE, this->q->GetId());
 			evt->SetExtraLong(AE_EXIT);
-			evt->SetInt(status);
-			wxQueueEvent(q, evt);
+			evt->SetInt(this->avrdude->exit_code());
+			wxQueueEvent(this->q, evt);
 		}))
 		.run();
 }
 
-void FirmwareDialog::priv::cancel()
+void FirmwareDialog::priv::user_cancel()
 {
 	if (avrdude) {
-		cancelled = true;
-		txt_status->SetLabel(_(L("Cancelling...")));
+		user_cancelled = true;
 		avrdude->cancel();
 	}
 }
@@ -474,19 +636,17 @@ void FirmwareDialog::priv::on_avrdude(const wxCommandEvent &evt)
 	case AE_EXIT:
 		BOOST_LOG_TRIVIAL(info) << "avrdude exit code: " << evt.GetInt();
 
-		complete_kind = cancelled ? AC_CANCEL : (evt.GetInt() == 0 ? AC_SUCCESS : AC_FAILURE);
+		// Figure out the exit state
+		if (user_cancelled) { complete_kind = AC_USER_CANCELLED; }
+		else if (avrdude->cancelled()) { complete_kind = AC_NONE; } // Ie. cancelled programatically
+		else { complete_kind = evt.GetInt() == 0 ? AC_SUCCESS : AC_FAILURE; }
+
 		flashing_done(complete_kind);
 		ensure_joined();
 		break;
 
-	case AE_ERROR:
-		txt_stdout->AppendText(evt.GetString());
-		flashing_done(AC_FAILURE);
-		ensure_joined();
-		{
-			GUI::ErrorDialog dlg(this->q, evt.GetString());
-			dlg.ShowModal();
-		}
+	case AE_STATUS:
+		set_txt_status(evt.GetString());
 		break;
 
 	default:
@@ -494,6 +654,16 @@ void FirmwareDialog::priv::on_avrdude(const wxCommandEvent &evt)
 	}
 }
 
+void FirmwareDialog::priv::on_async_dialog(const wxCommandEvent &evt)
+{
+	wxMessageDialog dlg(this->q, evt.GetString(), wxMessageBoxCaptionStr, wxYES_NO | wxNO_DEFAULT | wxICON_QUESTION);
+	{
+		std::lock_guard<std::mutex> lock(mutex);
+		modal_response = dlg.ShowModal();
+	}
+	response_cv.notify_all();
+}
+
 void FirmwareDialog::priv::ensure_joined()
 {
 	// Make sure the background thread is collected and the AvrDude object reset
@@ -521,41 +691,47 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) :
 	wxFont mono_font(wxFontInfo().Family(wxFONTFAMILY_TELETYPE));
 	mono_font.MakeSmaller();
 
+	// Create GUI components and layout
+
 	auto *panel = new wxPanel(this);
 	wxBoxSizer *vsizer = new wxBoxSizer(wxVERTICAL);
 	panel->SetSizer(vsizer);
 
+	auto *label_hex_picker = new wxStaticText(panel, wxID_ANY, _(L("Firmware image:")));
+	p->hex_picker = new wxFilePickerCtrl(panel, wxID_ANY);
+
 	auto *label_port_picker = new wxStaticText(panel, wxID_ANY, _(L("Serial port:")));
 	p->port_picker = new wxComboBox(panel, wxID_ANY);
+	p->port_autodetect = new wxStaticText(panel, wxID_ANY, _(L("Autodetected")));
 	p->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(p->btn_rescan, 0);
+	port_sizer->Add(p->port_autodetect, 1, wxEXPAND);
+	p->enable_port_picker(true);
 
-	auto *label_hex_picker = new wxStaticText(panel, wxID_ANY, _(L("Firmware image:")));
-	p->hex_picker = new wxFilePickerCtrl(panel, wxID_ANY);
+	auto *label_progress = new wxStaticText(panel, wxID_ANY, _(L("Progress:")));
+	p->progressbar = new wxGauge(panel, wxID_ANY, 1, wxDefaultPosition, wxDefaultSize, wxGA_HORIZONTAL | wxGA_SMOOTH);
 
 	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 *label_progress = new wxStaticText(panel, wxID_ANY, _(L("Progress:")));
-	p->progressbar = new wxGauge(panel, wxID_ANY, 1, wxDefaultPosition, wxDefaultSize, wxGA_HORIZONTAL | wxGA_SMOOTH);
-
 	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_port_picker, 0, wxALIGN_CENTER_VERTICAL);
+	grid->Add(port_sizer, 0, wxEXPAND);
 
 	grid->Add(label_progress, 0, wxALIGN_CENTER_VERTICAL);
 	grid->Add(p->progressbar, 1, wxEXPAND | wxALIGN_CENTER_VERTICAL);
 
+	grid->Add(label_status, 0, wxALIGN_CENTER_VERTICAL);
+	grid->Add(p->txt_status, 0, wxEXPAND);
+
 	vsizer->Add(grid, 0, wxEXPAND | wxTOP | wxBOTTOM, SPACING);
 
 	p->spoiler = new wxCollapsiblePane(panel, wxID_ANY, _(L("Advanced: avrdude output log")));
@@ -571,6 +747,7 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) :
 
 	p->btn_close = new wxButton(panel, wxID_CLOSE);
 	p->btn_flash = new wxButton(panel, wxID_ANY, p->btn_flash_label_ready);
+	p->btn_flash->Disable();
 	auto *bsizer = new wxBoxSizer(wxHORIZONTAL);
 	bsizer->Add(p->btn_close);
 	bsizer->AddStretchSpacer();
@@ -585,6 +762,15 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) :
 	SetSize(std::max(size.GetWidth(), static_cast<int>(MIN_WIDTH)), std::max(size.GetHeight(), static_cast<int>(MIN_HEIGHT)));
 	Layout();
 
+	// Bind events
+
+	p->hex_picker->Bind(wxEVT_FILEPICKER_CHANGED, [this](wxFileDirPickerEvent& evt) {
+		if (wxFileExists(evt.GetPath())) {
+			this->p->load_hex_file(evt.GetPath());
+			this->p->btn_flash->Enable();
+		}
+	});
+
 	p->spoiler->Bind(wxEVT_COLLAPSIBLEPANE_CHANGED, [this](wxCollapsiblePaneEvent &evt) {
 		// Dialog size gets screwed up by wxCollapsiblePane, we need to fix it here
 		if (evt.GetCollapsed()) {
@@ -593,7 +779,6 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) :
 			this->SetMinSize(wxSize(MIN_WIDTH, MIN_HEIGHT_EXPANDED));
 		}
 
-		this->Fit();
 		this->Layout();
 	});
 
@@ -608,7 +793,10 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) :
 				_(L("Confirmation")),
 				wxYES_NO | wxNO_DEFAULT | wxICON_QUESTION);
 			if (dlg.ShowModal() == wxID_YES) {
-				this->p->cancel();
+				if (this->p->avrdude) {
+					this->p->set_txt_status(_(L("Cancelling...")));
+					this->p->user_cancel();
+				}
 			}
 		} else {
 			// Start a flashing task
@@ -619,6 +807,7 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) :
 	Bind(wxEVT_TIMER, [this](wxTimerEvent &evt) { this->p->progressbar->Pulse(); });
 
 	Bind(EVT_AVRDUDE, [this](wxCommandEvent &evt) { this->p->on_avrdude(evt); });
+	Bind(EVT_ASYNC_DIALOG, [this](wxCommandEvent &evt) { this->p->on_async_dialog(evt); });
 
 	Bind(wxEVT_CLOSE_WINDOW, [this](wxCloseEvent &evt) {
 		if (this->p->avrdude) {

xs/src/slic3r/Utils/HexFile.cpp

@@ -46,8 +46,7 @@ static size_t hex_num_sections(fs::ifstream &file)
 }
 
 HexFile::HexFile(fs::path path) :
-	path(std::move(path)),
-	device(DEV_GENERIC)
+	path(std::move(path))
 {
 	fs::ifstream file(this->path);
 	if (! file.good()) {

xs/src/slic3r/Utils/HexFile.hpp

@@ -19,9 +19,10 @@ struct HexFile
 	};
 
 	boost::filesystem::path path;
-	DeviceKind device;
+	DeviceKind device = DEV_GENERIC;
 	std::string model_id;
 
+	HexFile() {}
 	HexFile(boost::filesystem::path path);
 };
 

xs/src/slic3r/Utils/Serial.cpp

@@ -373,7 +373,7 @@ void Serial::set_DTR(bool on)
 void Serial::reset_line_num()
 {
 	// See https://github.com/MarlinFirmware/Marlin/wiki/M110
-	printer_write_line("M110 N0", 0);
+	write_string("M110 N0\n");
 	m_line_num = 0;
 }
 
@@ -390,9 +390,9 @@ bool Serial::read_line(unsigned timeout, std::string &line, error_code &ec)
 		asio::async_read(*this, boost::asio::buffer(&c, 1), [&](const error_code &read_ec, size_t size) {
 			if (ec || size == 0) {
 				fail = true;
-				ec = read_ec;
+				ec = read_ec;   // FIXME: only if operation not aborted
 			}
-			timer.cancel();
+			timer.cancel();   // FIXME: ditto
 		});
 
 		if (timeout > 0) {
@@ -444,6 +444,7 @@ bool Serial::printer_ready_wait(unsigned retries, unsigned timeout)
 			}
 			line.clear();
 		}
+
 		line.clear();
 	}
 
@@ -469,7 +470,7 @@ void Serial::printer_reset()
 	this->set_DTR(true);
 	std::this_thread::sleep_for(std::chrono::milliseconds(200));
 	this->set_DTR(false);
-	std::this_thread::sleep_for(std::chrono::milliseconds(2000));
+	std::this_thread::sleep_for(std::chrono::milliseconds(1000));
 }
 
 std::string Serial::printer_format_line(const std::string &line, unsigned line_num)

xs/src/slic3r/Utils/Serial.hpp

@@ -51,12 +51,13 @@ public:
 	// Reads a line or times out, the timeout is in milliseconds
 	bool read_line(unsigned timeout, std::string &line, boost::system::error_code &ec);
 
-	// Perform setup for communicating with a printer
+	// Perform an initial setup for communicating with a printer
 	void printer_setup();
 
 	// Write data from a string
 	size_t write_string(const std::string &str);
-	
+
+	// Attempts to reset the line numer and waits until the printer says "ok"
 	bool printer_ready_wait(unsigned retries, unsigned timeout);
 
 	// Write Marlin-formatted line, with a line number and a checksum