lynxis lazus has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-asf4-dfu/+/42174?usp=email )
Change subject: dfu: rewrite firmware downloading ......................................................................
dfu: rewrite firmware downloading
Improve handling of dfu_state by moving more state changing towards the IRQ handler. Having both the main loop and IRQ changes dfu_state within the same state could lead to races.
The main loop is now only a simple worker which reports back via dfu_flash_done & dfu_flash_status.
Change-Id: I345d5948455b25cd8a2efb1abfd9d0986ebd8cef --- M usb/class/dfu/device/dfudf.c M usb/class/dfu/device/dfudf.h M usb_start.c 3 files changed, 78 insertions(+), 34 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-asf4-dfu refs/changes/74/42174/1
diff --git a/usb/class/dfu/device/dfudf.c b/usb/class/dfu/device/dfudf.c index c66454b..0f5ec0e 100644 --- a/usb/class/dfu/device/dfudf.c +++ b/usb/class/dfu/device/dfudf.c @@ -42,9 +42,15 @@ volatile enum usb_dfu_state dfu_state = USB_DFU_STATE_DFU_IDLE; volatile enum usb_dfu_status dfu_status = USB_DFU_STATUS_OK;
+/* flashed the last given block */ uint8_t dfu_download_data[512]; -uint16_t dfu_download_length = 0; -size_t dfu_download_offset = 0; +volatile uint16_t dfu_download_length = 0; +volatile size_t dfu_download_offset = 0; + +/* only when flash done is true, flash rc is valid */ +volatile bool dfu_flash_done = false; +volatile enum usb_dfu_status dfu_flash_status = USB_DFU_STATUS_ERR_UNKNOWN; + bool dfu_manifestation_complete = false;
/** @@ -159,10 +165,20 @@ dfu_state = USB_DFU_STATE_DFU_ERROR; // unsupported class request to_return = ERR_UNSUPPORTED_OP; // stall control pipe (don't reply to the request) break; - case USB_DFU_GETSTATUS: // get status + case USB_DFU_GETSTATUS: switch (dfu_state) { - case USB_DFU_STATE_DFU_DNLOAD_SYNC: // download has not completed - dfu_state = USB_DFU_STATE_DFU_DNBUSY; // switch to busy state + case USB_DFU_STATE_DFU_DNLOAD_SYNC: + case USB_DFU_STATE_DFU_DNBUSY: + if (!dfu_flash_done) { + dfu_state = USB_DFU_STATE_DFU_DNBUSY; + break; + } + + dfu_status = dfu_flash_status; + if (dfu_status == USB_DFU_STATUS_OK) + dfu_state = USB_DFU_STATE_DFU_DNLOAD_IDLE; + else + dfu_state = USB_DFU_STATE_DFU_ERROR; break; case USB_DFU_STATE_DFU_MANIFEST_SYNC: if (!dfu_manifestation_complete) { @@ -230,36 +246,49 @@ if (!(usb_dfu_func_desc->bmAttributes & USB_REQ_DFU_DNLOAD)) { // download is not enabled dfu_state = USB_DFU_STATE_DFU_ERROR; // unsupported class request to_return = ERR_UNSUPPORTED_OP; // stall control pipe (don't reply to the request) + break; } else if (USB_DFU_STATE_DFU_IDLE != dfu_state && USB_DFU_STATE_DFU_DNLOAD_IDLE != dfu_state) { // wrong state to request download // warn about programming error dfu_status = USB_DFU_STATUS_ERR_PROG; dfu_state = USB_DFU_STATE_DFU_ERROR; to_return = ERR_INVALID_ARG; // stall control pipe to indicate error + break; } else if (USB_DFU_STATE_DFU_IDLE == dfu_state && (0 == req->wLength)) { // download request should not start empty // warn about programming error dfu_status = USB_DFU_STATUS_ERR_PROG; dfu_state = USB_DFU_STATE_DFU_ERROR; to_return = ERR_INVALID_ARG; // stall control pipe to indicate error + break; } else if (USB_DFU_STATE_DFU_DNLOAD_IDLE == dfu_state && (0 == req->wLength)) { // download completed dfu_manifestation_complete = false; // clear manifestation status + to_return = usbdc_xfer(ep, NULL, 0, false); // send ack to the setup request to get the data dfu_state = USB_DFU_STATE_DFU_MANIFEST_SYNC; // prepare for manifestation phase - to_return = usbdc_xfer(ep, NULL, 0, false); // send ACK + break; } else if (req->wLength > sizeof(dfu_download_data)) { // there is more data to be flash then our buffer (the USB control buffer size should be less or equal) // warn about programming error dfu_status = USB_DFU_STATUS_ERR_PROG; dfu_state = USB_DFU_STATE_DFU_ERROR; to_return = ERR_INVALID_ARG; // stall control pipe to indicate error - } else { // there is data to be flash - if (USB_SETUP_STAGE == stage) { // there will be data to be flash - to_return = usbdc_xfer(ep, dfu_download_data, req->wLength, false); // send ack to the setup request to get the data - } else { // now there is data to be flashed - dfu_download_offset = req->wValue * sizeof(dfu_download_data); // remember which block to flash - dfu_download_length = req->wLength; // remember the data size to be flash - dfu_state = USB_DFU_STATE_DFU_DNLOAD_SYNC; // go to sync state - to_return = usbdc_xfer(ep, NULL, 0, false); // ACK the data - // we let the main application flash the data because this can be long and would stall the USB ISR - } + break; } + + /* The error cases are handled */ + if (USB_SETUP_STAGE == stage) { // there will be data to be flash + to_return = usbdc_xfer(ep, dfu_download_data, req->wLength, false); // send ack to the setup request to get the data + break; + } + + // now there is data to be flashed + if (USB_DFU_STATE_DFU_IDLE == dfu_state) { + /* first packet */ + dfu_download_offset = 0; + } + + /* main loop will increment offset after flashing */ + dfu_download_length = req->wLength; + dfu_state = USB_DFU_STATE_DFU_DNLOAD_SYNC; + to_return = usbdc_xfer(ep, NULL, 0, false); + dfu_flash_done = false; break; default: // all other DFU class OUT request dfu_state = USB_DFU_STATE_DFU_ERROR; // unknown class request diff --git a/usb/class/dfu/device/dfudf.h b/usb/class/dfu/device/dfudf.h index 818252d..b9db46f 100644 --- a/usb/class/dfu/device/dfudf.h +++ b/usb/class/dfu/device/dfudf.h @@ -48,9 +48,14 @@ */ extern uint8_t dfu_download_data[512]; /** Length of downloaded data in bytes */ -extern uint16_t dfu_download_length; +extern volatile uint16_t dfu_download_length; /** Offset of where the downloaded data should be flashed in bytes */ -extern size_t dfu_download_offset; +extern volatile size_t dfu_download_offset; + +/** when flash done is true, flash status is valid */ +extern volatile bool dfu_flash_done; +/** Result of the last blocked flashed. */ +extern volatile enum usb_dfu_status dfu_flash_status; /** If manifestation (firmware flash and check) is complete */ extern bool dfu_manifestation_complete;
diff --git a/usb_start.c b/usb_start.c index 78711f1..bf06596 100644 --- a/usb_start.c +++ b/usb_start.c @@ -164,25 +164,35 @@ // run the second part of the USB DFU state machine handling non-USB aspects switch (last_dfu_state) { case USB_DFU_STATE_DFU_DNLOAD_SYNC: - case USB_DFU_STATE_DFU_DNBUSY: // there is some data to be flashed + case USB_DFU_STATE_DFU_DNBUSY: + if (dfu_flash_done) + break; + + /* FIXME: check if dfu_download_offset / length are valid */ + LED_SYSTEM_off(); // switch LED off to indicate we are flashing if (dfu_download_length > 0) { // there is some data to be flashed - int32_t rc = flash_write(&FLASH_0, application_start_address + dfu_download_offset, dfu_download_data, dfu_download_length); // write downloaded data chunk to flash - if (ERR_NONE == rc) { - dfu_state = USB_DFU_STATE_DFU_DNLOAD_IDLE; // indicate flashing this block has been completed - } else { // there has been a programming error - dfu_state = USB_DFU_STATE_DFU_ERROR; - if (ERR_BAD_ADDRESS == rc) { - dfu_status = USB_DFU_STATUS_ERR_ADDRESS; - } else if (ERR_DENIED == rc) { - dfu_status = USB_DFU_STATUS_ERR_WRITE; - } else { - dfu_status = USB_DFU_STATUS_ERR_PROG; - } + int32_t rc = flash_write(&FLASH_0, + application_start_address + dfu_download_offset, + dfu_download_data, dfu_download_length); // write downloaded data chunk to flash + CRITICAL_SECTION_ENTER(); + switch (rc) { + case ERR_NONE: + dfu_flash_status = USB_DFU_STATUS_OK; + dfu_download_offset += dfu_download_length; + break; + case ERR_BAD_ADDRESS: + dfu_flash_status = USB_DFU_STATUS_ERR_ADDRESS; + break; + case ERR_DENIED: + dfu_flash_status = USB_DFU_STATUS_ERR_WRITE; + break; + default: + dfu_flash_status = USB_DFU_STATUS_ERR_PROG; + break; } - } else { // there was no data to flash - // this case should not happen, but it's not a critical error - dfu_state = USB_DFU_STATE_DFU_DNLOAD_IDLE; // indicate flashing can continue + dfu_flash_done = true; + CRITICAL_SECTION_LEAVE(); } LED_SYSTEM_on(); // switch LED on to indicate USB DFU can resume break;