drivers: wifi: siwx91x: Add support for BG Scan and Roaming by Nitin-Pandey-01 · Pull Request #87037 · zephyrproject-rtos/zephyr

Nitin-Pandey-01 · 2025-03-13T10:39:06Z

Added support for background scan
Resolved the dwell time related bugs
Updated the case for rejecting scan in channel 12, 13 and 14

jerome-pouiller · 2025-04-02T15:54:55Z

 {
-	sl_wifi_scan_configuration_t sl_scan_config = { };
+	sl_wifi_scan_configuration_t sl_scan_config = {};
+	sl_wifi_advanced_scan_configuration_t advanced_scan_config = {0};


The prefered syntax is: = { };

jerome-pouiller · 2025-04-02T16:04:06Z

+#define SL_WIFI_MIN_ACTIVE_SCAN_TIME_MS  5
+#define SL_WIFI_MAX_ACTIVE_SCAN_TIME_MS  1000
+#define SL_WIFI_MIN_PASSIVE_SCAN_TIME_MS 10
+#define SL_WIFI_MAX_PASSIVE_SCAN_TIME_MS 1000


I don't think it it is useful to expose these values.

Removed the macros and added Raw numbers for checking edge cases

jerome-pouiller · 2025-04-02T16:19:12Z

+					    SL_WIFI_MAX_ACTIVE_SCAN_TIME_MS) {
+					LOG_ERR("Invalid active scan dwell time");
+					return -EINVAL;
+				}


This function starts to be too complex (5 levels of indentation, 150 lines).

I believe you can place the input validation for dwell interval with the other ones on the top the function:

if (z_scan_config->dwell_time_active && (z_scan_config->dwell_time_active < SL_WIFI_MIN_ACTIVE_SCAN_TIME_MS || z_scan_config->dwell_time_active > SL_WIFI_MAX_ACTIVE_SCAN_TIME_MS)) { return -EINVAL; }

You may also introduce one function to take care of sl_wifi_set_advanced_scan_configuration() and another one to take care of sl_si91x_configure_timeout().

Note, I understand SL_WIFI_(MAX|MIN)_(ACTIVE|PASSIVE)_SCAN_TIME_MS are there to avoid magic value in the code. However, since they are used only once in the code and their names do not help to understand from where these value are coming from, I think you could use raw numbers here (same comment for the default values for advanced_scan_config).

Got it, Updated the code as suggested

jerome-pouiller · 2025-04-02T16:32:07Z

-		sl_scan_config.type = SL_WIFI_SCAN_TYPE_ACTIVE;
-		ret = sl_si91x_configure_timeout(SL_SI91X_CHANNEL_ACTIVE_SCAN_TIMEOUT,
-						 z_scan_config->dwell_time_active);
+	if (sidev->state == WIFI_STATE_COMPLETED) {


I wonder if it wouldn't make sense to add a field int period to z_scan_config. We would start a background scan of period != 0. What do you think @jukkar ?

There isn't a bgscan support in struct wifi_scan_params, and if we want to add it, we need more than just period, no?

Indeed, we probably also need a couple of threshold values.

jerome-pouiller · 2025-04-02T16:33:25Z

+			advanced_scan_config.passive_channel_time =
+				z_scan_config->dwell_time_passive;
+		}
+		advanced_scan_config.passive_channel_time = SL_WIFI_ADV_PASSIVE_SCAN_DURATION;


Previous assignments to this field are probably useless.

Shifted the assignments of these fields at the declaration time.

jerome-pouiller · 2025-04-02T16:45:16Z

 		advanced_scan_config.enable_instant_scan = SL_WIFI_ENABLE_INSTANT_SCAN;
+		roam_configuration.trigger_level = SL_WIFI_NEVER_ROAM;
+
+#if CONFIG_WIFI_MGMT_ENABLE_ROAM


Use IS_ENABLE().

jerome-pouiller · 2025-04-02T16:46:35Z

+	help
+	  Enable this option to configure roaming parameters,
+	  improving connectivity and performance during
+	  transitions between access points.


Explain the roaming will be enable as soon as the background scan is enabled.

jerome-pouiller · 2025-04-02T16:50:14Z

+	bool "Roam with Deauth enable/disable"
+	depends on WIFI_MGMT_ENABLE_ROAM
+	help
+	  Enable this to roam using deauthentication frames.


Can you explain a bit how this option diverge of the usual roaming?

This enables roaming using Deauthentication frames, whereas, by default, the firmware performs roaming using Null data packets

jerome-pouiller · 2025-04-02T16:51:40Z

