Introduce TA WIFI agent library by maximmenshikov · Pull Request #3 · ts-factory/test-environment

maximmenshikov · 2025-08-22T11:06:28Z

This test agent library adds an ability to set up the WiFi configuration using UCI and hostapd/wpa_supplicant (eventually).

It is based on the design & requirements by Elena V.

The main idea is that there is a very small set of options in the configurator tree. This set of options is merged into the real UCI configuration that is already present on the test host. Then, the "wifi up" is called to restart the network on the device.

ol-arteman · 2025-09-05T15:24:04Z

No commit is formatted according to Contributor's Guide
Would you mind squashing fixup commits into its base commits, e.g. d956ae6 should be part of 815c1a0.

ol-arteman · 2025-09-05T15:26:50Z

+    rc = te_strtou_size(value, 0, &val,
+                        sizeof(val));


It can definitely fit into one line.

ol-arteman · 2025-09-05T15:28:21Z

+    if (port == NULL)
+        return TE_RC(TE_TA_UNIX, TE_ENOENT);
+
+    port->enable = (atoi(value) > 0) ? true : false;


te_strtol_bool ?

ol-arteman · 2025-09-05T15:30:57Z

+
+    port->enable = (atoi(value) > 0) ? true : false;
+
+    return ta_unix_conf_wifi_apply();


You should implement commit method instead.

ol-arteman · 2025-09-05T15:31:59Z

+        .f = NULL, \
+        .radio_instance = 0, \
+        .iface_instance = 0, \
+        .port = NULL, \
+        .tmpl_data = NULL, \
+    }


Misaligned backslashes.

ol-arteman · 2025-09-05T15:34:42Z

+        int   n = 0;
+        char *tok[3] = {0};
+        char *p;
+
+        p = strtok(line, " \t\r\n");
+        while (p && n < 3)
+        {
+            if (*p == '\'' && p[strlen(p) - 1] == '\'' && strlen(p) > 1)
+            {
+                p[strlen(p) - 1] = 0;
+                p++;
+            }
+            tok[n++] = p;
+            if (n == 2)
+            {
+                p = strtok(NULL, "\n");
+            }
+            else
+            {
+                p = strtok(NULL, " \t\r\n");
+            }
+        }
+
+        if (n == 3)


te_vec_split_string() ?

Obviously this function is not suitable. I will, however, introduce string->vec tokenizer.

tokenizer is introduced

ol-arteman · 2025-09-05T17:07:36Z

Also, we do already have tapi_wifi. I would like to hear some rationale why we would need a parallel set of API for the same task.

ol-arteman · 2025-09-05T17:04:21Z

+         Name: empty
+         Value: 0 or 1
+
+    - oid: "/agent/wifi/port/ifname"


This should be related to /agent/interface in some way/

Rather SSID is related.

@ol-arteman in general it is not guaranteed that the port would always be mapped to the existing interface. Depending on configurator, it may run it later, making it impossible to bind to.

ol-arteman · 2025-09-05T17:06:20Z

+         Root object of the WiFi configuration tree.
+         Name: empty
+
+    - oid: "/agent/wifi/port"


I bet it should be a resource.

also the same problem - it could be non-existing interface and thus we can't bind it easily.
@evengerova please tell what you think.

Typically PHYs are created on startup and may be really be considered as resources. Of course in theory we can load kernel modules later, but IMHO it's not that probable.

Ports are indexed by user-friendly name, therefore it is not feasible to add them to resources (ifname might not be the same as the instance name)

ol-arteman · 2025-09-05T17:10:10Z

+         Name: Internal SSID name
+
+    - oid: "/agent/wifi/port/ssid/aname"
+      access: read_write
+      type: string
+      d: |
+         SSID name.
+         Name: empty
+         Value: SSID name


Why aname ? And why do we need a SSID name as an attribute if we have a SSID name as a name of an instance?

Internal SSID name is something we can index SSIDs by, while real SSID name is something that is passed over the air.

ol-arteman · 2025-09-05T17:19:28Z

+    - oid: "/agent/wifi/port/ssid/option"
+      access: read_create
+      type: none
+      d: |
+         Additional options passed to underlying configurator.
+         Name: any
+         Value: none
+
+    - oid: "/agent/wifi/port/ssid/option/value"
+      access: read_write
+      type: string
+      d: |
+         Value of the option passed to underlying configurator.
+         Name: empty
+         Value: any option permitted by underlying configurator.


I am kind of skeptic of this design. It pretends to be kind of backend-neutral, but it's actually not - there is absolutely no warrant that all possible wifi configurators can be expressed with a simple option-value scheme. Also, there is no introspection: the testsuite has no way of learning which options are supported.

