Add AP mode support and error propagation improvements #87

ArunmaniAlagarsamy2710 · 2025-01-30T15:43:11Z

This PR adds AP mode support to the siwx917 Wi-Fi driver, enabling commands to manage the AP and connected stations. It also introduces error handling to map WiseConnect SDK error codes to corresponding Zephyr error codes, ensuring consistent and meaningful error reporting.

jhedberg · 2025-01-31T12:42:21Z

drivers/wifi/siwx917/Kconfig.siwx917

+config WIFI_SIWX917_AP_MODE
+	bool "Enable AP Mode"
+	help
+	 Enable this option to allow the device to operate in
+	 Access Point (AP) mode instead of the default Station (STA) mode.
+
+config WIFI_MAX_STATIONS
+	int "Maximum number of stations supported by AP"
+	range 1 16
+	default 3
+	help
+	 Sets the maximum number of stations that the Access Point (AP) can support.
+
+config BEACON_INTERVAL
+	int "Beacon interval (in milliseconds)"
+	range 100 1000
+	default 100
+	help
+	 Defines the interval between beacon transmissions in AP mode.
+
+config DTIM_BEACON_COUNT


Shouldn't all of these have the WIFI_SIWX917_* prefix? All Kconfig symbols are globally visible, so it's important that

Option names follow strict namespacing rules

Options don't get unnecessarily enabled, i.e. they have the appropriate dependencies on higher level options

jhedberg · 2025-02-04T15:49:12Z

drivers/wifi/siwx917/siwx917_wifi.c

+		return -EINVAL;
+	}
+
+	strncpy(mac.octet, mac_addr, ARRAY_SIZE(mac.octet));


From what I can see from this APIs implementation upstream (the documentation itself is a bit unclear) mac_addr is not a string, so it's invalid and misleading to use str*() operations on it. Seems like you should be using memcpy() instead. Btw, this might be an opportunity to improve and clarify the upstream API, since AFAIK it's still experimental.

jerome-pouiller · 2025-02-05T11:01:15Z

drivers/wifi/siwx917/Kconfig.siwx917

+	depends on WIFI_SIWX917_AP_MODE
+	help
+	 Specifies the number of beacon intervals that must elapse
+	 before the AP sends buffered multicast/broadcast traffic.


An implementation of AP may provides tens of tuning options. Do we want to expose all of them in Kconfig? In addition, I feel it make sense (at least it does not consume more memory) to set these value during runtime.

So, maybe it is better to hardcode sane values and leave the opportunity to the user to patch the code if he really want to do something else?

I think it is not a blocker for now, because the number of options is limited. However, I would take care of the inflation of the number of options.

jerome-pouiller · 2025-02-05T11:07:15Z

drivers/wifi/siwx917/siwx917_wifi.c

+	configuration.beacon_stop = 0;
+	configuration.tdi_flags = SL_WIFI_TDI_NONE;
+	configuration.is_11n_enabled = 1;
+	configuration.ssid.length = params->ssid_length;


If new member are added to sl_wifi_ap_configuration_t, this code won't initialize them. It is better to use:

sl_wifi_ap_configuration_t configuration = { };

You can take this opportunity to initialize some members:

sl_wifi_ap_configuration_t configuration = { .channel.channel = params->channel, .encryption = SL_WIFI_CCMP_ENCRYPTION, .channel.band = SL_WIFI_BAND_2_4GHZ, ... };

It is not required to initialize beacon_stop or options since they are already 0.

