luci-app-openthread: read the stopped-network dataset via ubus status - #8890
luci-app-openthread: read the stopped-network dataset via ubus status#8890LorbusChris wants to merge 2 commits into
Conversation
Add a LuCI web interface for the OpenThread Border Router (openthread-br package): network overview with a live neighbor table, network scan and join, network settings including MAC-filter management, joiner commissioning, and a network topology list view. The application is client-side JS (view.extend / rpc.declare) with an rpcd ucode backend (luci.openthread) that brokers ACL-gated access to otbr-agent's ubus API; the raw otbr ubus object is not exposed to LuCI sessions. This migrates the app out of the ot-br-posix repository, where a legacy Lua/htm version was previously maintained under src/openwrt and packaged by the openthread-br feed package, per the maintainers' request in openthread/ot-br-posix#3431 and openwrt/packages#29791. The UI has been rewritten for the modern client-side LuCI framework as part of the move. Signed-off-by: Christian Glombek <c.glombek@cosa.systems> Assisted-By: Claude Fable 5
The otbr ubus series for Matter integration adds a status method that returns the committed Active Operational Dataset as TLVs. Prefer it for the stopped-network summary: one round-trip instead of three, and the values come from the dataset itself rather than the per-field getters. Without the method the stock fallback path stays in use, so the app keeps working against an unpatched otbr-agent. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems>
openwrt-ai
left a comment
There was a problem hiding this comment.
First review of this PR. The base commit 155161b is the head of #8871 and is being reviewed there, so this pass is scoped to the one new commit, f5b4b00 — a single hunk in the rpcd ucode backend.
What I checked:
- Commit message vs. diff. Matches: the subject scope (
luci-app-openthread:) is the only package touched, and the body describes exactly the status-preferred path plus getter fallback that the diff implements. Nothing to flag. - Fallback safety.
otbr()substitutes{ error: 255 }whenubus.callreturns null, sootbr('status').ActiveDatasetis null rather than a throw on a daemon without the method, anddataset_summary()rejects it on thetype(...) != 'string'guard. The stock path is reached, as the commit claims. - TLV decoding. Types 0/1/3 are Channel, PAN ID and Network Name per the MeshCoP dataset TLV numbering, the Channel branch correctly skips the channel-page byte, and the walk is bounded at both ends (
i + 4 <= n, truncated-value break,l == 0still advances).chr()in ucode is byte-based (lib.c:711-737), so a UTF-8 network name round-trips intact rather than being re-encoded per byte. - Type/format parity with the branch it replaces.
panidis emitted as'0x' + lc(v), matching the0x%04xstring thepanidgetter returns — thelc()added in this commit is what makes the two agree, andoverview.jsrenders it throughString()either way.channelis a number on both paths. - ACL. No new
luci.openthreadmethod, and thestatuscall is made by the plugin as root, soacl.d/luci-app-openthread.jsonneeds no change. Correct as-is. - Backend coupling. Per
.github/llm-review-rules.mdthis would normally want a consumer check, butstatuslives in ot-br-posix, which is not among the pre-cloned trees; the PR body already states the method is unmerged upstream and that the PR stays draft until it lands, so I have not flagged the dependency. - CI. No checks have run on
f5b4b00yet (statuspending, 0 checks), so there is nothing to ground against.
Two follow-ups inline: one on configured losing the unprovisioned guard on the new path, one nit on the per-poll cost against an unpatched agent.
Generated by Claude Code
| // disabled, with the broadcast PAN id marking an unprovisioned | ||
| // device. | ||
| let ds = dataset_summary(otbr('status').ActiveDataset); | ||
| if (ds) { |
There was a problem hiding this comment.
The unprovisioned check that the fallback branch keeps (lc(panid) != '0xffff', line 96) has no counterpart on the status path: here configured is set to true for any truthy ds. Two ways that diverges from the branch it is preferred over:
dataset_summary()doesout ??= {}(line 35) before testing the TLV type, so a TLV stream that parses but contains none of Channel / PAN ID / Network Name returns a truthy{}.configuredthen goes true whilenetworkname,panidandchannelare all null.- Even with a well-formed dataset, a committed PAN ID of
0xffffis reported as configured, where the getter path deliberately treats it as unprovisioned.
configured is not cosmetic: it removes the Create button at overview.js:654 and swaps the "No Thread network is configured yet…" placeholder for a network row at [`overview.js:739`](https://github.com/openwrt/luci/blob/f5b4b0072509e0215beb73b33de4d71cd8f0d129/applications/luci-app-openthread/htdocs/luci-static/resources/view/openthread/overview.js#L739).`` So the failure mode is a Thread row reading Network ? | PAN ID <absent> on a device that has nothing configured, with no way to create one.
This may well be unreachable in practice — if the patched otbr-agent reports an empty/absent ActiveDataset on an unprovisioned device, dataset_summary() returns null and the getter path handles it correctly. But that is a property of a daemon method that does not exist upstream yet, and the guard is one condition. Requiring the PAN ID TLV also means a dataset the decoder cannot make sense of degrades to the getters rather than to a half-filled row:
| if (ds) { | |
| if (ds != null && ds.panid != null && lc(ds.panid) != '0xffff') { |
Generated by Claude Code
| // which reflect the committed dataset even while Thread is | ||
| // disabled, with the broadcast PAN id marking an unprovisioned | ||
| // device. | ||
| let ds = dataset_summary(otbr('status').ActiveDataset); |
There was a problem hiding this comment.
nit: on a stock otbr-agent this probe is repaid every tick. state_summary is on the overview's 2 s poll (overview.js:966-972),`` and while the network is disabled every tick now makes a status call that returns `UBUS_STATUS_METHOD_NOT_FOUND` before falling through to the three getters — four round-trips where the previous revision made one to three. Since the answer ("this daemon has no `status`") cannot change without otbr-agent restarting, latching it in a module-level flag the first time the call fails would make the unpatched path cost exactly what it did before. That is the same reasoning as the `srp_cache` TTL added one round earlier on this file, so it seemed worth raising rather than assuming it was considered.
Also worth stating in the commit message: "one round-trip instead of three" holds only against a patched agent; against a stock one the change adds a round-trip rather than saving two.
Generated by Claude Code
Stacked on #8871 — only the top commit is new. Draft until its backend prerequisite exists upstream: the
statusubus method is part of the otbr Matter-integration series being upstreamed to openthread/ot-br-posix (coordination with its author in project-chip/matter-openwrt#53; the surrounding series is openthread/ot-br-posix#3489 plus a follow-up). The ot-br-posix PR will be linked here once it is open.The
statusmethod returns the committed Active Operational Dataset as TLVs. Prefer it for the stopped-network summary: one round-trip instead of three, and the values come from the dataset itself rather than the per-field getters. Without the method the stock fallback path stays in use, so the app keeps working against an unpatched otbr-agent either way.