There is no fundamentally good solution to the problem of representability. It is up to particular configurator to make this scheme work.

evengerova · 2025-09-09T12:26:22Z

+         Root object of the WiFi configuration tree.
+         Name: empty
+
+    - oid: "/agent/wifi/configurator"


Why is this necessary? Isn't it's possible to do auto-detection?

It is possible to auto detect, but it is impossible to make wise and reproducible decision with autodetection.

Why? Let's at least allow to keep this field empty to get auto detection.

auto detection is added

evengerova · 2025-09-09T12:27:04Z

+         Name: empty
+         Value: 0 or 1
+
+    - oid: "/agent/wifi/port"


It's not possible to add a port.

Indeed it is possible. It is not autodetected from the configuration, and realistically there may be configurators that would run interface afterwards.

evengerova · 2025-09-09T12:27:31Z

+         Name: empty
+         Value: 0 or 1
+
+    - oid: "/agent/wifi/port/ifname"


Rather SSID is related.

evengerova · 2025-09-09T12:29:12Z

+         Name: empty
+         Value: Channel number
+
+    - oid: "/agent/wifi/port/htmode"


htmode is UCI-specific thing. Ive asked (and Misha has done) channel width - number 20/40/80/160

evengerova · 2025-09-09T12:32:33Z

+         Name: empty
+         Value: Any supported by Linux
+
+    - oid: "/agent/wifi/port/ssid/aname"


"ssid" is better

evengerova · 2025-09-09T12:33:10Z

+      d: |
+         SSID security.
+         Name: empty
+         Value: open, wpa, wpa2, wpa3


wep is missing

evengerova · 2025-09-09T12:34:21Z

+         Name: empty
+         Value: Passphrase 8 to 63 characters long
+
+    - oid: "/agent/wifi/port/ssid/option"


I do not like OS-specific and hw-specific options. From the other hand it's not quite clear how to get default values for mandatory things.

evengerova · 2025-09-09T12:36:44Z

+
+/** Supported WiFi standards */
+typedef enum ta_wifi_standard {
+    TA_WIFI_STANDARD_G = 0,             /**< G standard (2.4GHz) */


AX and BE are applied for both 2.4 and 5.

evengerova · 2025-09-09T12:40:25Z

+
+#include "te_queue.h"
+
+#define MAX_LINE (1024)


Just wonder why do we need parser if there is an API (or in the worst case CLI). But, I suppose, it's too late...

extra dependencies is always a bad idea

evengerova · 2025-09-09T12:44:26Z

Also, we do already have tapi_wifi. I would like to hear some rationale why we would need a parallel set of API for the same task.

Because tapi_wifi is not generic, but operates Linux-specific wpa_supplicant concepts. Also does not support AP.

ol-arteman · 2025-09-09T23:03:16Z

No commit is formatted according to Contributor's Guide

Would you mind squashing fixup commits into its base commits, e.g. d956ae6 should be part of 815c1a0.

New and updated commits still do not conform to our Contributor's Guide. There is a script scripts/te_check_patches that can be used to validate them.

Also I would really appreciate it if fixup commits were squashed into their base commits.

maximmenshikov · 2025-09-09T23:06:28Z

No commit is formatted according to Contributor's Guide

Would you mind squashing fixup commits into its base commits, e.g. d956ae6 should be part of 815c1a0.

New and updated commits still do not conform to our Contributor's Guide. There is a script scripts/te_check_patches that can be used to validate them.

Also I would really appreciate it if fixup commits were squashed into their base commits.

I didn't say it is ready for a new review (there are several projects that depend on that branch and all of them have to be verified) and I haven't marked your request as processed in any way.

ol-arteman · 2025-09-10T16:35:17Z

May I also ask for some close-to-real-world examples of an instance tree?

maximmenshikov · 2025-09-12T00:45:05Z

May I also ask for some close-to-real-world examples of an instance tree?

    RING("Enable wpa supplicant mode");
    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(STRING, "hostapd_wpa_supplicant"),
                                  "/agent:%s/wifi:/configurator:",
                                  host->ta));

    RING("Add WiFi port");
    CHECK_RC(cfg_add_instance_fmt(&port_handle,
                                  CFG_VAL(NONE, NULL),
                                  "/agent:%s/wifi:/port:wifi",
                                  host->ta));

    RING("Set WiFi port interface");
    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(STRING, conf->iface_wifi_a),
                                  "/agent:%s/wifi:/port:wifi/ifname:",
                                  host->ta));

    RING("Set WiFi port HT mode");
    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(INTEGER,
                                          (std == TSAPI_WIFI_STD_G ||
                                           std == TSAPI_WIFI_STD_N)
                                            ? 20
                                            : 80),
                                  "/agent:%s/wifi:/port:wifi/width:",
                                  host->ta));

    RING("Set WiFi port standard");
    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(STRING, tsapi_wifi_std2str(std)),
                                  "/agent:%s/wifi:/port:wifi/standard:",
                                  host->ta));

    RING("Set WiFi port interface enable status");
    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(INTEGER, 1),
                                  "/agent:%s/wifi:/port:wifi/enable:",
                                  host->ta));

    RING("Add WiFi SSID");
    CHECK_RC(cfg_add_instance_fmt(&ssid_handle,
                                  CFG_VAL(NONE, NULL),
                                  "/agent:%s/wifi:/port:wifi/ssid:ssid1",
                                  host->ta));

    RING("Set WiFi SSID sta mode");
    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(STRING, "sta"),
                                  "/agent:%s/wifi:/port:wifi/ssid:ssid1/mode:",
                                  host->ta));

    RING("Set WiFi SSID name");
    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(STRING, tsapi_wifi_get_ssid(conf, std)),
                                  "/agent:%s/wifi:/port:wifi/ssid:ssid1/name:",
                                  host->ta));
    if (conf->dut_wifi_ssid_passphrase == NULL ||
        strlen(conf->dut_wifi_ssid_passphrase) == 0)
    {
        RING("Disable passphrase");
        CHECK_RC(cfg_set_instance_fmt(CFG_VAL(STRING, "open"),
                                      "/agent:%s/wifi:/port:wifi/ssid:ssid1/security:",
                                      host->ta));
    }
    else
    {
        RING("Enable passphrase");
        CHECK_RC(cfg_set_instance_fmt(CFG_VAL(STRING, std == TSAPI_WIFI_STD_BE ? "wpa3" : "wpa2"),
                                      "/agent:%s/wifi:/port:wifi/ssid:ssid1/security:",
                                      host->ta));
        CHECK_RC(cfg_set_instance_fmt(CFG_VAL(STRING, conf->dut_wifi_ssid_passphrase),
                                      "/agent:%s/wifi:/port:wifi/ssid:ssid1/passphrase:",
                                      host->ta));
    }

    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(STRING, "ccmp"),
                                  "/agent:%s/wifi:/port:wifi/ssid:ssid1/protocol:",
                                  host->ta));

    RING("Set WiFi SSID enable status");
    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(INTEGER, 1),
                                  "/agent:%s/wifi:/port:wifi/ssid:ssid1/enable:",
                                  host->ta));

    RING("Enable WiFi");
    CHECK_RC(cfg_set_instance_fmt(CFG_VAL(INTEGER, 1),
                                  "/agent:%s/wifi:/enable:",
                                  host->ta));

