Skip to content
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.

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

modules: hostap: 11k/v/r roaming support #78684

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nxf58150
Copy link
Contributor

Added 11k/v/r roaming support.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Sep 19, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hostap zephyrproject-rtos/hostap@f6792cb zephyrproject-rtos/hostap#38 zephyrproject-rtos/hostap#38/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hostap DNM This PR should not be merged (Do Not Merge) labels Sep 19, 2024
@decsny decsny removed their request for review September 20, 2024 16:58
@nxf58150
Copy link
Contributor Author

The 11kvr roaming RFC link: #76307

Comment on lines 1113 to 1114
if (params && strlen(params->ssid)) {
if (strlen(params->ssid) > WIFI_SSID_MAX_LEN) {
Copy link
Member

Choose a reason for hiding this comment

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

We could use here a temporary variable for ssid length, now the length is calculated three times in this function which is pointless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a local variable for ssid length. Thanks!


context.sh = sh;

if (argc != 2) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we show the current 11k status if user does not give a parameter. And only show the warning if user gives wrong number of parameters.

Copy link
Contributor Author

@nxf58150 nxf58150 Sep 26, 2024

Choose a reason for hiding this comment

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

Updated. The new command "wifi 11k" will be used to get 11k status (enabled or disabled) if no parameter was given.

@@ -2909,6 +2978,10 @@ SHELL_STATIC_SUBCMD_SET_CREATE(wifi_commands,
cmd_wifi_btm_query,
2, 0),
#endif
SHELL_CMD_ARG(11k_enable, NULL, "<0/1>\n",
Copy link
Member

Choose a reason for hiding this comment

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

I am not a big fan of just giving 0/1 here, it would be more logical if user could just say "enable" or "disable".
So wifi 11k enable or wifi 11k disable or similar.

Copy link
Contributor Author

@nxf58150 nxf58150 Sep 26, 2024

Choose a reason for hiding this comment

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

Updated. The new command "wifi 11k [enable/disable]" can be used to enable or disable 11k, instead of giving paramter 0/1.

}
(void)memcpy((void *)params.ssid, (const void *)argv[2],
(size_t)strlen(argv[2]));

Copy link
Member

Choose a reason for hiding this comment

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

Remove extra empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Comment on lines 1085 to 1140
if (argc == 3)
PR("%s %s %s requested\n", argv[0], argv[1], argv[2]);
else
PR("%s requested\n", argv[0]);
Copy link
Member

Choose a reason for hiding this comment

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

{} are missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added {}

if (!wpa_cli_cmd_v("set_network %d key_mgmt WPA-PSK",
resp.network_id)) {
if (!wpa_cli_cmd_v("set_network %d key_mgmt WPA-PSK%s",
resp.network_id, params->ft_used ? " FT-PSK" : "")) {
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@@ -249,6 +249,9 @@ if WIFI_NM_WPA_SUPPLICANT_CRYPTO_ENTERPRISE
default 16384
endif

config WIFI_NM_WPA_SUPPLICANT_ROAMING
bool "Roaming support"
Copy link
Member

Choose a reason for hiding this comment

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

Could there be a help text also describing the supported roaming functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added help text.

@@ -75,6 +75,8 @@ static void supp_shell_connect_status(struct k_work *work);
static K_WORK_DELAYABLE_DEFINE(wpa_supp_status_work,
supp_shell_connect_status);

#define STR_CUR_TO_END(cur) (cur) = (&(cur)[0] + strlen((cur)))
Copy link
Member

Choose a reason for hiding this comment

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

This causes the length to be calculated multiple times when this is called, why not have a len parameter in this macro and calculate the value only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This macro define is not created in this PR. I just moved it to this location. This macro define was created and used for DPP feature.

Comment on lines 1156 to 1158
char cmd[100] = {0};
char *pos = cmd;
char *end = pos + 100;
Copy link
Member

Choose a reason for hiding this comment

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

Could you have a define for the array length, or alternatively use sizeof. It is error prone to hard code the value as they can become mismatched if changed later.
So we could say here char *end = pos + sizeof(cmd);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Added new macro define for the array length.

@@ -73,6 +73,8 @@ CONFIG_ZPERF_WORK_Q_THREAD_PRIORITY=3
CONFIG_NET_SOCKETS_SERVICE_THREAD_PRIO=3
Copy link
Member

Choose a reason for hiding this comment

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

In commit subsys: net: Increase net_mgmt task priority, please remove the subsys: prefix as that is not needed to mention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed subsys: in commit message.

@nxf58150 nxf58150 force-pushed the roaming_support branch 4 times, most recently from f9f47bd to e8bd2a4 Compare September 27, 2024 02:18
Add 11k cmds support. User can issue 11k cmds to enable/disable 11k and
send Neighbor Report Request packet.

Signed-off-by: Hui Bai <hui.bai@nxp.com>
Add 80211R support in hostap.
Add cmd wifi connect option '-R' to enable 80211R.

Signed-off-by: Hui Bai <hui.bai@nxp.com>
Added new ops and events in glue layer to support roaming.
Added new flag WIFI_NM_WPA_SUPPLICANT_ROAMING to control roaming
feature.

Signed-off-by: Hui Bai <hui.bai@nxp.com>
Originally, the net_mgmt task priority is very low. Based on roaming
implementation, roaming is triggered in net_mgmt task. When running UDP
test while doing roaming, the net_mgmt task won't have much chance to
run and roaming can't be triggered.
Increase it to 3, which is same value of supplicant task.

Signed-off-by: Hui Bai <hui.bai@nxp.com>
Update hostap repo in west.yml

Signed-off-by: Hui Bai <hui.bai@nxp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking area: Samples Samples area: Wi-Fi Wi-Fi DNM This PR should not be merged (Do Not Merge) manifest manifest-hostap platform: NXP Drivers NXP Semiconductors, drivers
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants