Assets OTA #2668

avtolstoy · 2023-07-11T10:28:40Z

Description

Assets OTA

TODO

Automated test for assets OTA
system describe communication unit tests are broken due to migration from JSON to protobuf

Steps to Test

P2 / Photon 2

Update device to latest available Device OS (5.4.1)
1.1. The easiest is device-os-flash --all-devices 5.4.1
Build/flash bootloader
2.2. (bootloader) $ make PLATFORM=p2 COMPILE_LTO=n clean all
2.3. Flash bootloader device-os-flash --all-devices $DEVICE_OS_DIR/build/target/bootloader/platform-32-m/bootloader.bin
Build/flash Device OS and application in whichever convenient way (e.g. Workbench with deviceOS@source)

Gen 3 (nRF52840 platforms)

Update device to latest available Device OS (5.4.1)
1.1. The easiest is device-os-flash --all-devices 5.4.1
Build/flash bootloader
2.2. (bootloader) $ make PLATFORM=boron COMPILE_LTO=n clean all
2.3. Flash bootloader device-os-flash --all-devices $DEVICE_OS_DIR/build/target/bootloader/platform-13-m/bootloader.bin
Build/flash Device OS and application in whichever convenient way (e.g. Workbench with deviceOS@source)

Example App

See app/assets.

References

N/A

Completeness

User is totes amazing for contributing!
Contributor has signed CLA (Info here)
Problem and Solution clearly stated
Run unit/integration/application tests on device
Added documentation
Added to CHANGELOG.md after merging (add links to docs and issues)

monkbroc · 2023-07-26T20:16:55Z

modules/shared/nRF52840/part1_build.mk

@@ -49,6 +49,11 @@ ASFLAGS += -I$(COMMON_BUILD)/arm/startup
 ASFLAGS +=  -Wa,--defsym -Wa,SPARK_INIT_STARTUP=0
 ASFLAGS += -D__STACKSIZE__=$(MAIN_STACK_SIZE) -D__STACK_SIZE=$(MAIN_STACK_SIZE)

+# Tracker
+ifeq ($(PLATFORM_ID),26)
+USE_PRINTF_FLOAT ?= n


Is this a breaking change for customer applications using printf?

Yes, but I have a workaround for this.

Can you elaborate on the workaround? I tried Serial.printlnf("pi=%f", M_PI); on a Tracker with 5.4.1 and I got the expected pi=3.141593 but with this branch, I get pi=

What is the workaround because Tracker Edge/Monitor Edge uses a lot of floats and doubles

Mostly in the jsmn JSON writer but also logging of various sensor data and GPS

Just to re-iterate: this is a temporary change and will be resolved (restored and some other stuff thrown away) before this PR is merged. This is failing a lot of our automated tests, so won't be unnoticed.

eberseth · 2023-07-27T01:14:47Z

user/tests/app/assets/assets.cpp

