luci-base, luci-mod-network: support wifi-iface sections spanning multiple radios - #8893
luci-base, luci-mod-network: support wifi-iface sections spanning multiple radios#8893smazurov wants to merge 2 commits into
Conversation
67b77ee to
6902032
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits. Both commit messages match their diffs. Four inline findings, mostly about device-as-array sites that were not converted along with the rest.
Generated by Claude Code
| const uciWifiIface = (networkInstance.sid ? uci.get('wireless', networkInstance.sid) : null); | ||
|
|
||
| if (uciWifiIface == null || uciWifiIface['.type'] != 'wifi-iface' || uciWifiIface.device != this.sid) | ||
| if (uciWifiIface == null || uciWifiIface['.type'] != 'wifi-iface' || L.toArray(uciWifiIface.device).indexOf(this.sid) == -1) |
There was a problem hiding this comment.
One device comparison in this same class was missed: WifiDevice.deleteWifiNetwork() at network.js:3703 still does uci.get('wireless', sid, 'device') != this.sid.
For a section spanning several radios that value is an array, so the comparison is always true and the method returns Promise.resolve(false) without ever removing the section — i.e. deleting a multi-radio network through the public API silently fails. It needs the same L.toArray(...).indexOf(this.sid) == -1 treatment applied here and in getWifiNetwork().
Generated by Claude Code
|
|
||
| wifi_ifaces.forEach(wifi_iface => { | ||
| if (wifi_iface.device == radio && wifi_iface.disabled != '1') | ||
| if (L.toArray(wifi_iface.device).indexOf(radio) != -1 && wifi_iface.disabled != '1') |
There was a problem hiding this comment.
radio here is uci.get('wireless', id, 'device') from line 249,`` which for exactly the sections this PR adds support for is an array, not a radio name. So:
indexOf(radio)compares an array object against strings and never matches —all_networks_disabledstaystrueunconditionally.- Lines 251, 255 and 269 pass that array as a section id to
uci.get/uci.set/uci.unset.resolveSID()returns non-strings unchanged, so the lookup key becomes the stringified array ("radio0,radio1,radio2"); the read yieldsnulland the write is dropped by theif (!v[conf]?.[sid]) return;guard.
The user-visible consequence is a state mismatch with the row button: renderRowActions() computes isDisabled from uci.get('wireless', inst.getWifiDeviceName(), 'disabled') — a real radio name — so when the radio is disabled at the wifi-device level the row reads "Enable", but network_updown() evaluates disabled as false (the radio term is null) and disables the section instead of enabling it. Clearing the radio's disabled flag never happens either.
Since the row is already per-radio, the cleanest fix is to hand the radio name in from renderRowActions() (it already has inst.getWifiDeviceName()) rather than re-deriving it from device inside this function.
Generated by Claude Code
| const editWifiNetwork = this.editWifiNetwork; | ||
|
|
||
| return network.getWifiNetwork(s.section).then(function(radioNet) { | ||
| radioNet = editWifiNetwork || radioNet; |
There was a problem hiding this comment.
This correctly binds the modal to the clicked radio, but the modal's Status field does not stay bound to it.
_wifistat_modal renders from this radioNet on open, yet render_modal_status() stamps data-network with radioNet.getName() — the shared sid — at wireless.js:183, and [`poll_status()` resolves it back with `n.getName() == ...` at wireless.js:819](https://github.com/openwrt/luci/blob/69020328f255129392a3b3beabe59615f2e79711/modules/luci-mod-network/htdocs/luci-static/resources/view/network/wireless.js#L819).`` For a multi-radio section every per-radio instance shares that sid, so [0] always picks the first radio. Opening the 5 GHz row therefore shows the correct BSSID/channel/signal initially and then flips to the first radio's values on the first poll tick.
Keying the badge on the netid and matching it with the same getID() == ... || getName() == ... pattern you already use for rows on line 703 would keep it consistent.
Generated by Claude Code
| if (replopt.formvalue('_new_') == '1') { | ||
| for (let ws of wifi_sections) | ||
| if (ws.device == radioDev.getName()) | ||
| if (L.toArray(ws.device).indexOf(radioDev.getName()) != -1) |
There was a problem hiding this comment.
nit: widening this to a membership test changes what "Replace wireless configuration" does to multi-radio sections. Before this PR they were skipped here (ws.device was not a string); now joining a network on one radio removes the whole shared wifi-iface, tearing down the BSSes it also provides on the other radios. The same applies to the disabled propagation on line 2445, which now marks the shared section disabled on every radio it spans.
Is that the intent, or should the join flow drop just radioDev from the section's device list — the way handleRemove() now does — and only delete the section when it is the last radio? The PR description covers per-radio Remove but not this path.
Generated by Claude Code
netifd allows a single wifi-iface section to be instantiated on several radios at once (list device + option mlo), creating one BSS per radio. The network model assumed option device is a single string: netid resolution returned null for such sections, and section-name lookups collapsed all runtime instances to the first radio. Instantiate one WifiNetwork per (section, radio) pair with per-radio netids and runtime state, and treat every device comparison as a membership test. Sections whose device is a plain string keep taking the previous code paths unchanged. Signed-off-by: Stepan Mazurov <smazurov@gmail.com>
A wifi-iface section spanning several radios renders one row per radio, keyed by netid so the rows stay unique, showing that radio's BSSID, signal and assoclist. Sections configured with a single radio keep their UCI section name as the row key and are rendered exactly as before. Edit opens the modal bound to the clicked radio: the Device Configuration tab and modal title follow the row's radio while UCI reads and writes still target the shared wifi-iface section. Remove on a per-radio row drops only that radio from the section's device list and deletes the section when the last radio is removed. The modal status badge is keyed by netid so that polling keeps it on the radio whose row was opened. Signed-off-by: Stepan Mazurov <smazurov@gmail.com>
6902032 to
71a55f3
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits; no new issues found.
The force-pushed versions address two of the four earlier findings: WifiDevice.deleteWifiNetwork() now uses the same membership test as getWifiNetwork(), and the modal status badge is stamped/resolved by netid with a getID() == key || getName() == key fallback, matching the row lookup at wireless.js:702. Both amended commit messages describe what their diffs actually do.
Still open from the previous review: network_updown() deriving its radio from the device list, and the "Replace wireless configuration" join flow deleting a whole multi-radio section. Those threads are unchanged, so I have not re-posted them.
Generated by Claude Code
|
I am not sure if support should be introduced like this. A rework of the wifi settings page is overdue and likely would greatly benefit from a general split into "SSIDs" (wifi-iface sections) and "Radios" (wifi-device sections). The UX concept would be to define a SSID and during the creation of this SSID (= wifi-iface) simply check the radios it shall be broadcasted on (either a naive select like "2G only", "2G+5G", ... or a multi select). Radios (= wifi-device sections) would be fixed and simply expose the radio specific settings |
I totally agree, but I don't think im qualified to make those changes. I simply wanted to make the mainline wifi 7 story slightly less broken than it currently is. |
Problem
netifd supports a single
wifi-ifacesection spanning several radios, which is the shape Wi-Fi 7 multi-link configurations use:netifd instantiates such a section once per member radio, producing one BSS per band, and
mlois part of the wifi-scriptswifi-ifaceschema.LuCI's network model assumes
option deviceholds a single string, so on these configurations:getWifiNetidBySid()returnsnull, because it teststypeof(s.device) == 'string'. Radios other than the first render asChannel: ? (? GHz)with no network row beneath them.getWifiStateBySid()returns the first runtime instance whose section name matches, collapsing every BSS onto one radio.On a tri-band MT7996 device the result is a single network row under the first radio, an associated-stations list that only ever queries that one interface, so clients on the other bands are missing entirely, and two radios that look inactive.
There is a functional consequence as well. Network rows carry the only Edit button, and that modal binds its Device Configuration tab to
getWifiDeviceName(), which resolves to the first radio. The device configuration of every other radio is therefore unreachable from the wireless page, and edits made there land on the first radio'swifi-devicesection.Changes
luci-base instantiates one
WifiNetworkper (section, radio) pair, with per-radio netids and runtime state, and treats everydevicecomparison as a membership test.luci-mod-network renders one row per radio for such a section, keyed by netid so the rows stay unique, each showing that radio's own BSSID, signal and assoclist. Edit binds the modal to the clicked radio while UCI reads and writes still target the shared section. Remove on a per-radio row drops only that radio from the
devicelist, deleting the section once the last radio goes.Compatibility
The new behavior is gated on a section actually spanning more than one radio, so configurations where
deviceis a plain string are untouched:getWifiNetworks()calls the previous single-argumentlookupWifiNetwork()whenever a section names fewer than two radios, which covers both ordinary sections and sections with nodeviceat all.lookupWifiNetwork()narrows the runtime-state lookup to a specific radio only for multi-radio sections. Otherwise it scans all radios exactly as before.cfgsections()switches to netid keys only for multi-radio sections. Every other section keeps its UCI section name, so rendered row ids anddata-section-idattributes are unchanged.For single-radio sections the widened comparisons are equivalences rather than behavior changes:
L.toArray(x).indexOf(y) == -1matches the previousx != ywhenxis a string or unset, and the added function parameters fall back to the old behavior when null.Two of those comparisons do change behavior, but only for sections that span several radios, since those never matched a string equality test before:
handleScan) now sees such a section where it previously skipped it, so joining a network on one radio removes the sharedwifi-ifaceand the BSSes it provides on the other radios. Making this drop a single radio, the way per-radio Remove now does, would be more consistent, and I am happy to change it if that is preferred.network_updown()derives its radio fromuci.get('wireless', id, 'device'), which is an array for these sections. That predates this PR and already misfires there: the value is passed touci.get/uci.setas a section id, so the radio-leveldisabledflag is never read or written. This PR does not fix it, and rendering one row per radio makes it reachable from each of them. The fix is to pass the row's radio name in fromrenderRowActions()instead of re-deriving it, which seems better as its own change.Testing
Tested on a Gemtek W1700K (airoha/an7581, MT7996E tri-band, OpenWrt SNAPSHOT r35572-8393548d2c) carrying the section above, compared against unpatched LuCI on the same device.
Before: one network row under the first radio, the other two showing
Channel: ? (? GHz), and only the 2.4 GHz clients listed, all attributed to the first radio's interface.After: one row per radio with that radio's own BSSID, the 5 GHz and 6 GHz radios showing their real channels, and every associated client listed against the interface it is actually on, cross-checked against
iw dev <ifname> station dumpon all three interfaces.Also verified: the Edit modal on the 5 GHz row opens with its Device Configuration tab bound to that radio rather than the first one; Remove on that row stages the remaining two radios and leaves the section in place; pending changes to the shared section are reflected on all of its rows; and saving an unmodified modal produces the same default-normalization diff as unpatched LuCI, differing only in which radio it names.
For the single-radio path, a synthetic
wifi-ifacewithoption device 'radio0'was staged in the browser session. It produced exactly oneWifiNetwork, keyed by its section name rather than by the netid that was available for it.Notes
mloitself is deliberately not exposed here. This is limited to rendering an existing configuration correctly. Enable and Disable remain whole-section, since UCI has no per-radio disable within onewifi-iface.Related: #8461 adds Wi-Fi 7 (EHT) device options and is independent of the multi-radio section handling here.