sys/riotboot/flashwrite: display progress bar during copy/download

[aabadie]

Contribution description

This PR makes use of the progress bar introduced in #12550 to nicely follow the copy/download of a firmware on flash.
The logic for updating the progress bar is added in riotboot/flashwrite submodule. The API had to be updated to add a firmware_size argument. There might be a better way, so suggestions are welcome.

The suit module is adapted to retrieve the firmware size from the manifest and pass it to the flashwrite module.

In the riotboot_flashwrite, the max uint32_t value is used for the firmware_size. It is used as a fallback when the firmware_size is not known in advance.

Testing procedure

Run the examples/suit_update testing procedure. The progress bar is displayed during the update, the automatic test works.

Issues/PRs references

Depends on #12550

[kaspar030]

I'm not happy with integrating the progress bar into the flashwrite module.

The suit module is adapted to retrieve the firmware size from the manifest and pass it to the flashwrite module.

A much less intrusive option for getting the progress bar into suit would be:

1. as soon as the image size is known, store it in sth like static size_t _imgsize;, as global in suit/coap.c
2. replace the DEBUG in suit_flashwrite_helper() with the appropriate progress_bar_print(prefix, suffix, (offset + len) * 100 / _imgsize) call.

[kaspar030]

I'm not happy with integrating the progress bar into the flashwrite module.

riotboot_flashwrite is a quite low-level module. At the moment, it is quite verbose through LOG_INFO, but in general, its caller has all the information on how, if and when to display a progress bar. flashwrite can only globally enable it.

[aabadie]

I've reworked this PR following your suggestion @kaspar030. The problematic part now is how to share the image size between the handler code and the coap code in suit (it's using an ugly extern now). Maybe a better solution is to define the manifest static in coap.c ?
What do you think ?

[aabadie]

I added the change to move the manifest as a static variable in 20ccbc6. I think it's a little better.

[kaspar030]

pls rebase!

[fjmolinas]

I have a suggestion that also allows the code to be the same as before if MODULE_PROGRESS_BAR is not included.

[fjmolinas]

I don't know why part of my comment got lost:

With the proposed change we only allocate a mainfest_t* on .bss and not the whole manifest which is not needed. You only really need the pointer to access the data you need.

The ptr can be set to NULL of the end of the function and the progress_bar code can check against it being defined.

• current

   text	   data	    bss	    dec	    hex	filename
  71784	    372	  24404	  96560	  17930	/home/francisco/workspace/RIOT-test/examples/suit_update/bin/iotlab-m3/suit_update.elf

• proposed

   text	   data	    bss	    dec	    hex	filename
  71784	    372	  23776	  95932	  176bc	/home/francisco/workspace/RIOT-test/examples/suit_update/bin/iotlab-m3/suit_update.elf

[aabadie]

The ptr can be set to NULL of the end of the function and the progress_bar code can check against it being defined.

I don't understand which function you are talking about here.

Otherwise, I think I mostly addressed your other comments.

[fjmolinas]

The ptr can be set to NULL of the end of the function and the progress_bar code can check against it being defined.

I don't understand which function you are talking about here.

At the end of _suit_handle_url(), the memmory address where manfiest_ptr points to might be occupied by something else after the function exits. I think it should probably be set to NULL and in:
_print_download_progress verify the pointer is not NULL, maybe I'm worried for nothing?

[aabadie]

At the end of _suit_handle_url(), the memmory address where manfiest_ptr points to might be occupied by something else after the function exits. I think it should probably be set to NULL and in:
_print_download_progress verify the pointer is not NULL, maybe I'm worried for nothing?

I think your whole suggestion is dangerous because as you says the pointer to the manifest could be invalid when the function exits. _print_download_progress is called many times during the download, so it's important to ensure the manifest variable is not lost. Maybe it should be static ?

[fjmolinas]

As discussed offline with @aabadie I think the new state is the best approach. But it does change suit_flashwrite_helper() since it know receives a pointer to the manifest. The function seems specific enough to suit code for this not to mater. Is the helper was somehow needed but something else it could be redefined to flashwrite_helper and suit_flashwrite_helper wrapped around it to pass a pointer to the writer and not the manifest.

@kaspar030 do you agree with the latest approach?

[fjmolinas]

I removed the ready for build for now, to not waste CO2.

[aabadie]

@kaspar030 ?

[aabadie]

@kaspar030 @fjmolinas, are you ok with the current state of this PR ? May I squash/rebase ?

[fjmolinas]

ACK, @kaspar030 does your change request still stand?

EDIT: @aabadie please squash

comments addressed

[aabadie]

Thanks @kaspar030, @fjmolinas. This PR is now squashed and rebased.