src/cyw43_ctrl: Implement roaming configuration

[mitchellcairns]

Add ctrl and ll functions to configure and query WiFi roaming behavior on the STA.

Why? Until now, this has been enabled by default with no option to configure/alter the behavior. If a device drops below the trigger threshold for RSSI, it would drop packets due to activating roam.

interface:

• cyw43_ll_wifi_set_roam_enabled / cyw43_wifi_set_roam_enabled
  Control the roam_off iovar to enable or disable roaming entirely.

• cyw43_ll_wifi_set_roam_params / cyw43_wifi_set_roam_params
  Set the RSSI trigger threshold, candidate delta, and scan period.

• cyw43_ll_wifi_get_roam_params / cyw43_wifi_get_roam_params
  Read back current roam parameters. All output pointers are nullable.

[mitchellcairns]

Kindly requesting review from @dpgeorge

[dpgeorge]

Thanks for the contribution.

Note that it is already possible to configure these roaming parameters -- if you know the correct IOCTL codes. You can use cyw43_ioctl().

There are many, many IOCTLs and I don't think it scales to add them all as both LL functions (in cyw43_ll.c) and higher-level control functions (in cyw43_ctrl.c).

Instead, look how CYW43_IOCTL_GET_SSID is used:

uint8_t buf[36];
cyw43_ioctl(&cyw, CYW43_IOCTL_GET_SSID, 36, buf, itf);

Or how CYW43_IOCTL_GET_RSSI is implemented without any additional LL helpers.

What I would suggest is:

• add a new public facing cyw43_ioctl_get_u32() and cyw43_ioctl_set_u32() helpers which wrap cyw43_ioctl() for the case of the buffer being an unsigned 32-bit value
• add CYW43_IOCTL_xxx constants to get/set roaming parameters
• possibly still add cyw43_wifi_set_roam_enabled() as in this PR, but implement it in terms of cyw43_ioctl()
• possibly add helper functions cyw43_wifi_set_roam_params() and cyw43_wifi_get_roam_params() which are implemented in terms of cyw43_ioctl() as well

We need to remember that this code executes in an embedded environment and needs to have a small footprint.

[mitchellcairns]

Thanks for the feedback

I can definitely restructure this so that there is one base function, and wrapper static functions to avoid consuming binary space unless it's actually used.

While we're on the topic of roaming, I do think that there should be a consideration to have roaming disabled as the default configuration parameter.

I can confirm that doing so entirely resolves the issues here:
#111
raspberrypi/pico-sdk#2835

I understand the concern that there are hundreds of IOCTLS, but I'm not asking to configure all of them, these are critical to get performance under control and resolve those specific types of issues. The current driver offers no visibility that this is an issue whatsoever, and it seems that Infineon doesn't like to share this information. Consider it an improvement to the average developer experience

[dpgeorge]

While we're on the topic of roaming, I do think that there should be a consideration to have roaming disabled as the default configuration parameter.

Yes, I tend to agree. For consumer applications (laptops, mobile phones) enabling roaming makes sense, but for microcontroller applications (what this driver aims for) it might indeed be better to prioritise sticking with the one AP.

But that could be quite a big breaking change for some users who (implicitly, maybe without realising) rely on this feature. In that case we could tune trigger_dbm so it's lower, eg -90.

I understand the concern that there are hundreds of IOCTLS, but I'm not asking to configure all of them, these are critical to get performance under control and resolve those specific types of issues. The current driver offers no visibility that this is an issue whatsoever, and it seems that Infineon doesn't like to share this information. Consider it an improvement to the average developer experience

I fully support adding these new ioctls. I was thinking the use could be something like this (eg in MicroPython), schematically:

struct command_table[] = {
    "trigger_dbm", CYW43_IOCTL_TRIGGER_DBM,
    "candidate_delta_db", CYW43_IOCTL_CANDIDATE_DELTA_DB,
    "scan_interval_ms", CYW43_IOCTL_SCAN_INTERVAL_MS,
    "other_option", CYW43_IOCTL_OTHER_OPTION,
    ...
};

set_param(const char *name, int value) {
    int ioctl = lookup_name_in(name, command_table);
    if (ioctl == -1) {
        return EINVAL;
    }
    return cyw43_ioctl_set_u32(&cyw43, ioctl, value);
}

In that case all it needs is the CYW43_IOCL_xxx defined, and helper functions to get/set ioctl. Also documentation 😄

Of course, we can also add static inline helper functions to make it easier to use directly from C.