luci-app-wwand + luci-proto-wwand: add LuCI support for the wwand connection manager - #8917
luci-app-wwand + luci-proto-wwand: add LuCI support for the wwand connection manager#8917ddimension wants to merge 2 commits into
Conversation
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits (both messages match their diffs). 12 inline comments, the main ones being an rpc expect unwrapping bug in wwand.modemopts, two ACL gaps (modem_scan_start/modem_scan_status, file list /dev/), the undeclared protocol/qmi.js file collision with luci-proto-qmi, and a poller that outlives the interface-edit modal.
Generated by Claude Code
|
Updated to the current sources, addressing the inline review:
Still on the list (follow-ups, not blockers): a modem selector on the settings page instead of hard-binding |
e9a073a to
f52b465
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits (343ab75, f52b465). The earlier findings are addressed: the protocol/qmi.js path collision is gone, the statusRaw/.modems unwrap and sl.physical are fixed, the async-scan ACL grants are in, status.js uses literal _() headers, and the interface-edit poller now unregisters itself. 4 new inline comments — the notable one is a modem_reset ACL gap plus a section_id/modem-name mixup, both fallout from the modem_repower → modem_reset rename in the shared wwand.modemopts module.
Commit checks
- 343ab75 "luci-app-wwand: sync modem UI to current sources" — the two bullets cover the Modems-page additions and the review fixes, but not the largest parts of the diff. Undocumented: the Status → Modem detail page is deleted outright (
view/wwand/detail.js, −306 lines, plus itsadmin/status/wwand-detailmenu entry);view/wwand/settings.jsis split into five new shared resources (wwand.rpc/format/esim/netsel/modemsid, −729 lines there); the_repoweraction and itswwand.modem_repowerubus call become_reset/wwand.modem_reset; the Modem Tools menu node loses its title; and a newfcc_authmodem option is added. Removing a user-visible page and swapping a ubus method are the kind of thing worth naming in the message — the ACL fallout of that swap is the subject of one of the inline comments.
Generated by Claude Code
Package-G artefacts for the openwrt/packages#30185 + openwrt/luci#8917 push: - upstream/dossier-packages-30185.md — maturity/coexistence dossier to post as a PR comment (ModemManager precedent, independent-verification story, HW/test matrix, audit changelog, device-ownership answer, re-review request). - upstream/rfc-openwrt-devel.md — the architecture RFC (improve-existing-stack vs. a separate opt-in proto) for openwrt-devel. - upstream/checklist.md — the push/tag/PR/feed action sequence. Drafts for author review; sending/posting is manual. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Synced to the wwand 1.2.0 sources (paired with openwrt/packages#30185).
All JS |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (5da1788). 5 inline comments — the blocking one is a var plmn redeclaration in status.js that both kills the new NAS panel and throws on an unregistered modem; the rest are an unreachable plmnEditor() in settings.js and three items in the reworked ACL file.
Checked the new UCI surface (wwand_plmnlist / plmn_list / hard_reconnect_on_ip_change) against the backend-coupling rule: wwand is not in the packages feed yet, but the PR body and the follow-up comment already name openwrt/packages#30185 as the pairing, so not flagged. CI on the head SHA is green.
Commit checks
- 5da1788 "luci-app-wwand, luci-proto-wwand: sync to wwand 1.2.0 sources" — the four bullets describe real changes, but they leave out the largest one. Undocumented: the whole preferred-PLMN manager (
renderPlmnManager, ~200 new lines insettings.js), the newmodem_plmn_set/modem_plmn_restoreubus methods, a new UCI section typewwand_plmnlist(type+plmnlist) and theplmn_listoptions it adds to bothwwand.modemoptsandwwand.simlist. Also unmentioned: the new Datapath & muxing status panel (modem_datapath), the Preferred networks (NAS) panel, thecachedCall/slowCacherework that moves cells/slots/datapath/PLMN off the 1 s poll onto per-call TTLs, thefmt.term()tooltip pass acrossstatus.js, addingmodemmanagerto the migration list, and the tab→space reindent of the ACL file. Introducing a new config section type and a new poll-caching model are the kind of thing worth naming in the message.
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits (12ef4b1, d7bb9ee). These replace the single 5da1788 I reviewed last round — git diff 5da1788..d7bb9ee is empty, so this is a pure re-split of that commit into an app half and a proto half (plus the dropped Co-Authored-By trailer). The split itself is clean: 12ef4b1 touches only applications/luci-app-wwand/, d7bb9ee only protocols/luci-proto-wwand/.
Because the tree is unchanged, the five findings from the previous round are all still open, including the blocking one: the var plmn redeclaration in status.js, which leaves the new "Preferred networks (NAS)" panel permanently unrendered and throws on an unregistered modem. The modem_repower leftover grant and its mirror in luci-proto-wwand.json (missing modem_reset) are likewise untouched. One new nit inline, on the only hunk in this round's diff that I had not already commented on. CI on the head SHA is green (all three FormalityCheck runs pass).
Commit checks
- 12ef4b1 "luci-app-wwand: sync to wwand 1.2.0 sources" — the message is 5da1788's verbatim minus the proto bullet, so the omissions I flagged there carry over unchanged. Still undocumented in a 913-line diff: the whole preferred-PLMN manager (
renderPlmnManager, ~200 lines insettings.js), the newmodem_plmn_set/modem_plmn_restoreubus methods, a new UCI section typewwand_plmnlist(type+plmnlist) with theplmn_listoptions it adds to bothwwand.modemoptsandwwand.simlist, the Datapath & muxing panel (modem_datapath), the Preferred networks (NAS) panel, thecachedCall/slowCacherework that moves cells/slots/datapath/PLMN off the 1 s poll onto per-call TTLs, thefmt.term()tooltip pass overstatus.js,modemmanagerjoining the migration list, and the tab→space reindent of the ACL file. A new config section type and a new poll-caching model are worth naming in the message — and the ACL reindent is what hid the two grant bugs. - d7bb9ee "luci-proto-wwand: sync to wwand 1.2.0 sources" — "add the hard_reconnect_on_ip_change option" covers 4 of the 10 added lines; the other three hunks add help text to pre-existing
password,metricandpeerdnsoptions. Minor, but a "- plus help text for the PAP/CHAP password, gateway metric and peer-DNS options" bullet would make the diff match the body.
Generated by Claude Code
- status.js: fix the `var plmn` redeclaration. `plmn` was declared three times in one function scope (`res[5]` for the PLMN lists, then `reg.plmn` for the serving PLMN); by the NAS panel line `plmn` was the serving object, so `plmn.nas` was always undefined — the "Preferred networks (NAS)" panel never rendered and threw on an unregistered modem (swallowed by the status poller's error handler → the page silently stopped refreshing). Rename the lists reply to `plmnLists`. - settings.js: drop the unreachable `plmnEditor()` (~62 dead lines; the only render path is `renderPlmnManager`). It also carried a latent `list.map` index-as-noRat bug. - ACL: remove the dead `modem_repower` grant (the ubus method was renamed to `modem_reset`) and the `modem_sms_send` grant (no caller in the app — the SMS UI only lists/deletes). Restore tab indentation + inline single-element arrays to match the tree convention (100/104 acl.d files use tabs), which keeps the ACL delta reviewable. - format/esim/netsel: move the shared `.wwe-*` stylesheet into wwand.format and inject it once from the settings view. Previously esim.js and netsel.js each emitted their own <style>, so the settings page (rendering both) got it twice, and netsel depended on esim purely for that string. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- ACL: the write grant still named the removed `modem_repower` and did not grant `modem_reset`, so the Reset button rendered from the shared wwand.modemopts on the interface-edit page hit access-denied. Grant `modem_reset` instead. - protocol/wwand.js: drop the redundant `o.default = '0'` on the hard_reconnect_on_ip_change flag — form.Flag already defaults to disabled, and it was the only literal '0'/'1' default in the file (the flags around it use the symbolic `o.enabled`/`o.disabled`). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits (7e1c0ec, 442ba27). Both messages match their diffs, so no commit checks this round — and this pair closes every finding from the last two rounds. One nit inline; CI on the head SHA is green (all three FormalityCheck runs).
What I verified:
status.jsscope bug — fixed properly.plmnListsis now the only name bound tores[5], andvar plmn = reg.plmnat :405 is the soleplmnleft inrenderLive(used only at :427, guarded byplmn ? …).renderNasList()already returnsnullfor a missing/empty list, so an unregistered modem no longer throws into the poller's error handler.plmnEditor()removal — no dangling references.plmnEditRowsurvives becauserenderPlmnManagerstill uses it (:358, :437) with an explicit wrapper rather than a barelist.map, andcallPlmnSetis still reached at :407. No require insettings.jsbecame unused by the deletion.- ACL — both files now match the actual call surface exactly. Every method
wwand.rpcdeclares is granted and has at least one caller; conversely nothing is granted without a caller.modem_sms_sendis correctly dropped (sms_sendappears nowhere in either package), andluci-proto-wwand.json'smodem_resetcloses the access-denied on the Reset button thatwwand.modemoptsrenders on the interface-edit Modem & SIM tab.modemopts' other calls from that page (status,modem_sim_slots,modem_probe,file list /sys/class/gpio/) are all covered by the proto grant too. Tab indentation matches the tree convention. Both files still parse as JSON. - CSS move — safe.
wwand.esimandwwand.netselare required only byview/wwand/settings.js, which is exactly wherefmt.injectStyle()now runs, and it sits in the same returned tree asnetsel.render()(:806) andesim.render()(:807), so neither panel loses its.wwe-*rules. NoESIM_CSS/esim.CSSreferences remain anywhere, and dropping thewwand.esimrequire fromnetsel.jsleaves all of its remaining requires in use. o.default = '0'— confirmed a no-op:form.js:5036-5037setsthis.disabled = '0'; this.default = this.disabled;.
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (eba976f). git diff 442ba27..eba976f is a single blank line removed from wwand/esim.js — the message matches the diff exactly, so no commit checks this round. Everything closed in the previous two rounds stays closed; nothing in this diff reopens a prior finding. CI on the head SHA is green (all three FormalityCheck runs).
The one nit inline is about where the commit sits in the series rather than what it does — the blank line it removes was introduced by 7e1c0ec earlier in this same series, so it reads as squash material.
Generated by Claude Code
|
Rebased onto current master and squashed from nine commits to two — one per package — and the sources are re-synced from upstream, which they had drifted from (the branch was still at the "wwand 1.2.0 sources" state). Six files changed in the re-sync, the two substantive ones both being fixes found in the field since:
The rest are the status/modems/format/rpc/ACL updates that go with them. All JS passes No |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the rebase: the nine-commit series is now two commits, one per package, and both messages match their diffs exactly (d41c76c touches only protocols/luci-proto-wwand/, b0f6e4e only applications/luci-app-wwand/) — so no commit checks this round.
The content delta against the previous head (eba976f) is the six files the PR comment names: git diff eba976f..b0f6e4e -- applications/luci-app-wwand protocols/luci-proto-wwand is +124/−6. The two fixes called out in that comment both check out — MODE_BITS_MASK correctly preserves the 0x23 (CDMA/EVDO/TD-SCDMA) bits the picker does not render, and describeError() recovers the detail.result/detail.code that a QMI-level rejection carries. Everything closed in the previous rounds stays closed; nothing here reopens a prior finding. CI on the head SHA is green (all three FormalityCheck runs).
Six inline comments, all on the new hunks. The two worth a second look are the control-protocol switch, whose comment claims a "supports both" gate that the code does not implement (and which the daemon documents as a Quectel-only AT command, broken on the RG650E for MBIM), and "Reset to defaults", which still sends DEFAULTS.mode_preference = 0x50 verbatim and so re-clobbers exactly the unrendered bits collect() now preserves.
I also re-checked the backend coupling for the two new ubus methods rather than assuming: modem_repower and modem_set_protocol both exist in the daemon (src-ucode/ubus.uc, src-ucode/hwops.uc, src-ucode/daemon.uc), both are called only from luci-app-wwand pages, and both are granted under write in luci-app-wwand.json — so no proto-side ACL gap this time, and no frontend-only option.
Generated by Claude Code
|
All six addressed. Five changed the code; one did not, and I want to be explicit about which. Control-protocol switch. You were right that the comment promised a gate the code did not implement — but the underlying capability was already there and simply unexposed, so the fix is a real gate rather than a reworded comment. One correction to the review: the switch is not Quectel-specific. Your RG650E point survives the gate, though, and it is the sharper half: that model has a recipe, so "Reset to defaults". Correct, and it was worse than the bug it sat next to: 0x7F → 0x50 versus the 0x5C the Apply path produced. A reset now means "defaults for what is on screen" and keeps the bits the picker cannot render. Protocol-switch failure path. Fixed — it now uses Duplicated lock formatting. Moved to
Note the gate needs the wwand release carrying the |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the amended head. d41c76c is byte-identical to the previous round; ad0894b replaces b0f6e4e, so the new content is git diff b0f6e4e..ad0894b — +88/−28 across five files, still scoped to applications/luci-app-wwand/ only, and the message matches the diff. No commit checks this round. CI on the head SHA is green (all three FormalityCheck runs).
All six findings from the last round are closed, and the follow-up comment's account of them checks out:
- Duplicated lock formatting —
fmt.fmtLocks()is now the single implementation, and both call sites use it (status.js:409,settings.js:616). It renders theearfcn:pci/pci:arfcn:scs:bandspelling the lock editor accepts, matching the placeholders inrenderCellLock(), instead ofJSON.stringifyoutput. The disarmed-lock omission you called out is visible in the code (l.enabled === falseskips the group), and unknown keys still fall through to the JSON branch, so nothing the daemon grows later is silently dropped. - "Reset to defaults" — now seeds from
~MODE_BITS_MASKexactly likecollect(), andObject.assignkeepsDEFAULTSitself unmutated across repeated presses. One question inline about which bits "defaults" should mean. - Control-protocol switch — gated on
data.info.proto_switch, which comes off the same per-modem status objectstatus.jsreadscaps.ratsfrom, and fails closed when the daemon predates the field. The MBIM direction now warns about the MBIM_OPEN rejection; the QMI direction keeps the old wording. Failure path usesdescribeError(), so a QMI-level rejection reports result/code instead of bare "qmi". - NR band ordering — the narrower-before-broader invariant now holds at every overlap I could check: n14 before n28, n18 and n5 before the superset n26, n12 before n85, n261 before the superset n257. One nit inline on the new tie-break paragraph.
- Em dash in the
format.jsblock comment — literal now. admin/network/wwand-tools— title and order restored, andload()falls back to the first modem when no?modem=is present, so the menu entry lands on a usable page rather than an empty one.
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the amended head (9a6dc44 replaces b0f6e4e; d41c76c is unchanged). The commit message is byte-identical to the one I reviewed last round and still matches the package it adds, so no commit checks this round. git diff b0f6e4e..9a6dc44 is +100/−31 across five files, all in luci-app-wwand, and it is exactly the follow-up the previous round's comments asked for. CI on the head SHA is green (all three FormalityCheck runs).
What I verified closed:
Reset to defaultsre-clobbering the unrendered RAT bits — fixed on both halves.DEFAULTS.mode_preferenceis now derived fromMODE_BITS_MASKinstead of the hardcoded0x50, and the button ORs in the& ~MODE_BITS_MASKremainder of the reported value, so a reset can no longer drop CDMA/EVDO/TD-SCDMA.Object.assign({}, DEFAULTS)keeps the shared band arrays unmutated.- Control-protocol switch gate — the section now actually gates on a capability (
data.info.proto_switch) rather than on "the modem currently speaks QMI or MBIM", and the MBIM direction no longer promises the connections come back on their own. The dependency on a not-yet-released daemon field is called out in your comment, and the failure direction is safe (section simply does not render), so not flagged. - Protocol-switch failure path — now routed through
describeError(), which is in scope at :90. - Duplicated lock formatting —
fmt.fmtLocks()is a genuine single source: the widths it groups by (2 for LTE, 4 for NR5G) match the spellings the cell-lock editor accepts (earfcn:pci,pci:arfcn:scs:band, placeholders at settings.js :585/:587), and both former call sites are gone. NolockRows/liveLocksleftovers, andfmtis required in both files. Modem Toolsmenu title — restored, withorder: 60afterModemsat 55. No collision with the coreadmin/network/*orders (10–50). The JSON parses and all four changed JS files passnode --check.
Three inline comments. The one worth a look is in bands.js: the reorder pass fixed n14, n18/n5 and n261, but n74 is still listed behind n50/n75, which leaves it reachable only for a 1 MHz sliver — the same class of bug this hunk was written to remove.
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the amended app commit. d41c76c is unchanged since last round; c14c503 replaces b0f6e4e with the same message and +115/−33 of content, and since this is the single "add the package" commit the message still describes the whole package accurately — no commit checks this round. CI on the head SHA is green (all three FormalityCheck runs).
The six findings from the previous round all land, and the two you argued rather than changed hold up:
MODE_BITS_MASKasDEFAULTS.mode_preference— correct on both halves.DEFAULTSis now 0x5C instead of the hardcoded 0x50, so a reset no longer drops the 2G/3G fallback, and the Reset handler re-applies the same& ~MODE_BITS_MASKpreservationcollect()does, so the 0x01/0x02/0x20 bits survive the button that used to clobber them.sis in scope at the handler (settings.js:728),`` andObject.assign({}, DEFAULTS)is safe here — `apply()` only forwards the band arrays to `callSet`, it never mutates them.- Protocol-switch gate —
data.info.proto_switchis checked before theqmi/mbimtest, so an older daemon hides the section entirely rather than offering a switch it cannot perform. The MBIM-direction confirmation no longer promises the connections come back on their own, which was the RG650E half of the concern, and the failure path now goes throughdescribeError()(defined atsettings.js:89,`` in scope at the call site). fmt.fmtLocks()— both call sites collapse onto it and both files already requirewwand.format as fmt. The colon spelling matches what the editors accept in each direction: LTE width 2 rendersearfcn:pciagainst the1300:246 5230:118placeholder, NR5G width 4 renderspci:arfcn:scs:bandagainst242:431070:1:78, andsave()splits on/[\s,]+/so the', 'join round-trips. Thel.enabled != null && !l.enabledguard correctly skips a disarmed lock while still rendering the bare{ lte: true }shape.- Menu title —
admin/network/wwand-toolsregains the title it lost several rounds ago, at order 60 behindadmin/network/wwand(55), and itsdepends.ubusnamesmodem_get_settings, whichwrpc.getSettingsreally declares and the ACL really grants.
No new UCI option or ubus method in this diff, so nothing new to check against the backend-coupling rule. All nine app resources plus the proto handler pass node --check; all three JSON files parse.
Two inline comments, both on wwand/ shared resources: a question about which band the NR table should hand 1475–1517 MHz to now that n74 moved ahead of n50/n75, and a nit on the fmtLocks() fallback loop skipping its own enabled check.
Generated by Claude Code
| /* n74 (1475-1518) before n50/n75 (1432-1517): same narrower-before-broader | ||
| rule — behind them only the 1 MHz sliver above 1517 would ever reach it. */ | ||
| ['n74',1475,1518],['n50',1432,1517],['n75',1432,1517], |
There was a problem hiding this comment.
Moving n74 ahead of n50/n75 does not just recover a dead entry — it hands 1475–1517 MHz to n74, and that is where the European L-band SDL carriers actually sit. Resolving the two tables against nrArfcn()'s first-match rule:
1455 MHz: before=n50 after=n50
1477 MHz: before=n50 after=n74
1490 MHz: before=n50 after=n74
1510 MHz: before=n50 after=n74
n74 is 1427–1470 UL / 1475–1518 DL, a Japanese FDD band; n50/n75 (1432–1517 SDL) are what the 1452–1492 MHz SDL deployments in Europe use, and the upper half of that window is exactly the range that flips. So the narrower-before-broader rule buys n74 reachability by mislabelling the more commonly deployed band — the opposite of the deployment argument the block four lines below applies to n66 vs n65`` ("n65 … is essentially undeployed there"). Was that trade intended here, or should n50/`n75` keep the overlap and `n74` stay behind them?
Same question, smaller stakes, for the sub-1 GHz reorder on line 45:`` putting n18/`n5` before `n26` makes both reachable but leaves `n26` (859–894) with only the 859–860 sliver, so 865/880/890 MHz now report `n18`/`n5` where they previously reported `n26`.
For what it's worth, I ran the reachability check the new header comment describes over the whole table — nothing else regressed. Only n51, n75, n65 and n90 are fully unreachable, and those are the four identical-range tie-breaks the comment already names.
Generated by Claude Code
| for (var k in locks) | ||
| if (k != 'lte' && k != 'nr5g' && locks[k] != null && locks[k] !== false) | ||
| out.push('%s: %s'.format(k, JSON.stringify(locks[k]))); |
There was a problem hiding this comment.
nit: this fallback bypasses the enabled check that group() applies, so it contradicts the rule stated in the doc comment above ("A lock that is present but DISARMED is omitted"). locks[k] !== false only catches a literal false; a future RAT reported in the same shape the daemon already uses for lte/nr5g — { enabled: false, values: [...] } — passes both guards and gets listed under "Locked to" as raw JSON, disarmed and all.
Routing unknown keys through group() with no width fixes that and drops the JSON.stringify output at the same time ({ n8: { enabled: true, values: [1, 2] } } → n8 1, 2, { n8: true } → n8 armed):
| for (var k in locks) | |
| if (k != 'lte' && k != 'nr5g' && locks[k] != null && locks[k] !== false) | |
| out.push('%s: %s'.format(k, JSON.stringify(locks[k]))); | |
| for (var k in locks) | |
| if (k != 'lte' && k != 'nr5g') | |
| group(locks[k], 0, k); |
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the amended head. Both commits are new objects (d406f26 replaces d41c76c, 466fd78 replaces c14c503), but both messages are byte-identical to the ones I reviewed last round and still match what they add — so no commit checks this round.
The content delta against my last review is three lines: git diff c14c503..466fd78 is only the LUCI_DEPENDS comment in protocols/luci-proto-wwand/Makefile (+3/−3). The app half is unchanged. CI on the head SHA is green (all three FormalityCheck runs).
One inline comment, on that hunk: the new comment names six shared modules, and wwand.simlist is required by the handler but never used — the interface modal deliberately punts SIM overrides to the Modems page, so that require is dead.
Still open from the previous round, since neither file changed:
bands.js—n74ahead ofn50/n75, i.e. which band should own 1475–1517 MHz.format.js—fmtLocks()'s fallback loop skipping its ownenabledcheck.
Backend coupling: no new UCI option or ubus method in this diff, so nothing new to check against the rule. The wwand.rpc method list and both ACL files are unchanged and still match exactly — every method the app declares is granted in luci-app-wwand.json, and the five read + two write methods the proto handler and the shared wwand.modemopts reach (status, modem_signal, modem_cells, modem_sim_slots, modem_probe, modem_reset, modem_sim_pin_verify, plus file list /sys/class/gpio/) are all covered by luci-proto-wwand.json.
Generated by Claude Code
| # This handler requires six resource modules that luci-app-wwand ships — | ||
| # wwand.bands, .format, .modemopts, .modemsid, .rpc and .simlist — so it | ||
| # depends on the app rather than carrying a second copy of them. |
There was a problem hiding this comment.
Five of the six are used; wwand.simlist is not. 'require wwand.simlist as simlist' at wwand.js:9 is the only reference to the module in the whole handler — there is no simlist. call anywhere in the file, and the _sims_hint option that would have used it deliberately punts to the Modems page instead`` ("can't be embedded as a section list in the interface config modal"). So this comment, which is the only change in this round, documents a dependency that is really a dead require.
The other five check out: bands (3 uses), modemopts (4), wrpc (4), fmt (15), modemsid (2).
Drop the unused require at wwand.js:9 as well, and the count here matches what the file actually pulls in:
| # This handler requires six resource modules that luci-app-wwand ships — | |
| # wwand.bands, .format, .modemopts, .modemsid, .rpc and .simlist — so it | |
| # depends on the app rather than carrying a second copy of them. | |
| # This handler requires five resource modules that luci-app-wwand ships — | |
| # wwand.bands, .format, .modemopts, .modemsid and .rpc — so it depends on | |
| # the app rather than carrying a second copy of them. |
Generated by Claude Code
Found in review on openwrt/luci#8917. The handler requires wwand.simlist and never touches it: SIM overrides moved to the dedicated Network → Modems page, which the comment further down already says. Only the require stayed behind. Five shared modules are actually used — wwand.bands, .modemopts, .rpc, .format and .modemsid.
Netifd protocol handler for `proto wwand`, the cellular connection manager proposed in openwrt/packages#30185. It renders the connection options the daemon consumes — APN, PDP type, authentication, MUX id, MTU handling — and resolves the modem an interface is bound to. wwand registers `proto wwand` only; the historical `qmi` proto name stays uqmi's, so this handler never competes with the stock one for an interface. Signed-off-by: André Valentin <avalentin@marcant.net>
wwand -> v1.4.6 (373e18e). A maintainer on openwrt/packages#30185 pointed out that the device blocklist had been checked against qmi/mbim/ncm/wwan but not against comgt's 3g and directip or against modemmanager. Checked all three against their handlers: 3g and directip declare `device:device` (a real device node) and were already covered; modemmanager declares a plain `device` and puts a SYSFS PATH in it, which could never match a /dev node or a netdev name — so such an interface claimed its modem invisibly. A `device` under /sys/ is now a path-shaped claim too, resolved like uqmi's devpath and wwan.sh's bus. luci-proto-wwand -> 680692f. Drops a dead `require wwand.simlist`: SIM overrides live on the Modems page, only the require stayed behind. Found in review on openwrt/luci#8917. The Makefile comment named the shared modules wrongly in both directions — it claimed one (bands) when five are used, and after a first correction claimed six by counting the dead one.
|
@patrakov Fixed in v1.4.7, which this PR now pins. Thank you for the screenshot Cause. Fix. A branch the serving read does not report is deleted. That is safe The QMI backend never had this: it replaces the whole signal snapshot per poll. While in there, one thing your report implies but does not say: the same modem |
…ocks honours its own rule Both raised in review on openwrt/luci#8917. bands.js — n74 had been moved ahead of n50/n75 to rescue it from being dead behind them. That bought its reachability by handing 1475-1517 MHz to a Japanese FDD band, and 1477/1490/1510 MHz is exactly where the European L-band SDL carriers (n50/n75) sit. The table's own rule says the more commonly deployed band wins an overlap — it is applied four lines below for n66 over n65 — so n74 goes back behind them and keeps only the sliver above 1517. The same argument settles the sub-1 GHz reorder: n5 (global 850) now precedes n26 and n18, so 872/880/890 MHz read n5 instead of a regional band. n26 keeps 859-869 and n18 becomes unreachable. Ordering n18 first would relabel ordinary n5 spectrum as Japan-only, which is the worse of the two errors. A first-match table cannot be right in every region at once. Where it must choose, it now consistently chooses what is deployed, and the header says which bands that costs and why. format.js — the fallback loop that renders unknown lock keys bypassed the `enabled` check group() applies, contradicting the documented rule that a present-but-disarmed lock is omitted. `locks[k] !== false` catches only a literal false, so a future RAT reported in the shape the daemon already uses for lte/nr5g ({ enabled: false, values: [...] }) would have been listed as raw JSON, disarmed and all. Routed through group() instead, which also drops the JSON.stringify output.
|
Both open threads addressed.
The same argument decides the sub-1 GHz case you raised alongside it, and it A first-match table cannot be right in every region simultaneously. The header
Worth noting that the shape you described as hypothetical is what the daemon The third thread — the |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the amended head. Both commits are new objects (1a66040 replaces d406f26, 0e41f7c replaces 466fd78), but both messages are byte-identical to the ones I reviewed last round and both are still single "add the package" commits scoped to one package each — so no commit checks this round.
git diff 466fd78..0e41f7c is +32/−13 across four files, and it is exactly the three threads left open from the last two rounds. All three close:
bands.js— who owns 1475–1517 MHz.n74now followsn50/n75, so 1477/1490/1510 MHz resolve ton50again andn74keeps only the sliver above 1517. I recomputed reachability across the whole table (interval subtraction in table order, not pairwise containment) rather than taking the new paragraph's word for it, and it is exactly right: the dead set is{n18, n51, n65, n75, n90}— the five bands it names, no more and no fewer.n26's surviving remainder really is 859–869, which is what then5/n26/n18swap buys. The sub-1 GHz call is a regional judgement either way, but the comment now states what the rule costs instead of implying only identical-range ties are unreachable, which was the thing worth fixing.format.js— the fallback loop. Unknown keys go throughgroup()now, so the disarmed check applies to them and theJSON.stringifyoutput is gone. One residual nit inline, on whatgroup()does with a payload that is not avaluesarray.Makefile/wwand.simlist. The require is dropped and the comment corrected to five. Checked rather than assumed: each of the handler's five requires (bands,modemopts,rpc,format,modemsid) is actually referenced inprotocol/wwand.js; none of the five pullssimlistin transitively, so the count is right at both levels; andwwand.simliststill has a live consumer inview/wwand/modems.js, so removing the require does not orphan the module the app ships. The remainingwwand.simlistmention at wwand.js:550 is prose pointing at the Modems page, not a dangling reference.
No new UCI option or ubus method in this diff, so nothing new to check against the backend-coupling rule. All 13 JS files pass node --check and all three JSON files parse. CI on the head SHA is green (all three FormalityCheck runs).
Generated by Claude Code
| for (var k in locks) | ||
| if (k != 'lte' && k != 'nr5g') | ||
| group(locks[k], 0, k); |
There was a problem hiding this comment.
nit: routing the fallback through group() fixes the enabled check, but it also drops the payload for every shape except { values: [...] } — which is the half of the comment above that says "shown rather than silently dropped".
group() reads the value only from var v = l.values || [] at format.js:231,`` so a future key that is a scalar ({ band_lock: 'n78' }) or a plain object with no `values` array (`{ band_lock: { earfcn: 1300, pci: 246 } }`) falls straight past both branches and renders as `band_lock armed`. The old `JSON.stringify` branch listed disarmed locks — which is why it had to go — but it did print what the lock held.
Widening the payload read inside group() closes it without reinstating that branch: treat a non-object, non-true l as a single value, and fall back to the object's own entries when values is absent. { lte: true } keeps saying LTE armed, the known keys are unaffected, and an unknown key keeps its content instead of being reduced to "armed".
Not live today — the daemon emits only lte/nr5g — so purely about the fallback matching what the doc comment promises of it.
Generated by Claude Code
Follow-up finding on the previous fix, from review on openwrt/luci#8917. Routing the fallback loop through group() fixed the disarmed check, but group() reads the payload only from `l.values` — so a scalar ({ band_lock: 'n78' }) or an object without a values array ({ band_lock: { earfcn: 1300, pci: 246 } }) rendered as a bare "band_lock armed" and lost what the lock held. The removed JSON.stringify branch had to go because it listed disarmed locks, but it did print the payload; dropping it silently traded one half of the doc comment for the other. group() now reads the payload from whatever shape arrived: a scalar is its own value, a lone non-array `values` is a single value, and an object without `values` is rendered from its own entries. `true` stays payload-free, since that IS the "armed, no detail" spelling. The known keys are untouched — lte/nr5g still pair their values by width (LTE 1:2, 3:4), and a disarmed lock is still omitted.
|
Right on both counts — the fix traded one half of the comment for the other.
Widened the payload read inside
Checked each shape against the real function rather than by reading it. |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the amended head. 1a66040 is byte-identical to the d406f26 I reviewed last round; 46fb9e1 replaces 0e41f7c with the same message, and it is still the single "add the package" commit for luci-app-wwand, so no commit checks this round.
git diff 0e41f7c..46fb9e1 is +19/−1 in one file — the group() payload widening from the last open thread. CI on the head SHA is green (all three FormalityCheck runs).
The widening does what your table says for every shape it lists — I ran the function rather than reading it: {enabled:true,values:[1300,246,5230,118]} → LTE 1300:246, 5230:118, 'n78' → band_lock n78, {values:'n41'} → band_lock n41, true → band_lock armed, {enabled:false,values:[7]} omitted, and the known keys still pair by width. Nothing from the previous rounds reopens, and no new UCI option or ubus method here, so nothing new for the backend-coupling rule.
Two inline comments, both on the new branch chain. The one worth a look is a shape the chain does not cover: a bare array ({ lte: [1300, 246] }) reaches the l.values != null test, and since Array.prototype.values is a function that test passes — the row renders as LTE function values() { [native code] }.
Generated by Claude Code
| else if (Array.isArray(l.values)) | ||
| v = l.values; /* lte/nr5g and anything like them */ | ||
| else if (l.values != null) | ||
| v = [ l.values ]; /* a lone non-array value */ |
There was a problem hiding this comment.
A bare-array payload — { lte: [1300, 246] }, the most obvious shape after { values: [...] } — falls into the l.values != null branch, because Array.prototype.values is a real function on every array. v becomes [ Array.prototype.values ] and the row renders as:
LTE function values() { [native code] }
Ran the actual function over the shapes in your table plus this one: {lte:{enabled:true,values:[1300,246,5230,118]}} → LTE 1300:246, 5230:118, {band_lock:'n78'} → band_lock n78, {band_lock:{values:'n41'}} → band_lock n41 — all as documented; {lte:[1300,246]} and {nr5g:[242,431070,1,78]} are the two that produce the native-code string. An explicit array branch before the l.values tests fixes it and also restores the colon pairing for that shape (LTE 1300:246):
| else if (Array.isArray(l.values)) | |
| v = l.values; /* lte/nr5g and anything like them */ | |
| else if (l.values != null) | |
| v = [ l.values ]; /* a lone non-array value */ | |
| else if (Array.isArray(l)) | |
| v = l; /* bare array: Array.prototype.values is a function */ | |
| else if (Array.isArray(l.values)) | |
| v = l.values; /* lte/nr5g and anything like them */ | |
| else if (l.values != null) | |
| v = [ l.values ]; /* a lone non-array value */ |
Generated by Claude Code
| else | ||
| v = Object.keys(l).filter(function(kk) { return kk != 'enabled'; }) | ||
| .map(function(kk) { return '%s=%s'.format(kk, l[kk]); }); |
There was a problem hiding this comment.
nit: the k=v strings from this branch still go through the width pairing below, so a known key that arrives in this shape gets colon-joined key/value pairs rather than the , spelling your table shows. { lte: { earfcn: 1300, pci: 246 } } renders LTE earfcn=1300:pci=246, while the same payload under an unknown key (width 0) correctly renders band_lock earfcn=1300, pci=246. Colon pairing only means something for a positional values array, so dropping the width here keeps the two consistent:
| else | |
| v = Object.keys(l).filter(function(kk) { return kk != 'enabled'; }) | |
| .map(function(kk) { return '%s=%s'.format(kk, l[kk]); }); | |
| else { | |
| v = Object.keys(l).filter(function(kk) { return kk != 'enabled'; }) | |
| .map(function(kk) { return '%s=%s'.format(kk, l[kk]); }); | |
| width = 0; /* k=v pairs are not positional */ | |
| } |
Generated by Claude Code
…ing k=v Two follow-up findings on the previous fix, both from review on openwrt/luci#8917 and both real. A bare array — { lte: [1300, 246] }, the most obvious shape after { values: [...] } — fell into the `l.values != null` branch, because Array.prototype.values is a real function on every array. v became [ Array.prototype.values ] and the row rendered as LTE function values() { [native code] } An explicit Array.isArray(l) branch ahead of the l.values tests fixes it and restores the colon pairing for that shape (LTE 1300:246). And the k=v branch fed its strings through the width pairing, so the same payload rendered differently depending on the key it arrived under: { lte: { earfcn: 1300, pci: 246 } } gave "LTE earfcn=1300:pci=246" while { band_lock: {...} } (width 0) gave "band_lock earfcn=1300, pci=246". Colon pairing only means something for a positional values array, so that branch clears the width. Checked every shape against the real function, not by reading it.
Status and management UI for the wwand cellular connection manager proposed in openwrt/packages#30185: modem overview, per-modem tools (radio technologies, band selection, network selection and scan, cell lock), SIM and eSIM profile management, SMS, and the per-interface migration entry point that converts a stock cellular interface to `proto wwand` in place. Everything is driven over the daemon's ubus API; the ACL file lists exactly the methods the pages call. Signed-off-by: André Valentin <avalentin@marcant.net>
|
Both correct, both fixed — the array one is a good catch.
The width point is right too, and it was inconsistent in a way that would have Ran every shape through the real function again:
The PR body is updated to v1.4.7 as well — you were right that it had fallen |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the amended head. 1a66040 is byte-identical to the proto commit I reviewed last round; bc1667f replaces 46fb9e1 with a byte-identical message and is still the single "add the package" commit scoped to applications/luci-app-wwand/ — so no commit checks this round.
git diff 46fb9e1..bc1667f is +5/−1 in one file, exactly the two threads left open from the last round, and both close:
- Bare-array payload. The explicit
Array.isArray(l)branch now sits ahead of thel.valuestests, soArray.prototype.valuescan no longer be mistaken for a payload. I ran the function rather than reading it:{lte:[1300,246]}→LTE 1300:246and{nr5g:[242,431070,1,78]}→NR5G 242:431070:1:78, both with the colon pairing restored, in place of thefunction values() { [native code] }string. Branch order is right —l === trueis still tested first, so the "armed, no detail" spelling is unaffected, andtypeof l != 'object'still catches scalars, since an array never reaches it. - Width on the
k=vbranch.width = 0there makes the same payload render identically under a known and an unknown key:{lte:{earfcn:1300,pci:246}}→LTE earfcn=1300, pci=246, matching{band_lock:{earfcn:1300,pci:246}}→band_lock earfcn=1300, pci=246. Previously the first producedLTE earfcn=1300:pci=246. Reassigning the parameter is local to the call, so the widths passed at the three call sites are untouched.
Every row of the table in your comment reproduces against the real function, including the ones this round did not change ({enabled:false,values:[7]} omitted, 'n78' → band_lock n78, {values:'n41'} → band_lock n41, true → band_lock armed, and the two known keys pairing by width). Nothing from the previous rounds reopens.
No new UCI option or ubus method in this diff, so nothing new to check against the backend-coupling rule. All 13 JS files pass node --check and all three JSON files parse. CI on the head SHA is green (all three FormalityCheck runs).
One nit inline, and it is about the PR description rather than the code.
Generated by Claude Code
| include $(TOPDIR)/rules.mk | ||
|
|
||
| LUCI_TITLE:=Modem status and settings pages for the wwand connection manager | ||
| LUCI_DEPENDS:=+wwand +luci-base |
There was a problem hiding this comment.
nit: the PR body edit that went with this push did not land — it still reads "Pairs with openwrt/packages#30185 (wwand 1.2.0)", while the comment accompanying this head says "The PR body is updated to v1.4.7 as well". The 2026-08-22 comment had already moved the pin past 1.2.0 ("which now points at wwand v1.4.1"), so the body has been three releases behind since then, not one.
Anchoring here because this line is where the tree states the dependency, and it carries no version: +wwand accepts whatever the feed pins, so the PR body is the only place a reader learns which wwand release these pages actually need — and it currently names one that predates proto_switch, fcc_auth and the {"enabled":…,"values":[…]} lock shape this round's fmtLocks() is written against.
(No suggestion block — the fix is in the PR description, not in this file.)
Generated by Claude Code
LuCI companion packages for the wwand cellular connection manager (submitted to openwrt/packages as
net/wwand, upstream repo https://github.com/ddimension/wwand).proto qmi/mbim/ncminterface in place toproto wwand(user-triggered). Ships the sharedwwand.bandsband/frequency resource.proto 'wwand'(Network → Interfaces): modem selection, APN / PDP type / auth, per-interface options.Good-citizen coexistence. luci-proto-wwand ships only
protocol/wwand.js— it does not install aprotocol/qmi.jsalias and does not conflict with luci-proto-qmi.proto qmi/mbim/ncminterfaces stay with the stock LuCI proto handlers; wwand manages onlyproto wwand, and moving an interface across is the explicit "Migrate" action above. Pairs with openwrt/packages#30185 (wwand 1.2.0).🤖 Generated with Claude Code