Kévin Redon has uploaded this change for review. ( 
https://gerrit.osmocom.org/12595


Change subject: avoid mutli-packet USB transfer
......................................................................

avoid mutli-packet USB transfer

the control endpoint packet size is 64 bytes for USB full speed.
it is possible to make larger data transfer by transferring
multiple packets which can then by re-assembled.
this feature is also supported by the SAM E54 USB stack, but for
an unknown reason is the transfer size is larger than 64 bytes but
not a multiple of it, the transfer cannot complete (it is not
ACKed).

reducing the transfer size to 64 bytes removes this issue.
the data needs to be re-assembled in software to fill a flash page.

sadly a new issue has been uncovered using this method.
the size of the last packet (e.g. request bLength) is kept by the
USB stack for the next packet. Thus is the last packet is less
than 6 bytes, the status request after the download request fails.

Change-Id: Icb4c5f4bc06095f5f962152b8d8247054ef6a520
---
M usb/class/dfu/device/dfudf.c
M usb/class/dfu/device/dfudf.h
M usb/class/dfu/device/dfudf_desc.h
M usb_start.c
4 files changed, 53 insertions(+), 21 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-asf4-dfu refs/changes/95/12595/1

diff --git a/usb/class/dfu/device/dfudf.c b/usb/class/dfu/device/dfudf.c
index 8232979..545f57c 100644
--- a/usb/class/dfu/device/dfudf.c
+++ b/usb/class/dfu/device/dfudf.c
@@ -42,7 +42,7 @@
 enum usb_dfu_state dfu_state = USB_DFU_STATE_DFU_IDLE;
 enum usb_dfu_status dfu_status = USB_DFU_STATUS_OK;

-uint8_t dfu_download_data[512];
+uint8_t dfu_download_data[64];
 uint16_t dfu_download_length = 0;
 size_t dfu_download_offset = 0;
 bool dfu_manifestation_complete = false;
@@ -244,12 +244,12 @@
                        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
+                               to_return = usbdc_xfer(ep, dfu_download_data, 
req->wLength, req->wLength < 64); // 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
+                               //to_return = usbdc_xfer(ep, NULL, 0, true); // 
send ACK
                                // we let the main application flash the data 
because this can be long and would stall the USB ISR
                        }
                }
diff --git a/usb/class/dfu/device/dfudf.h b/usb/class/dfu/device/dfudf.h
index cee5845..a7143f9 100644
--- a/usb/class/dfu/device/dfudf.h
+++ b/usb/class/dfu/device/dfudf.h
@@ -43,10 +43,9 @@
 extern enum usb_dfu_status dfu_status;

 /** Downloaded data to be programmed in flash
- *
- *  512 is the flash page size of the SAM D5x/E5x
+ *  64 bytes is the control buffer size
  */
-extern uint8_t dfu_download_data[512];
+extern uint8_t dfu_download_data[64];
 /** Length of downloaded data in bytes */
 extern uint16_t dfu_download_length;
 /** Offset of where the downloaded data should be flashed in bytes */
diff --git a/usb/class/dfu/device/dfudf_desc.h 
b/usb/class/dfu/device/dfudf_desc.h
index cd6ba41..68880c7 100644
--- a/usb/class/dfu/device/dfudf_desc.h
+++ b/usb/class/dfu/device/dfudf_desc.h
@@ -75,9 +75,14 @@
                                   CONF_USB_DFUD_BMATTRI, \
                                   CONF_USB_DFUD_BMAXPOWER)

+/** \remark ideally the transfer size should be the same size as flash pages 
(512 byte for SAM E5x/D5x).
+            the control endpoint transfer size is 64 byte for full speed 
device.
+            thus 512 transfers must be split in a mutli-packet transfer.
+            this should be supported by the hardware, but I did not manage to 
have complete/successful multi-packet transfer is the transfer is larger than 
64 bytes but not a multiple of 64
+ */
 #define DFUD_IFACE_DESCB 
