fix: address all 9 findings from final calibration code review
- calibration.js: fix baseSignalName('[0]') parity with Go/C++ (>= 0 not > 0)
- calibration.test.js: add assertions for '[0]' edge case in two existing tests
- app.js: remove stale typeof guard around refreshVScaleMenu (always defined)
- app.js: call refreshTrigThresholdField on trig-signal change (both assignment sites)
- index.html: drop maxlength='16' on unit input; normaliseCal is the sole enforcer
- configcheck/main.go: delete dead nextOneOf function (no callers)
- hub_calibration_test.go: delete orphaned waitBroadcast comment (function never existed)
- calibration.go: correct arrayIndexSuffix comment to document known Go/C++ difference
- Docs/StreamHub-API.md: add calibration entry count and unit byte limits to §5 table
- spec: fix configReloaded missing path field, '16 chars'→'16 UTF-8 bytes', StreamString→char[], chain scenario→configcheck program, four→five new frames
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Martino Ferrari
co-authored by
Claude Sonnet 4.6
Claude Sonnet 4.6
parent
42f5a726af
commit
1f8592f854
@@ -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 `<input id="vscale-cal-unit">`.
|
||||
|
||||
`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)
|
||||
```
|
||||
@@ -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(); }
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -220,7 +220,7 @@
|
||||
<label class="vstb-lbl">Offset</label>
|
||||
<input type="number" id="vscale-cal-offset" class="ctx-num ctx-num-sm" step="any" value="0">
|
||||
<label class="vstb-lbl">Unit</label>
|
||||
<input type="text" id="vscale-cal-unit" class="ctx-num ctx-num-xs" maxlength="16" placeholder="—">
|
||||
<input type="text" id="vscale-cal-unit" class="ctx-num ctx-num-xs" placeholder="—">
|
||||
<button class="ctx-btn" id="btn-cal-reset" title="Clear this signal's calibration">Reset</button>
|
||||
</div>
|
||||
<button id="btn-vscale-close" class="vstb-close" title="Close">✕</button>
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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) }
|
||||
|
||||
@@ -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 |
|
||||
|
||||
@@ -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
|
||||
|
||||
+14
-10
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user