DFU: progresive image erase

[nvlsianpu]

Patch adds option for progressive erase of firmware image.
Using this flash is erased as necessary when receiving
new firmware, instead of erasing the whole image slot at once.
This is necessary on some hardware that has long erase times,
to prevent long wait times at the beginning of the DFU process.

usb/class/usb_dfu: support progressive image erase

Disable bulk slot image erase when progressive erase is on.
Erase of image bank is performed by image collection procedure
progressively.

For some SoC like nRF52840 bulk erase take too much time, which
cause timeout on tool on existing system out-of-the-box (dfu-util
or just by downloading package from sourceforge for Windows).

resolves #8734

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

[codecov-io]

Codecov Report

Merging #14662 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #14662      +/-   ##
==========================================
- Coverage   52.51%   52.49%   -0.02%     
==========================================
  Files         309      309              
  Lines       45048    45060      +12     
  Branches    10419    10431      +12     
==========================================
  Hits        23656    23656              
- Misses      16584    16592       +8     
- Partials     4808     4812       +4

Impacted Files	Coverage Δ
...s/bluetooth/controller/ll_sw/nordic/lll/lll_scan.c	54.57% <0%> (-0.96%)	⬇️
subsys/bluetooth/controller/ll_sw/ctrl.c	54.35% <0%> (-0.08%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 373a42b...d906898. Read the comment docs.

[Qbicz]

it could be a single multi-line comment

[nvlsianpu]

@Qbicz I spotted that the last erase (while flushing the buffer) has the bug you showed me. Will fix this.

[nvlsianpu]

@Qbicz it is fixed. Will squash once you tested it.

[Qbicz]

Hi @nvlsianpu, I tested the progressive image erase and it works. I was able to perform USB DFU on nRF52840 even with the dfu-util with 5 second timeout.

[Qbicz]

Please change commit message (last paragraph) to the following:
For some SoC like nRF52840 bulk erase take more than 5 seconds, which cause timeout on existing versions of dfu-util.

[Qbicz]

For me this PR can be merged, only the minor changes need to be done + rebase on master.

[nvlsianpu]

@Qbicz revisit

[Qbicz]

Suggested change

	* If progressive erase is enabled, then erase take place while
	* If progressive erase is enabled, erase takes place during

[Qbicz]

Suggested change

	* image collection, so not erase whole bank at DFU beginning
	* image collection, in order not to erase whole bank at DFU beginning

[Qbicz]

@finikorg @jfischer-phytec-iot can you review?

[tormodvolden]

If this is meant to solve issues with dfu-util, it seems like the wrong fix or at the best a complicated workaround. When performing long operations like a bulk erase, the device should signal to the host (dfu-util) the time before it should resume polling the device, through the bwPollTimeout field. Long operations are only to be initiated after the host has requested the status through the DFU_GETSTATUS request (following the DFU_DNLOAD request that typically encodes an erase operation), whose reply includes the poll timeout value. Please see the DFU 1.1 specification at https://www.usb.org/sites/default/files/DFU_1.1.pdf

There is a 5 second timeout value in dfu-util for the DFU request USB transaction, but any long operation should not hold up this transaction.

BTW, there is also a 5 second poll timeout default value in dfu-util, but it is only applied as a last resort for known, broken devices that doesn't fill in the bwPollTimeout field correctly.

[tormodvolden]

Looking at your code, here is what you will need to fix:
https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/usb/class/usb_dfu.c#L396

You have mistakenly hardcoded the bwPollTimeout to 256 ms. This value should be set dynamically, according to the time the device will need for the previously requested operation, e.g. how long it will be unresponsive after a bulk erase. It will typically be set differently for read/write operations and RAM/flash.

[jfischer-no]

@tormodvolden thanks for the hint

You have mistakenly hardcoded the bwPollTimeout to 256 ms. This value should be set dynamically, according to the time the device will need for the previously requested operation, e.g. how long it will be unresponsive after a bulk erase. It will typically be set differently for read/write operations and RAM/flash.

I will change it.

[nvlsianpu]

@tormodvolden Thanks for investigation and explanation.
I will keep this PR main content (progresive erase) as it was requested for other adoption as well.
Question is: should I remove it dfu class commit?

[tormodvolden]

At least the commit messages should be changed IMO, there is no problem with long erase times in the beginning of a DFU process, now that the device implements the bwPollTimeout correctly. So that part of the motivation for this pull request goes out. And no other reason is mentioned :)

[Qbicz]

While the real fix for #8734 is #15330, I would still like to merge this PR, of course with changed commit message.
Progressive image erase has some benefits:

• it prevents user from waiting long time before update process is started.
• according to @nvlsianpu in bulk erase whole image slot is erased. For progressive erase only the pages that need to be erased

Moreover, this is an optional feature. I would be happy to have it available to improve user experience.

[Qbicz]

Can we change first commit message to

Patch adds option for progressive erase of firmware image.
When using this, flash is erased as necessary when receiving
new firmware, instead of erasing the whole image slot at once.
This is useful on some hardware (like nRF52840) that has
long erase times, to prevent long wait times at the beginning
of the DFU process.

And the second commit message to

Disable bulk slot image erase when progressive erase is on.
Erase of image bank is performed by image collection procedure
progressively.

i.e. remove information about this fixing an issue with dfu-util.

[nvlsianpu]

@Qbicz Thanks a lot for your comment. Of course I will rehearsed messages.

[tormodvolden]

IMO, whether dfu-util or not, "useful [...] to prevent long wait times" isn't a much of a technical motivation though. Or is it just for the user experience that the DFU process seems to be hold up with no visible progress during some seconds?

OTOH, what I see as a huge advantage here (if I understand correctly) is that you can download to parts of the flash without erasing other parts.

[nvlsianpu]

@tormodvolden
Progressive erase feature is required by some applications we have to build (is not necessary USB DFU, just better User experience) class. It is optional and disabled by default.

[nvlsianpu]

@tormodvolden @jfischer-phytec-iot commit message updated.

[jfischer-no]

@carlescufi @nvlsianpu @Qbicz I will revert ea177e7, as it was not thought to end and does not work properly. So this should be merged asap and backported to 1.14.