diff --git a/.superpowers/sdd/final-review-fix-2-report.md b/.superpowers/sdd/final-review-fix-2-report.md new file mode 100644 index 0000000..9ee3392 --- /dev/null +++ b/.superpowers/sdd/final-review-fix-2-report.md @@ -0,0 +1,203 @@ +# Final code review fix — report 2 + +Date: 2026-08-17 +Branch: feature/signal-calibration-config + +## Summary + +Nine findings from the final code review of the signal-calibration feature. +No C++ files were modified (those were already fixed in a separate commit). + +--- + +## Finding 1 — `calibration.js:20` cross-hub parity break in `baseSignalName` + +**File:** `Client/udpstreamer/static/calibration.js` + +**Change:** Line 20: `open > 0` → `open >= 0`. + +An input of `"[0]"` has the `[` at index 0. The old guard `open > 0` let it +fall through and return `"[0]"` unchanged, which then passed the non-empty check +in `normaliseCal` and was accepted as a signal name. Go's `arrayIndexSuffix` +regexp and the C++ `strchr` truncation both reduce `"[0]"` to the empty string +and reject the entry. The fix makes JS match. + +Two assertions added to the existing tests in +`Client/udpstreamer/test/calibration.test.js`: + +- In `'baseSignalName strips an element suffix'`: + `assert.strictEqual(C.baseSignalName('[0]'), '');` +- In `'normaliseCal rejects invalid entries'`: + `assert.strictEqual(C.normaliseCal({source: 'w', signal: '[0]'}), null);` + +--- + +## Finding 2 — `app.js:3470` stale transitional guard + +**File:** `Client/udpstreamer/static/app.js` + +**Change:** Replaced +```js +if (typeof refreshVScaleMenu === 'function') refreshVScaleMenu(); // Task 8 +``` +with: +```js +refreshVScaleMenu(); +``` +`refreshVScaleMenu` is a hoisted function declaration and is always defined. +The guard and the task-number comment were both remnants of incremental +development and are now removed. + +--- + +## Finding 3 — `index.html:223` wrong length cap on unit input + +**File:** `Client/udpstreamer/static/index.html` + +**Change:** Removed `maxlength="16"` from ``. + +`maxlength` counts UTF-16 code units, not UTF-8 bytes, so it under-counted +multi-byte characters. `normaliseCal` (using `TextEncoder`) is the correct and +sole enforcement point. + +--- + +## Finding 4 — stale trigger-threshold unit hint on signal change + +**File:** `Client/udpstreamer/static/app.js` + +**Change:** Added `refreshTrigThresholdField();` at both places where +`trig.signal` is assigned in the `trig-signal` change handler (lines ~2555 +and ~2565). Previously the hint was only updated when calibration changed, +not when the user picked a different trigger signal, leaving a stale unit name +in the tooltip. + +--- + +## Finding 5 — `configcheck/main.go:159` dead code + +**File:** `Test/E2E/suite/client/configcheck/main.go` + +**Change:** Deleted `func (c *conn) nextOneOf(...)` (33 lines) and its preceding +doc comment. The function had no callers; the actual reload check uses +`c.next("configReloaded")` and a separate `c.nextWithin("sources", ...)` peek. +No import became orphaned. + +--- + +## Finding 6 — `hub_calibration_test.go:132-134` orphaned comment + +**File:** `Common/Client/go/wshub/hub_calibration_test.go` + +**Change:** Deleted the three-line comment block that introduced `waitBroadcast`, +a function that was never written. The comment sat directly above +`func sleepMillis(...)` and served no purpose. + +--- + +## Finding 7 — `calibration.go:14` false parity claim in comment + +**File:** `Common/Client/go/wshub/calibration.go` + +**Change:** Rewrote the comment above `arrayIndexSuffix`. The old comment said +the regexp "Mirrors the C++ `strchr(signal,'[')` truncation" — which is false. +The regexp is anchored to the end of the string and requires digits; `strchr` +finds the first `[` anywhere in the name. For `A[1]B`, Go's regexp leaves it +unchanged while C++ truncates to `A`. The new comment describes both +implementations accurately and calls out the known difference explicitly. + +--- + +## Finding 8 — `Docs/StreamHub-API.md` missing calibration limits + +**File:** `Docs/StreamHub-API.md` + +**Change:** Added two rows to the §5 Limits table: + +| Limit | Value | +|-------|-------| +| Calibration entries | 256 (C++ hub, `kMaxCalibration`); unbounded (Go hub) | +| Calibration unit override | 16 UTF-8 bytes | + +--- + +## Finding 9 — spec discrepancies + +**File:** `docs/superpowers/specs/2026-08-16-udpstreamer-signal-calibration-and-config-design.md` + +Five sub-items: + +**9a** — `configReloaded` row missing `path` field. +Both hubs always include `path` in `configReloaded` (verified: Go +`buildConfigAckMsg` passes `sm.Path()` as path; C++ `BroadcastConfigAck` +formats `sourcesFile_` as `"path"` in the ok branch for `configReloaded`). +Fixed: added `"path":string` to the `configReloaded` row in the WebSocket +protocol table. + +**9b** — "max 16 chars" → "max 16 UTF-8 bytes". +Fixed the unit validation cell in the data-model table. + +**9c** — `StreamString` fields → fixed `char[]` arrays. +`CalibrationEntry` in `StreamHub.h` uses `char source[128]`, `char signal[128]`, +`char unit[17]` — no `StreamString`. Updated the C++ hub-implementation +paragraph to name the struct and its actual field types. + +**9d** — E2E chain scenario → standalone `configcheck` program. +Replaced the description of a chain-scenario extension with an accurate +description of `Test/E2E/suite/client/configcheck/`, quoting its actual behaviour +(connects to either hub, asserts calibration protocol, exits non-zero on failure). + +**9e** — "four new frames" → five. +Updated the documentation paragraph to list all five frames by name: +`calibration`, `setCalibration`, `configSaved`, `reloadConfig`, `configReloaded`. + +--- + +## Verification output + +### `node --check` + `node --test` + +``` +(node --check static/app.js && node --check static/calibration.js) → SYNTAX OK + +TAP version 13 +ok 1 - baseSignalName strips an element suffix +ok 2 - calKey is stable and separates the two fields +ok 3 - normaliseCal accepts a valid entry and fills defaults +ok 4 - normaliseCal strips an element suffix from the signal name +ok 5 - normaliseCal truncates an over-long unit +ok 6 - normaliseCal leaves short non-ASCII units untouched +ok 7 - normaliseCal truncates an over-long ASCII unit to exactly 16 bytes +ok 8 - normaliseCal cuts a mid-rune byte boundary back to the last complete rune +ok 9 - normaliseCal leaves a unit that is exactly 16 bytes ending on a complete multi-byte rune untouched +ok 10 - normaliseCal rejects invalid entries +ok 11 - applyCal and invertCal round-trip +ok 12 - applyCal passes non-finite samples through untouched +ok 13 - calRange re-orders when the scale is negative +ok 14 - CalTable.get returns IDENTITY for an unknown signal +ok 15 - CalTable.get resolves an element name to its base signal +ok 16 - CalTable.set stores, overwrites, and deletes identity entries +ok 17 - CalTable.replaceAll drops the previous contents +ok 18 - CalTable.list is sorted by source then signal +1..18 +# tests 18 +# pass 18 +# fail 0 +``` + +Note: 18 tests (unchanged from before) because the new assertions were added +inside two existing test functions, not as separate test cases. + +### Go wshub + +``` +cd Common/Client/go && go build ./... && go vet ./... && go test ./wshub/ +ok marte2/common/wshub 1.288s +``` + +### Go configcheck + +``` +cd Test/E2E/suite/client/configcheck && go build ./... && go vet ./... +(silent — CONFIGCHECK OK) +``` diff --git a/Client/udpstreamer/static/app.js b/Client/udpstreamer/static/app.js index 10afc84..a80ca33 100644 --- a/Client/udpstreamer/static/app.js +++ b/Client/udpstreamer/static/app.js @@ -2552,7 +2552,7 @@ document.getElementById('trig-signal').addEventListener('change', e => { const n = meta ? numElements(meta) : 1; if (meta && !isTemporal(meta) && n > 1) { showArrayIdxPicker(val, n, idx => { - trig.signal = val + '[' + idx + ']'; sendTrigConfig(); + trig.signal = val + '[' + idx + ']'; refreshTrigThresholdField(); sendTrigConfig(); if (trig.enabled) trigArm(); }, () => { // Cancelled: revert selection to current trig.signal base or empty. @@ -2562,7 +2562,7 @@ document.getElementById('trig-signal').addEventListener('change', e => { }); return; } - trig.signal = val; sendTrigConfig(); + trig.signal = val; refreshTrigThresholdField(); sendTrigConfig(); if (trig.enabled && trig.signal) trigArm(); else if (!trig.signal) trigDisarm(); }); document.getElementById('trig-edge').addEventListener('change', e => { trig.edge = e.target.value; sendTrigConfig(); }); @@ -3467,7 +3467,7 @@ function applyCalibrationChanged() { persistCalibration(); buildSidebar(); // unit badges plots.forEach(p => { p.needsRedraw = true; }); - if (typeof refreshVScaleMenu === 'function') refreshVScaleMenu(); // Task 8 + refreshVScaleMenu(); // The threshold is held in calibrated units, so a calibration change alters // the raw value the hub must compare against — resend it. if (trig.signal) { refreshTrigThresholdField(); sendTrigConfig(); } diff --git a/Client/udpstreamer/static/calibration.js b/Client/udpstreamer/static/calibration.js index 1c3c557..ce61f93 100644 --- a/Client/udpstreamer/static/calibration.js +++ b/Client/udpstreamer/static/calibration.js @@ -17,7 +17,7 @@ function baseSignalName(name) { var s = String(name == null ? '' : name); var open = s.lastIndexOf('['); - if (open > 0 && s.charAt(s.length - 1) === ']') { + if (open >= 0 && s.charAt(s.length - 1) === ']') { var idx = s.slice(open + 1, s.length - 1); if (idx.length > 0 && /^[0-9]+$/.test(idx)) return s.slice(0, open); } diff --git a/Client/udpstreamer/static/index.html b/Client/udpstreamer/static/index.html index 2d7ec4c..9ea64b9 100644 --- a/Client/udpstreamer/static/index.html +++ b/Client/udpstreamer/static/index.html @@ -220,7 +220,7 @@ - + diff --git a/Client/udpstreamer/test/calibration.test.js b/Client/udpstreamer/test/calibration.test.js index 9a108d7..8b4577f 100644 --- a/Client/udpstreamer/test/calibration.test.js +++ b/Client/udpstreamer/test/calibration.test.js @@ -8,6 +8,9 @@ test('baseSignalName strips an element suffix', () => { assert.strictEqual(C.baseSignalName('Adc[12]'), 'Adc'); assert.strictEqual(C.baseSignalName('A[1]B'), 'A[1]B'); assert.strictEqual(C.baseSignalName(''), ''); + // A name that is entirely the suffix "[0]" must reduce to the empty string, + // matching Go (arrayIndexSuffix regexp) and C++ (strchr truncation) behaviour. + assert.strictEqual(C.baseSignalName('[0]'), ''); }); test('calKey is stable and separates the two fields', () => { @@ -85,6 +88,9 @@ test('normaliseCal rejects invalid entries', () => { assert.strictEqual(C.normaliseCal({source: 'w', signal: 's', scale: Infinity}), null); assert.strictEqual(C.normaliseCal({source: 'w', signal: 's', offset: NaN}), null); assert.strictEqual(C.normaliseCal({source: 'w', signal: 's', scale: '2'}), null); + // A signal name that is entirely an array-index suffix reduces to the empty + // string after stripping, so the entry must be rejected — matching Go and C++. + assert.strictEqual(C.normaliseCal({source: 'w', signal: '[0]'}), null); }); test('applyCal and invertCal round-trip', () => { diff --git a/Common/Client/go/wshub/calibration.go b/Common/Client/go/wshub/calibration.go index 08a1683..b264cec 100644 --- a/Common/Client/go/wshub/calibration.go +++ b/Common/Client/go/wshub/calibration.go @@ -13,7 +13,14 @@ import ( // arrayIndexSuffix matches a trailing "[digits]" at the very end of a signal // name, used to strip array-element suffixes so one entry covers the whole -// array. Mirrors the C++ `strchr(signal,'[')` truncation and the JS equivalent. +// array. The regexp is anchored to the end of the string and requires digits, +// so it only removes a well-formed trailing element index: "Adc[3]" → "Adc", +// "[0]" → "", "A[1]B" → "A[1]B" (no match). +// +// Known difference vs C++: the C++ hub uses strchr(signal,'[') which finds the +// FIRST '[' anywhere in the name, so C++ reduces "A[1]B" to "A" while this +// regexp leaves it unchanged. Both implementations agree on the common cases +// ("Name[i]" and "[i]" alone) that arise from real UDPS signal names. var arrayIndexSuffix = regexp.MustCompile(`\[\d+\]$`) // maxUnitLen bounds the calibration unit override. Mirrored by kMaxUnitLen in diff --git a/Common/Client/go/wshub/hub_calibration_test.go b/Common/Client/go/wshub/hub_calibration_test.go index d9ccf4f..7bf52eb 100644 --- a/Common/Client/go/wshub/hub_calibration_test.go +++ b/Common/Client/go/wshub/hub_calibration_test.go @@ -129,8 +129,4 @@ func waitMsg(t *testing.T, sendCh chan wsMessage, msgType string) []byte { } } -// waitBroadcast is kept for compatibility with the test helper interface; -// it delegates to waitMsg using a pre-registered client send channel. -// Callers that need it should register a client and use waitMsg directly. - func sleepMillis(n int) { time.Sleep(time.Duration(n) * time.Millisecond) } diff --git a/Docs/StreamHub-API.md b/Docs/StreamHub-API.md index 330f5ef..658cb8c 100644 --- a/Docs/StreamHub-API.md +++ b/Docs/StreamHub-API.md @@ -392,3 +392,5 @@ written by either hub loads in the other. | UDPS source sessions | 32 | | Max received WS payload | 64 KiB | | Max sent WS payload | 4 MiB | +| Calibration entries | 256 (C++ hub, `kMaxCalibration`); unbounded (Go hub) | +| Calibration unit override | 16 UTF-8 bytes | diff --git a/Test/E2E/suite/client/configcheck/main.go b/Test/E2E/suite/client/configcheck/main.go index 08bd502..dde4653 100644 --- a/Test/E2E/suite/client/configcheck/main.go +++ b/Test/E2E/suite/client/configcheck/main.go @@ -150,40 +150,6 @@ func (c *conn) nextSkipping(want string, also []string) (frame, error) { } } -// nextOrOptional reads from the background reader, skipping ambient frames, -// and returns (frame, frametype) where frametype is the type of the first -// protocol frame that arrives, regardless of whether it matches want. -// If the optional type arrives instead, that is returned too. -// This is used for the reload sequence where C++ may emit an extra "sources" -// frame after "calibration". -func (c *conn) nextOneOf(want, optional string) (frame, string, error) { - deadline := time.NewTimer(c.timeout) - defer deadline.Stop() - for { - select { - case <-deadline.C: - return frame{}, "", fmt.Errorf("timeout waiting for %q (or %q)", want, optional) - case r, ok := <-c.readCh: - if !ok { - return frame{}, "", fmt.Errorf("reader closed while waiting for %q", want) - } - if r.err != nil { - return frame{}, "", fmt.Errorf("read while waiting for %q: %w", want, r.err) - } - if r.f.Type == want || r.f.Type == optional { - return r.f, r.f.Type, nil - } - // Any other protocol frame is unexpected. - if protocolFrameTypes[r.f.Type] { - return frame{}, "", fmt.Errorf( - "unexpected protocol frame %q while waiting for %q or %q", - r.f.Type, want, optional) - } - fmt.Printf("[skip ambient %q]\n", r.f.Type) - } - } -} - // nextWithin reads from the background reader until a frame with the wanted // type arrives within d, returning (frame, true) or (frame{}, false). Unlike // next() it does NOT return an error on timeout, making it suitable for the diff --git a/docs/superpowers/specs/2026-08-16-udpstreamer-signal-calibration-and-config-design.md b/docs/superpowers/specs/2026-08-16-udpstreamer-signal-calibration-and-config-design.md index 42b56ac..e129409 100644 --- a/docs/superpowers/specs/2026-08-16-udpstreamer-signal-calibration-and-config-design.md +++ b/docs/superpowers/specs/2026-08-16-udpstreamer-signal-calibration-and-config-design.md @@ -44,7 +44,7 @@ A calibration entry is keyed by `(source label, signal base name)`: | `signal` | string | — | base signal name, no `[i]` suffix | | `scale` | float64 | `1` | finite, non-zero | | `offset` | float64 | `0` | finite | -| `unit` | string | `""` | trimmed, max 16 chars; empty means "use the streamer's unit" | +| `unit` | string | `""` | trimmed, max 16 UTF-8 bytes; empty means "use the streamer's unit" | The key uses the source **label**, not the runtime id (`s1`, `s2`). Ids are assigned in add-order at startup, so a saved calibration keyed by id would rebind @@ -90,7 +90,7 @@ New frames, implemented identically in both hubs: | client → hub | `{"type":"setCalibration","source","signal","scale","offset","unit"}` | | hub → client | `{"type":"configSaved","ok":bool,"path":string,"error":string}` | | client → hub | `{"type":"reloadConfig"}` | -| hub → client | `{"type":"configReloaded","ok":bool,"error":string}` | +| hub → client | `{"type":"configReloaded","ok":bool,"path":string,"error":string}` | `calibration` is broadcast when a client connects and after every accepted `setCalibration` or successful `reloadConfig`. It is a separate message rather @@ -128,9 +128,10 @@ sibling `CalConfig` type; `Save` writes both slices into one array; `Load` decodes into `[]map[string]json.RawMessage` and discriminates per element. **C++ (`Source/Applications/StreamHub/StreamHub.{h,cpp}`).** A fixed -`kMaxCalibration = 256` array of -`{StreamString source, signal, unit; float64 scale, offset;}` — no STL, per the -`Source/Components` and StreamHub style rules. `HandleSetCalibration`, +`kMaxCalibration = 256` array of `CalibrationEntry` structs with fixed +`char[]` fields (`source[128]`, `signal[128]`, `unit[17]`) and `float64` scale +and offset — no STL and no per-entry heap allocation, per the `Source/Components` +and StreamHub style rules. `HandleSetCalibration`, `HandleReloadConfig` and `BroadcastCalibration` mirror the existing `HandleAddSource` / `BroadcastSources` shape, with `BroadcastCalibration` using its own 16 KiB buffer like `BroadcastConfig`. `LoadSourcesFile` gains the @@ -203,10 +204,12 @@ new-format file, unrecognised block, malformed entry), `setCalibration` validation including `scale = 0` and non-finite values, and a save→load round-trip asserting sources and calibration both survive. -**C++.** Extend an E2E `chain` scenario's config file with a calibration entry and -assert the hub re-serialises it unchanged after a `saveSources`, which exercises -the discriminator branch in `LoadSourcesFile` and the writer in -`HandleSaveSources`. +**C++.** A standalone Go program at `Test/E2E/suite/client/configcheck/` connects +to either hub (Go or C++) via WebSocket and asserts identical calibration +behaviour: it exercises `setCalibration`, `saveSources → configSaved`, +`reloadConfig → configReloaded`, and verifies that the saved config file and all +broadcast frames are sorted and complete. The program exits non-zero on any +deviation from the protocol, making it runnable against both hubs in CI. **Browser.** `node --check static/app.js`, plus a manual pass: set a scale and offset on a live signal and confirm the plot, hover readout, cursor readout, CSV @@ -216,6 +219,7 @@ the live source keeps streaming. ## Documentation -`Docs/StreamHub-API.md` gains the four new frames; `Docs/WebUI.md` gains the +`Docs/StreamHub-API.md` gains the five new frames (`calibration`, `setCalibration`, +`configSaved`, `reloadConfig`, `configReloaded`); `Docs/WebUI.md` gains the calibration row and the Sources & Config section; `ARCHITECTURE.md` §6 gains the config file format.