Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asset OTA fixes #2711

Merged
merged 6 commits into from Nov 30, 2023
Merged

Asset OTA fixes #2711

merged 6 commits into from Nov 30, 2023

Conversation

avtolstoy
Copy link
Member

@avtolstoy avtolstoy commented Nov 17, 2023

Description

This PR introduces a number of Asset OTA related fixes/enhancements:

  1. CTRL_REQUEST_GET_ASSET_INFO control request to fetch information about required/available assets on device over BLE/USB
  2. CTRL_REQUEST_GET_MODULE_INFO refactored to align with new protobuf-based System Describe and encoding for both the control request and system describe coalesced into a single implementation to save on flash space
  3. System.assetsAvailable() hides extra assets that might still be in storage but are not currently required, essentially this is now available ∩ required
  4. seek() forward and backwards support in wiring Asset OTA APIs
  5. Factory application with asset dependencies is now correctly parsed
  6. Overwriting an asset in use by the application (due to OTA or local flashing) is handled safely

Testing

  • ota/assets

NOTE: Requires latest @particle/device-os-test-runner

Related

@avtolstoy avtolstoy added this to the 5.6.0 milestone Nov 30, 2023
@avtolstoy avtolstoy marked this pull request as ready for review November 30, 2023 12:29
if (!is_module_function_valid((module_function_t)module.info.module_function)) {
continue;
}
// If sending a describe, skip factory modules entirely just in case
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be reverted once this is safe.

@avtolstoy avtolstoy force-pushed the feature/asset-ota-fixes branch 2 times, most recently from adc22ac to 6eaff5e Compare November 30, 2023 13:38
Copy link
Member

@monkbroc monkbroc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I tested the control requests. I left a comment about making the list of available assets consistent between describe, control request and System.assetsAvailable

pbDesc.assets.arg = (void*)&availableAssets;
pbDesc.assets.funcs.encode = encodeAssetDependencies;
}
EncodeAssets assets(&pbDesc.assets, AssetManager::instance().availableAssets());
Copy link
Member

@monkbroc monkbroc Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should use availableAndRequiredAssets() to avoid showing assets still on the device but not required by the application on the Console.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for cloud: we want the DS to be aware of what is entirely on the device to avoid OTAing things over unnecessarily.

Copy link
Member Author

@avtolstoy avtolstoy Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed before the "use-case" for this is:

  1. product fw version 1 assets (1,2,3)
  2. product fw version 2 assets (2,3)
  3. product fw version 3 assets (1,2,3)

Currently apart from the product application OTA (and depending on whether Device OS upgrade happened of course) the assets in this particular case will not get OTAd at all post #1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assets wouldn't get flashed unnecessarily if this used availableAndRequiredAssets() either since after 1, it would return assets 1, 2, 3 available, after 2, it would return 2, 3 available and after 3, it would return 1, 2, 3. The OTA planner only triggers after the device sends the new describe message. But since it works both ways it's fine like this.

int getAssetInfo(ctrl_request* req) {
PB(GetAssetInfoReply) pbRep = {};
using namespace particle::system;
EncodeAssets assetsAvailable(&pbRep.available, AssetManager::instance().availableAssets());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly I think this should use availableAndRequiredAssets() for consistency with the describe and Wiring API. The CLI flashes the new application before assets so returning available and required assets in the control request will work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd disagree. I'd like for system describe and control requests to represent the full picture and for wiring APIs to be representative of what's required by the application. Again, same thing: to avoid re-flashing things unnecessarily that are already on the device.

Copy link
Contributor

@keeramis keeramis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested available and required works as expected.
Tested assetsAvailable() return the assets which are available and required 👍
Checked getFirmwareModuleInfo() and getAssetInfo() work as expected 👍

I did not test the app crashing issue.

@scott-brust scott-brust merged commit c4c0105 into develop Nov 30, 2023
13 checks passed
@scott-brust scott-brust deleted the feature/asset-ota-fixes branch November 30, 2023 19:33
@technobly technobly added the bug label Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants