Implement proper manifestation according to the spec

The initial behavior of entering dfuMANIFEST-SYNC was incorrect, sorry about
that. I've now thoroughly read the DFU spec, which actually states that
dfuMANIFEST-SYNC shall be entered *twice* after a download operation if
bitMainfestationTolerant = 1 (i.e. DFU_WILL_DETACH == 0), once after the
download completes, which begins manifestation by transitioning to dfuMANIFEST,
and again after manifestation has completed successfully, and only after that
should the transition to dfuIDLE be made. This means that there should be *two*
DFU_GETSTATUS calls from the host, and indeed dfu-util does exactly this
(verified using Wireshark).

Manifesting on USB reset or on detach is not according to spec. It should only
happen right after a download operation has finished. For this I've overhauled
the dfu_on_manifest_request() function to work as it should with both
DFU_WILL_DETACH == 1 and DFU_WILL_DETACH == 0. Validation should actually be
part of manifestation, and I've now added it as the manifestation callback
(since there is no other manifestation work to be done). The scb_reset_system()
call in the USB reset callback is what you need to perform detach-by-USB-reset,
but to avoid having random reset calls in multiple places it makes much more
sense to call the actual dfu_on_detach_complete() callback which will then
perform the reset. The USB reset callback should also not validate, e.g. a
blank flash with just the bootloader should not trigger a firmware error
status, so I've removed that logic.

Author: twelho

src/dapboot.c

@@ -32,10 +32,7 @@ static inline void __set_MSP(uint32_t topOfMainStack) {
 }
 
 bool validate_application(void) {
-    if (((uint32_t)(APP_INITIAL_STACK) & 0x2FFE0000) == 0x20000000) {
-        return true;
-    }
-    return false;
+    return ((uint32_t)(APP_INITIAL_STACK) & 0x2FFE0000) == 0x20000000;
 }
 
 static void jump_to_application(void) __attribute__ ((noreturn));
@@ -75,7 +72,7 @@ int main(void) {
         }
 
         usbd_device* usbd_dev = usb_setup();
-        dfu_setup(usbd_dev, target_manifest_app, NULL, NULL);
+        dfu_setup(usbd_dev, validate_application, NULL, NULL);
         webusb_setup(usbd_dev);
         winusb_setup(usbd_dev);
 

src/dfu.c

@@ -51,13 +51,13 @@ static enum dfu_state current_dfu_state;
 static enum dfu_status current_dfu_status;
 static size_t current_dfu_offset;
 
-static bool dfu_modified_firmware = false;
+static bool manifestation_complete = false;
 
 static uint8_t dfu_download_buffer[USB_CONTROL_BUF_SIZE];
 static size_t dfu_download_size;
 
 /* User callbacks */
-static GenericCallback dfu_manifest_request_callback = NULL;
+static ManifestationCallback dfu_manifest_request_callback = NULL;
 static StateChangeCallback dfu_state_change_callback = NULL;
 static StatusChangeCallback dfu_status_change_callback = NULL;
 
@@ -133,9 +133,6 @@ static void dfu_on_download_request(usbd_device* usbd_dev, struct usb_setup_data
     bool ok = target_flash_program_array(dest, data, dfu_download_size/2);
     target_flash_lock();
 
-    /* Record that we touched the application firmware */
-    dfu_modified_firmware = true;
-
     if (ok) {
         current_dfu_offset += dfu_download_size;
         /* We could go back to STATE_DFU_DNLOAD_SYNC, but then
@@ -151,12 +148,22 @@ static void dfu_on_manifest_request(usbd_device* usbd_dev, struct usb_setup_data
     (void)req;
 
     if (dfu_manifest_request_callback) {
-        dfu_manifest_request_callback();
+        /* The manifestation callback returns a boolean indicating if it succeeded */
+        if (dfu_manifest_request_callback()) {
+            manifestation_complete = true;
+            dfu_set_state(STATE_DFU_MANIFEST_SYNC);
+        } else {
+            dfu_set_status(DFU_STATUS_ERR_FIRMWARE);
+            return; /* Avoid resetting on error */
+        }
     }
 
-    /* Reset and maybe launch the application if the manifest callback
-       didn't already do it */
-    scb_reset_system();
+#if DFU_WILL_DETACH
+    /* DFU_WILL_DETACH being enabled equates to transitioning to the dfuMANIFEST-WAIT-RESET state,
+     * which combined with bitWillDetach being set with DFU_WILL_DETACH means that the device should
+     * generate a detach-attach sequence and enter the application, i.e. reset itself, here */
+    dfu_on_detach_complete(NULL, NULL);
+#endif
 }
 
 static enum usbd_request_return_codes
