Refactors system describe json generation to reduce size, removes invalid factory modules from it #2347

avtolstoy · 2021-08-20T23:33:38Z

Problem

If factory module (whether valid or seemingly valid) is flashed onto an Electron and it is running < Device OS 3.0, such module configuration will overflow the maximum supported CoAP message size and will cause an assertion failure, which causes a reset.
There is no automated way of catching system describe overflows under such conditions

Solution

Refactor system_module_info() to use JSONWriter, which should save few valuable bytes and also make generation a lot cleaner. Factory modules that are not valid are also no longer included in the describe.
Add a set of automated tests to wiring/no_fixture that put valid and seemingly valid user part binaries into the factory location and perform cloud connection. If overflow occurs, we should be able to easily catch it during the release process as the device will reset due to assertion failure and the tests will fail.
Add unit tests for system_module_info() to make sure that the new implementation is comparable to and old one and works as intended
system_module_info() and other functions that deal with describe generation or system module info are decoupled into a separate source file system_info.cpp
In order for the on-device tests to be able to write binaries into factory location a new minimal storage hal was added and exposed through dynalib
(Unrelated) build/create_module.py script updated to generate product binaries
Some other bugs resolved in CRC and suffix access with modules located in non-standard locations (e.g. factory)
wiring/no_fixture tests overflow on Electron, so some cellular tests have been moved out into wiring/no_fixture_cellular
Some other minor refactoring to achieve everything above

Steps to Test

Run unit tests
Run wiring/no_fixture and specifically SYSTEM_06 and SYSTEM_07 tests
Run the same tests with these changes rebased onto 2.x branch (I'll push ready-to-use one later)

Example App

N/A

References

[CH84019]

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)

avtolstoy · 2021-08-20T23:44:02Z

system/src/system_info.cpp

+        char tmp[4] = {};
+        snprintf(tmp, sizeof(tmp), "%u", (unsigned)module_index(info));
+        // FIXME: is this really supposed to be a string?
+        // json.name("n").value(module_index(info));


@sergeuz Can we make this an integer instead of a string? Does DS care about this? CLI seems to take it okay. As of right now the describe with factory module on an Electron takes 800 bytes which is exactly MAX_CONTENT_LEN on 2.x, with this turned into an int we end up with 776 instead.

(some notes from the discussion)
Looks like we'd better not, so instead when backporting to 2.x, we'll increase MBEDTLS_SSL_MAX_CONTENT_LEN and PROTOCOL_BUFFER_SIZE to 900 on Electron specifically.

sergeuz · 2021-08-24T12:52:17Z

hal/src/nRF52840/ota_flash_hal.cpp

-                    info->modules[i].info.module_index = 1; 
-                    hybrid_module_found = true;
-                }
-#endif // HYBRID_BUILD


Nice 👍 Not in this PR, but we should remove everything related to hybrid builds from Device OS altogether.

Big thumbs up.

sergeuz · 2021-08-24T12:56:32Z

hal/src/nRF52840/ota_module.cpp

@@ -56,7 +56,7 @@ int get_module_crc_suffix(const module_bounds_t* bounds, const module_info_t* in
        // This should be done in fetch_module().
        return SYSTEM_ERROR_NOT_SUPPORTED;
    } else {
-        return FLASH_ModuleCrcSuffix(crc, suffix, bounds->location == MODULE_BOUNDS_LOC_INTERNAL_FLASH ? FLASH_INTERNAL : FLASH_SERIAL, (uint32_t)info->module_end_address);
+        return FLASH_ModuleCrcSuffix(crc, suffix, bounds->location == MODULE_BOUNDS_LOC_INTERNAL_FLASH ? FLASH_INTERNAL : FLASH_SERIAL, bounds->start_address + module_length(info));


Nice catch.

sergeuz · 2021-08-24T13:00:43Z

hal/inc/storage_hal.h

+#endif // __cplusplus
+
+typedef enum hal_storage_id {
+    HAL_STORAGE_ID_DEFAULT = 0,


Let's rename this to HAL_STORAGE_ID_INVALID as the actual implementation in storage_hal.cpp has no notion of the default storage device.

sergeuz · 2021-08-24T13:12:16Z

hal/src/nRF52840/storage_hal.cpp

+    }
+
+    return SYSTEM_ERROR_NOT_FOUND;
+}


If feels like this function should either return an error if the address is not aligned to the sector size and the destination storage device requires that, or adjust the address and number of sectors accordingly.

Refactored to use flash_common.cpp, no longer a concern.

sergeuz · 2021-08-24T13:21:51Z

hal/src/stm32f2xx/storage_hal.cpp

+        /* Data received are Word multiple */
+        FLASH_Status status = FLASH_COMPLETE;
+        for (size_t pos = 0; pos < size && status == FLASH_COMPLETE; pos += sizeof(uint32_t)) {
+            uint32_t tmp = 0xffffff;


It would be better to initialize this with the current contents of the flash at the destination address. Also, does STM32 allow unaligned writes? I'm wondering if this is what is causing the issue with the in place modification of the OTA binary on Gen 2 in my OTA test that is based on this branch.

Yeah, you are right on the unaligned writes. As this was only for tests I was mainly focusing on whatever is required there in the context of this PR, but since you are running into issues in your tests, I'll quickly refactor to use existing functionality in flash_common.cpp for Gen 3 which tackles alignment.

For visibility: the issue in my tests was caused by a bug in this code that uses 0xffffff instead 0xffffffff to initialize tmp, not by an unaligned write. Big thumbs up for still refactoring the code to reuse the functionality from flash_common.cpp 👍

Refactored to use flash_common.cpp

sergeuz · 2021-08-24T14:06:29Z

system/src/system_info.cpp

+        va_start(args, fmt);
+        const auto r = vsnprintf(buf, sizeof(buf), fmt, args);
+        va_end(args);
+        if (r > (int)sizeof(buf)) {


>= should be used here as the output is always null-terminated. In fact, this method has a default implementation in JSONWriter which is good enough for most cases including this one.

sergeuz · 2021-08-24T14:11:31Z

system/src/system_info.cpp

+    }
+private:
+    bool state_;
+};


👍 👍 👍

sergeuz · 2021-08-24T14:15:16Z

system/src/system_info.cpp

+        json.name("u").value(bytes2hexbuf(module->suffix.sha, sizeof(module->suffix.sha), buf), sizeof(buf));
+    }
+    // FIXME?
+    if (info) {


info can no longer be null in this code. I'd make it a const reference.

sergeuz · 2021-08-24T14:28:37Z

system/src/system_info.cpp

+        json.name("f").value(module_function_string(module_function(info)));
+        char tmp[4] = {};
+        snprintf(tmp, sizeof(tmp), "%u", (unsigned)module_index(info));
+        // FIXME: is this really supposed to be a string?


Historically, yes. It's worth mentioning this in the comment to avoid confusion.

Added a note comment.

sergeuz · 2021-08-24T14:49:54Z

system/src/system_info.cpp

+int system_info_get(hal_system_info_t* info, uint32_t flags, void* reserved) {
+    CHECK_TRUE(info, SYSTEM_ERROR_INVALID_ARGUMENT);
+    return HAL_System_Info(info, true, nullptr);
+}


I have one important suggestion: let's clearly mark system_info_get() and system_info_free() as unstable APIs that should not be used in customer applications, perhaps even add an _unstable suffix to both of them, and remove all precaution comments added in this PR about the fact that certain structures that used to be internal can't be easily changed anymore. Making this change in the scope of this PR would be too much, but we should really have a proper system wrapper for HAL_System_Info() that would dynamically allocate all the nested structures of hal_system_info_t so that we could extend any of them individually in the future.

Agreed, let's add a comment and add _unstable suffix 👍

Renamed with _unstable suffix.

- refactor json module info generation to use JSONWriter - add storage_hal and system_info_get()/system_info_free() - module operation bugfixes

…owed_when_factory_module_present and SYSTEM_07_system_describe_is_not_overflowed_when_factory_module_present_but_invalid

…edicated source file

avtolstoy requested review from sergeuz, XuGuohui and keeramis August 20, 2021 23:33

avtolstoy commented Aug 20, 2021

View reviewed changes

avtolstoy force-pushed the feature/refactor-system-info-generation branch from 34340ec to 0d2538c Compare August 23, 2021 05:53

sergeuz mentioned this pull request Aug 23, 2021

Clear module slots in DCT when preparing for an OTA update #2346

Merged

sergeuz approved these changes Aug 24, 2021

View reviewed changes

avtolstoy added 13 commits August 25, 2021 00:48

[build] create_module: add product option field

68e91a1

various

f15661b

- refactor json module info generation to use JSONWriter - add storage_hal and system_info_get()/system_info_free() - module operation bugfixes

[test] move some tests into wiring/no_fixture_cellular

9258204

[test] wiring/no_fixture: add SYSTEM_06_system_describe_is_not_overfl…

a7ff6fb

…owed_when_factory_module_present and SYSTEM_07_system_describe_is_not_overflowed_when_factory_module_present_but_invalid

minor

f7ed620

[user] add dynalib as dependency even if SPARK_NO_PLATFORM is set

d6d419d

[dynalib] module_info: add MODULE_FUNCTION_MAX

7974ede

[system] move functions dealing with generation of describes into a d…

9897d77

…edicated source file

[test] unit: generation of system describes

db95440

[test] wiring/no_fixture: minor

b92287b

minor

49be4b5

[system] info: add cstdio header for vsnprintf

95c5e34

address review comments

f3f7ca4

avtolstoy force-pushed the feature/refactor-system-info-generation branch from 0d2538c to f3f7ca4 Compare August 24, 2021 17:48

avtolstoy merged commit fcc76ac into develop Aug 24, 2021

avtolstoy deleted the feature/refactor-system-info-generation branch August 24, 2021 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors system describe json generation to reduce size, removes invalid factory modules from it #2347

Refactors system describe json generation to reduce size, removes invalid factory modules from it #2347

avtolstoy commented Aug 20, 2021

avtolstoy Aug 20, 2021

avtolstoy Aug 23, 2021

sergeuz Aug 24, 2021

avtolstoy Aug 24, 2021

sergeuz Aug 24, 2021

sergeuz Aug 24, 2021

avtolstoy Aug 24, 2021

sergeuz Aug 24, 2021

avtolstoy Aug 24, 2021

sergeuz Aug 24, 2021

avtolstoy Aug 24, 2021

sergeuz Aug 24, 2021 •

edited

avtolstoy Aug 24, 2021

sergeuz Aug 24, 2021

avtolstoy Aug 24, 2021

sergeuz Aug 24, 2021

sergeuz Aug 24, 2021

avtolstoy Aug 24, 2021

sergeuz Aug 24, 2021

avtolstoy Aug 24, 2021

sergeuz Aug 24, 2021

avtolstoy Aug 24, 2021

avtolstoy Aug 24, 2021

Refactors system describe json generation to reduce size, removes invalid factory modules from it #2347

Refactors system describe json generation to reduce size, removes invalid factory modules from it #2347

Conversation

avtolstoy commented Aug 20, 2021

Problem

Solution

Steps to Test

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeuz Aug 24, 2021 • edited

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 24, 2021 •

edited