[Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature #37611

shgutte · 2025-02-17T05:39:35Z

Description of Problem/Feature:
Functionality to support arbitrary image downloads in a single OTA file. This can use pre-defined TLVs or custom TLVs.
ota_image_tool.py script updated to create a custom OTA file with TLVs embedded.

Testing

Tested on 917 SoC

semanticdiff-com · 2025-02-17T05:39:38Z

Review changes with

Changed Files

File	Status
examples/platform/silabs/SiWx917/BUILD.gn	Unsupported file format
scripts/tools/silabs/ota/ota_multi_image_tool.py	0% smaller
src/platform/silabs/SiWx917/BUILD.gn	Unsupported file format
src/platform/silabs/efr32/BUILD.gn	Unsupported file format
src/platform/silabs/multi-ota/OTAHooks.cpp	Unsupported file format
src/platform/silabs/multi-ota/OTAMultiImageProcessorImpl.cpp	Unsupported file format
src/platform/silabs/multi-ota/OTATlvProcessor.cpp	Unsupported file format
src/platform/silabs/multi-ota/OTATlvProcessor.h	Unsupported file format
src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp	Unsupported file format
src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h	Unsupported file format
third_party/silabs/efr32_sdk.gni	Unsupported file format

github-actions · 2025-02-24T07:58:07Z

PR #37611: Size comparison from 43a8a9b to 66e2d0c

Full report (1 build for stm32)

platform	target	config	section	`43a8a9b`	66e2d0cf	change	% change
stm32	light	STM32WB5MM-DK	FLASH	459800	459800	0	0.0
			RAM	141472	141472	0	0.0

bhmanda-silabs · 2025-02-28T10:01:34Z

scripts/tools/silabs/ota/ota_multi_image_tool.py

@@ -312,6 +342,8 @@ def any_base_int(s): return int(s, 0)

    create_parser.add_argument('-app', "--app-input-file",
                               help='Path to application input file')
+    create_parser.add_argument('-wifi', "--wifi-input-file",
+                               help='Path to OTA image for SiWx917 (TA/M4/Combined file)')


Please add 917 NCP combination also for --wifi-input-file

bhmanda-silabs · 2025-02-28T11:22:31Z

src/platform/silabs/multi-ota/OTAFirmwareProcessor.cpp

@@ -68,6 +68,7 @@ CHIP_ERROR OTAFirmwareProcessor::Clear()
 CHIP_ERROR OTAFirmwareProcessor::ProcessInternal(ByteSpan & block)
 {
    uint32_t err = SL_BOOTLOADER_OK;
+    ChipLogProgress(SoftwareUpdate,"ProcessInternal Thread Block processing");


Could you please clarify what is "Thread Block"?

bhmanda-silabs · 2025-02-28T11:26:22Z

src/platform/silabs/multi-ota/OTAHooks.cpp

+#endif
+
+#if SL_WIFI
+    auto descWiFi = static_cast<chip::OTAWiFiFirmwareProcessor::Descriptor *>(descriptor);


In 917 NCP case, it will have both descriptors?

No only TA descriptor

bhmanda-silabs · 2025-02-28T11:32:31Z

src/platform/silabs/multi-ota/OTAMultiImageProcessorImpl.cpp

+#endif // SL_BTLCTRL_MUX
+#include "em_bus.h" // For CORE_CRITICAL_SECTION
+#ifndef SLI_SI91X_MCU_INTERFACE // required for 917 NCP
+#include "btl_interface.h"


This "#include "btl_interface.h" can be included once.

bhmanda-silabs · 2025-02-28T11:34:48Z

src/platform/silabs/multi-ota/OTAMultiImageProcessorImpl.cpp

@@ -115,7 +115,9 @@ void OTAMultiImageProcessorImpl::HandlePrepareDownload(intptr_t context)

    ChipLogProgress(SoftwareUpdate, "HandlePrepareDownload: started");

+#ifndef SLI_SI91X_MCU_INTERFACE // required for 917 NCP


Could you please change the comment, as this is not required only for 917 SoC and it is needed for EFR host applications.

bhmanda-silabs · 2025-02-28T11:35:17Z

src/platform/silabs/multi-ota/OTAMultiImageProcessorImpl.cpp

    // This reboots the device
+    // TODO: check where to put this
+#ifndef SLI_SI91X_MCU_INTERFACE // required for 917 NCP


same here as above

bhmanda-silabs · 2025-02-28T11:36:40Z

src/platform/silabs/multi-ota/OTATlvProcessor.cpp

+    BOOTLOADER,
+    FACTORY_DATA,
+    WIFI_917_TA_M4_COMBINED,
+} OTAImageType;


Are you using this Enum OTAImageType any where?

bhmanda-silabs · 2025-02-28T11:38:40Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp

+#define RPS_HEADER 1
+#define RPS_DATA 2
+
+#define SL_STATUS_FW_UPDATE_DONE SL_STATUS_SI91X_FW_UPDATE_DONE


Check this SL_STATUS_FW_UPDATE_DONE can be used directly from wifi-sdk.

bhmanda-silabs · 2025-02-28T11:40:51Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp

+
+    OTATlvProcessor::vOtaProcessInternalEncryption(mBlock);
+    block = mBlock;
+#endif


Can you please add #endif //OTA_ENCRYPTION_ENABLE

rosahay-silabs · 2025-03-04T12:49:01Z

scripts/tools/silabs/ota/ota_multi_image_tool.py

@@ -62,6 +62,7 @@ class TAG:
    APPLICATION = 1
    BOOTLOADER = 2
    FACTORY_DATA = 3
+    WIFI_917_TA_M4 = 4


WIFI_917_TA_M4 why are we using device specific tag name here?

rosahay-silabs · 2025-03-04T12:53:56Z

scripts/tools/silabs/ota/ota_multi_image_tool.py

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


comment needs to be updated

rosahay-silabs · 2025-03-04T13:31:48Z

scripts/tools/silabs/ota/ota_multi_image_tool.py

+    """
+    logging.info("App descriptor information:")
+
+    descriptor = generate_descriptor(args.app_version, args.app_version_str, args.app_build_date)


is this required? looks to be application specific

rosahay-silabs · 2025-03-04T13:36:58Z

src/platform/silabs/multi-ota/OTAHooks.cpp


    return CHIP_NO_ERROR;
 }

 CHIP_ERROR chip::OTAMultiImageProcessorImpl::OtaHookInit()
 {
-    static chip::OTAFirmwareProcessor sApplicationProcessor;
-    static chip::OTAFactoryDataProcessor sFactoryDataProcessor;


sFactoryDataProcessor why was this required before and not now?

this is present in the code just went down in the macros which are there for WIFI and EFR

rosahay-silabs · 2025-03-04T13:38:27Z

src/platform/silabs/multi-ota/OTAHooks.cpp


    auto & imageProcessor = chip::OTAMultiImageProcessorImpl::GetDefaultInstance();
+
+#ifndef SLI_SI91X_MCU_INTERFACE
+    static chip::OTAFirmwareProcessor sApplicationProcessor;


why is sApplicationProcessor not required for SLI_SI91X_MCU_INTERFACE, the Si917 should also have an application processor right?

Because 917 SoC directly support the combined image so we don't need to implement the separate processor for it

rosahay-silabs · 2025-03-04T13:42:17Z

src/platform/silabs/multi-ota/OTATlvProcessor.h

+    kWiFiProcessor        = 4,
+    kCustomProcessor1     = 8,
+    kCustomProcessor2     = 9,
+    kCustomProcessor3     = 10,
 };


a limit or bound for the enum would be recommended for which we can test if the OTAProcessorTag is not unbound.

rosahay-silabs · 2025-03-04T13:42:28Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp

@@ -0,0 +1,176 @@
+/*
+ *
+ *    Copyright (c) 2023 Project CHIP Authors


Suggested change

* Copyright (c) 2023 Project CHIP Authors

* Copyright (c) 2025 Project CHIP Authors

rosahay-silabs · 2025-03-04T13:47:53Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h

@@ -0,0 +1,54 @@
+/*
+ *
+ *    Copyright (c) 2023 Project CHIP Authors


Suggested change

* Copyright (c) 2023 Project CHIP Authors

* Copyright (c) 2025 Project CHIP Authors

rosahay-silabs · 2025-03-04T13:49:24Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h

this file looks to be a generic WiFi Firmware Processor, why keep it under SiWx917?

I don't seem like we need to have a seperate wi-fi firmware processor. We should try to make to make it common for all platforms.

rosahay-silabs · 2025-03-04T13:50:40Z

third_party/silabs/efr32_sdk.gni

@@ -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" ]


any specific reason to rename SL_MATTER_ENABLE_OTA_ENCRYPTION?

mkardous-silabs

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?

mkardous-silabs · 2025-03-04T14:15:02Z

src/platform/silabs/SiWx917/BUILD.gn

+  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",
+    ]


We should move these GN changes to a BUILD.gn within the multi-ota directory and have a depency in the efr32 and SiwX917.

mkardous-silabs · 2025-03-04T14:17:54Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h

+
+    OTADataAccumulator mAccumulator;
+    bool mDescriptorProcessed = false;
+#if OTA_ENCRYPTION_ENABLE


Define has changed - This won't work anymore.

mkardous-silabs · 2025-03-04T14:18:51Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h

I don't seem like we need to have a seperate wi-fi firmware processor. We should try to make to make it common for all platforms.

mkardous-silabs · 2025-03-04T14:19:41Z

third_party/silabs/efr32_sdk.gni

@@ -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" ]


This change is reverting the define to its previous version when this was part for the provision structure refactor.
Can you remove this change?

mkardous-silabs · 2025-03-04T14:24:26Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp

+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;
+}


This is the same as the Non-Wifi processor. We should move this to header to make it common and reduce duplication.

mkardous-silabs · 2025-03-04T14:28:47Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp

+#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


I don't understand this comment. The SLI_SI91X_MCU_INTERFACE define is only present for the 917 SoC.

mkardous-silabs · 2025-03-04T14:29:35Z

src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp

+#define RPS_HEADER 1
+#define RPS_DATA 2
+
+uint8_t flag = RPS_HEADER;


Does this need to be global?

mkardous-silabs · 2025-03-04T14:34:39Z

scripts/tools/silabs/ota/ota_multi_image_tool.py

@@ -163,6 +164,32 @@ def generate_app(args: object):
        return [OTA_APP_TLV_TEMP, args.app_input_file]


+def generate_wifi_image(args: object):


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.

mkardous-silabs · 2025-03-04T14:37:55Z

scripts/tools/silabs/ota/ota_multi_image_tool.py

+    create_parser.add_argument('-wifi', "--wifi-input-file",
+                               help='Path to OTA image for SiWx917 (TA/M4/Combined file), 917 NCP (TA)')


If we don't need a specific tag, we don't need this change either.

mkardous-silabs · 2025-03-04T14:38:43Z

src/platform/silabs/efr32/BUILD.gn

+    if (chip_enable_wifi) {
+      sources += [
+        "${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp",
+        "${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h",
+      ]
+    }


Why do we need these for all NCP combos?

github-actions bot added examples platform silabs scripts labels Feb 17, 2025

shgutte added 25 commits February 24, 2025 11:45

Added changes for the multi ota 917 soc

17c5611

Added changes for the build errores

5eef7f6

Added changes for 917 soc

5b07de3

Added changes for the name for 917 NCP

ea8c80f

Deleted the 917 NCP file

ad4dd34

Added fixed for 917 SoC build changes

7762e37

Added changes for 917 SOC

5949e9f

Added changes for the 917 SOC build error

6e80e03

Added changes for the 917 SoC ota reset

4cfdfd5

Added changes for OTA 917 SoC script

627f9ba

Added changes for 917 build

5a7d83b

Added debugging logs for the files

08bab24

Removed the duplicate code

f4bf610

Added changes for the SoC Application

e6c3a13

Added changes for builkd issue with EFR32 multi OTA

cdc63ed

Added changes for the OTA 917 NCP

593eaf3

Added changes for the OTA tlv

14ed08e

Added changes for the comment

479d2a2

Added changes for the process block for WIfi

4cf6b09

Removed the mWriteOffset in firmware processor

23cea72

Removed unessory comment

47d37f0

Added changes for the ProcessInternal OTA WiFi

f9934da

Added changes for WiFi OTA processor

14bc689

Removed the unwanted line

d855d64

Cleanup of the code

f799676

shgutte and others added 9 commits February 24, 2025 12:07

Corrected #ifdef comments

298adc8

Removed unwanted comment

a672c7b

Added back the set_logger function

ec02a69

Applied suggestion

d09abe5

Added changes realted to comment

d76ef10

Discription for input command is updated

cae741a

Added changes for build file of 917 SoC

df781e7

Added changes for encryption

a3dd261

Adds changes for the multi-ota updated

293f69c

shgutte force-pushed the feature/wifi/multi-ota-processor branch from 80b36ac to 293f69c Compare February 24, 2025 07:27

shgutte changed the title ~~[WIP][Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature~~ [Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature Feb 24, 2025

shgutte marked this pull request as ready for review February 24, 2025 07:31

shgutte requested a review from a team as a code owner February 24, 2025 07:31

pullapprove bot added the review - pending label Feb 24, 2025

Adds changes for the customProcessor

66e2d0c

shgutte added 3 commits February 25, 2025 12:41

Refacred the code and removed the not required code

1e93535

Adds change for script

a7b01b0

Adds changes for the 917 NCP

3ddad76

bhmanda-silabs reviewed Feb 28, 2025

View reviewed changes

shgutte added 7 commits March 4, 2025 17:28

Restyle done

1150a2a

Adds OTA_ENCRYPTION_ENABLE commnet

072d222

Added SL_STATUS_SI91X_FW_UPDATE_DONE directly

420fa5c

Removed the OTAImageTypex

65f7c04

Added comment for 917 NCP

0bc1e0d

Removed the comment

187a3da

Changed the comment for SLI_SI91X_MCU_INTERFACE

f06b8b2

rosahay-silabs reviewed Mar 4, 2025

View reviewed changes

mkardous-silabs requested changes Mar 4, 2025

View reviewed changes

		@@ -115,7 +115,9 @@ void OTAMultiImageProcessorImpl::HandlePrepareDownload(intptr_t context)

		ChipLogProgress(SoftwareUpdate, "HandlePrepareDownload: started");

		#ifndef SLI_SI91X_MCU_INTERFACE // required for 917 NCP

	* Copyright (c) 2023 Project CHIP Authors
	* Copyright (c) 2025 Project CHIP Authors

		@@ -163,6 +164,32 @@ def generate_app(args: object):
		return [OTA_APP_TLV_TEMP, args.app_input_file]


		def generate_wifi_image(args: object):

		create_parser.add_argument('-wifi', "--wifi-input-file",
		help='Path to OTA image for SiWx917 (TA/M4/Combined file), 917 NCP (TA)')

[Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature #37611

Are you sure you want to change the base?

[Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature #37611

Conversation

shgutte commented Feb 17, 2025 • edited Loading

Testing

semanticdiff-com bot commented Feb 17, 2025 • edited Loading

github-actions bot commented Feb 24, 2025

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

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

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

mkardous-silabs 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

shgutte commented Feb 17, 2025 •

edited

Loading

semanticdiff-com bot commented Feb 17, 2025 •

edited

Loading