wifi: TWT improvements #82312

Rex-Chen-NXP · 2024-11-29T06:53:44Z

A. Add two new parameters for TWT setup
B. Fix TWT quick setup: Add default exponent value
C. Add Broadcast TWT feature support for AP
D. Convert TWT command to use getopt

krish2718 · 2024-11-29T07:57:01Z

Please update the title and add description to explain these set of changes.

Rex-Chen-NXP · 2024-11-29T08:06:02Z

Please update the title and add description to explain these set of changes.

Yes, of course, updated title and changes details , changes info also in separated PRs commit mesg

Rex-Chen-NXP · 2024-12-02T02:22:51Z

Hi @krish2718 @jukkar
Could you help to review?

jukkar · 2024-12-02T08:33:42Z

subsys/net/l2/wifi/wifi_shell.c

@@ -3184,15 +3200,16 @@ SHELL_STATIC_SUBCMD_SET_CREATE(wifi_twt_ops,
 		"<setup_cmd: 0: Request, 1: Suggest, 2: Demand>\n"
 		"<dialog_token: 1-255> <flow_id: 0-7> <responder: 0/1> <trigger: 0/1> <implicit:0/1> "
 		"<announce: 0/1> <twt_wake_interval: 1-262144us> <twt_interval: 1us-2^31us>.\n"
-		"<twt_wake_ahead_duration>: 0us-2^31us>\n",
+		"<twt_wake_ahead_duration>: 0us-2^31us>\n"


We are getting now quite many positional parameters for these commands and it looks like not very user friendly way to implement this. Could this changed to use parameter "keywords", so something like -s, --setup <cmd> .... so like what we have in other commands?

Yes, of course, will implement as comment.

Hi @jukkar Updated as comment, could you review again?

Rex-Chen-NXP · 2024-12-04T02:55:57Z

Hi @krish2718 @jukkar
Could you help review.
Thanks!

jukkar · 2024-12-04T13:53:54Z

subsys/net/l2/wifi/wifi_shell.c

-	params.negotiation_type = (enum wifi_twt_negotiation_type)value;
+		case 'c':
+			if (!parse_number(sh, &value, state->optarg, NULL,
+			    WIFI_TWT_SETUP_CMD_REQUEST, WIFI_TWT_SETUP_CMD_DEMAND)) {


Minor nit, but the indentation is wrong here and also for the other added lines.
So here the code should look like

if (!parse_number(sh, &value, state->optarg, NULL, WIFI_TWT_SETUP_CMD_REQUEST, WIFI_TWT_SETUP_CMD_DEMAND)) {

Is it possible to adjust your editor to indent the lines automatically?

Thanks, all updated as comment

jukkar · 2024-12-04T13:55:30Z

subsys/net/l2/wifi/wifi_shell.c

-	params.dialog_token = (uint8_t)value;
+		case 'f':
+			if (!parse_number(sh, &value, state->optarg, NULL, 0,
+			    (WIFI_MAX_TWT_FLOWS - 1))) {


indentation

updated as comment

jukkar · 2024-12-04T13:55:44Z

subsys/net/l2/wifi/wifi_shell.c

-	params.setup.announce = (bool)value;
+		case 'w':
+			if (!parse_number(sh, &value, state->optarg, NULL, 1,
+			    WIFI_MAX_TWT_WAKE_INTERVAL_US)) {


indentation

updated as comment

jukkar · 2024-12-04T13:56:00Z

subsys/net/l2/wifi/wifi_shell.c

-	params.setup.twt_wake_interval = (uint32_t)value;
+		case 'i':
+			if (!parse_number(sh, &value, state->optarg, NULL, 1,
+			    WIFI_MAX_TWT_INTERVAL_US)) {


indentation

updated as comment

jukkar · 2024-12-04T13:56:05Z

subsys/net/l2/wifi/wifi_shell.c

-		return -EINVAL;
+		case 'D':
+			if (!parse_number(sh, &value, state->optarg, NULL, 0,
+			  WIFI_MAX_TWT_WAKE_AHEAD_DURATION_US)) {


indentation

updated as comment

jukkar · 2024-12-04T13:56:14Z

subsys/net/l2/wifi/wifi_shell.c

+
+		case 'e':
+			if (!parse_number(sh, &value, state->optarg, NULL, 0,
+			    WIFI_MAX_TWT_EXPONENT)) {


indentation

updated as comment

krish2718 · 2024-12-09T10:02:52Z

Hi @krish2718 @jukkar I want to insert other discussion about parameters usage of twt setup cmd. The below two usage items, from the value range and spec description , 'twt_wake_interval' should named as 'TWT Wake Duration', and 'twt_interval' should calculated from TWT Wake Interval(TWT Wake Interval Mantissa) and Exponent, so name as 'TWT Wake Interval' will be suitable. A. <twt_wake_interval: 1-262144us> B. <twt_interval: 1us-2^31us>
As I didn't find description about 'twt_interval' from spec, if my above suggestion and understanding is error, pls tell me. If you agree, I will raise other PR to change the usage after this PR merged. Thanks.

Please see https://github.com/zephyrproject-rtos/zephyr/pull/82312/files#r1875537287 , it's about keeping UI simple.

Sure, even if we don't use exponent, but the range of 'twt_wake_interval' is matched with 'TWT Wake Duration', so should we change the usage to 'TWT Wake Duration', twt_wake_interval make user confuse.

twt_wake_interval is mapped to Nominal Minimum TWT Wake Duration field and in 802.11ax-2021, the max value is https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/net/wifi_mgmt.h#L765 i.e., Wake Duration Unit subfield == 1 and Nominal Minimum TWT Wake Duration field == 256 , can you please let me know how the range is incorrect? Please note that depending on the user value, the Wake Duration Unit subfield == 1 is auto-encoded to 0/1 in the driver/firmware.

Yes, I mean why don't named as 'TWT Wake Duration' directly?

Sure we can rename it to twt_wake_duration. But it would be a breaking API change.

Okay, will raise PR later after this PR merged.

Please note we have to follow the rules here for any stable API https://docs.zephyrproject.org/latest/develop/api/api_lifecycle.html but I guess there aren't many users of TWT, so, not sure if we should call it stable or not. @jukkar WDYT?

Rex-Chen-NXP · 2024-12-10T02:49:03Z

Hi @krish2718 @jukkar @MaochenWang1
Follow @krish2718 comment to remove teardown changes, and there is no conclusion of 'exponent ', so keep exponent as shell input.

krish2718 · 2024-12-10T06:42:15Z

Hi @krish2718 @jukkar @MaochenWang1
Follow @krish2718 comment to remove teardown changes, and there is no conclusion of 'exponent ', so keep exponent as shell input.

Should we add both the options? If the user gives time we derive (exponent, mantissa) and if the user gives (exponent, mantissa) we use it directly, WDYT?

Rex-Chen-NXP · 2024-12-10T07:00:38Z

Hi @krish2718 @jukkar @MaochenWang1
Follow @krish2718 comment to remove teardown changes, and there is no conclusion of 'exponent ', so keep exponent as shell input.

Should we add both the options? If the user gives time we derive (exponent, mantissa) and if the user gives (exponent, mantissa) we use it directly, WDYT?

My concern is exist multiple sets of exponent and mantissa derived from a given time, and the time calculated from the derived exponent and mantissa may not equal to the given time. Customer may complain about why time set not as expectation.

krish2718 · 2024-12-10T07:02:50Z

Hi @krish2718 @jukkar @MaochenWang1
Follow @krish2718 comment to remove teardown changes, and there is no conclusion of 'exponent ', so keep exponent as shell input.

Should we add both the options? If the user gives time we derive (exponent, mantissa) and if the user gives (exponent, mantissa) we use it directly, WDYT?

My concern is exist multiple sets of exponent and mantissa derived from a given time, and the time calculated from the derived exponent and mantissa may not equal to the given time. Customer may complain about why time set not as expectation.

We share the (exponent, mantissa) in the frame to the AP, so, at least AP and STA will have the same time. Anyways giving both options is better we can leave it to the users to choose the appropriate one, no?

Rex-Chen-NXP · 2024-12-10T07:10:57Z

Hi @krish2718 @jukkar @MaochenWang1
Follow @krish2718 comment to remove teardown changes, and there is no conclusion of 'exponent ', so keep exponent as shell input.

Should we add both the options? If the user gives time we derive (exponent, mantissa) and if the user gives (exponent, mantissa) we use it directly, WDYT?

My concern is exist multiple sets of exponent and mantissa derived from a given time, and the time calculated from the derived exponent and mantissa may not equal to the given time. Customer may complain about why time set not as expectation.

We share the (exponent, mantissa) in the frame to the AP, so, at least AP and STA will have the same time. Anyways giving both options is better we can leave it to the users to choose the appropriate one, no?

Okay, how can we derive a set of exponent and mantissa from a given time with high accuracy, is there any algorithm for refer?

krish2718 · 2024-12-10T07:19:18Z

Hi @krish2718 @jukkar @MaochenWang1
Follow @krish2718 comment to remove teardown changes, and there is no conclusion of 'exponent ', so keep exponent as shell input.

Should we add both the options? If the user gives time we derive (exponent, mantissa) and if the user gives (exponent, mantissa) we use it directly, WDYT?

My concern is exist multiple sets of exponent and mantissa derived from a given time, and the time calculated from the derived exponent and mantissa may not equal to the given time. Customer may complain about why time set not as expectation.

We share the (exponent, mantissa) in the frame to the AP, so, at least AP and STA will have the same time. Anyways giving both options is better we can leave it to the users to choose the appropriate one, no?

Okay, how can we derive a set of exponent and mantissa from a given time with high accuracy, is there any algorithm for refer?

We took the simpler approach: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/wifi/nrf_wifi/src/wifi_mgmt.c#L250

krish2718 · 2024-12-10T07:22:11Z

Hi @krish2718 @jukkar @MaochenWang1
Follow @krish2718 comment to remove teardown changes, and there is no conclusion of 'exponent ', so keep exponent as shell input.

Should we add both the options? If the user gives time we derive (exponent, mantissa) and if the user gives (exponent, mantissa) we use it directly, WDYT?

My concern is exist multiple sets of exponent and mantissa derived from a given time, and the time calculated from the derived exponent and mantissa may not equal to the given time. Customer may complain about why time set not as expectation.

We share the (exponent, mantissa) in the frame to the AP, so, at least AP and STA will have the same time. Anyways giving both options is better we can leave it to the users to choose the appropriate one, no?

Okay, how can we derive a set of exponent and mantissa from a given time with high accuracy, is there any algorithm for refer?

We took the simpler approach: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/wifi/nrf_wifi/src/wifi_mgmt.c#L250

Sonnet 3.5 gave this for binary scaling input.

def convert_time_to_mantissa_exponent(time_ms):
    """
    Convert time in milliseconds to mantissa and exponent representation
    
    Args:
        time_ms (float): Time in milliseconds
    
    Returns:
        tuple: (mantissa, exponent)
    """
    # Start with base-2 exponent of 0
    exponent = 0
    mantissa = time_ms
    
    # Reduce mantissa by dividing by 2 and incrementing exponent
    while mantissa > 255:  # Typical max mantissa value
        mantissa /= 2
        exponent += 1
    
    return (int(mantissa), exponent)

# Example usage
time = 50  # milliseconds
mantissa, exponent = convert_time_to_mantissa_exponent(time)
print(f"Original time: {time}ms")
print(f"Mantissa: {mantissa}")
print(f"Exponent: {exponent}")
print(f"Reconstructed time: {mantissa * (2 ** exponent)}ms")

Rex-Chen-NXP · 2024-12-10T07:46:17Z

Hi @krish2718 @jukkar @MaochenWang1
Follow @krish2718 comment to remove teardown changes, and there is no conclusion of 'exponent ', so keep exponent as shell input.

Should we add both the options? If the user gives time we derive (exponent, mantissa) and if the user gives (exponent, mantissa) we use it directly, WDYT?

My concern is exist multiple sets of exponent and mantissa derived from a given time, and the time calculated from the derived exponent and mantissa may not equal to the given time. Customer may complain about why time set not as expectation.

We share the (exponent, mantissa) in the frame to the AP, so, at least AP and STA will have the same time. Anyways giving both options is better we can leave it to the users to choose the appropriate one, no?

Okay, how can we derive a set of exponent and mantissa from a given time with high accuracy, is there any algorithm for refer?

We took the simpler approach: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/wifi/nrf_wifi/src/wifi_mgmt.c#L250

Sonnet 3.5 gave this for binary scaling input.
def convert_time_to_mantissa_exponent(time_ms):
    """
    Convert time in milliseconds to mantissa and exponent representation
    
    Args:
        time_ms (float): Time in milliseconds
    
    Returns:
        tuple: (mantissa, exponent)
    """
    # Start with base-2 exponent of 0
    exponent = 0
    mantissa = time_ms
    
    # Reduce mantissa by dividing by 2 and incrementing exponent
    while mantissa > 255:  # Typical max mantissa value
        mantissa /= 2
        exponent += 1
    
    return (int(mantissa), exponent)

# Example usage
time = 50  # milliseconds
mantissa, exponent = convert_time_to_mantissa_exponent(time)
print(f"Original time: {time}ms")
print(f"Mantissa: {mantissa}")
print(f"Exponent: {exponent}")
print(f"Reconstructed time: {mantissa * (2 ** exponent)}ms")

Thanks for sharing, as Nordic driver implemented the convert of (exponent, mantissa, interval) , and need interval as argv, to compatible Nordic's exist implementation, the shell should prepare all of parameters(exponent, mantissa, interval) according to the input (exponent, mantissa) or interval, right? If you agree, I will make change.

Rex-Chen-NXP · 2024-12-11T07:31:04Z

Hi @krish2718 @jukkar @MaochenWang1
Follow @krish2718 comments to support both interval or (exponent, mantissa) as input parameters, format check pass in local, test pass base on RW612 board, could help review after CI check pass?

subsys/net/l2/wifi/wifi_shell.c

Enhance the parse parameters code for twt. Signed-off-by: Rex Chen <[email protected]>

Add btwt_setup cmd for sap. Signed-off-by: Rex Chen <[email protected]>

Add two parameters for twt setup. Signed-off-by: Rex Chen <[email protected]>

The default exponent is 0, will cause twt setup quick failed, set exponent value derived from twt interval to fix it. Signed-off-by: Rex Chen <[email protected]>

Rex-Chen-NXP · 2024-12-12T04:56:58Z

Hi @krish2718 @jukkar
As CI check failed early, I rebased for PR, could you help review?

Rex-Chen-NXP · 2024-12-16T03:25:49Z

Hi @jukkar @dleach02
Could you help review and merge PR?

zephyrbot added area: Wi-Fi Wi-Fi area: Networking platform: NXP Drivers NXP Semiconductors, drivers labels Nov 29, 2024

zephyrbot requested review from danieldegrasse, dbaluta, decsny, dleach02, jukkar, Kludentwo, krish2718, manuargue, MaochenWang1, MarkWangChinese, mmahadevan108 and rlubos November 29, 2024 06:54

zephyrbot assigned jukkar Nov 29, 2024

Rex-Chen-NXP changed the title ~~Main twt~~ twt support for zephyr L2 cmd Nov 29, 2024

MaochenWang1 previously approved these changes Dec 2, 2024

View reviewed changes

jukkar reviewed Dec 2, 2024

View reviewed changes

Rex-Chen-NXP dismissed MaochenWang1’s stale review via 97a92a2 December 3, 2024 08:27

Rex-Chen-NXP force-pushed the main_twt branch 4 times, most recently from d46163d to 13ac64e Compare December 3, 2024 08:37

MaochenWang1 previously approved these changes Dec 3, 2024

View reviewed changes

jukkar reviewed Dec 4, 2024

View reviewed changes

Rex-Chen-NXP force-pushed the main_twt branch from 77c69bf to 6091204 Compare December 10, 2024 02:42

decsny removed their request for review December 10, 2024 14:31

Rex-Chen-NXP force-pushed the main_twt branch from 6091204 to ba07eb9 Compare December 11, 2024 07:25

krish2718 reviewed Dec 11, 2024

View reviewed changes

subsys/net/l2/wifi/wifi_shell.c Outdated Show resolved Hide resolved

subsys/net/l2/wifi/wifi_shell.c Outdated Show resolved Hide resolved

Rex-Chen-NXP force-pushed the main_twt branch 2 times, most recently from 9b152d2 to 117b20d Compare December 11, 2024 09:13

MaochenWang1 requested review from krish2718 and jukkar December 11, 2024 11:04

MaochenWang1 approved these changes Dec 11, 2024

View reviewed changes

Rex-Chen-NXP added 4 commits December 12, 2024 11:16

net: wifi: shell: enhance twt parse parameters code

0d7720d

Enhance the parse parameters code for twt. Signed-off-by: Rex Chen <[email protected]>

net: wifi: shell: add btwt feature support

6e9c34e

Add btwt_setup cmd for sap. Signed-off-by: Rex Chen <[email protected]>

net: wifi: shell: add parameters for twt setup

4f51128

Add two parameters for twt setup. Signed-off-by: Rex Chen <[email protected]>

net: wifi: shell: fix twt setup quick failed issue

be24089

The default exponent is 0, will cause twt setup quick failed, set exponent value derived from twt interval to fix it. Signed-off-by: Rex Chen <[email protected]>

Rex-Chen-NXP force-pushed the main_twt branch from 117b20d to be24089 Compare December 12, 2024 02:17

krish2718 approved these changes Dec 12, 2024

View reviewed changes

jukkar approved these changes Dec 16, 2024

View reviewed changes

kartben merged commit ab948b0 into zephyrproject-rtos:main Dec 16, 2024
25 checks passed

Rex-Chen-NXP deleted the main_twt branch December 17, 2024 03:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wifi: TWT improvements #82312

wifi: TWT improvements #82312

Rex-Chen-NXP commented Nov 29, 2024 •

edited by krish2718

Loading

krish2718 commented Nov 29, 2024

Rex-Chen-NXP commented Nov 29, 2024

Rex-Chen-NXP commented Dec 2, 2024

jukkar Dec 2, 2024

Rex-Chen-NXP Dec 2, 2024

Rex-Chen-NXP Dec 3, 2024

Rex-Chen-NXP commented Dec 4, 2024

jukkar Dec 4, 2024

Rex-Chen-NXP Dec 5, 2024

jukkar Dec 4, 2024

Rex-Chen-NXP Dec 5, 2024

jukkar Dec 4, 2024

Rex-Chen-NXP Dec 5, 2024

jukkar Dec 4, 2024

Rex-Chen-NXP Dec 5, 2024

jukkar Dec 4, 2024

Rex-Chen-NXP Dec 5, 2024

jukkar Dec 4, 2024

Rex-Chen-NXP Dec 5, 2024

krish2718 commented Dec 9, 2024

Rex-Chen-NXP commented Dec 10, 2024

krish2718 commented Dec 10, 2024

Rex-Chen-NXP commented Dec 10, 2024

krish2718 commented Dec 10, 2024

Rex-Chen-NXP commented Dec 10, 2024 •

edited

Loading

krish2718 commented Dec 10, 2024

krish2718 commented Dec 10, 2024

Rex-Chen-NXP commented Dec 10, 2024

Rex-Chen-NXP commented Dec 11, 2024

Rex-Chen-NXP commented Dec 12, 2024

Rex-Chen-NXP commented Dec 16, 2024

wifi: TWT improvements #82312

wifi: TWT improvements #82312

Conversation

Rex-Chen-NXP commented Nov 29, 2024 • edited by krish2718 Loading

krish2718 commented Nov 29, 2024

Rex-Chen-NXP commented Nov 29, 2024

Rex-Chen-NXP commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rex-Chen-NXP commented Dec 4, 2024

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

krish2718 commented Dec 9, 2024

Rex-Chen-NXP commented Dec 10, 2024

krish2718 commented Dec 10, 2024

Rex-Chen-NXP commented Dec 10, 2024

krish2718 commented Dec 10, 2024

Rex-Chen-NXP commented Dec 10, 2024 • edited Loading

krish2718 commented Dec 10, 2024

krish2718 commented Dec 10, 2024

Rex-Chen-NXP commented Dec 10, 2024

Rex-Chen-NXP commented Dec 11, 2024

Rex-Chen-NXP commented Dec 12, 2024

Rex-Chen-NXP commented Dec 16, 2024

Rex-Chen-NXP commented Nov 29, 2024 •

edited by krish2718

Loading

Rex-Chen-NXP commented Dec 10, 2024 •

edited

Loading