-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature #37611
base: master
Are you sure you want to change the base?
[Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature #37611
Conversation
Changed Files
|
80b36ac
to
293f69c
Compare
src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp
Outdated
Show resolved
Hide resolved
src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp
Outdated
Show resolved
Hide resolved
@@ -62,6 +62,7 @@ class TAG: | |||
APPLICATION = 1 | |||
BOOTLOADER = 2 | |||
FACTORY_DATA = 3 | |||
WIFI_917_TA_M4 = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIFI_917_TA_M4
why are we using device specific tag name here?
@@ -163,6 +164,32 @@ def generate_app(args: object): | |||
return [OTA_APP_TLV_TEMP, args.app_input_file] | |||
|
|||
|
|||
def generate_wifi_image(args: object): | |||
""" | |||
Generate app payload with descriptor. If a certain option is not specified, use the default values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment needs to be updated
""" | ||
logging.info("App descriptor information:") | ||
|
||
descriptor = generate_descriptor(args.app_version, args.app_version_str, args.app_build_date) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this required? looks to be application specific
|
||
return CHIP_NO_ERROR; | ||
} | ||
|
||
CHIP_ERROR chip::OTAMultiImageProcessorImpl::OtaHookInit() | ||
{ | ||
static chip::OTAFirmwareProcessor sApplicationProcessor; | ||
static chip::OTAFactoryDataProcessor sFactoryDataProcessor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sFactoryDataProcessor
why was this required before and not now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is present in the code just went down in the macros which are there for WIFI and EFR
|
||
auto & imageProcessor = chip::OTAMultiImageProcessorImpl::GetDefaultInstance(); | ||
|
||
#ifndef SLI_SI91X_MCU_INTERFACE | ||
static chip::OTAFirmwareProcessor sApplicationProcessor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is sApplicationProcessor
not required for SLI_SI91X_MCU_INTERFACE
, the Si917 should also have an application processor right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because 917 SoC directly support the combined image so we don't need to implement the separate processor for it
kWiFiProcessor = 4, | ||
kCustomProcessor1 = 8, | ||
kCustomProcessor2 = 9, | ||
kCustomProcessor3 = 10, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a limit or bound for the enum would be recommended for which we can test if the OTAProcessorTag
is not unbound.
@@ -0,0 +1,176 @@ | |||
/* | |||
* | |||
* Copyright (c) 2023 Project CHIP Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Copyright (c) 2023 Project CHIP Authors | |
* Copyright (c) 2025 Project CHIP Authors |
@@ -0,0 +1,54 @@ | |||
/* | |||
* | |||
* Copyright (c) 2023 Project CHIP Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Copyright (c) 2023 Project CHIP Authors | |
* Copyright (c) 2025 Project CHIP Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file looks to be a generic WiFi Firmware Processor, why keep it under SiWx917?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doesn't seem like we need to have a seperate wi-fi firmware processor. We should try to make it common for all platforms.
@@ -547,12 +547,14 @@ template("efr32_sdk") { | |||
] | |||
} | |||
|
|||
if (chip_enable_multi_ota_encryption && chip_enable_multi_ota_requestor) { | |||
defines += [ "SL_MATTER_ENABLE_OTA_ENCRYPTION=1" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any specific reason to rename SL_MATTER_ENABLE_OTA_ENCRYPTION
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR a step in the right direction but it adds a lot fo what seems to be duplication with the existing feature.
Is is possible to reduce the amount of defines and duplication between the existing code and the newly added code?
if (chip_enable_multi_ota_requestor) { | ||
sources += [ | ||
"${silabs_platform_dir}/multi-ota/OTAHooks.cpp", | ||
"${silabs_platform_dir}/multi-ota/OTAMultiImageProcessorImpl.cpp", | ||
"${silabs_platform_dir}/multi-ota/OTAMultiImageProcessorImpl.h", | ||
"${silabs_platform_dir}/multi-ota/OTATlvProcessor.cpp", | ||
"${silabs_platform_dir}/multi-ota/OTATlvProcessor.h", | ||
"${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp", | ||
"${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move these GN changes to a BUILD.gn
within the multi-ota directory and have a depency in the efr32 and SiwX917.
|
||
OTADataAccumulator mAccumulator; | ||
bool mDescriptorProcessed = false; | ||
#if OTA_ENCRYPTION_ENABLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define has changed - This won't work anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doesn't seem like we need to have a seperate wi-fi firmware processor. We should try to make it common for all platforms.
@@ -547,12 +547,14 @@ template("efr32_sdk") { | |||
] | |||
} | |||
|
|||
if (chip_enable_multi_ota_encryption && chip_enable_multi_ota_requestor) { | |||
defines += [ "SL_MATTER_ENABLE_OTA_ENCRYPTION=1" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is reverting the define to its previous version when this was part for the provision structure refactor.
Can you remove this change?
CHIP_ERROR OTAWiFiFirmwareProcessor::Init() | ||
{ | ||
VerifyOrReturnError(mCallbackProcessDescriptor != nullptr, CHIP_OTA_PROCESSOR_CB_NOT_REGISTERED); | ||
mAccumulator.Init(sizeof(Descriptor)); | ||
#if OTA_ENCRYPTION_ENABLE | ||
mUnalignmentNum = 0; | ||
#endif //OTA_ENCRYPTION_ENABLE | ||
|
||
return CHIP_NO_ERROR; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as the Non-Wifi processor. We should move this to header to make it common and reduce duplication.
#ifdef SLI_SI91X_MCU_INTERFACE // This is not needed for the 917 SoC; it is required for EFR host applications | ||
// send system reset request to reset the MCU and upgrade the m4 image | ||
ChipLogProgress(SoftwareUpdate, "SoC Soft Reset initiated!"); | ||
// Reboots the device | ||
sl_si91x_soc_nvic_reset(); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment. The SLI_SI91X_MCU_INTERFACE
define is only present for the 917 SoC.
#define RPS_HEADER 1 | ||
#define RPS_DATA 2 | ||
|
||
uint8_t flag = RPS_HEADER; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be global?
@@ -163,6 +164,32 @@ def generate_app(args: object): | |||
return [OTA_APP_TLV_TEMP, args.app_input_file] | |||
|
|||
|
|||
def generate_wifi_image(args: object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is a copy paste of the generate app function outside of the tag change.
Why do we need a specific tag to the M4 / TA image?
If we do need a specific tag, we should modify the function to take the tag as input instead.
create_parser.add_argument('-wifi', "--wifi-input-file", | ||
help='Path to OTA image for SiWx917 (TA/M4/Combined file), 917 NCP (TA)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't need a specific tag, we don't need this change either.
if (chip_enable_wifi) { | ||
sources += [ | ||
"${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp", | ||
"${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h", | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these for all NCP combos?
Description of Problem/Feature:
Previously, we used a normal OTA file to update the 917 SoC for both the Application image and TA processor image. We could combine these updates into a single OTA image only. With the introduction of multi-OTA, we added a new tag in the header of the OTA file to detect the OTA processor that will process the image. This allows us to update multiple processor images using OTA in one go. In future, if we add another co-processor, we will be able to update all processor images at once using single OTA.
This PR introduces functionality to support arbitrary image downloads in a single OTA file. It can use pre-defined TLVs or custom TLVs. The ota_image_tool.py script has been updated to create a custom OTA file with TLVs embedded.
Testing
Before the Change:
After the Change: