Add UpdateHub.io support lib

[otavio]

UpdateHub provides an end-to-end solution for Over-The-Air update
of embedded devices.

Signed-off-by: Christian Tavares christian.tavares@ossystems.com.br
Reviewed-by: Otavio Salvador otavio@ossystems.com.br

[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.

[pfalcon]

This looks pretty cool, but I doubt it belongs to subsys/ . Rather, lib/ . I also would suggest to join initiative of #12129 and put headers under include/zephyr/ right away. But don't haste with these changes, until others provided feedback too.

But you definitely would need to act on @zephyrbot's comments ;-).

[pfalcon]

Also, please consider how would we ensure that this works over time. We'd need a sample, tests, etc.

[carlescufi]

Thank you for this contribution!
This looks pretty good already. I agree that with @pfalcon this does not belong directly on subsys/. I think the reasonable options are:

• lib/dfu/updatehub
• lib/updatehub

Also please include a buildable test (a simple application that uses the library) in tests/. This will allow us to include the new code in CI so that we make sure it is not broken in the future.

Finally we would need a bit of documentation in doc/.

[Laczen]

Hi @chtavares592 and @otavio, thanks for this PR. My review is only limited to the use of nvs; as my knowledge on the other (most important part) is limited:
You are using NVS to store the id, in the creation of the nvs filesystem you are using a very short (64 bytes) sector size. The sector size should be a multiple of the flash_erase_page size, so probably 64 is to small. You are also using FLASH_AREA_STORAGE as the offset of the nvs file system, this would make it impossible for other modules to use it.
My advice would be to use the settings module to store the id, this will allow other modules to use the same storage area. If you need advice/help on how to do this, let me know.

[otavio]

@pfalcon

This looks pretty cool, but I doubt it belongs to subsys/ . Rather, lib/ . I also would suggest to join initiative of #12129 and put headers under include/zephyr/ right away. But don't haste with these changes, until others provided feedback too.

We can do the inclusion of the headers on the new location for sure; we also are in process of moving it to lib/updatehub as suggested by @carlescufi.

@Laczen

You are using NVS to store the id, in the creation of the nvs filesystem you are using a very short (64 bytes) sector size. The sector size should be a multiple of the flash_erase_page size, so probably 64 is to small. You are also using FLASH_AREA_STORAGE as the offset of the nvs file system, this would make it impossible for other modules to use it.
My advice would be to use the settings module to store the id, this will allow other modules to use the same storage area. If you need advice/help on how to do this, let me know.

We are looking at this and hope to have something to share soon (or questions, hehe)...

[Laczen]

@Laczen

You are using NVS to store the id, in the creation of the nvs filesystem you are using a very short (64 bytes) sector size. The sector size should be a multiple of the flash_erase_page size, so probably 64 is to small. You are also using FLASH_AREA_STORAGE as the offset of the nvs file system, this would make it impossible for other modules to use it.
My advice would be to use the settings module to store the id, this will allow other modules to use the same storage area. If you need advice/help on how to do this, let me know.

We are looking at this and hope to have something to share soon (or questions, hehe)...

OK, a example on how it is used can be found in:
zephyr/samples/boards/nrf52/mesh/onoff_level_lighting_vnd_app/src/storage.c

[otavio]

We did get the settings integrated by then we found a problem: we lose the information when upgrade is done.

Essentially, what seems to be happening is:

Step 1: updatehub -> set updatehub/ip -> reboot
Step 2: mcuboot -> swap slots -> reboot
Step 3: updatehub -> get updatehub/ip (fail!)

@Laczen any suggestion how to approach this?

[mike-scott]

@otavio Thank you for the PR. Overall I agree with the others in several regards (location in lib/, PR quality is good and the need for a sample / test).

I notice you are using device_identity.c which has some hard-coded values per SoC for reading device unique values. There is a Zephyr-generic solution mostly in place here: https://github.com/zephyrproject-rtos/zephyr/tree/master/drivers/hwinfo And I believe NXP support is nearly approved. You might consider using that API for getting the initial seed for your unique identifier.

RE: the settings values going away after update, make sure the partition sizes aren't overlapping between MCUboot, slot0, slot1, mcuboot's scratch area, the NVS settings, etc. If they do overlap it could explain why those settings go away during OTA.

[Laczen]

I agree with @mike-scott , probably there is some problem with the flash partitions. If you can put your sample somewhere I can have a look (please don't forget to mention your board).

[mike-scott]

@otavio could provide a diff of the previous commit vs. the new one pushed a few days ago?

[dbkinder]

Please review my clarity edits that I didn't introduce a technical error.

[otavio]

@mike-scott

@otavio could provide a diff of the previous commit vs. the new one pushed a few days ago?

I am afraid we can't. We amended the changes and we later dropped those branches. We are now working on fixing the mcuboot image swap issue and we ought to have it ready for a new review.

[jukkar]

LGTM

[jukkar]

ping @d3zd3z @galak are you ok with the PR?

addressed

[otavio]

@galak @d3zd3z do you mind to have a new look at this? We are not aware of any pending change.

[otavio]

Hello everyone. We believe it is ready to go and we addressed all issues. Is it possible for you guys to check it?

stale

[carlescufi]

Looked through this and the only real unresolved question is where the updatehub.h header belongs. Since @nashif is doing a cleanup of headers anyway, I suggest we discuss where to place it after the fact in order to prevent this PR from bitrotting.

It could be in include/dfu or include/mgmt