Signed-off-by: Maxim Menshikov <maxim.menshikov@interpretica.io> Acked-by: Elena Vengerova <elena.vengerova@aprocsys.com>

This is the first bit to enable configuration of WiFi - generic definitions that should be used in both tests and the agent implementation. Signed-off-by: Maxim Menshikov <maxim.menshikov@interpretica.io> Acked-by: Elena Vengerova <elena.vengerova@aprocsys.com>

maximmenshikov · 2025-09-19T10:17:20Z

@ol-arteman we may continue this public execution I think.
There were a few real-world checks by Michael K. and Elena V., a few comments by Konstantin U. to help define naming for tapi_cfg_wifi, among a few other things.
Finally, the commits are squashed & properly split according to contributor's guide.

I also merged wpa_supplicant configuration code here.

ol-kochkin · 2025-09-24T01:07:57Z

return 0 in NOK case looks wrong for me, wpa_supplicat failed to start,
there is "Failed to start WPA Supplicant" printout
but
"Set /agent:agt_wifi_d/wifi:/enable: = 1"
succeeded and test fails later then it should.

I've looked at another commits,
g 'commit(unsigned int gid' .
they all return rc in NOK case.

It's about node_wifi_commit()

Returning 0 may break TA and it is not what you expect. You should rather check status before continuing.

Could you elaborate?
0 is returned in NOK case now and I propose to fix it

TA WiFi is based on several concepts: - It tries to be backend-agnostic. Right now, UCI and wpa_supplicant are supported. - It still supports backend-specific optinons. It is clear that real-world devices require unique options that can't be covered by unified set of options. - The backend is auto-detected by default but can be fixed to one of them. For UCI, there are several implications: - The configuration is first read, then augmented. To generate it correctly, the port interface name must match the one in the configuration. - The SSID configuration is based on the first SSID from the configuration that matches the band of the port. - The configuration must reside in /etc/config/wireless. - The script to up/down the interfaces is 'wifi up'. For wpa_supplicant, the binary must reside in /usr/sbin/wpa_supplicant. For hostapd, the binary must reside in /usr/sbin/hostapd Signed-off-by: Maxim Menshikov <maxim.menshikov@interpretica.io> Acked-by: Elena Vengerova <elena.vengerova@aprocsys.com>