+	depends on WIFI_MGMT_ENABLE_ROAM
+	help
+	  Sets the default roam threshold. Lower values trigger
+	  earlier roaming; higher values delay it.


The difference with the previous option is unclear.

Trigger level - determines when to start searching for a new AP. Also known as Roaming threshold.

Trigger level change - determines the signal difference needed to switch to a new AP. Also known as Roaming Hysteresis. It tells when to actually switch to a new AP.

So, In short roam threshold specifies the signal strength at which the device begins searching for a better access point, while roam hysteresis ensures the new access point's signal is sufficiently stronger than the current one to avoid unnecessary or frequent switching.

jerome-pouiller · 2025-04-02T16:54:54Z

+		/*
+		 * Use legacy roam configuration values if enabled,
+		 * otherwise use user-defined values
+		 */


I don't understand this comment. I don't see when legacy values are used.

Removed the comment

krish2718

Can you please split the commit cac7460 in to two? implement dwell times and adding bgscan?

jerome-pouiller · 2025-04-04T14:16:50Z

+#define SL_WIFI_ADV_PASSIVE_SCAN_DURATION    20
+#define SL_WIFI_ADV_MULTIPROBE               0
+#define SL_WIFI_ADV_SCAN_PERIODICITY         10
+#define SL_WIFI_ENABLE_INSTANT_SCAN          1


You don't need to keep them in the header file.

In addition, I am not sure they are helpful.

Yes, they are useful because they define the parameters for background scanning, which controls how it works. As these parameters are set in application only with recommended values, so I moved them to the header file. What do you suggest ?

Without the context of the code, the user won't be able to modify these values anyway. So:

Either a precise documentation have to be provided (and the values can migrate to Kconfig)

Or the value are directly used in the code and the user have the whole context to figure out the impact.

(Note, my comment only apply because these symbols are only used once and in the context of the initialisation of a struct)

We documented the BG Scan values and shifted parameters to Kconfig file

jerome-pouiller · 2025-04-04T14:25:06Z

+			if (dwell_time_active < 5 || dwell_time_active > 1000) {
+				LOG_ERR("Invalid active scan dwell time");
+				return -EINVAL;
+			}


You can probably factorize this code at the top of the function:

if (dwell_time_active && (dwell_time_active < 5 || dwell_time_active > 1000)) { return -EINVAL; } if (dwell_time_passive && (dwell_time_passive < 10 || dwell_time_passive > 1000)) { return -EINVAL; }

jerome-pouiller · 2025-04-04T14:27:11Z

 	if (sidev->state == WIFI_STATE_COMPLETED) {
+		siwx91x_configure_scan_dwell_time(
+			SL_WIFI_SCAN_TYPE_ADV_SCAN, z_scan_config->dwell_time_active,
+			z_scan_config->dwell_time_passive, &advanced_scan_config);


Align to the opening parenthese:

siwx91x_configure_scan_dwell_time(SL_WIFI_SCAN_TYPE_ADV_SCAN, z_scan_config->dwell_time_active, z_scan_config->dwell_time_passive, &advanced_scan_config);

This is done by Clang format or else I am getting compliance errors

Probably your line was too long. Is it still the case?

No, The issue got resolved, Updated the code.

jerome-pouiller · 2025-04-04T14:27:33Z

 			sl_scan_config.type = SL_WIFI_SCAN_TYPE_ACTIVE;
+			ret = siwx91x_configure_scan_dwell_time(
+				SL_WIFI_SCAN_TYPE_ACTIVE, z_scan_config->dwell_time_active,
+				z_scan_config->dwell_time_passive, NULL);


Align to the opening parenthesis.

This is done by Clang format or else I am getting compliance errors

What are the errors exactly? Too long lines? Seems like some refactoring to helper functions or rethinking the function flow to avoid deep indentation would be a good idea.

The error was due to a line being too long, but it was resolved, and I updated the code as recommended.

jerome-pouiller · 2025-04-09T08:59:08Z

+		} else {
+			sl_scan_config.type = SL_WIFI_SCAN_TYPE_PASSIVE;
+		}
+


A blank line before a closing brace is probably not useful.

- Added support for background scan and configured the default values of BG scan configuration Signed-off-by: Nitin Pandey <nitin.pandey@silabs.com>

- Verified and configured scan dwell time for active, passive and advance scan Signed-off-by: Nitin Pandey <nitin.pandey@silabs.com>

- Defined Kconfig macros for Roam config - Added set roam configuration API call after BGSCAN Signed-off-by: Nitin Pandey <nitin.pandey@silabs.com>