+        } else if (c == 'h') {
+            LOG(INFO, "Hook executed=%d", hookExecuted);
+            if (hookExecuted) {
+                LOG(INFO, "%u ssets captured in hook:", hookAssets.size());


eberseth · 2023-07-27T02:11:47Z

wiring/inc/spark_wiring_asset.h

+    int read() override;
+    int peek() override;
+    // Not present in Stream
+    virtual int read(char* buffer, size_t size);


For convenience, can we add an uint8_t overload virtual int read(uint8_t* buffer, size_t size);?

XuGuohui

Great job 👍 ! This is a huge PR and I just had a brief review. The main problem I found is we use memory allocation in many places without checking the result. This is a potential risk. I'll go though the asset OTA process as well.

hal/src/argon/hal_platform_config.h

platform/MCU/rtl872x/src/flash_mal.c

services/inc/storage_streams.h

system/src/asset_manager_api.cpp

system/src/system_cloud_internal.cpp

system/src/system_info.cpp

wiring/src/spark_wiring_system.cpp

eberseth · 2023-07-31T22:06:05Z

wiring/inc/spark_wiring_asset.h

+namespace particle {
+
+class ApplicationAsset: public Stream {
+public:


doxygen comments please for public methods here

eberseth · 2023-07-31T22:07:08Z

wiring/inc/spark_wiring_system.h

+    static spark::Vector<ApplicationAsset> assetsAvailable();
+    static int assetsHandled(bool state = true);
+
+    static int onAssetsOta(OnAssetsOtaCallback cb, void* context = nullptr);


doxygen comments for public methods please

eberseth · 2023-08-01T01:59:41Z

hal/src/rtl872x/ota_module_bounds.c

@@ -48,9 +48,9 @@ const module_bounds_t module_km0_part1 = {

 // OTA region, to be updated.
 module_bounds_t module_ota = {
-    .maximum_size = 0x200000, //2M
+    .maximum_size = 0x180000, // 1.5MB


Can we get these values from the linker script?

eberseth · 2023-08-01T02:23:44Z

system/inc/asset_manager_api.h

+struct asset_manager_stream;
+typedef struct asset_manager_stream asset_manager_stream;
+
+typedef enum asset_manager_consumer_state {


Please comment these cases about what the do or represent

system/src/asset_manager.cpp

eberseth · 2023-08-01T02:36:08Z

system/src/asset_manager.cpp

+
+    LOG(INFO, "prefix func=%d flags=%x start=%x end=%x compressed=%d", prefix.module_function, prefix.flags, prefix.module_start_address, prefix.module_end_address, compressed);
+
+    // Comperssion header


"Compression"

technobly

Works great!

sergeuz

Posting the comments I have so far. Will continue looking into this tomorrow.

sergeuz · 2023-08-08T13:20:58Z

hal/shared/inflate.cpp

    (*ctx)->buf_size = bufSize;
    (*ctx)->output = output;
    (*ctx)->user_data = user_data;
-    inflate_reset(*ctx);
+    CHECK(inflate_reset(*ctx));


The context will be leaked if inflate_reset fails.

sergeuz · 2023-08-08T13:32:17Z

hal/shared/inflate_impl.cpp

+}
+
+int inflate_reset_impl(inflate_ctx* ctx) {
+#if HAL_PLATFORM_INFLATE_USE_FILESYSTEM


The case when HAL_PLATFORM_INFLATE_USE_FILESYSTEM is 1 but the entire buffer is allocated in RAM doesn't seem to be handled by this function.

sergeuz · 2023-08-08T19:38:39Z

hal/src/nRF52840/product_store_hal.cpp

+    uintptr_t productDataStart = (uintptr_t)desc.info.module_end_address - sizeof(module_info_suffix_base_t) - sizeof(module_info_product_data_ext_t);
+    CHECK(hal_storage_read(storageId, productDataStart, (uint8_t*)&product, sizeof(product)));
+
+    return product.version;


sergeuz · 2023-08-08T20:04:23Z

platform/MCU/nRF52840/inc/flash_mal.h

@@ -81,28 +81,41 @@ extern "C" {
 #        define EXTERNAL_FLASH_RESERVED1_XIP_ADDRESS    (EXTERNAL_FLASH_RESERVED1_ADDRESS + EXTERNAL_FLASH_XIP_BASE)
 #        define EXTERNAL_FLASH_RESERVED1_LENGTH         (2048*1024)
 #        define EXTERNAL_FLASH_FAC_ADDRESS              (EXTERNAL_FLASH_RESERVED1_ADDRESS + EXTERNAL_FLASH_RESERVED1_LENGTH)
+#        define EXTERNAL_FLASH_ASSET_STORAGE_FIRST_PAGE ((EXTERNAL_FLASH_RESERVED1_XIP_ADDRESS) / sFLASH_PAGESIZE)


Why is EXTERNAL_FLASH_RESERVED1_XIP_ADDRESS and not EXTERNAL_FLASH_RESERVED1_ADDRESS used here?

sergeuz · 2023-08-08T20:36:30Z

services/inc/buffer.h

+
+class Buffer {
+public:
+    Buffer(size_t size = 0);


Consider making the constructor explicit and removing implicit conversion operators to avoid unintended conversions.

sergeuz · 2023-08-08T20:50:23Z

services/inc/buffer.h

+        return !memcmp(buffer_.get(), other.buffer_.get(), size());
+    }
+    return false;
+}


The way it's implemented now, the operator will return false if both buffers are empty.

sergeuz · 2023-08-08T20:57:14Z

services/inc/storage_streams.h

+#pragma once
+
+#undef LOG_COMPILE_TIME_LEVEL
+#define LOG_COMPILE_TIME_LEVEL LOG_LEVEL_NONE


We shouldn't be overriding the logging settings in header files.

sergeuz · 2023-08-08T21:11:41Z

services/inc/storage_streams.h

+              isOpen_(false) {
+        fs_ = filesystem_get_instance(instance, nullptr);
+        open(filename);
+    }


Consider only providing a default constructor and exposing the open() method so that the possible error when opening the file can be properly handled by the calling code.

~~I will leave this one as-is just for simplicity of not having to carry around filename (see AssetReader usage as well).~~ UGH Fixed 😄

sergeuz · 2023-08-08T21:15:07Z

services/inc/storage_streams.h

+        CHECK_TRUE(data, SYSTEM_ERROR_INVALID_ARGUMENT);
+        size = std::min(size, remaining_);
+        fs::FsLock lock(fs_);
+        CHECK(lfs_file_seek(&fs_->instance, &file_, offset_, LFS_SEEK_SET));


Make sure CHECK_FS is used with LittleFS functions here and in other places in this PR.

sergeuz

Good job. I don't think I found anything major, although I must admit I don't fully follow the logic where a file is used to store the decompression buffer. Hopefully that is well covered by tests.

sergeuz · 2023-08-09T08:58:14Z

services/inc/storage_streams.h

+              offset_(0),
+              error_(0) {
+
+        error_ = inflate_create(&inflate_, nullptr, [](const char* data, size_t size, void* ctx) -> int {


This approach to error handling is tedious and prone to bugs. In this class specifically, error_ is only checked in rewind() so the calling code will highly likely miss the error occurred when creating a decompressor context in the constructor. Consider separating the initialization logic in a checkable method such as init().

sergeuz · 2023-08-09T09:02:39Z

services/inc/storage_streams.h

+            }
+        }
+        return availForRead();
+    }


It's pretty cool that this class doesn't require an intermediate buffer.

sergeuz · 2023-08-09T09:08:04Z

system/inc/asset_manager.h

@@ -0,0 +1,158 @@
+/*


This header file seems to be internal to the system. If so, consider moving it to system/src as e.g. asset_manager_internal.h.

Also required by hal unfortunately (ota_flash_hal.cpp)

sergeuz · 2023-08-09T09:27:44Z

system/inc/asset_manager_api.h

+        Buffer buf(hash_.size() * 2 + 1);
+        if (buf.size()) {
+            memset(buf.data(), 0, buf.size());
+            bytes2hexbuf_lower_case((const uint8_t*)hash_.data(), hash_.size(), buf.data());


Just in case, there's a safer alternative that always null-terminates the output buffer.

sergeuz · 2023-08-09T09:47:29Z

system/inc/asset_manager_api.h

+ * @param reserved Reserved (NULL)
+ * @return 0 on success or `system_error_t` error code 
+ */
+int asset_manager_free_info(asset_manager_info* info, void* reserved);


Let's use void as the return type of destructor-like functions where possible as it's often not straightforward to handle errors during deinitialization in the calling code. If it does make sense to return an error from such a function, it should be documented what happens to the instance being deinitialized when an error occurs.

Same here.

sergeuz · 2023-08-09T11:19:02Z

system/src/asset_manager.cpp

+    CHECK_TRUE(prefix.module_function == MODULE_FUNCTION_ASSET, SYSTEM_ERROR_BAD_DATA);
+    CHECK_TRUE(prefix.flags & MODULE_INFO_FLAG_DROP_MODULE_INFO, SYSTEM_ERROR_BAD_DATA);
+    CHECK_FALSE(prefix.flags & MODULE_INFO_FLAG_PREFIX_EXTENSIONS, SYSTEM_ERROR_BAD_DATA);
+    CHECK_TRUE(moduleSize == ((uintptr_t)prefix.module_end_address - (uintptr_t)prefix.module_start_address + sizeof(uint32_t)), SYSTEM_ERROR_BAD_DATA);


Let's add a comment that sizeof(uint32_t) is the CRC.

sergeuz · 2023-08-09T11:24:19Z

system/src/asset_manager_api.cpp

+    info->internal = (void*)internal;
+
+    bool ok = false;
+    SCOPE_GUARD({


We also have NAMED_SCOPE_GUARD that can be dismissed.

sergeuz · 2023-08-09T11:25:56Z

system/src/asset_manager_api.cpp

+        delete internal;
+    }
+    if (info->required) {
+        free(info->required);


free() is used to free the memory allocated with new.

Same here.

sergeuz · 2023-08-09T11:29:25Z

system/src/asset_manager_api.cpp

+    internal->required = AssetManager::instance().requiredAssets();
+    internal->available = AssetManager::instance().availableAssets();
+
+    info->internal = (void*)internal;


This internal info doesn't seem to be used by any of the API calls. If so, let's remove it and save on memory.

That's the storage for at the very least hash and name.

Ah, didn't notice that 👍

sergeuz · 2023-08-09T11:39:24Z

system/inc/asset_manager_api.h

+}
+
+inline bool AssetHash::operator==(const AssetHash& other) const {
+    return (type_ == other.type_ && hash_ == other.hash_);


~~Is it intentional that the asset name is not taken into account here? It's worth adding a comment if so.~~ Oops, this class is just an asset's hash.

This reverts commit 7691f36.

…alidate open files

…ILESYSTEM

avtolstoy added feature 5.x-only labels Jul 11, 2023

avtolstoy added this to the 5.5.0 milestone Jul 11, 2023

avtolstoy force-pushed the feature/third-party-ota branch from 998da42 to d73c9de Compare July 24, 2023 13:57

monkbroc force-pushed the feature/third-party-ota branch from 8f3b366 to d067fc1 Compare July 25, 2023 14:39

monkbroc reviewed Jul 26, 2023

View reviewed changes

eberseth reviewed Jul 27, 2023

View reviewed changes

eberseth requested review from technobly, XuGuohui and scott-brust July 27, 2023 21:02

mrlambchop self-requested a review July 30, 2023 21:28

mrlambchop marked this pull request as ready for review July 30, 2023 21:29

mrlambchop approved these changes Jul 30, 2023

View reviewed changes

XuGuohui requested changes Jul 31, 2023

View reviewed changes

eberseth approved these changes Jul 31, 2023

View reviewed changes

eberseth reviewed Jul 31, 2023

View reviewed changes

eberseth reviewed Aug 1, 2023

View reviewed changes

system/src/asset_manager.cpp Show resolved Hide resolved

eberseth reviewed Aug 1, 2023

View reviewed changes

avtolstoy force-pushed the feature/third-party-ota branch 2 times, most recently from 42e7dad to 5f86de1 Compare August 2, 2023 10:23

technobly approved these changes Aug 2, 2023

View reviewed changes

scott-brust modified the milestones: 5.5.0, 5.5.0-rc.1 Aug 7, 2023

sergeuz reviewed Aug 8, 2023

View reviewed changes

sergeuz approved these changes Aug 9, 2023

View reviewed changes

avtolstoy requested a review from sergeuz August 10, 2023 16:34

avtolstoy added 25 commits August 15, 2023 20:40

[test] ota/assets: additional logging

af81502

[services] bring over LFS/filesystem utils/error checks from #2660

d756ffa

[tmp] temporary system error message verbose debug

f1b71f7

[test] ota/assets: temp verbose

ee624a3

[test] debug

61ae2c9

[test] ota/assets: fix product update tests

5f24d41

test

4c9f423

[test] ota/assets: improve product ota test

5283010

[test] ota/assets: retry flashing in ad-hoc test

25afd84

[assets] add .storageSize()

116e48d

[test] ota/assets: update

c5c475c

Revert "[tmp] temporary system error message verbose debug"

026635d

This reverts commit 7691f36.

[tracker] bring back _printf_float

bebe797

[test] ota/assets: minor

03d4559

[test] ota/assets: flash using test-runner flash method

9d28461

address review comments

e532551

[filesystem] gate asset filesystem read/write/erase if unmounted, inv…

c62d982

…alidate open files

minor

2eaba68

[third_party] miniz: submodule updated

66b1564

[test] ota/assets: minor

3b7cb74

[test] ota/assets: minor

1f8ce5a

[test] ota/assets: reset asset after calculating CRC

18a84d0

[wiring] asset: doxygen

87df642

[tests] ota/assets

1d28036

[inflate] resolve decompression issue with HAL_PLATFORM_INFLATE_USE_F…

2858d78

…ILESYSTEM

avtolstoy force-pushed the feature/third-party-ota branch from da322aa to 2858d78 Compare August 15, 2023 14:12

[mson] system-part1: add system_asset_manager to dynalib export

c104d85

avtolstoy merged commit 684713e into develop Aug 15, 2023
1 of 11 checks passed

avtolstoy deleted the feature/third-party-ota branch August 15, 2023 15:41

technobly mentioned this pull request Aug 29, 2023

Device OS 5.5.0 Changelog #2691

Closed


		LOG(INFO, "prefix func=%d flags=%x start=%x end=%x compressed=%d", prefix.module_function, prefix.flags, prefix.module_start_address, prefix.module_end_address, compressed);

		// Comperssion header

Assets OTA #2668

Assets OTA #2668

Conversation

avtolstoy commented Jul 11, 2023 • edited

Description

TODO

Steps to Test

P2 / Photon 2

Gen 3 (nRF52840 platforms)

Example App

References

Completeness

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuGuohui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technobly left a comment

Choose a reason for hiding this comment

sergeuz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avtolstoy Aug 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeuz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeuz Aug 9, 2023 • edited

Choose a reason for hiding this comment

avtolstoy commented Jul 11, 2023 •

edited

avtolstoy Aug 9, 2023 •

edited

sergeuz Aug 9, 2023 •

edited