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

[Martinhoff-maker]

This patch removes the use of sscanf to maintain compatibility with tests that use the minimal cpp library. The expected version is now defined using multiple individual values rather than a single formatted string.

Fixes CI failure here:
https://github.com/zephyrproject-rtos/zephyr/actions/runs/19623866468/job/56189276469?pr=99860
and here:
#99400
and here:
#100078

[jerome-pouiller]

I suggest to avoid these intermediate symbols. You can directly declare a static const sl_wifi_firmware_version_t siwx91x_nwp_fw_expected_version ... in modules/hal_silabs/wiseconnect/nwp_fw_version.h.

[Martinhoff-maker]

I've done that to avoid #include "sl_wifi_types.h in the file and to limit propagation of dependency

[Martinhoff-maker]

changed

[jerome-pouiller]

0x0B ?

[Martinhoff-maker]

agree, it will be more user friendly to update the version if he clearly see the pattern

[Copilot]

Pull request overview

This PR removes the use of sscanf for parsing the NWP firmware version to maintain compatibility with tests using the minimal C++ library. Instead of parsing a version string at runtime, the expected firmware version is now defined as a static const struct with explicit field values.

• Replaced string-based version parsing with compile-time struct initialization
• Updated version from 3.5.0 to 3.5.2
• Refactored error logging to format individual version fields instead of using a pre-formatted string

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
modules/hal_silabs/wiseconnect/nwp_fw_version.h	Replaced macro-defined version string with a static const struct containing explicit version field values
soc/silabs/silabs_siwx91x/siwg917/siwx91x_nwp.c	Removed sscanf-based parsing logic and updated references to use the new struct-based version definition

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

[Copilot]

Defining a static const variable in a header file will create a separate copy in each translation unit that includes this header. Consider using extern const declaration in the header and defining the variable in a .c file to avoid duplicate definitions:

In header:

extern const sl_wifi_firmware_version_t siwx91x_nwp_fw_expected_version;

In a .c file:

const sl_wifi_firmware_version_t siwx91x_nwp_fw_expected_version = { /* ... */ };

Alternatively, if it must remain in the header, use static inline or define it as a macro to avoid multiple copies.

Suggested change

	static 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,
	};
	extern const sl_wifi_firmware_version_t siwx91x_nwp_fw_expected_version;

[Martinhoff-maker]

it will be fine since it's only a one time reference so there will only be 1 copy.

[jhedberg]

it will be fine since it's only a one time reference so there will only be 1 copy.

Header files usually come with the implication that they may be included in multiple places (otherwise there's little benefit in a header file compared to just putting the content in the single c-file that needs it). Would it make sense to just move this into the c-file then? Alternatively, put the struct into the c-file, but leave defines in the header file for each of these values, which the c-file then uses to initialize the struct.

Another thought (although admittedly out of scope for this PR): even though hal_silabs contains an import of actual wiseconnect files, would it perhaps make sense to have our import script generate a header file in the hal_tree with the imported version, which is then incuded from the Zephyr main tree? That way we'd get stronger linkage with what's actually in the HAL tree and less risk of mistakes (forgetting to update the main tree).

[Martinhoff-maker]

but adressed

[Martinhoff-maker]

To answer your comment, I don't want to only have it in the c-file because I think we lost the link that this is associated to the Wiseconnect module. For the alternative, this what I've done at the start (see the diff).

Yeah that could be a solution. I thought that we disallow to have out of hal content (files) in the hal_silabs repo in order to have a better maintainability of the repo. That said, if it's part of the import script, that will not create maintenance issue.

[Copilot]

This comment is outdated. It refers to parsing behavior from the removed sscanf implementation. Since the version is now defined as a struct with explicit values, the comment should be updated to explain why rom_id is ignored in the comparison (e.g., "rom_id is not checked due to a known firmware issue with hex/decimal conversion").

Suggested change

	/* 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
	/* rom_id is intentionally not checked due to a known firmware issue
	* with hex/decimal conversion, which can cause mismatches between
	* expected and actual values.

[Martinhoff-maker]

updated

[Copilot]

"Actually" should be "Currently" for more idiomatic English in this context.

Suggested change

	* Actually mapped to Wiseconnect SDK 3.5.2
	* Currently mapped to Wiseconnect SDK 3.5.2

[Martinhoff-maker]

changed

Dismissing my approval, since the couple of issues brought up by the Copilot review seem valid

[Martinhoff-maker]

V2: doing a compromise between information location and declaration.

Plan to take a action to create a file in the hal with the import script for each new hal update.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[jhedberg]

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

	...

[Martinhoff-maker]

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.

[jerome-pouiller]

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

[jhedberg]

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

[jerome-pouiller]

My main concern was redundancy between defines and expected_version variable.

Outside of that, I don't really mind.