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

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

Add error handling to map WiseConnect SDK error codes to their corresponding zephyr error codes. This ensures that the driver returns meaningful and standardized error codes to the application. This change improves error propagation and allows applications to handle Wi-Fi driver errors more effectively. Signed-off-by: Arunmani Alagarsamy <[email protected]>

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.

jerome-pouiller · 2025-02-05T11:09:06Z

drivers/wifi/siwx917/siwx917_wifi.c

+	default:
+		LOG_ERR("Invalid bandwidth");
+		return -EAGAIN;
+	}


I suggest to introduce a function (we already use this pattern in DMA and other drivers) for this kind of conversion of value.

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?

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 added 4 commits February 4, 2025 12:20

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

62333f0

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

90b4d62

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

Merge branch 'SiliconLabsSoftware:main' into driver/access_point

d885219

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 5, 2025

Add AP mode support and error propagation improvements #87

Are you sure you want to change the base?

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