Compile will complain __ASSERT() is called after access to params. Personally, I would use params without check (it's bug in the caller). However, it is up to you.

drivers/wifi/siwx917/siwx917_wifi.c

jerome-pouiller · 2025-02-05T11:11:49Z

drivers/wifi/siwx917/siwx917_wifi.c

+	case WIFI_SECURITY_TYPE_PSK:
+		configuration.security = SL_WIFI_WPA2;
+		sl_net_wifi_psk_credential_entry_t wifi_ap_credential = {
+			.type = SL_NET_WIFI_PSK, .data_length = params->psk_length};


Avoid statements after declarations.

BTW, what is purpose of this struct? Why not:

ret = sl_net_set_credential(SL_NET_DEFAULT_WIFI_AP_CREDENTIAL_ID, SL_NET_WIFI_PSK, params->psk, params->psk_length);

jerome-pouiller · 2025-02-05T11:23:11Z

drivers/wifi/siwx917/siwx917_wifi.c

@@ -322,7 +424,8 @@ static void siwx917_iface_init(struct net_if *iface)
 	sl_wifi_set_scan_callback(siwx917_on_scan, sidev);
 	sl_wifi_set_join_callback(siwx917_on_join, sidev);

-	status = sl_wifi_get_mac_address(SL_WIFI_CLIENT_INTERFACE, &sidev->macaddr);
+	sidev->interface = sl_wifi_get_default_interface();
+	status = sl_wifi_get_mac_address((sidev->interface & GET_INTERFACE), &sidev->macaddr);


I suggest FIELD_GET(GET_INTERFACE, sidev->interface). Also GET_INTERFACE is not well chosen. I should be prefixed by SIWX91X, so we know this value does not come zephyr of wiseconnect. Then, it should contains MASK rather than GET.

In addition, you mean the value returned sl_wifi_get_default_interface() can't be used as-is?

jerome-pouiller · 2025-02-05T11:38:51Z

drivers/wifi/siwx917/siwx917_wifi.c

+	ret = sl_wifi_get_wireless_info(&info);
+	if (ret) {
+		LOG_ERR("Failed to get the wireless info: 0x%x", ret);
+		return -ENOTSUP;


Is ENOTSUP the correct error? I believe this error is not expected whatever the context. I would use EIO in this case.

jerome-pouiller · 2025-02-05T11:41:30Z

drivers/wifi/siwx917/siwx917_wifi.c

+
+	strncpy(status->ssid, info.ssid, WIFI_SSID_MAX_LEN);
+	status->ssid_len = strnlen(info.ssid, WIFI_SSID_MAX_LEN);
+	status->ssid[status->ssid_len] = '\0';


ssid is declared as: char ssid[WIFI_SSID_MAX_LEN + 1];. So:

strncpy(status->ssid, info.ssid, WIFI_SSID_MAX_LEN); status->ssid_len = strlen(status->ssid);

jerome-pouiller · 2025-02-05T11:43:23Z

drivers/wifi/siwx917/siwx917_wifi.c

+		status->band = WIFI_FREQ_BAND_2_4_GHZ;
+	}
+
+	if (sidev->interface & SL_WIFI_CLIENT_INTERFACE) {


I am confused in use of sidev->interface. Is a field or an index?

jerome-pouiller · 2025-02-05T11:45:02Z

drivers/wifi/siwx917/siwx917_wifi.c

@@ -516,6 +638,9 @@ static const struct wifi_mgmt_ops siwx917_mgmt = {
 	.ap_sta_disconnect = siwx917_ap_sta_disconnect,
 #endif
 	.iface_status = siwx917_status,
+#if defined(CONFIG_NET_STATISTICS_WIFI)
+	.get_stats = siwx917_wifi_stats,


siwx917_stats is sufficient.

jerome-pouiller · 2025-02-05T11:46:03Z

drivers/wifi/siwx917/siwx917_wifi.h

+	 : (sdk_error) == SL_STATUS_ALLOCATION_FAILED     ? -ENOMEM                                \
+	 : (sdk_error) == SL_STATUS_NOT_AVAILABLE                                                  \
+		 ? -EBUSY                                                                          \
+		 : -EIO) /* Default mapping to EIO for unknown errors */


Why a macro?

This commit introduces Access Point (AP) mode support It enables the following AP-related commands: - ap enable: Enable the AP mode. - ap disable: Disable the AP mode. Signed-off-by: Arunmani Alagarsamy <[email protected]>

This commit adds support the following wifi commands: - wifi ap disconnect: Disconnect a specific station from the AP. - wifi ap stations: List connected devices. Signed-off-by: Arunmani Alagarsamy <[email protected]>

This commit adds support for Wi-Fi status and statistics functionalities: - wifi status: Displays the current AP status. - wifi statistics: Provides details on the transmitted and received packet counts. Signed-off-by: Arunmani Alagarsamy <[email protected]>

jerome-pouiller · 2025-02-12T15:04:15Z

drivers/wifi/siwx917/siwx917_wifi.c

+}
+
+static sl_status_t siwx917_on_connect(sl_wifi_event_t event, void *data,
+					      uint32_t data_length, void *arg)


Align with opened parenthesis.

jerome-pouiller · 2025-02-12T15:04:21Z

drivers/wifi/siwx917/siwx917_wifi.c

+}
+
+static sl_status_t siwx917_on_disconnect(sl_wifi_event_t event, void *data,
+						 uint32_t data_length, void *arg)


