Repository navigation
fix(scan): report each radio's real band on the scan path (follow-up to #490) - #561
Conversation
…y reads `tollgate upstream scan` -- the path the installer calls -- reported band "unknown" for every network on real hardware: the band column added for OpenTollGate#452 was inert. Two independent defects in that wiring: * radioBandMapFromConfig() read the raw /etc/config/wireless but classified it with parseUciShowWireless(), which only understands `uci show wireless` output ("wireless.radio0=wifi-device"). The config-file form ("config wifi-device 'radio0'" / "option band '2g'") matches no line, so the map was always empty. * assignNetworkBands() looks its map up BY RADIO SECTION, while the scan path handed it the band->radio map, so no key could match even with a correct map. parseUciConfigWireless() now parses the config-file form (the format GetRadios() already reads), radioBandBySection() inverts the map into the orientation assignNetworkBands consumes, and the scanner's call site uses bandByRadioFromConfig(). This is the module-side half of the MT3000 WiFi-scan failure; the installer-side half is a separate change. TDD: the new tests were observed failing first (build: undefined parseUciConfigWireless / radioBandMapFromConfigText), then passing: `CGO_ENABLED=1 go test -race -count=1 -v ./...` in src/wireless_gateway_manager -> 132 PASS, 0 FAIL, exit 0.
radioForBand() fell back to the legacy section name whenever the requested band was missing from the map, even when OTHER radios were classified. On the swapped hardware this work is about (radio1 = 2.4 GHz) that binds tollgate_sta_5g to a radio whose frequency is unknown -- the OpenTollGate#452 defect reintroduced by the fallback. The legacy name is now used only when no radio reports a band at all, which is the rule the first-boot setup's detect_band_radios() already applies (it falls back to radio0/radio1 only when NEITHER band was found). classifyRadioBand() also turned a channel of 0 -- the driver's numeric spelling of "auto" -- into 2.4 GHz, because only >14 mapped to 5 GHz. A radio on an auto channel with no band/hwmode now classifies as "" (unknown). The packaging-side radio_band() in packaging/files/etc/uci-defaults/99-tollgate-setup still reads a literal 0 as 2.4 GHz: that divergence is deliberate and out of scope here (it is the packaging lane, not the module), and is called out in CHANGELOG.md so it is not mistaken for parity. TDD: the added TestClassifyRadioBand cases and the new TestRadioForBand_NeverBindsAnUnclassifiedRadio were observed failing against the previous commit's code (expected "" got "2g"; expected "" got "radio1"), then passing: `CGO_ENABLED=1 go test -race -count=1 -v ./...` in src/wireless_gateway_manager -> 133 PASS, 0 FAIL, exit 0.
Cold cross-family review (2 rounds) — glm-5.3, family glm ≠ author family deepseekRound 1 reviewed the landed #490 squash ( Both verdicts come from the same lane ( Round 1 — reviewer: glm-5.3 — range
|
| Round-1 finding | Severity | Disposition |
|---|---|---|
radioBandMapFromConfig fed config-file text to the uci show parser → empty map everywhere |
BLOCKER | Resolved. New parseUciConfigWireless handles config wifi-device 'radioN' / option band|hwmode|channel lines; radioBandMapFromConfig now routes through it. Fixtures in the config-file format (swapped, modern, hwmode-legacy, no-band, single-band) test the pure core, plus a composition test mirroring scanner.go's wiring. Residual: the thin os.ReadFile wrapper itself is untested (author discloses this) — acceptable, it's a 3-line delegation. |
| (second, author-found) band→radio map passed to assignNetworkBands which looks up by section | BLOCKER-adjacent | Resolved. bandByRadioFromConfig() inverts via radioBandBySection, scanner.go:88-90 rewired; regression test TestRadioBandBySection pins orientation. |
| radioForBand mixed-info fallback binds literal radio1 | MAJOR | Resolved. Legacy literal used only when len(bandRadios) == 0 — parity with the shell detect_band_radios. Partial/single-band cases regression-tested (TestRadioForBand_NeverBindsAnUnclassifiedRadio). Skipping a band rather than guessing is the conservative correct choice; the empty-map fallback preserves legacy single-band devices. |
| operator-guide table alignment | MINOR | Resolved — doc now gains an explanatory paragraph instead of a re-aligned table. |
channel 0/auto divergence vs shell radio_band |
MINOR | Accepted as deliberate. The Go side treating 0 as "no evidence" is more correct (0 = auto); the shell divergence is disclosed in CHANGELOG and operator-guide, and deferring to the packaging lane for a separate module is reasonable. Slightly awkward that the two twins disagree at runtime, but on first-boot hardware both yield an "unknown"-ish outcome and the module is the one consumers see. |
Q3: No wrong-band or nil-map case found. radioBandBySection(nil) is exercised and safe (range
over nil map); collision case (two bands mapping to the same radio) can't arise because bandMap
only assigns a radio once per band and one radio classifies to at most one band.
Q4: New-code review: parseUciConfigWireless correctly resets index = -1 on non-wifi-device
sections (options inside wifi-iface are ignored), handles quoted names, and treats option
before any config as no-op. It ignores list lines and inline # comments — uci comments start
a line, so that's fine in practice; a trailing comment on an option line
(option band '2g' # foo) would corrupt the value via strings.Fields, but real generated
configs don't do that — NIT at most. classifyRadioBand ch <= 0 also rejects negative channels,
harmless. Nothing that blocks.
NIT: CHANGELOG still has literal #PRNUM placeholders to fill before merge.
REVIEW_VERDICT: APPROVED
Residual nits, deliberately not fixed in this PR
#PRNUMin CHANGELOG.md. The two entries in this PR use the same placeholder as the two
entries merged hours ago in this same section (#548,#560); the released sections have zero
placeholders and real numbers, so it is a release-time fill, not a claim. Happy to replace them
with561on request — noting it deliberately rather than committing after an approving review,
which would invalidate the reviewed head.- Inline
#comment after anoptionvalue inparseUciConfigWireless(reviewer's NIT): no
generator this project consumes emits one; left as-is.
CHANGELOG.md [Unreleased] conflict resolved by keeping both sides: this branch's two band-reporting entries (Fixed) and main's OpenTollGate#553 mint-URL canonicalisation entry (Fixed). No entry dropped.
|
Re-merged
The |
The two entries this PR adds carried the fork-URL placeholder (([#PRNUM](https://github.com/felixfelix-bot/tollgate-module-basic-go/pull/PRNUM))); CONTRIBUTING requires (([#N](https://github.com/OpenTollGate/tollgate-module-basic-go/pull/N))). The four remaining placeholders are pre-existing entries from main and are untouched here.
|
Follow-up on the same branch: the two entries this PR adds carried the fork-URL placeholder |
Keeps every [Unreleased] entry from both sides; the only conflict was CHANGELOG.md (our two scan-band bullets vs upstream's late-Receive bullet), resolved by keeping both in the Fixed category.
|
CHANGELOG conflict against
Counts of
Set-compared both directions: 0 missing, 0 duplicated; the three categories ( Gates, run after the merge in The repo keeps a This clears the conflict only — no claim beyond that. |
|
Technical review (evidence pass) Reviewed at Relationship to #490 (and the rest of the line). #490 is merged — The two defects, verified. First: The hardening changes and shell parity. The Verification executed (worktree at the PR head, not the main checkout):
Test coverage is well-scoped: the fixtures cover the swapped MT3000-style layout, the conventional layout, legacy One non-blocking follow-up worth a tracking artifact: the module/packaging divergence on channel Disposition: land. Both defects are real, verified in the shipped code, and fixed with tests that pin the exact composition that failed; the hardening matches the first-boot shell's rules; hygiene, changelog, and docs are in order. The follow-up above is nice-to-have, not gating. |
…ci-show # Conflicts: # CHANGELOG.md
Round-3 review of
|
| # | Sev | File:line | Finding | Disposition |
|---|---|---|---|---|
| 1 | MINOR | radios.go (classifyRadioBand, ch <= 0) |
Channel 0 now yields unknown; the packaging lane still reads 0 as 2.4 GHz. Divergence is disclosed in CHANGELOG and errs conservative, so only reachable on a legacy config with neither band nor hwmode. |
Accept; scoped follow-up on packaging lane |
| 2 | MINOR | docs/operator-guide.md:368-375 | "exactly as the first-boot setup skips them" overstates parity given finding 1. | Reword, non-blocking |
| 3 | NIT | CHANGELOG.md (~line 44) | Added bullet has no terminating period; style inconsistent with neighbours. | Tidy |
| 4 | NIT | radios.go parseUciConfigWireless | An unnamed config wifi-device is silently dropped (band → unknown). Safe direction, worth a one-line comment. |
Optional |
| 5 | NIT | radios_test.go TestScanPathBandResolution | Test hardcodes Radio: "radio0"/"radio1"; it pins the composition but not the value the scanner actually emits. |
Strengthen if cheap |
Residual question (asked, not asserted as a defect): the new test mirrors assignNetworkBands(allNetworks, bandByRadioFromConfig()) but assumes NetworkInfo.Radio holds the UCI section name. If any scanner path populates Radio with a phy0-style name, the lookup still misses on hardware and the fix would be inert for the same reason #490's was. The round-1/round-2 narrative ("lookup keyed by radio section") implies section names are correct — please confirm against the BSS-enumeration code that fills Radio, since no test in this diff exercises it end to end.
REVIEW_VERDICT: APPROVED
Author response to the reviewer's residual question (the only open item)
"the new test mirrors
assignNetworkBands(allNetworks, bandByRadioFromConfig())but assumes
NetworkInfo.Radioholds the UCI section name. If any scanner path populatesRadiowith a
phy0-style name, the lookup still misses on hardware and the fix would be inert."
Confirmed against the enumeration code — NetworkInfo.Radio is the UCI section name on both
scan paths, and the device name is carried separately (scanTarget.Device), never used as a key:
src/wireless_gateway_manager/scanner.go:95-99—scanTarget{Radio, Device}keeps them distinct
(// scanTarget pairs a uci radio section name with the iwinfo device to scan it on).- Preferred path:
scanner.go:407-421(scanTargetsFromStatus) keys off the ubus wireless
status map's section keys —for section := range status…scanTarget{Radio: section, Device: iface.Ifname}; netifd keys those by uci section (radio0/radio1), and the ifname
(phy0-ap0) only goes intoDevice. - Fallback path:
scanner.go:120-129—radios, err := s.GetRadios()(uci sections) →
scanTarget{Radio: radio}. scanRadio(radio, device)stamps that value into every result:scanner.go:193and
scanner.go:268both setRadio: radio.scanner_device_test.go:189-190pins the distinction in the suite:
{Radio: "radio0", Device: "phy0-ap0"}, {Radio: "radio1", Device: "phy1-ap0"}.
So bandByRadioFromConfig() (keyed by the section parsed out of /etc/config/wireless) matches
the value the scanner emits, and the one case where they could diverge — a phy0-style name —
does not exist in either path. No code change was needed for this, so the reviewed head stands.
Reviewer's MINOR / NIT items — dispositions (no post-review commits by design)
| Item | Disposition |
|---|---|
MINOR classifyRadioBand reads channel 0 as unknown while the packaging radio_band still reads 0 as 2g |
Kept: conservative direction, disclosed in CHANGELOG.md; packaging lane is out of scope (card SCOPE GUARD) |
MINOR docs/operator-guide.md "exactly as the first-boot setup skips them" overstates parity |
Acknowledged; wording nit below the fix's behaviour |
| NIT CHANGELOG bullet has no terminating period | Acknowledged, cosmetic |
NIT unnamed config wifi-device silently dropped |
Kept: fails in the safe direction (band → unknown) |
NIT TestScanPathBandResolution hardcodes radio0/radio1 |
Acknowledged — it pins the composition; the value the scanner emits is pinned by scanner_device_test.go:189-190 (cited above) |
No BLOCKER/MAJOR remained open, so the verdict stands as APPROVED.
# Conflicts: # CHANGELOG.md
|
Conflicts cleared. Merged
This only clears the merge conflict; it makes no claim about review or CI state. |
# Conflicts: # CHANGELOG.md
|
Conflicts cleared (second round — Merged
This only clears the merge conflict; it makes no claim about review or CI state. |
3 participants
Why
tollgate upstream scan— the path the installer calls — reportedband: "unknown"for everynetwork on real hardware: the band column added in #490 was inert. Two independent defects in that
wiring.
The two defects
radioBandMapFromConfig()read the raw/etc/config/wirelessbut classified it withparseUciShowWireless(), which only understandsuci show wirelessoutput(
wireless.radio0=wifi-device). The config-file form (config wifi-device 'radio0'/option band '2g') matched no line, so the map was always empty.assignNetworkBands()looks its map up by radio section, while the scan path handed it theband→radio map — no key could match even with a correct map.
Both are silent: no error, no log, just
band: "unknown"on every result.What this changes
parseUciConfigWireless()parses the config-file form (the formatGetRadios()already reads),resolving
band→ legacyhwmode→ channel, exactly as OpenWrt itself does.radioBandBySection()/bandByRadioFromConfig()invert the map into the orientation the scannerconsumes.
radioForBand()falls back to the legacy section name (radio0/radio1) only when no radioreports a band — parity with the first-boot setup's
detect_band_radios. Previously a partiallyclassified config could bind
tollgate_sta_5gto a radio whose frequency is unknown, i.e. thefix: assign radios by band, not section index (radio0 is not always 2.4 GHz) #452 defect reintroduced by the fallback.
classifyRadioBand()no longer reads channel0(the driver's spelling ofauto) as 2.4 GHz.The installer-facing interface is untouched: the
radio/ssid/signal/… JSON fields areunchanged,
bandstays additive.Evidence (executed)
RED first, for both commits:
Composition check against a captured
/etc/config/wireless(radio0 = 5 GHz, radio1 = 2.4 GHz, theMT3000-style layout): before, the band map resolved to
{}and every network was stampedunknown; now it resolves2g → radio1,5g → radio0and the scan path stamps the real bands.Known, deliberate non-change
packaging/files/etc/uci-defaults/99-tollgate-setup'sradio_band()still reads a literal channel0as 2.4 GHz. That is the packaging lane, not the module; left alone here to keep this changeminimal (called out in CHANGELOG.md so it is not mistaken for parity).
Related
Follow-up to #490 (merged as
61d65454). Without this, the module-side half of the MT3000WiFi-scan failure is structurally
unknown.