diff --git a/.superpowers/sdd/task-4-report.md b/.superpowers/sdd/task-4-report.md new file mode 100644 index 0000000..86b6b2e --- /dev/null +++ b/.superpowers/sdd/task-4-report.md @@ -0,0 +1,134 @@ +# 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 ``, 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 ``). + +### 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).