jerome-pouiller · 2025-02-12T15:07:13Z

drivers/wifi/siwx917/siwx917_wifi.c

+	return ret;
+}
+
+static sl_status_t siwx917_on_connect(sl_wifi_event_t event, void *data,


Because the reader may think this function is called when the swix connect, I suggest to name it siwx917_on_ap_sta_connect().

jerome-pouiller · 2025-02-12T15:09:13Z

drivers/wifi/siwx917/siwx917_wifi.c

@@ -500,6 +555,9 @@ static void siwx917_iface_init(struct net_if *iface)

 	sl_wifi_set_scan_callback(siwx917_on_scan, sidev);
 	sl_wifi_set_join_callback(siwx917_on_join, sidev);
+	sl_wifi_set_callback(SL_WIFI_CLIENT_CONNECTED_EVENTS, siwx917_on_connect, sidev);
+	sl_wifi_set_callback(SL_WIFI_CLIENT_DISCONNECTED_EVENTS, siwx917_on_disconnect,
+			     sidev);


For consistency with the new code, I suggest to change sl_wifi_set_scan_callback() and sl_wifi_set_join_callback() in sl_wifi_set_callback(SL_WIFI_SCAN_RESULT_EVENTS, ..) and sl_wifi_set_callback(SL_WIFI_JOIN_EVENTS, ..)

jerome-pouiller · 2025-02-12T15:18:21Z

drivers/wifi/siwx917/siwx917_wifi.c

+		sl_net_set_credential(configuration.credential_id,
+					default_wifi_ap_credential.type,
+					(const void *)default_wifi_ap_credential.data,
+					default_wifi_ap_credential.data_length);


Wiseconnect says:

#define DEFAULT_WIFI_AP_CREDENTIAL "MY_AP_PASSPHRASE" static sl_net_wifi_psk_credential_entry_t default_wifi_ap_credential = { .type = SL_NET_WIFI_PSK, .data_length = sizeof(DEFAULT_WIFI_AP_CREDENTIAL) - 1, .data = DEFAULT_WIFI_AP_CREDENTIAL };

Do we really want to use these values?

jerome-pouiller · 2025-02-12T15:25:23Z

drivers/wifi/siwx917/siwx917_wifi.c

+
+	case WIFI_SECURITY_TYPE_PSK_SHA256:
+		configuration.security = SL_WIFI_WPA2;
+		if (params->mfp != WIFI_MFP_REQUIRED) {


Are you sur the difference between WIFI_SECURITY_TYPE_PSK and WIFI_SECURITY_TYPE_PSK_SHA256 is the MFP? I thought it was the key derivation algorithm (PBKDF2 vs PBKDF2-HMAC-SHA256).

jerome-pouiller · 2025-02-12T15:42:52Z

drivers/wifi/siwx917/siwx917_wifi.c

-	status = sl_wifi_get_mac_address(SL_WIFI_CLIENT_INTERFACE, &sidev->macaddr);
+	sidev->interface = sl_wifi_get_default_interface();
+	status = sl_wifi_get_mac_address(FIELD_GET(SIWX917_INTERFACE_MASK, sidev->interface),
+					&sidev->macaddr);


I have to admit I am still a bit lost with the interfaces:

I seems sidev->interface is never used after siwx917_iface_init(). So, do we need to store it in sidev?

I believe at the end, this code will call sl_wifi_get_mac_address(SL_WIFI_CLIENT_INTERFACE, &sidev->macaddr), that's it? So, is it useful to call sl_wifi_get_default_interface()?

Sometime SL_WIFI_AP_2_4GHZ_INTERFACE is used, sometime SL_WIFI_AP_INTERFACE. To make thing easier, I suggest to avoid use of SL_WIFI_AP_2_4GHZ_INTERFACE and rather use SL_WIFI_AP_INTERFACE | SL_WIFI_2_4GHZ_INTERFACE (ditto for SL_WIFI_CLIENT_INTERFACE in the existing code).

jerome-pouiller · 2025-02-13T16:31:15Z

@ArunmaniAlagarsamy2710, upstream now support 917. Can you resend your PR to https://github.com/zephyrproject-rtos/zephyr ?

ArunmaniAlagarsamy2710 · 2025-02-18T16:00:36Z

Let's close this PR by opening to zephyrproject-rtos/zephyr#85906

ArunmaniAlagarsamy2710 requested a review from a team as a code owner January 30, 2025 15:43

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from b094454 to f813197 Compare January 30, 2025 16:10

ArunmaniAlagarsamy2710 marked this pull request as draft January 30, 2025 16:20

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from f813197 to d24c3e1 Compare January 31, 2025 07:20

jhedberg reviewed Jan 31, 2025

View reviewed changes

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch 2 times, most recently from 26e313e to 7b58b14 Compare February 3, 2025 12:47

jhedberg requested a review from jerome-pouiller February 3, 2025 13:41

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from 7b58b14 to ef3b648 Compare February 4, 2025 14:23

jhedberg requested changes Feb 4, 2025

View reviewed changes

jerome-pouiller requested changes Feb 5, 2025

View reviewed changes

ArunmaniAlagarsamy2710 added 3 commits February 11, 2025 18:50

drivers: wifi: siwx917: Add Access Point (AP) mode support

c08ae5b

This commit introduces Access Point (AP) mode support It enables the following AP-related commands: - ap enable: Enable the AP mode. - ap disable: Disable the AP mode. Signed-off-by: Arunmani Alagarsamy <[email protected]>

drivers: wifi: siwx917: Add AP station disconnect functionality

63eda93

This commit adds support the following wifi commands: - wifi ap disconnect: Disconnect a specific station from the AP. - wifi ap stations: List connected devices. Signed-off-by: Arunmani Alagarsamy <[email protected]>

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from d885219 to 0678bba Compare February 12, 2025 14:57

jerome-pouiller reviewed Feb 12, 2025

View reviewed changes

ArunmaniAlagarsamy2710 closed this Feb 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AP mode support and error propagation improvements #87

Add AP mode support and error propagation improvements #87

ArunmaniAlagarsamy2710 commented Jan 30, 2025

jhedberg Jan 31, 2025

jhedberg Feb 4, 2025

jerome-pouiller Feb 5, 2025

jerome-pouiller Feb 5, 2025

jerome-pouiller Feb 5, 2025

jerome-pouiller Feb 5, 2025

jerome-pouiller Feb 5, 2025

jerome-pouiller Feb 5, 2025

jerome-pouiller Feb 5, 2025

jerome-pouiller Feb 5, 2025

jerome-pouiller Feb 5, 2025

jerome-pouiller Feb 12, 2025

jerome-pouiller Feb 12, 2025

jerome-pouiller Feb 12, 2025

jerome-pouiller Feb 12, 2025

jerome-pouiller Feb 12, 2025

jerome-pouiller Feb 12, 2025

jerome-pouiller Feb 12, 2025

jerome-pouiller commented Feb 13, 2025

ArunmaniAlagarsamy2710 commented Feb 18, 2025

Add AP mode support and error propagation improvements #87

Add AP mode support and error propagation improvements #87

Conversation

ArunmaniAlagarsamy2710 commented Jan 30, 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

jerome-pouiller commented Feb 13, 2025

ArunmaniAlagarsamy2710 commented Feb 18, 2025