DFU improvements

[HiFiPhile]

Describe the PR

• Add alt settings support in DFU class, in order to flash multiples partitions (FLASH1, FLASH2, EEPROM, etc.)
• Fix inverted ATTR_MANIFESTATION_TOLERANT logic.

[HiFiPhile]

@xmos-jmccarthy could you take a look if it ok ?

Maybe additional buffer check is needed if transfer size is different for each alt settings.

[xmos-jmccarthy]

Good catch on the manifest bit logic.

For the alt setting, can you add an explicit initialized value in void dfu_moded_init(void) and the reset value in dfu_moded_reset.

[xmos-jmccarthy]

Maybe additional buffer check is needed if transfer size is different for each alt settings.

CFG_TUD_DFU_TRANSFER_BUFFER_SIZE will have to be set to the largest buffer size used by an any given storage type. The recently added checks to prevent out of bounds memory accesses should be sufficient to catch these cases and alert the user if they make a mistake, but if you could add a note of that to the API header that would be great.

[HiFiPhile]

For the alt setting, can you add an explicit initialized value in void dfu_moded_init(void) and the reset value in dfu_moded_reset.

you could add a note of that to the API header that would be great.

It's done in cf4220a

[hathach]

Thank for the PR, using alt interface as partition doesn't seem to be standard for USB DFU but it is popular with dfu-util. Adding Alt to callback is a good option. Though we need to keep an array of attributes and make sure it is multiple ALTs and not multiple instances of DFUs.

[HiFiPhile]

using alt interface as partition doesn't seem to be standard for USB DFU

In fact it's in the spec:

* Alternate settings can be used by an application to access additional memory segments.  In this case, 
it is suggested that each alternate setting employ a string descriptor to indicate the target memory 
segment;  e.g., “EEPROM”.  Details concerning other possible uses of alternate settings are beyond the 
scope of this document.  However, their use is intentionally not restricted because the authors 
anticipate that implementers will devise additional creative uses for alternate settings.   

Something useful but out of spec is add DFU_DETACH support in DFU mode, It's workaround for WinUSB who can't issue a Bus reset.
If there is only one partition MCU can reboot once flash is finished, but if there are multiple partitions MCU don't know when to reboot.

we need to check to make sure the ALT is consecutive to make sure it is not 2 instances of DFU interface

And I think we can limit support to one interface only, multiple DFU interfaces doesn't make sense I think ?

---

When alt intf is switched, do you think we should reset state and status to default ?

[hathach]

In fact it's in the spec:

* Alternate settings can be used by an application to access additional memory segments.  In this case, 
it is suggested that each alternate setting employ a string descriptor to indicate the target memory 
segment;  e.g., “EEPROM”.  Details concerning other possible uses of alternate settings are beyond the 
scope of this document.  However, their use is intentionally not restricted because the authors 
anticipate that implementers will devise additional creative uses for alternate settings.   

Ah right, I totally missed that, thanks for the update.

And I think we can limit support to one interface only, multiple DFU interfaces doesn't make sense I think ?

Yeah, we did limit the dfu to only once interface, since there is not itf in the callback arguments. However, we still need to check the integrity of the Alt interface with consecutive number.

When alt intf is switched, do you think we should reset state and status to default ?

We probably should, I am not entirely sure. Just make sure you have a bit of explanation comment. We could update it later on when needed.

[HiFiPhile]

dfu-util doesn't take care wTransferSize of Alt settings, I've opened a ticket about this.

[HiFiPhile]

In fact I missed out that only one functional descriptor is used in DFU class. (Thanks to dfu-util maintainer)

After the host and device agree to perform DFU operations, the host re-enumerates the device.  It is at 
this time that the device exports the DFU descriptor set, which contains: 
•  A DFU device descriptor  
•  A single configuration descriptor 
•  A single interface descriptor (including descriptors for alternate settings, if present) 
•  A single functional descriptor 

I think in this case we need a refactor which expose only one time parameters in functional descriptor. Some VA_ARGS trick like TUD_BTH_DESCRIPTOR ?

[hathach]

In fact I missed out that only one functional descriptor is used in DFU class. (Thanks to dfu-util maintainer)

Indeed, I totally missed this one as well, I just checked the specs, it will be only 1 Functional DFU descriptor at the end after the series of the Alt Interface

ALT 0
ATL 1

ATL n
DFU Functional

I think in this case we need a refactor which expose only one time parameters in functional descriptor. Some VA_ARGS trick like TUD_BTH_DESCRIPTOR ?

we probably needs something similar, but I hope we could get a much simpler version.

[HiFiPhile]

@HiFiPhile oh, I see, device need to response to get status first before performing the flashing. In that case, It make sense

Yes, it's exactly the case.

[hathach]

@HiFiPhile oh, I see, device need to response to get status first before performing the flashing. In that case, It make sense

Yes, it's exactly the case.

I am doing more rename and update to DFU, will push when complete.

[hathach]

@HiFiPhile it need a bit more work, the get_status should allow application to report error such as incorrect address (such error can be from incorrect download file from host). I will do a bit more update for that

[HiFiPhile]

it need a bit more work, the get_status should allow application to report error such as incorrect address (such error can be from incorrect download file from host).

Ok, errAddress could be useful.

[hathach]

@HiFiPhile @xmos-jmccarthy I have updated with lots of rename (minor) and also tweak behavior as well as flow for DFU callback . I also migrated away from state machine since I found it a bit more complicated (left in code for review only). In sum up

• get_timeout() will be invoked (by GET_STATUS) before download_cb() and manifest_cb(). Since either has potential flashing op.
• finish_flashing(status) ( renamed from download_complete() ) is called from application after download/manifest cb() when flashing is complete. Should the address is out of bound, or flash verification failed, status can be set to reflect the error. DFU will then report that to host and enter dfuError state.
• manifest_cb() replace both firmware_valid_check_cb() / device_data_done_check_cb(). It can be used to flash (if buffered whole image so far) or perform checksum and still can report error.

Please let me know what do you think, I am open to all suggestion.

[HiFiPhile]

I think it's in good form with a quick look, will do some real world tests.

[xmos-jmccarthy]

I like the callback changes, this is much nicer to look at.

I will update my port and run some tests on hardware. I should be able to do that next week at the latest.

[hathach]

I will also integrate this to tinyuf2 project as well to see if there is any issue with existing API https://github.com/adafruit/tinyuf2

[HiFiPhile]

I've done some tests on LPC4337, so far so good.

ps: @hathach Do you use any auto format script ? Since my format is completely different and I always miss something with manual format...

[hathach]

@HiFiPhile I only use auto format with portion of text selection where I want to. Normally I will just manual type it. Auto format is not enabled, since I couldn't figure out how to get it auto align =. Also it is a bit too strict, as long as the code look good, it is ok to go over line boundary (which I set to 120) or break rule. It is not too important, don't worry too much about it.

[hathach]

Thank you @HiFiPhile for stating this PRs, and @xmos-jmccarthy for helping with the review. This PR is now ready, and I am glad that you are both happy with the changes. There is quite a bit of rename though, but it is to make it more like other class. If there is nothing else that need changes, I wink we could merge this. Any issues can be fixed as follow up PRs.

[xmos-jmccarthy]

Everything looks good.

[HiFiPhile]

It's good for me :)