USB_DFU_FUNC_DESC_BYTES(USB_DFU_ATTRIBUTES_CAN_DOWNLOAD | 
USB_DFU_ATTRIBUTES_WILL_DETACH, \
                                                     0, /**< detaching makes 
only sense in run-time mode */ \
-                                                    512, /**< transfer size 
corresponds to page size for optimal flash writing */ \
+                                                    64, /**< transfer size 
corresponds to the control endpoint packet size */ \
                                                     0x0110 /**< DFU 
specification version 1.1 used */ )

 #define DFUD_IFACE_DESCES \
diff --git a/usb_start.c b/usb_start.c
index ad91840..487e672 100644
--- a/usb_start.c
+++ b/usb_start.c
@@ -42,8 +42,14 @@
 /** USB DFU functional descriptor (with DFU attributes) */
 static const uint8_t usb_dfu_func_desc_bytes[] = {DFUD_IFACE_DESCB};
 static const usb_dfu_func_desc_t* usb_dfu_func_desc = 
(usb_dfu_func_desc_t*)&usb_dfu_func_desc_bytes;
-/** Ctrl endpoint buffer */
+/** Control endpoint buffer */
 static uint8_t ctrl_buffer[64];
+/** Buffer to store downloaded data before flashing a complete page */
+static uint8_t flash_page[512];
+/** Address offset where the flash page should be flashed */
+static uint32_t flash_offset = 0;
+/** If the page has already been flashed */
+static bool flash_programmed = true;

 /**
  * \brief USB DFU Init
@@ -55,6 +61,11 @@

        usbdc_start(single_desc);
        usbdc_attach();
+
+       // initialise flash page with empty flash
+       for (uint16_t i = 0; i < ARRAY_SIZE(flash_page); i++) {
+               flash_page[i] = 0xff;
+       }
 }

 /**
@@ -91,21 +102,38 @@
                if (USB_DFU_STATE_DFU_DNLOAD_SYNC == dfu_state || 
USB_DFU_STATE_DFU_DNBUSY == dfu_state) { // there is some data to be flashed
                        gpio_set_pin_level(LED_SYSTEM, true); // 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;
+/*
+                               if (dfu_download_offset >= flash_offset + 
sizeof(flash_page)) { // we start a new page
+                                       if (!flash_programmed) { // the 
previous page needs to be flashed
+                                               int32_t rc = 
flash_write(&FLASH_0, application_start_address + flash_offset, flash_page, 
sizeof(flash_page)); // write re-assembled downloaded data chunks to flash
+                                               if (ERR_NONE == rc) {
+                                                       // nothing to do for now
+                                               } 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;
+                                                       }
+                                               }
+                                               // initialise flash page with 
empty flash
+                                               for (uint16_t i = 0; i < 
ARRAY_SIZE(flash_page); i++) {
+                                                       flash_page[i] = 0xff;
+                                               }
+                                               flash_programmed = true; // 
remember we programmed the flash page
                                        }
+                                       flash_offset = (dfu_download_offset / 
sizeof(flash_page)) * sizeof(flash_page); // remember new page offset
                                }
-                       } else { // there was no data to flash
-                               // this case should not happen, but it's not a 
critical error
+                               // copy data from download buffer to our flash 
page
+                               for (uint16_t i = 0; i < dfu_download_length && 
i < ARRAY_SIZE(dfu_download_data) && i + (dfu_download_offset % 
ARRAY_SIZE(flash_page)) < ARRAY_SIZE(flash_page); i++) {
+                                       flash_page[dfu_download_offset % 
ARRAY_SIZE(flash_page) + i] = dfu_download_data[i];
+                               }
+                               flash_programmed = false; // remember there is 
data to be flashed
+*/
+                               dfu_download_length = 0; // remember we copied 
the data (in the next turn the state will be idle again if there was no error 
before)
+                       } else { // there was no data to be copied/flashed
                                dfu_state = USB_DFU_STATE_DFU_DNLOAD_IDLE; // 
indicate flashing can continue
                        }
                        gpio_set_pin_level(LED_SYSTEM, false); // switch LED on 
to indicate USB DFU can resume

--
To view, visit https://gerrit.osmocom.org/12595
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-asf4-dfu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icb4c5f4bc06095f5f962152b8d8247054ef6a520
Gerrit-Change-Number: 12595
Gerrit-PatchSet: 1
Gerrit-Owner: Kévin Redon <kre...@sysmocom.de>

Reply via email to