@@ -190,17 +197,18 @@ dfu_control_class_request(usbd_device *usbd_dev,
                     break;
                 }
                 case STATE_DFU_MANIFEST_SYNC: {
-                    if (validate_application()) {
-#if DFU_WILL_DETACH
-                        /* Manifest by resetting after responding */
-                        dfu_set_state(STATE_DFU_MANIFEST);
-                        *complete = &dfu_on_manifest_request;
-#else
-                        /* Return to the idle state and await commands */
+                    /* According to the DFU spec the dfuMANIFEST-SYNC state is entered twice,
+                     * once after the download completes, and again after manifestation if
+                     * the device is manifestation tolerant (DFU_WILL_DETACH == 0) */
+                    if (manifestation_complete) {
+                        /* Only enter idle state after manifestation has completed successfully */
+                        manifestation_complete = false;
                         dfu_set_state(STATE_DFU_IDLE);
-#endif
                     } else {
-                        dfu_set_status(DFU_STATUS_ERR_FIRMWARE);
+                        /* Perform manifestation after download as described in the
+                         * spec regardless of if DFU_WILL_DETACH is enabled or not */
+                        dfu_set_state(STATE_DFU_MANIFEST);
+                        *complete = &dfu_on_manifest_request;
                     }
                     break;
                 }
@@ -317,11 +325,7 @@ dfu_control_class_request(usbd_device *usbd_dev,
 #endif
 
         case DFU_DETACH: {
-            if (dfu_modified_firmware) {
-                *complete = &dfu_on_manifest_request;
-            } else {
-                *complete = &dfu_on_detach_complete;
-            }
+            *complete = &dfu_on_detach_complete;
             status = USBD_REQ_HANDLED;
             break;
         }
@@ -355,36 +359,20 @@ static void dfu_set_config(usbd_device* usbd_dev, uint16_t wValue) {
 }
 
 static void dfu_on_usb_reset(void) {
-    /* Ignore the first USB reset after boot, before anyone has had
-       a chance to do anything to the device; otherwise we'd never
-       enter DFU mode when the application is valid */
+    /* Ignore all USB resets until the DFU control callback has been registered, since
+     * reset callback will fire once as the USB connection is established. Without this
+     * the target enters a reset loop when trying to enter the bootloader. */
     if (!dfu_enumerated) {
         return;
     }
 
-    /* On subsequent USB reset requests, either reset/manifest
-       or enter the error state if firmware is invalid, per the spec */
-    if (validate_application()) {
-        if (dfu_modified_firmware) {
-            /* Manifest by resetting after responding */
-            dfu_set_state(STATE_DFU_MANIFEST);
-
-            if (dfu_manifest_request_callback) {
-                dfu_manifest_request_callback();
-            }
-        }
-
-        /* Reset and launch the application if the manifest callback
-           didn't already do it */
-        scb_reset_system();
-    } else {
-        /* Enter the error state and await further commands */
-        dfu_set_status(DFU_STATUS_ERR_FIRMWARE);
-    }
+    /* Perform a DFU detach (which resets the target), this enables issuing a USB bus
+     * reset as an alternative means to submitting a DFU_DETACH command post-download. */
+    dfu_on_detach_complete(NULL, NULL);
 }
 
 void dfu_setup(usbd_device* usbd_dev,
-               GenericCallback on_manifest_request,
+               ManifestationCallback on_manifest_request,
                StateChangeCallback on_state_change,
                StatusChangeCallback on_status_change) {
     dfu_manifest_request_callback = on_manifest_request;

src/dfu.h

@@ -29,12 +29,12 @@
 
 extern const struct usb_dfu_descriptor dfu_function;
 
-typedef void (*GenericCallback)(void);
+typedef bool (*ManifestationCallback)(void);
 typedef void (*StateChangeCallback)(enum dfu_state);
 typedef void (*StatusChangeCallback)(enum dfu_status);
 
 extern void dfu_setup(usbd_device* usbd_dev,
-                      GenericCallback on_manifest_request,
+                      ManifestationCallback on_manifest_request,
                       StateChangeCallback on_state_change,
                       StatusChangeCallback on_status_change);