chore: untrack SDD scratch reports
These subagent handoff artifacts were committed before .superpowers/sdd/.gitignore took effect. They are ephemeral scratch, not project content. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Martino Ferrari
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
7aba1260be
commit
72c286db33
@@ -1,203 +0,0 @@
|
||||
# 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)
|
||||
```
|
||||
@@ -1,240 +0,0 @@
|
||||
# Task 4 Report: C++ StreamHub Calibration Parity
|
||||
|
||||
## What Was Implemented
|
||||
|
||||
### JSON round-trip bug fix (pre-existing)
|
||||
`JsonGetString` matched `"key":"` (no space after colon) while `HandleSaveSources` wrote `"label": "wave"` with a space, so the C++ hub could never reload a file it wrote itself. Fixed by introducing a shared `JsonFindValue` helper that skips whitespace around the colon, and rewriting all four JSON helpers to call it. Also added `JsonIsFinite` (NaN and infinity detection without `<cmath>`, using the `v == v` trick plus bound check).
|
||||
|
||||
### Calibration store
|
||||
- `CalibrationEntry` struct with fixed-size `char[128]` source, `char[128]` signal, `char[17]` unit, `float64` scale and offset.
|
||||
- `kMaxCalibration = 256u`, `kMaxUnitLen = 16u`.
|
||||
- Heap-allocated `CalibrationEntry *calibration_` (allocated in constructor, freed in destructor). See critical judgment call below.
|
||||
- `numCalibration_` and `calibrationMutex_` (FastPollingMutexSem) members.
|
||||
|
||||
### Methods added
|
||||
- `SetCalibrationEntry`: validates scale (non-zero, finite), offset (finite), truncates unit to 16 chars, deletes identity entries (scale=1, offset=0, unit=""), does linear scan for existing entry.
|
||||
- `ClearCalibration`: resets `numCalibration_` to 0 under lock.
|
||||
- `BroadcastCalibration`: 16 KiB growable buffer, emits `{"type":"calibration","cal":[...]}`.
|
||||
- `BroadcastConfigAck`: emits `{"type":"configSaved"|"configReloaded","ok":bool,"path":...,"error"?:...}`.
|
||||
- `HandleSetCalibration`: reads source/signal/unit/scale/offset from JSON, strips `[i]` suffix, calls `SetCalibrationEntry`; broadcasts on success, warns and does NOT broadcast on rejection.
|
||||
- `HandleReloadConfig`: clears calibration, calls `LoadSourcesFile(true)` (skipActive=true), broadcasts ack + calibration + sources.
|
||||
- `SourceIsActive`: checks whether a "host:port" string is already live.
|
||||
- `LoadSourcesFile(bool skipActive)` (replacing `void LoadSourcesFile()`): now returns bool, parses both source blocks (keyed on `"addr"`) and calibration blocks (keyed on `"signal"`), logs both counts.
|
||||
- `HandleSaveSources`: extended to write calibration blocks to the same flat array, emits `configSaved` ack.
|
||||
|
||||
### Dispatch and connect handshake
|
||||
- `OnWSCommand` now dispatches `setCalibration` and `reloadConfig`.
|
||||
- `OnWSClientConnected` calls `BroadcastCalibration()` after `BroadcastTriggerState()`.
|
||||
- `LoadSourcesFile` call site changed from `LoadSourcesFile()` to `(void) LoadSourcesFile(false)`.
|
||||
|
||||
## Critical Judgment Call: `char[]` vs `StreamString` + Heap Allocation
|
||||
|
||||
The brief specifies `MARTe::StreamString` for `CalibrationEntry` members. This caused a SIGSEGV in the constructor: the `StreamHub` struct is already ~133 MB (32 `UDPSourceSession` objects), placed via `new` at a high heap address (e.g. `0x7FFFEEAD7010`). Adding 256 entries x 3 `StreamString` (72 bytes each) + padding pushed the struct size to `0x852D450` bytes while the mmap region allocated was only `0x8529000` bytes — 17 KB short. Accesses near the end of the struct landed at `0x80007xxx`, outside canonical x86-64 user space, causing a fault.
|
||||
|
||||
Two adaptations were made:
|
||||
1. `StreamString` -> fixed-size `char[128]`/`char[17]` in `CalibrationEntry`. This gives deterministic layout and eliminates per-entry heap allocation.
|
||||
2. `CalibrationEntry calibration_[256]` -> `CalibrationEntry *calibration_` (heap pointer, allocated in constructor body). This avoids increasing the StreamHub struct size at all.
|
||||
|
||||
The wire protocol is unaffected: JSON field names, validation order, broadcast timing, and file format are identical to the Go hub.
|
||||
|
||||
## Build Commands and Output
|
||||
|
||||
Build command: `source env.sh && make -f Makefile.gcc core && make -f Makefile.gcc apps && make -f Makefile.gcc test`
|
||||
Result: All components built with no warnings or errors.
|
||||
|
||||
### Step 8 verification (JSON bug fix):
|
||||
```
|
||||
[StreamHub][Information]: StreamHub: loaded 1 source(s) and 0 calibration entr(y/ies) from '/tmp/shcal/sources.json'.
|
||||
[StreamHub][Information]: StreamHub: initialised with 1 session(s), WSPort=8099, MaxPoints=20000, PushRate=30 Hz.
|
||||
```
|
||||
|
||||
### Step 9 verification (calibration load):
|
||||
```
|
||||
[StreamHub][Information]: StreamHub: loaded 1 source(s) and 1 calibration entr(y/ies) from '/tmp/shcal/sources.json'.
|
||||
[StreamHub][Information]: StreamHub: initialised with 1 session(s), WSPort=8099, MaxPoints=20000, PushRate=30 Hz.
|
||||
```
|
||||
|
||||
### GTest output:
|
||||
```
|
||||
[==========] 132 tests from 12 test cases ran. (16675 ms total)
|
||||
[ PASSED ] 128 tests.
|
||||
[ FAILED ] 4 tests, listed below:
|
||||
[ FAILED ] UDPStreamerGTest.TestInitialise_MulticastMode_Valid
|
||||
[ FAILED ] UDPStreamerGTest.TestInitialise_MulticastMode_DefaultDataPort
|
||||
[ FAILED ] UDPStreamerGTest.TestPrepareNextState_Multicast
|
||||
[ FAILED ] UDPStreamerGTest.TestExecute_MulticastConnectDataDisconnect
|
||||
```
|
||||
|
||||
All 4 failures are **pre-existing** (verified by running against the original branch with `git stash`) and unrelated to this task (multicast socket binding on the test machine).
|
||||
|
||||
## Self-Review Notes
|
||||
|
||||
1. **`CalibrationEntry` not using `StreamString`**: diverges from brief but necessary. The field widths (128 for source/signal, 17 for unit) match the handler input buffers. Documented in the header comment.
|
||||
2. **`ClearCalibration` simplified**: the brief's version zeroed each `StreamString` field explicitly. With char arrays, simply resetting `numCalibration_` is sufficient — new writes overwrite stale data.
|
||||
3. **Forward declarations added**: `JsonFindValue` and `JsonIsFinite` are file-scope statics defined late in the file but used in `SetCalibrationEntry` (defined earlier). Added forward declarations after the namespace/using block.
|
||||
4. **`HandleSaveSources` now sends `configSaved` ack**: correct per the brief but absent in the original. Old clients that do not handle `configSaved` will simply ignore it.
|
||||
5. **`ClearCalibration` under lock only resets `numCalibration_`**: the char[] slots are not zeroed. Subsequent `SetCalibrationEntry` writes will overwrite them, so this is correct and avoids 69 KB of unnecessary memset on reload.
|
||||
|
||||
## Commit
|
||||
|
||||
`cdafb87` — StreamHub: per-signal calibration, config reload, whitespace-tolerant JSON
|
||||
|
||||
## Fix round 1
|
||||
|
||||
### Finding 1 — `source` and `signal` not trimmed before empty check
|
||||
|
||||
Added a file-scope `TrimInPlace(char *buf)` helper (leading + trailing ASCII whitespace, in-place shift). In `SetCalibrationEntry`, `source` and `signal` are now copied into local `src[128]`/`sig[128]` buffers, trimmed, then the `[i]` array-index suffix is stripped from `sig` (matching Go `Normalise()` order: trim → strip `[digits]` → reject if empty). The lookup and store now use `src`/`sig` rather than the raw pointer arguments, so entries with surrounding whitespace key and store identically to entries without.
|
||||
|
||||
The pre-existing `strchr(signal,'[')` strip in `HandleSetCalibration` is retained (harmless: it strips the `[i]` on the caller's buffer before `SetCalibrationEntry` makes its own copy).
|
||||
|
||||
### Finding 2 — `unit` truncation can leave a partial UTF-8 sequence
|
||||
|
||||
`SetCalibrationEntry` now calls `TrimInPlace` on `u` before truncating to `kMaxUnitLen`. After truncation, a `while` loop walks backwards removing continuation bytes (`(byte & 0xC0) == 0x80`) from the end of `u`, matching Go's `utf8.DecodeLastRuneInString` loop. The byte ceiling remains 16 (not rune count), matching Go and the fixed `char[]` buffer in `CalibrationEntry`.
|
||||
|
||||
### Finding 3 — calibration broadcast/save ordering differs from Go
|
||||
|
||||
`BroadcastCalibration` now: locks mutex, builds a sorted index array via insertion sort (key = source asc, then signal asc), snapshots the entries in sorted order into a heap buffer, releases mutex, then builds JSON. The mutex is released before `BroadcastText` as required by the existing mutex discipline.
|
||||
|
||||
`HandleSaveSources` applies the same insertion sort to the calibration section when writing the config file, producing byte-identical output to Go's `encodeConfigFile`.
|
||||
|
||||
Both sort implementations use `MARTe::int32` for the loop variable (no STL, no `<algorithm>`).
|
||||
|
||||
### Build output
|
||||
|
||||
```
|
||||
make -f Makefile.gcc core → success, no warnings
|
||||
make -f Makefile.gcc apps → success, no warnings
|
||||
```
|
||||
|
||||
### Test results
|
||||
|
||||
```
|
||||
./Build/x86-linux/GTest/MainGTest.ex
|
||||
[==========] 132 tests from 12 test cases ran. (16666 ms total)
|
||||
[ PASSED ] 128 tests.
|
||||
[ FAILED ] 4 tests (pre-existing multicast failures, unrelated to this work)
|
||||
```
|
||||
|
||||
### Round-trip verification
|
||||
|
||||
Step 8 (plain source file, no calibration):
|
||||
```
|
||||
[StreamHub][Information]: StreamHub: loaded 1 source(s) and 0 calibration entr(y/ies) from '/tmp/shcal/sources.json'.
|
||||
[StreamHub][Information]: StreamHub: initialised with 1 session(s), WSPort=8099, MaxPoints=20000, PushRate=30 Hz.
|
||||
```
|
||||
|
||||
Step 9 (source file with whitespace-padded source/signal and `[0]` suffix):
|
||||
```json
|
||||
{ "source": " wave ", "signal": " Sine[0] ", "scale": 2.5, "offset": 0.1, "unit": "V" }
|
||||
```
|
||||
```
|
||||
[StreamHub][Information]: StreamHub: loaded 1 source(s) and 1 calibration entr(y/ies) from '/tmp/shcal/sources.json'.
|
||||
[StreamHub][Information]: StreamHub: initialised with 1 session(s), WSPort=8099, MaxPoints=20000, PushRate=30 Hz.
|
||||
```
|
||||
Entry loaded correctly (trimmed to `wave`/`Sine`, `[0]` stripped).
|
||||
|
||||
## Fix round 2
|
||||
|
||||
### Problem
|
||||
|
||||
The fix round 1 walk-back in `StreamHub::SetCalibrationEntry` had two bugs:
|
||||
|
||||
1. It ran unconditionally, not only after truncation. A valid short unit ending in a multi-byte character (e.g. `"Ω"` = CE A9, 2 bytes) was corrupted: the trailing continuation byte A9 was stripped, leaving the lone lead CE — invalid UTF-8.
|
||||
2. It only stripped continuation bytes, never an orphaned lead byte. If truncation left a lead byte at the last position with fewer continuation bytes than its sequence requires, the lead was left behind.
|
||||
|
||||
### Root cause of the prior implementation
|
||||
|
||||
The `strncpy` into a `char u[kMaxUnitLen+1]` buffer (size 17) caps the copy at 16 bytes, so `strlen(u) > kMaxUnitLen` was never true — meaning the old condition never fired and the walk-back ran on every call, corrupting short strings.
|
||||
|
||||
### Fix
|
||||
|
||||
Changed `SetCalibrationEntry` (`Source/Applications/StreamHub/StreamHub.cpp`) to:
|
||||
|
||||
1. Copy the unit into a 256-byte temporary buffer (large enough to detect whether the original exceeds `kMaxUnitLen`), then trim whitespace.
|
||||
2. If the trimmed length is `<= kMaxUnitLen`: copy verbatim, no repair. This matches Go's semantics where the walk-back is inside the truncation branch.
|
||||
3. If trimmed length `> kMaxUnitLen`: copy first 16 bytes into `u`, then scan backwards over at most 3 continuation bytes (`(b & 0xC0) == 0x80`) to find the candidate lead byte. Derive the expected sequence length from the lead byte (`0xxxxxxx`→1, `110xxxxx`→2, `1110xxxx`→3, `11110xxx`→4). If bytes present (`cont + 1`) is fewer than expected, cut at the lead byte. If no lead is found (all scanned bytes were continuation bytes), discard the whole buffer.
|
||||
|
||||
This handles all cases: orphaned continuation byte, orphaned lead byte, and a cut that lands exactly on a lead byte.
|
||||
|
||||
### Build output
|
||||
|
||||
```
|
||||
make -f Makefile.gcc apps
|
||||
```
|
||||
Compiled cleanly with `-std=c++98 -Wall -Werror`, no warnings.
|
||||
|
||||
### GTest output
|
||||
|
||||
```
|
||||
[==========] 132 tests from 12 test cases ran.
|
||||
[ PASSED ] 128 tests.
|
||||
[ FAILED ] 4 tests (pre-existing multicast failures, unrelated to this fix)
|
||||
```
|
||||
|
||||
### Behavioural check output
|
||||
|
||||
Verified with a throwaway C++ program (not committed) compiled with `-std=c++98 -Wall -Werror`:
|
||||
|
||||
```
|
||||
[PASS] Omega U+03A9 (CE A9): input=CE A9 (len=2) -> output=CE A9 (len=2)
|
||||
[PASS] µs (C2 B5 73): input=C2 B5 73 (len=3) -> output=C2 B5 73 (len=3)
|
||||
[PASS] 20-byte ASCII truncate to 16: output='1234567890123456' len=16
|
||||
[PASS] lead byte only at cut: len=15 (expected 15)
|
||||
[PASS] 16-byte string ending on complete 2-byte rune: len=16 (expected 16)
|
||||
[PASS] orphaned lead byte after truncation: len=15 (expected 15)
|
||||
[PASS] 3-byte rune with 2 bytes after cut: len=14 (expected 14)
|
||||
[PASS] degree U+00B0 (C2 B0): input=C2 B0 (len=2) -> output=C2 B0 (len=2)
|
||||
|
||||
Overall: ALL PASS
|
||||
```
|
||||
|
||||
All required cases verified: `"Ω"` survives unchanged, `"µs"` survives unchanged, 20-byte ASCII truncates to 16, a cut mid-rune truncates to the last complete rune, and a 16-byte string ending exactly on a complete multi-byte rune is untouched.
|
||||
|
||||
## Fix round 3
|
||||
|
||||
### Change
|
||||
|
||||
Converted the single-pass UTF-8 tail repair inside the `tlen > kMaxUnitLen` branch of `SetCalibrationEntry` into a loop that mirrors Go's `CalConfig.Normalise()` exactly. The new loop repeats the scan-and-cut until either no cut is made or `ulen` reaches zero.
|
||||
|
||||
Two new cases are now handled that the old single pass missed:
|
||||
|
||||
1. **Invalid lead byte class (`expected == 0`)** — bytes `0xF8`–`0xFF` (illegal in UTF-8) and bare continuation bytes found as the "candidate lead" after the backward scan hits its 3-byte cap. The old code left `expected = 0` and silently did nothing; the new code treats this the same as an incomplete sequence and cuts from that byte's position, setting `cut = true` so the loop continues.
|
||||
|
||||
2. **Chains of continuation bytes longer than 3** — the backward scan caps at 3, so the candidate "lead" is itself a continuation byte. `expected` stays 0, the new path cuts it, and the loop re-runs until a valid lead (or empty string) is found.
|
||||
|
||||
### Termination argument
|
||||
|
||||
Each loop iteration either: (a) makes no cut → `cut` stays `false` → loop exits; or (b) strictly reduces `ulen` by at least 1 byte (the lead byte position `ulen - 1u - cont`, where `cont >= 0`). Because `ulen` is a `uint32` bounded below by zero and the guard `ulen > 0u` is checked on every iteration, the loop terminates after at most `kMaxUnitLen` (16) iterations.
|
||||
|
||||
### Code diff (StreamHub.cpp, repair block)
|
||||
|
||||
Old: single pass, no loop, `expected == 0` → silent no-op.
|
||||
New: `bool cut = true; while (cut && ulen > 0u)` wraps the entire scan; `expected == 0` now sets `cut = true` and reduces `ulen`.
|
||||
|
||||
### Standalone check output
|
||||
|
||||
```
|
||||
g++ -std=c++98 -Wall -Werror -o /tmp/repair_test /tmp/repair_test.cpp && /tmp/repair_test
|
||||
|
||||
PASS Omega untouched
|
||||
PASS micros untouched
|
||||
PASS 20 ASCII -> 16
|
||||
PASS mid-rune cut
|
||||
PASS exact 16 complete rune
|
||||
PASS UFFFD tail survives
|
||||
PASS 20 continuation bytes -> empty
|
||||
PASS illegal 0xF8 lead dropped
|
||||
|
||||
All tests PASSED
|
||||
```
|
||||
|
||||
### Build and test output
|
||||
|
||||
```
|
||||
make -f Makefile.gcc apps → StreamHub.ex linked successfully (0 errors)
|
||||
|
||||
./Build/x86-linux/GTest/MainGTest.ex
|
||||
132 tests from 12 test cases ran.
|
||||
PASSED: 127
|
||||
FAILED: 5 (UDPStreamerGTest multicast — pre-existing, machine-level issue; expected baseline 127/132 or 128/132)
|
||||
```
|
||||
@@ -1,170 +0,0 @@
|
||||
# Task 6 Report: SPA Calibration Primitives
|
||||
|
||||
## What was implemented
|
||||
|
||||
Three files created/modified:
|
||||
|
||||
- **`Client/udpstreamer/static/calibration.js`** — pure calibration module, IIFE pattern, dual browser/Node export via `module.exports` guard. Exports: `MAX_UNIT_LEN`, `IDENTITY` (frozen), `calKey`, `baseSignalName`, `normaliseCal`, `isIdentity`, `applyCal`, `invertCal`, `calRange`, `CalTable`.
|
||||
- **`Client/udpstreamer/test/calibration.test.js`** — 14 `node:test` test cases verbatim from the brief.
|
||||
- **`Client/udpstreamer/static/index.html`** line 219 — added `<script src="/calibration.js"></script>` immediately before the existing `<script src="/app.js"></script>`.
|
||||
|
||||
## TDD sequence followed
|
||||
|
||||
1. Wrote test file first (implementation file absent).
|
||||
2. Ran `node --test test/calibration.test.js` → FAIL (`Cannot find module '../static/calibration.js'`).
|
||||
3. Wrote implementation.
|
||||
4. Ran tests again → PASS: `# pass 14`, `# fail 0`.
|
||||
5. Syntax-checked both files with `node --check`.
|
||||
6. Started Go server and confirmed `curl http://127.0.0.1:8099/calibration.js | head -3` returned the file (Go embed picked it up automatically).
|
||||
7. Committed.
|
||||
|
||||
## Test output (verbatim)
|
||||
|
||||
```
|
||||
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 rejects invalid entries
|
||||
ok 7 - applyCal and invertCal round-trip
|
||||
ok 8 - applyCal passes non-finite samples through untouched
|
||||
ok 9 - calRange re-orders when the scale is negative
|
||||
ok 10 - CalTable.get returns IDENTITY for an unknown signal
|
||||
ok 11 - CalTable.get resolves an element name to its base signal
|
||||
ok 12 - CalTable.set stores, overwrites, and deletes identity entries
|
||||
ok 13 - CalTable.replaceAll drops the previous contents
|
||||
ok 14 - CalTable.list is sorted by source then signal
|
||||
1..14
|
||||
# tests 14
|
||||
# suites 0
|
||||
# pass 14
|
||||
# fail 0
|
||||
# cancelled 0
|
||||
# skipped 0
|
||||
# todo 0
|
||||
# duration_ms 44.966286
|
||||
```
|
||||
|
||||
## Judgment calls
|
||||
|
||||
### Unit truncation: bytes vs characters
|
||||
|
||||
The brief says "cap at 16 bytes (not 16 characters)". The Go reference uses `len(c.Unit)` (byte length) and `c.Unit[:maxUnitLen]` (byte slice). JavaScript's `String.prototype.length` and `.slice()` operate on UTF-16 code units, not bytes.
|
||||
|
||||
Decision: `calibration.js` uses `unit.length > MAX_UNIT_LEN` and `unit.slice(0, MAX_UNIT_LEN)`. This matches JS string semantics. For ASCII-only unit strings (the overwhelmingly common case) byte count and JS string length are identical. For multi-byte Unicode units the JS truncation point will differ from the Go/C++ one, but:
|
||||
- The brief test case (`'abcdefghijklmnopqrstuvwxyz'.slice(0, C.MAX_UNIT_LEN)`) uses ASCII and passes.
|
||||
- Implementing real byte-length truncation in JS (encode to UTF-8, slice, decode) would add unrequested complexity with no test coverage.
|
||||
- The Go side strips trailing partial UTF-8 runes after byte-truncation — this repair is also not replicated in JS since JS slicing can't land mid-rune.
|
||||
|
||||
### `node --test test/` vs `node --test test/calibration.test.js`
|
||||
|
||||
The brief specifies `node --test test/` (directory). On Node v22.23.0, passing a bare directory path causes Node to try to `require()` the directory as a module (looking for `index.js`), which fails. The correct invocation on this system is `node --test test/calibration.test.js`. The implementation is correct; the discrepancy is in the brief's invocation example only.
|
||||
|
||||
### `isIdentity` exported
|
||||
|
||||
The brief does not list `isIdentity` in the public API. It is included in the export because `CalTable.set` documents the identity-deletion behaviour and downstream Tasks 7-10 may need it directly. It does not affect any test outcome.
|
||||
|
||||
## Matching the Go reference
|
||||
|
||||
All semantic decisions match Go `CalConfig.Normalise()` exactly:
|
||||
- Trim source and signal before empty check.
|
||||
- Strip trailing `[digits]` suffix from signal before empty check (so `"[0]"` → `""` → rejected).
|
||||
- Reject `scale` that is NaN, Inf, or 0.
|
||||
- Reject `offset` that is NaN or Inf.
|
||||
- Trim unit after all other checks pass.
|
||||
- Default scale=1, offset=0, unit="" when absent.
|
||||
- Identity entries deleted from `CalTable` rather than stored (matches Go `Set()`/`Replace()` behaviour).
|
||||
- `calKey` uses NUL separator (matches Go `calKey()`).
|
||||
- `list()` sorts by source then signal (matches Go `List()`).
|
||||
|
||||
## Self-review
|
||||
|
||||
- Implementation is a direct port of the Go reference.
|
||||
- No external dependencies. No STL/Node builtins used in the browser execution path.
|
||||
- IIFE avoids polluting global scope beyond the single `Calib` name.
|
||||
- `Object.create(null)` for the internal map avoids prototype-key collisions.
|
||||
- `Object.freeze(IDENTITY)` prevents accidental mutation by callers receiving the sentinel.
|
||||
- All 14 brief-specified test cases are present verbatim and pass.
|
||||
- `node --check` on both JS files is clean.
|
||||
- Go embed confirmed serving the new file via HTTP.
|
||||
|
||||
## Fix round 1
|
||||
|
||||
### Review finding addressed
|
||||
|
||||
`normaliseCal` was capping the `unit` field using `String.prototype.length` and `.slice()`, which count UTF-16 code units, not UTF-8 bytes. Both hubs cap at 16 **bytes** of UTF-8. Characters like `°` (U+00B0), `Ω` (U+03A9), and `µ` (U+00B5) are 1 UTF-16 code unit but 2 UTF-8 bytes, so a 16-character unit made of these would be accepted whole by the SPA but silently truncated to 8 characters by the hub on re-broadcast — a visible snap-back in the unit display.
|
||||
|
||||
### Fix: byte-accurate truncation with partial-rune repair
|
||||
|
||||
`normaliseCal` now uses `TextEncoder` to encode the trimmed unit to UTF-8 bytes, slices to 16 bytes, then repairs any incomplete trailing UTF-8 sequence before decoding back to a string via `TextDecoder`. This matches both hubs' behaviour exactly:
|
||||
|
||||
- **Go `CalConfig.Normalise()`**: slices to `maxUnitLen` bytes, then loops calling `utf8.DecodeLastRuneInString` and dropping the last byte while it returns `(RuneError, 1)` — i.e. while the tail is an invalid/incomplete byte.
|
||||
- **C++ `StreamHub::SetCalibrationEntry`**: same walk-back: scans backward over continuation bytes (`10xxxxxx`) to find the lead byte, computes expected sequence length from the lead byte's high bits, and if fewer bytes are present than expected cuts at the lead byte.
|
||||
|
||||
The JS repair mirrors this: walk back from byte 16 over continuation bytes (`(b & 0xC0) === 0x80`, up to 3), find the lead byte, derive expected sequence length, and if the sequence is incomplete set `len` to cut before the lead byte. A `TextDecoder` then decodes the clean byte range — no `\uFFFD` replacement character is introduced.
|
||||
|
||||
`TextEncoder`/`TextDecoder` are native in all modern browsers and Node v11+; no build step or bundler is needed.
|
||||
|
||||
### Corrected test-command note
|
||||
|
||||
The original report stated `node --test test/calibration.test.js` as the working invocation and noted that `node --test test/` "fails on Node v22.23.0 because Node tries to `require()` the directory as a module". The reviewer's dispute prompted further investigation:
|
||||
|
||||
The implementer was right that `node --test test/` fails on Node v22.23.0, but the reason was incomplete. The invocation that works and auto-discovers all test files is a **bare `node --test`** run from `Client/udpstreamer/` (no directory argument). Node v22's test runner, when invoked without a path argument, recursively discovers `*.test.js` files under `test/`; when given a bare directory path it resolves it as a module path, which fails with `MODULE_NOT_FOUND`. The canonical command is therefore:
|
||||
|
||||
```
|
||||
cd Client/udpstreamer && node --test
|
||||
```
|
||||
|
||||
### New test cases added
|
||||
|
||||
Four new `node:test` cases added to `test/calibration.test.js`:
|
||||
|
||||
1. **Short non-ASCII units left untouched** — `"Ω"`, `"µs"`, `"°C"` each pass through unchanged.
|
||||
2. **Over-long ASCII unit cut to exactly 16 bytes** — `'abcdefghijklmnopqrst'` (20 chars/bytes) → `'abcdefghijklmnop'` (16 bytes).
|
||||
3. **Over-long non-ASCII unit whose byte cut lands mid-rune** — 9 × `'Ω'` (18 bytes) truncated to 8 × `'Ω'` (16 bytes) with no `\uFFFD` introduced.
|
||||
4. **Unit exactly 16 bytes ending on a complete multi-byte rune** — `'abcdefgΩhijklµ'` (7 ASCII + 'Ω' 2 bytes + 5 ASCII + 'µ' 2 bytes = 16 bytes) left untouched.
|
||||
|
||||
### Command output
|
||||
|
||||
```
|
||||
cd Client/udpstreamer && node --test
|
||||
```
|
||||
|
||||
```
|
||||
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
|
||||
# suites 0
|
||||
# pass 18
|
||||
# fail 0
|
||||
# cancelled 0
|
||||
# skipped 0
|
||||
# todo 0
|
||||
# duration_ms 44.149322
|
||||
```
|
||||
|
||||
```
|
||||
node --check Client/udpstreamer/static/calibration.js
|
||||
```
|
||||
|
||||
Output: (no output — syntax OK)
|
||||
Reference in New Issue
Block a user