Nitin-Pandey-01 force-pushed the add/bg-scan-handling branch 2 times, most recently from 3bf25a8 to 2f3c6fb Compare March 13, 2025 11:34

silabs-MuzaffarA reviewed Mar 19, 2025

View reviewed changes

Comment thread drivers/wifi/siwx91x/siwx91x_wifi.c Outdated

Nitin-Pandey-01 force-pushed the add/bg-scan-handling branch from 2f3c6fb to f669acc Compare March 20, 2025 09:41

Nitin-Pandey-01 force-pushed the add/bg-scan-handling branch 3 times, most recently from 94605f3 to 56e9fce Compare April 1, 2025 10:13

Nitin-Pandey-01 changed the title ~~Add support for BG Scan~~ Add support for BG Scan and Roaming Apr 1, 2025

silabs-NiteshS reviewed Apr 1, 2025

View reviewed changes

Comment thread soc/silabs/silabs_siwx91x/siwg917/nwp.c Outdated

Nitin-Pandey-01 force-pushed the add/bg-scan-handling branch 4 times, most recently from 195ba97 to 77d28b4 Compare April 1, 2025 13:18

Nitin-Pandey-01 marked this pull request as ready for review April 1, 2025 13:27

github-actions Bot added platform: Silabs Silicon Labs area: Wi-Fi Wi-Fi labels Apr 1, 2025

github-actions Bot requested review from MaochenWang1, Martinhoff-maker, jhedberg, jukkar and krish2718 April 1, 2025 13:28

github-actions Bot assigned jhedberg and asmellby Apr 1, 2025

jhedberg changed the title ~~Add support for BG Scan and Roaming~~ drivers: wifi: siwx91x: Add support for BG Scan and Roaming Apr 1, 2025

Nitin-Pandey-01 force-pushed the add/bg-scan-handling branch from 77d28b4 to 0ab3434 Compare April 2, 2025 05:51

asmellby requested a review from jerome-pouiller April 2, 2025 08:36

jerome-pouiller reviewed Apr 2, 2025

View reviewed changes

krish2718 reviewed Apr 3, 2025

View reviewed changes

Nitin-Pandey-01 force-pushed the add/bg-scan-handling branch 2 times, most recently from b71b1d7 to 1848d2b Compare April 4, 2025 11:12

Nitin-Pandey-01 requested a review from jerome-pouiller April 4, 2025 11:16

jerome-pouiller reviewed Apr 4, 2025

View reviewed changes

Nitin-Pandey-01 force-pushed the add/bg-scan-handling branch from 1848d2b to 91270cd Compare April 7, 2025 05:07

github-actions Bot requested review from Kludentwo, asmellby and rlubos April 7, 2025 05:08

Nitin-Pandey-01 requested a review from jerome-pouiller April 7, 2025 05:18

jerome-pouiller reviewed Apr 7, 2025

View reviewed changes

Comment thread drivers/wifi/siwx91x/siwx91x_wifi.c

Nitin-Pandey-01 force-pushed the add/bg-scan-handling branch 3 times, most recently from fee186b to ac0034f Compare April 9, 2025 04:02

Nitin-Pandey-01 requested a review from jerome-pouiller April 9, 2025 04:11

jerome-pouiller reviewed Apr 9, 2025

View reviewed changes

nipandey-01 added 3 commits April 12, 2025 11:10

driver: wifi: siwx91x: Add support for BG Scan

3493121

- Added support for background scan and configured the default values of BG scan configuration Signed-off-by: Nitin Pandey <nitin.pandey@silabs.com>

driver: wifi: siwx91x: Implement scan dwell time

fb479b6

- Verified and configured scan dwell time for active, passive and advance scan Signed-off-by: Nitin Pandey <nitin.pandey@silabs.com>

driver: wifi: siwx91x: Add roaming configuration

79c359e

- Defined Kconfig macros for Roam config - Added set roam configuration API call after BGSCAN Signed-off-by: Nitin Pandey <nitin.pandey@silabs.com>

Nitin-Pandey-01 force-pushed the add/bg-scan-handling branch from ac0034f to 79c359e Compare April 12, 2025 05:43

Nitin-Pandey-01 requested a review from jerome-pouiller April 12, 2025 05:46

jerome-pouiller approved these changes Apr 14, 2025

View reviewed changes

jhedberg approved these changes Apr 14, 2025

View reviewed changes

kartben merged commit 50e3609 into zephyrproject-rtos:main Apr 14, 2025
25 checks passed

Conversation

Nitin-Pandey-01 commented Mar 13, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerome-pouiller Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krish2718 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jerome-pouiller Apr 2, 2025 •

edited

Loading