Skip to content

soc: silabs: siwx91x: removed sscanf for nwp firmware version check #100168

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 1, 2025
+46 −28
Merged

soc: silabs: siwx91x: removed sscanf for nwp firmware version check #100168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions modules/hal_silabs/wiseconnect/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ if(CONFIG_SILABS_SIWX91X_NWP)
${WISECONNECT_DIR}/components/sli_wifi_command_engine/inc
)
zephyr_library_sources(
nwp_fw_version.c
${WISECONNECT_DIR}/components/common/src/sl_utility.c
${WISECONNECT_DIR}/components/device/silabs/si91x/wireless/ahb_interface/src/rsi_hal_mcu_m4_ram.c
${WISECONNECT_DIR}/components/device/silabs/si91x/wireless/ahb_interface/src/rsi_hal_mcu_m4_rom.c
Expand Down
21 changes: 21 additions & 0 deletions modules/hal_silabs/wiseconnect/nwp_fw_version.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright (c) 2025 Silicon Laboratories Inc.
*
* SPDX-License-Identifier: Apache-2.0
*/

/* This value needs to be updated when the Wiseconnect SDK (hal_silabs/wiseconnect) is updated
* Currently mapped to Wiseconnect SDK 3.5.2
*/

#include <nwp_fw_version.h>

const sl_wifi_firmware_version_t siwx91x_nwp_fw_expected_version = {
.rom_id = 0x0B,
.major = 2,
.minor = 14,
.security_version = 5,
.patch_num = 2,
.customer_id = 0,
.build_num = 7,
};
Comment on lines +13 to +21
Copy link
Member

@jhedberg jhedberg Nov 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not blocking this because of it, but I'm still a bit confused by why you need this struct variable at all. Can't you just have stuff like:

#define SIWX91X_NWP_EXPECTED_ROM_ID 0x0B
#define SIWX91X_NWP_EXPECTED_MAJOR  2
#define SIWX91X_NWP_EXPECTED_MINOR  14
...

And then in the version checking code you'd have stuff like

	if (version.major != SIWX91X_NWP_EXPECTED_MAJOR) {
		return -EINVAL;
	}

	...

Copy link
Member Author

@Martinhoff-maker Martinhoff-maker Nov 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That's what I've done first. I don't have strong opinion between the 2 ways of doing this. I suggest to merge it like this and I will follow up with the hal change, or you can discuss about it with @jerome-pouiller.

Copy link
Contributor

@jerome-pouiller jerome-pouiller Nov 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My main concern in the original PR was about the number of indirection layers to the real result (I wanted to drop either expected_version, either the defines).

In addition, I believe that exposing a structured data would make sense here (while the defines are only linked by the common prefix in their names).

Copy link
Member

@jhedberg jhedberg Nov 28, 2025

Choose a reason for hiding this comment

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

@jerome-pouiller if we did a direct comparison of two structs, that would indeed make a lot of sense. However, the code is doing component-by-component comparisons. Also:

  • We don't care about all struct fields (unless I misunderstood something), rather only a subset, so there's some wasted space with defining the entire struct
  • With the plan to move this to be auto-generated in the HAL (by the import script) having a struct would mean needing both a header file and a c-file, which seems a bit overkill for just a version check.

Anyway, I don't want to get stuck on this issue, since it's more important to get the CI problems sorted out. We can sort out the rest in a follow-up PR like @Martinhoff-maker proposes.

Copy link
Contributor

@jerome-pouiller jerome-pouiller Nov 28, 2025

Choose a reason for hiding this comment

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

My main concern was redundancy between defines and expected_version variable.

Outside of that, I don't really mind.

7 changes: 3 additions & 4 deletions modules/hal_silabs/wiseconnect/nwp_fw_version.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
* SPDX-License-Identifier: Apache-2.0
*/

/* This value needs to be updated when the Wiseconnect SDK (hal_silabs/wiseconnect) is updated
* Actually mapped to Wiseconnect SDK 3.5.0
*/
#define SIWX91X_NWP_FW_EXPECTED_VERSION "B.2.14.5.2.0.7"
#include <sl_wifi_types.h>

extern const sl_wifi_firmware_version_t siwx91x_nwp_fw_expected_version;
45 changes: 21 additions & 24 deletions soc/silabs/silabs_siwx91x/siwg917/siwx91x_nwp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,6 @@ static void siwx91x_configure_network_stack(sl_si91x_boot_configuration_t *boot_

static int siwx91x_check_nwp_version(void)
{
sl_wifi_firmware_version_t expected_version;
sl_wifi_firmware_version_t version;
int ret;

Expand All @@ -270,38 +269,29 @@ static int siwx91x_check_nwp_version(void)
return -EINVAL;
}

sscanf(SIWX91X_NWP_FW_EXPECTED_VERSION, "%hhX.%hhd.%hhd.%hhd.%hhd.%hhd.%hd",
&expected_version.rom_id,
&expected_version.major,
&expected_version.minor,
&expected_version.security_version,
&expected_version.patch_num,
&expected_version.customer_id,
&expected_version.build_num);

/* Ignore rom_id:
* B is parsed as an hex value and we get 11 in expected_version.rom_id
* We received rom_id=17 in version.rom_id, we suspect a double hex->decimal conversion
* the right value is 0x0B but we received 17 in version.rom_id, we suspect a double
* hex->decimal conversion
*/
if (expected_version.major != version.major) {
if (siwx91x_nwp_fw_expected_version.major != version.major) {
return -EINVAL;
}
if (expected_version.minor != version.minor) {
if (siwx91x_nwp_fw_expected_version.minor != version.minor) {
return -EINVAL;
}
if (expected_version.security_version != version.security_version) {
if (siwx91x_nwp_fw_expected_version.security_version != version.security_version) {
return -EINVAL;
}
if (expected_version.patch_num != version.patch_num) {
if (siwx91x_nwp_fw_expected_version.patch_num != version.patch_num) {
return -EINVAL;
}
if (expected_version.customer_id != version.customer_id) {
LOG_DBG("customer_id diverge: expected %d, actual %d", expected_version.customer_id,
version.customer_id);
if (siwx91x_nwp_fw_expected_version.customer_id != version.customer_id) {
LOG_DBG("customer_id diverge: expected %d, actual %d",
siwx91x_nwp_fw_expected_version.customer_id, version.customer_id);
}
if (expected_version.build_num != version.build_num) {
LOG_DBG("build_num diverge: expected %d, actual %d", expected_version.build_num,
version.build_num);
if (siwx91x_nwp_fw_expected_version.build_num != version.build_num) {
LOG_DBG("build_num diverge: expected %d, actual %d",
siwx91x_nwp_fw_expected_version.build_num, version.build_num);
}

return 0;
Expand Down Expand Up @@ -414,8 +404,15 @@ static int siwx91x_nwp_init(const struct device *dev)
/* Check if the NWP firmware version is correct */
ret = siwx91x_check_nwp_version();
if (ret < 0) {
LOG_ERR("Unexpected NWP firmware version (expected: %s)",
SIWX91X_NWP_FW_EXPECTED_VERSION);
LOG_ERR("Unexpected NWP firmware version (expected: %X.%d.%d.%d.%d.%d.%d)",
siwx91x_nwp_fw_expected_version.rom_id,
siwx91x_nwp_fw_expected_version.major,
siwx91x_nwp_fw_expected_version.minor,
siwx91x_nwp_fw_expected_version.security_version,
siwx91x_nwp_fw_expected_version.patch_num,
siwx91x_nwp_fw_expected_version.customer_id,
siwx91x_nwp_fw_expected_version.build_num);
return -EINVAL;
}

if (IS_ENABLED(CONFIG_SOC_SIWX91X_PM_BACKEND_PMGR)) {
Expand Down
Loading