Firmware updater: rework cancelling

10 files changed, 101 insertions, 45 deletions

diff --git a/xs/src/avrdude/avr.c b/xs/src/avrdude/avr.c
index b95cfe76a..73dcaf4ff 100644
--- a/xs/src/avrdude/avr.c
+++ b/xs/src/avrdude/avr.c
@@ -342,6 +342,7 @@ int avr_read(PROGRAMMER * pgm, AVRPART * p, char * memtype,
 
     /* load bytes */
     for (lastaddr = i = 0; i < mem->size; i++) {
+      RETURN_IF_CANCEL();
       if (vmem == NULL ||
           (vmem->tags[i] & TAG_ALLOCATED) != 0)
       {
@@ -358,7 +359,7 @@ int avr_read(PROGRAMMER * pgm, AVRPART * p, char * memtype,
           return -1;
         }
       }
-      if (!report_progress(i, mem->size, NULL)) return -99;
+      report_progress(i, mem->size, NULL);
     }
     return avr_mem_hiaddr(mem);
   }
@@ -392,6 +393,7 @@ int avr_read(PROGRAMMER * pgm, AVRPART * p, char * memtype,
     for (pageaddr = 0, failure = 0, nread = 0;
          !failure && pageaddr < mem->size;
          pageaddr += mem->page_size) {
+      RETURN_IF_CANCEL();
       /* check whether this page must be read */
       for (i = pageaddr, need_read = 0;
            i < pageaddr + mem->page_size;
@@ -415,7 +417,7 @@ int avr_read(PROGRAMMER * pgm, AVRPART * p, char * memtype,
                         progname, pageaddr / mem->page_size);
       }
       nread++;
-      if (!report_progress(nread, npages, NULL)) return -99;
+      report_progress(nread, npages, NULL);
     }
     if (!failure) {
       if (strcasecmp(mem->desc, "flash") == 0 ||
@@ -436,6 +438,7 @@ int avr_read(PROGRAMMER * pgm, AVRPART * p, char * memtype,
   }
 
   for (i=0; i < mem->size; i++) {
+    RETURN_IF_CANCEL();
     if (vmem == NULL ||
 	(vmem->tags[i] & TAG_ALLOCATED) != 0)
     {
@@ -448,7 +451,7 @@ int avr_read(PROGRAMMER * pgm, AVRPART * p, char * memtype,
 	return -2;
       }
     }
-    if (!report_progress(i, mem->size, NULL)) return -99;
+    report_progress(i, mem->size, NULL);
   }
 
   if (strcasecmp(mem->desc, "flash") == 0 ||
@@ -876,6 +879,7 @@ int avr_write(PROGRAMMER * pgm, AVRPART * p, char * memtype, int size,
 
     /* write words, low byte first */
     for (lastaddr = i = 0; i < wsize; i += 2) {
+      RETURN_IF_CANCEL();
       if ((m->tags[i] & TAG_ALLOCATED) != 0 ||
           (m->tags[i + 1] & TAG_ALLOCATED) != 0) {
 
@@ -896,7 +900,7 @@ int avr_write(PROGRAMMER * pgm, AVRPART * p, char * memtype, int size,
 
         while (avr_tpi_poll_nvmbsy(pgm));
       }
-      if (!report_progress(i, wsize, NULL)) return -99;
+      report_progress(i, wsize, NULL);
     }
     return i;
   }
@@ -926,6 +930,7 @@ int avr_write(PROGRAMMER * pgm, AVRPART * p, char * memtype, int size,
     for (pageaddr = 0, failure = 0, nwritten = 0;
          !failure && pageaddr < wsize;
          pageaddr += m->page_size) {
+      RETURN_IF_CANCEL();
       /* check whether this page must be written to */
       for (i = pageaddr, need_write = 0;
            i < pageaddr + m->page_size;
@@ -948,7 +953,7 @@ int avr_write(PROGRAMMER * pgm, AVRPART * p, char * memtype, int size,
                         progname, pageaddr / m->page_size);
       }
       nwritten++;
-      if (!report_progress(nwritten, npages, NULL)) return -99;
+      report_progress(nwritten, npages, NULL);
     }
     if (!failure)
       return wsize;
@@ -964,8 +969,9 @@ int avr_write(PROGRAMMER * pgm, AVRPART * p, char * memtype, int size,
   flush_page = 0;
 
   for (i=0; i<wsize; i++) {
+    RETURN_IF_CANCEL();
     data = m->buf[i];
-    if (!report_progress(i, wsize, NULL)) return -99;
+    report_progress(i, wsize, NULL);
 
     /*
      * Find out whether the write action must be invoked for this
@@ -1050,14 +1056,14 @@ int avr_signature(PROGRAMMER * pgm, AVRPART * p)
 {
   int rc;
 
-  if (!report_progress(0,1,"Reading")) return -99;
+  report_progress(0,1,"Reading");
   rc = avr_read(pgm, p, "signature", 0);
   if (rc < 0) {
     avrdude_message(MSG_INFO, "%s: error reading signature data for part \"%s\", rc=%d\n",
                     progname, p->desc, rc);
     return -1;
   }
-  if (!report_progress(1,1,NULL)) return -99;
+  report_progress(1,1,NULL);
 
   return 0;
 }
@@ -1106,6 +1112,7 @@ int avr_verify(AVRPART * p, AVRPART * v, char * memtype, int size)
   }
 
   for (i=0; i<size; i++) {
+    RETURN_IF_CANCEL();
     if ((b->tags[i] & TAG_ALLOCATED) != 0 &&
         buf1[i] != buf2[i]) {
       avrdude_message(MSG_INFO, "%s: verification error, first mismatch at byte 0x%04x\n"
@@ -1214,19 +1221,16 @@ int avr_chip_erase(PROGRAMMER * pgm, AVRPART * p)
  * call for each of start, during and end cases. As things stand now,
  * that is not possible and makes maintenance a bit more work.
  */
-// Prusa version modification: report_progress() returns bool to faciliate cancelation
-// the bool has "continue" semantics, ie. true = continue, false = interrupt
-bool report_progress (int completed, int total, char *hdr)
+void report_progress (int completed, int total, char *hdr)
 {
   static int last = 0;
   static double start_time;
   int percent = (total > 0) ? ((completed * 100) / total) : 100;
   struct timeval tv;
   double t;
-  bool res = true;
 
   if (update_progress == NULL)
-    return true;
+    return;
 
   gettimeofday(&tv, NULL);
   t = tv.tv_sec + ((double)tv.tv_usec)/1000000;
@@ -1234,7 +1238,7 @@ bool report_progress (int completed, int total, char *hdr)
   if (hdr) {
     last = 0;
     start_time = t;
-    res = update_progress (percent, t - start_time, hdr);
+    update_progress (percent, t - start_time, hdr);
   }
 
   if (percent > 100)
@@ -1242,11 +1246,9 @@ bool report_progress (int completed, int total, char *hdr)
 
   if (percent > last) {
     last = percent;
-    res = update_progress (percent, t - start_time, hdr);
+    update_progress (percent, t - start_time, hdr);
   }
 
   if (percent == 100)
     last = 0;                   /* Get ready for next time. */
-
-  return res;
 }
diff --git a/xs/src/avrdude/avrdude-slic3r.cpp b/xs/src/avrdude/avrdude-slic3r.cpp
index 13c37e508..a859200fb 100644
--- a/xs/src/avrdude/avrdude-slic3r.cpp
+++ b/xs/src/avrdude/avrdude-slic3r.cpp
@@ -21,10 +21,10 @@ static void avrdude_message_handler_closure(const char *msg, unsigned size, void
 }
 
 // Used by our custom code in avrdude to report progress in the GUI
-static bool avrdude_progress_handler_closure(const char *task, unsigned progress, void *user_p)
+static void avrdude_progress_handler_closure(const char *task, unsigned progress, void *user_p)
 {
 	auto *progress_fn = reinterpret_cast<AvrDude::ProgressFn*>(user_p);
-	return (*progress_fn)(task, progress);
+	(*progress_fn)(task, progress);
 }
 
 
@@ -134,6 +134,11 @@ AvrDude::Ptr AvrDude::run()
 	return self;
 }
 
+void AvrDude::cancel()
+{
+	::avrdude_cancel();
+}
+
 void AvrDude::join()
 {
 	if (p && p->avrdude_thread.joinable()) {
diff --git a/xs/src/avrdude/avrdude-slic3r.hpp b/xs/src/avrdude/avrdude-slic3r.hpp
index a9a3c8e5b..8d881b094 100644
--- a/xs/src/avrdude/avrdude-slic3r.hpp
+++ b/xs/src/avrdude/avrdude-slic3r.hpp
@@ -13,7 +13,7 @@ class AvrDude
 public:
 	typedef std::shared_ptr<AvrDude> Ptr;
 	typedef std::function<void(const char * /* msg */, unsigned /* size */)> MessageFn;
-	typedef std::function<bool(const char * /* task */, unsigned /* progress */)> ProgressFn;
+	typedef std::function<void(const char * /* task */, unsigned /* progress */)> ProgressFn;
 	typedef std::function<void(int /* exit status */)> CompleteFn;
 
 	AvrDude();
@@ -33,8 +33,7 @@ public:
 	AvrDude& on_message(MessageFn fn);
 
 	// Set progress report callback
-	// Progress is reported per each task (reading / writing), progress is reported in percents.
-	// The callback's return value indicates whether to continue flashing (true) or cancel (false).
+	// Progress is reported per each task (reading / writing) in percents.
 	AvrDude& on_progress(ProgressFn fn);
 
 	// Called when avrdude's main function finishes
@@ -42,6 +41,8 @@ public:
 
 	int run_sync();
 	Ptr run();
+
+	void cancel();
 	void join();
 private:
 	struct priv;
diff --git a/xs/src/avrdude/avrdude.h b/xs/src/avrdude/avrdude.h
index 501be642d..9f724433f 100644
--- a/xs/src/avrdude/avrdude.h
+++ b/xs/src/avrdude/avrdude.h
@@ -21,7 +21,6 @@
 #ifndef avrdude_h
 #define avrdude_h
 
-#include <stdbool.h>
 
 extern char * progname;		/* name of program, for messages */
 extern char progbuf[];		/* spaces same length as progname */
@@ -36,9 +35,12 @@ int avrdude_message(const int msglvl, const char *format, ...);
 
 // Progress reporting callback
 // `progress` is in range 0 ~ 100 percent
-typedef bool (*avrdude_progress_handler_t)(const char *task, unsigned progress, void *user_p);
+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);
-bool avrdude_progress_external(const char *task, unsigned progress);
+void avrdude_progress_external(const char *task, unsigned progress);
+
+// Cancellation
+void avrdude_cancel();
 
 #define MSG_INFO    (0) /* no -v option, can be supressed with -qq */
 #define MSG_NOTICE  (1) /* displayed with -v */
diff --git a/xs/src/avrdude/libavrdude.h b/xs/src/avrdude/libavrdude.h
index dc3ea2c10..e8197f9c2 100644
--- a/xs/src/avrdude/libavrdude.h
+++ b/xs/src/avrdude/libavrdude.h
@@ -727,12 +727,22 @@ void sort_programmers(LISTID programmers);
 
 /* formerly avr.h */
 
-typedef bool (*FP_UpdateProgress)(int percent, double etime, char *hdr);
+typedef void (*FP_UpdateProgress)(int percent, double etime, char *hdr);
 
 extern struct avrpart parts[];
 
 extern FP_UpdateProgress update_progress;
 
+extern bool cancel_flag;
+#define RETURN_IF_CANCEL() \
+  do { \
+    if (cancel_flag) { \
+      avrdude_message(MSG_INFO, "%s(): Cancelled, exiting...\n", __func__); \
+      return -99; \
+    } \
+  } while (0)
+
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -769,7 +779,7 @@ int avr_mem_hiaddr(AVRMEM * mem);
 
 int avr_chip_erase(PROGRAMMER * pgm, AVRPART * p);
 
-bool report_progress (int completed, int total, char *hdr);
+void report_progress (int completed, int total, char *hdr);
 
 #ifdef __cplusplus
 }
diff --git a/xs/src/avrdude/main.c b/xs/src/avrdude/main.c
index 3ac983e1e..71a886040 100644
--- a/xs/src/avrdude/main.c
+++ b/xs/src/avrdude/main.c
@@ -66,6 +66,8 @@ char   progbuf[PATH_MAX]; /* temporary buffer of spaces the same
 #define MSGBUFFER_SIZE 4096
 char msgbuffer[MSGBUFFER_SIZE];
 
+bool cancel_flag = false;
+
 static void avrdude_message_handler_null(const char *msg, unsigned size, void *user_p)
 {
     // Output to stderr by default
@@ -115,13 +117,12 @@ int avrdude_message(const int msglvl, const char *format, ...)
 }
 
 
-static bool avrdude_progress_handler_null(const char *task, unsigned progress, void *user_p)
+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;
-    return true;
 }
 
 static void *avrdude_progress_handler_user_p = NULL;
@@ -138,9 +139,14 @@ void avrdude_progress_handler_set(avrdude_progress_handler_t newhandler, void *u
     }
 }
 
-bool avrdude_progress_external(const char *task, unsigned progress)
+void avrdude_progress_external(const char *task, unsigned progress)
+{
+    avrdude_progress_handler(task, progress, avrdude_progress_handler_user_p);
+}
+
+void avrdude_cancel()
 {
-    return avrdude_progress_handler(task, progress, avrdude_progress_handler_user_p);
+    cancel_flag = true;
 }
 
 
@@ -251,7 +257,6 @@ static bool update_progress_no_tty (int percent, double etime, char *hdr)
   static int last = 0;
   static char *header = NULL;
   int cnt = (percent>>1)*2;
-  bool res = true;
 
   // setvbuf(stderr, (char*)NULL, _IONBF, 0);
 
@@ -260,7 +265,7 @@ static bool update_progress_no_tty (int percent, double etime, char *hdr)
     last = 0;
     done = 0;
     header = hdr;
-    res = avrdude_progress_external(header, 0);
+    avrdude_progress_external(header, 0);
   }
   else {
     while ((cnt > last) && (done == 0)) {
@@ -269,7 +274,7 @@ static bool update_progress_no_tty (int percent, double etime, char *hdr)
     }
 
     if (done == 0) {
-      res = avrdude_progress_external(header, percent > 99 ? 99 : percent);
+      avrdude_progress_external(header, percent > 99 ? 99 : percent);
     }
   }
 
@@ -283,8 +288,6 @@ static bool update_progress_no_tty (int percent, double etime, char *hdr)
     last = (percent>>1)*2;    /* Make last a multiple of 2. */
 
   // setvbuf(stderr, (char*)NULL, _IOLBF, 0);
-
-  return res;
 }
 
 static void list_programmers_callback(const char *name, const char *desc,
@@ -447,6 +450,8 @@ int avrdude_main(int argc, char * argv [], const char *sys_config)
 
   progname = strrchr(argv[0],'/');
 
+  cancel_flag = false;
+
 #if defined (WIN32NATIVE)
   /* take care of backslash as dir sep in W32 */
   if (!progname) progname = strrchr(argv[0],'\\');
diff --git a/xs/src/avrdude/ser_posix.c b/xs/src/avrdude/ser_posix.c
index 81e05da2e..91b18e945 100644
--- a/xs/src/avrdude/ser_posix.c
+++ b/xs/src/avrdude/ser_posix.c
@@ -340,6 +340,7 @@ static int ser_send(union filedescriptor *fd, const unsigned char * buf, size_t
   }
 
   while (len) {
+    RETURN_IF_CANCEL();
     rc = write(fd->ifd, p, (len > 1024) ? 1024 : len);
     if (rc < 0) {
       avrdude_message(MSG_INFO, "%s: ser_send(): write error: %s\n",
@@ -370,6 +371,7 @@ static int ser_recv(union filedescriptor *fd, unsigned char * buf, size_t buflen
 
   while (len < buflen) {
   reselect:
+    RETURN_IF_CANCEL();
     FD_ZERO(&rfds);
     FD_SET(fd->ifd, &rfds);
 
@@ -458,6 +460,7 @@ static int ser_drain(union filedescriptor *fd, int display)
     FD_SET(fd->ifd, &rfds);
 
   reselect:
+    RETURN_IF_CANCEL();
     nfds = select(fd->ifd + 1, &rfds, NULL, NULL, &timeout);
     if (nfds == 0) {
       if (display) {
diff --git a/xs/src/avrdude/ser_win32.c b/xs/src/avrdude/ser_win32.c
index b1f161291..20d085d13 100644
--- a/xs/src/avrdude/ser_win32.c
+++ b/xs/src/avrdude/ser_win32.c
@@ -415,6 +415,8 @@ static int ser_send(union filedescriptor *fd, const unsigned char * buf, size_t
 	DWORD written;
         const unsigned char * b = buf;
 
+	RETURN_IF_CANCEL();
+
 	HANDLE hComPort=(HANDLE)fd->pfd;
 
 	if (hComPort == INVALID_HANDLE_VALUE) {
@@ -571,6 +573,8 @@ static int ser_recv(union filedescriptor *fd, unsigned char * buf, size_t buflen
 	unsigned char * p = buf;
 	DWORD read;
 
+	RETURN_IF_CANCEL();
+
 	HANDLE hComPort=(HANDLE)fd->pfd;
 	
 	if (hComPort == INVALID_HANDLE_VALUE) {
@@ -653,6 +657,8 @@ static int ser_drain(union filedescriptor *fd, int display)
 	}
 
 	while (1) {
+		RETURN_IF_CANCEL();
+
 		readres=ReadFile(hComPort, buf, 1, &read, NULL);
 		if (!readres) {
 			LPVOID lpMsgBuf;
diff --git a/xs/src/avrdude/stk500v2.c b/xs/src/avrdude/stk500v2.c
index 6b36d42be..d3acb639c 100644
--- a/xs/src/avrdude/stk500v2.c
+++ b/xs/src/avrdude/stk500v2.c
@@ -654,6 +654,7 @@ static int stk500v2_recv(PROGRAMMER * pgm, unsigned char *msg, size_t maxsize) {
   tstart = tv.tv_sec;
 
   while ( (state != sDONE ) && (!timeout) ) {
+    RETURN_IF_CANCEL();
     if (serial_recv(&pgm->fd, &c, 1) < 0)
       goto timedout;
     DEBUG("0x%02x ",c);
@@ -758,6 +759,8 @@ static int stk500v2_getsync_internal(PROGRAMMER * pgm, int retries) {
 retry:
   tries++;
 
+  RETURN_IF_CANCEL();
+
   // send the sync command and see if we can get there
   buf[0] = CMD_SIGN_ON;
   if (stk500v2_send(pgm, buf, 1) != 0) {
@@ -765,9 +768,13 @@ retry:
     return -1;
   }
 
+  RETURN_IF_CANCEL();
+
   // try to get the response back and see where we got
   status = stk500v2_recv(pgm, resp, sizeof(resp));
 
+  RETURN_IF_CANCEL();
+
   // if we got bytes returned, check to see what came back
   if (status > 0) {
     if ((resp[0] == CMD_SIGN_ON) && (resp[1] == STATUS_CMD_OK) &&
@@ -844,15 +851,21 @@ static int stk500v2_command(PROGRAMMER * pgm, unsigned char * buf,
 retry:
   tries++;
 
+  RETURN_IF_CANCEL();
+
   // send the command to the programmer
   if (stk500v2_send(pgm, buf, len) != 0) {
     avrdude_message(MSG_INFO, "%s: stk500v2_command(): can't communicate with device\n", progname);
     return -1;
   }
 
+  RETURN_IF_CANCEL();
+
   // attempt to read the status back
   status = stk500v2_recv(pgm,buf,maxlen);
 
+  RETURN_IF_CANCEL();
+
   // if we got a successful readback, return
   if (status > 0) {
     DEBUG(" = %d\n",status);
diff --git a/xs/src/slic3r/GUI/FirmwareDialog.cpp b/xs/src/slic3r/GUI/FirmwareDialog.cpp
index 165b055fc..01ad04f6b 100644
--- a/xs/src/slic3r/GUI/FirmwareDialog.cpp
+++ b/xs/src/slic3r/GUI/FirmwareDialog.cpp
@@ -75,7 +75,7 @@ struct FirmwareDialog::priv
 	AvrDude::Ptr avrdude;
 	std::string avrdude_config;
 	unsigned progress_tasks_done;
-	bool cancel;
+	bool cancelled;
 
 	priv(FirmwareDialog *q) :
 		q(q),
@@ -83,12 +83,13 @@ struct FirmwareDialog::priv
 		btn_flash_label_flashing(_(L("Cancel"))),
 		avrdude_config((fs::path(::Slic3r::resources_dir()) / "avrdude" / "avrdude.conf").string()),
 		progress_tasks_done(0),
-		cancel(false)
+		cancelled(false)
 	{}
 
 	void find_serial_ports();
 	void flashing_status(bool flashing, AvrDudeComplete complete = AC_NONE);
 	void perform_upload();
+	void cancel();
 	void on_avrdude(const wxCommandEvent &evt);
 };
 
@@ -118,7 +119,7 @@ void FirmwareDialog::priv::flashing_status(bool value, AvrDudeComplete complete)
 		progressbar->SetRange(200);   // See progress callback below
 		progressbar->SetValue(0);
 		progress_tasks_done = 0;
-		cancel = false;
+		cancelled = false;
 	} else {
 		auto text_color = wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOWTEXT);
 		port_picker->Enable();
@@ -173,12 +174,11 @@ void FirmwareDialog::priv::perform_upload()
 			evt->SetString(msg);
 			wxQueueEvent(q, evt);
 		}))
-		.on_progress(std::move([this](const char * /* task */, unsigned progress) {
-			auto evt = new wxCommandEvent(EVT_AVRDUDE, this->q->GetId());
+		.on_progress(std::move([q](const char * /* task */, unsigned progress) {
+			auto evt = new wxCommandEvent(EVT_AVRDUDE, q->GetId());
 			evt->SetExtraLong(AE_PRORGESS);
 			evt->SetInt(progress);
-			wxQueueEvent(this->q, evt);
-			return !this->cancel;
+			wxQueueEvent(q, evt);
 		}))
 		.on_complete(std::move([q](int status) {
 			auto evt = new wxCommandEvent(EVT_AVRDUDE, q->GetId());
@@ -189,6 +189,15 @@ void FirmwareDialog::priv::perform_upload()
 		.run();
 }
 
+void FirmwareDialog::priv::cancel()
+{
+	if (avrdude) {
+		cancelled = true;
+		txt_status->SetLabel(_(L("Cancelling...")));
+		avrdude->cancel();
+	}
+}
+
 void FirmwareDialog::priv::on_avrdude(const wxCommandEvent &evt)
 {
 	AvrDudeComplete complete_kind;
@@ -219,7 +228,7 @@ void FirmwareDialog::priv::on_avrdude(const wxCommandEvent &evt)
 	case AE_EXIT:
 		BOOST_LOG_TRIVIAL(info) << "avrdude exit code: " << evt.GetInt();
 
-		complete_kind = cancel ? AC_CANCEL : (evt.GetInt() == 0 ? AC_SUCCESS : AC_FAILURE);
+		complete_kind = cancelled ? AC_CANCEL : (evt.GetInt() == 0 ? AC_SUCCESS : AC_FAILURE);
 		flashing_status(false, complete_kind);
 
 		// Make sure the background thread is collected and the AvrDude object reset
@@ -340,7 +349,7 @@ FirmwareDialog::FirmwareDialog(wxWindow *parent) :
 				_(L("Confirmation")),
 				wxYES_NO | wxNO_DEFAULT | wxICON_QUESTION);
 			if (dlg.ShowModal() == wxID_YES) {
-				this->p->cancel = true;
+				this->p->cancel();
 			}
 		} else {
 			// Start a flashing task