Signed-off-by: Maxim Menshikov <maxim.menshikov@interpretica.io> Acked-by: Elena Vengerova <elena.vengerova@aprocsys.com>

ol-kochkin · 2025-09-25T16:39:00Z

I don’t know openwrt logic well, but to me, a safer approach is:

"wifi down" with old /etc/config/wireless

update /etc/config/wireless

wifi up

I’m encountering a rare test failure and suspect that the old iface conflicts with the new one.
However, I have no proof, and the issue is quite rare.

ol-kochkin · 2025-09-29T16:49:45Z

Could you elaborate?
0 is returned in NOK case now and I propose to fix it

ol-kochkin · 2025-09-29T16:54:55Z

what about txpower, frag, rts, and other test written options?
I suspect they multiply as well e.g.

first run txpower = 10,
second run txpower = 20 and it produces
option txpower 10
option txpower 20

ol-kochkin · 2025-10-01T21:52:24Z

It is more correct to take ssid's protocol into account

- CHECKED_FPRINTF(ctx->f, "\toption encryption '%s'\n", - wifi_security2enc[node->security]); + CHECKED_FPRINTF(ctx->f, "\toption encryption '%s+%s'\n", + wifi_security2enc[node->security], + tapi_cfg_wifi_protocol_from_value(node->protocol));

to get psk2+tkip, psk2+ccmp
https://openwrt.org/docs/guide-user/network/wifi/basic#encryption_modes

ol-kochkin · 2026-01-08T13:29:06Z

+}
+
+/* See the description in tapi_cfg_wifi.h */
+tapi_cfg_wifi_standard


here and in a header
tapi_cfg_wifi_standard -> tapi_cfg_wifi_width

ol-kochkin · 2026-01-08T13:30:15Z

+}
+
+/* See the description in tapi_cfg_wifi.h */
+tapi_cfg_wifi_mode


here and in a header
tapi_cfg_wifi_mode -> tapi_cfg_wifi_protocol

There are more notes, but I don't remember them,
nobody in the office and bolger with my home offline.

maximmenshikov force-pushed the mmenshikov/ta_wifi branch 5 times, most recently from 631fa7a to 986c493 Compare August 22, 2025 12:39

ol-arteman suggested changes Sep 5, 2025

View reviewed changes

evengerova reviewed Sep 9, 2025

View reviewed changes

maximmenshikov force-pushed the mmenshikov/ta_wifi branch from 6aa84b7 to 90bb9aa Compare September 9, 2025 22:05

maximmenshikov added 3 commits September 19, 2025 10:07

lib/tools: introduce string to vector tokenizer function

61821b9

Signed-off-by: Maxim Menshikov <maxim.menshikov@interpretica.io> Acked-by: Elena Vengerova <elena.vengerova@aprocsys.com>

doc: introduce WiFi configurator model

77e37c1

Signed-off-by: Maxim Menshikov <maxim.menshikov@interpretica.io> Acked-by: Elena Vengerova <elena.vengerova@aprocsys.com>

maximmenshikov force-pushed the mmenshikov/ta_wifi branch from 8b708ab to baf4711 Compare September 19, 2025 10:09

ol-kochkin reviewed Sep 24, 2025

View reviewed changes

maximmenshikov force-pushed the mmenshikov/ta_wifi branch 2 times, most recently from edea483 to 1e8b669 Compare September 30, 2025 22:15

maximmenshikov added 2 commits October 4, 2025 23:15

doc: document lib/ta_wifi and lib/tapi_cfg_wifi

fba6d97

Signed-off-by: Maxim Menshikov <maxim.menshikov@interpretica.io> Acked-by: Elena Vengerova <elena.vengerova@aprocsys.com>

maximmenshikov force-pushed the mmenshikov/ta_wifi branch from 1e8b669 to fba6d97 Compare October 4, 2025 22:16

ol-kochkin reviewed Jan 8, 2026

View reviewed changes


		port->enable = (atoi(value) > 0) ? true : false;

		return ta_unix_conf_wifi_apply();

Conversation

maximmenshikov commented Aug 22, 2025

Uh oh!

ol-arteman commented Sep 5, 2025

Uh oh!

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!

ol-arteman commented Sep 5, 2025

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!

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!

evengerova commented Sep 9, 2025

Uh oh!

ol-arteman commented Sep 9, 2025

Uh oh!

maximmenshikov commented Sep 9, 2025

Uh oh!

ol-arteman commented Sep 10, 2025