Martino FerrariandClaude Sonnet 4.6
93e00d0c21
The previous walk-back in SetCalibrationEntry was unconditional, corrupting short valid units ending in multi-byte characters (e.g. Omega, mu, degree). Also failed to drop an orphaned lead byte left after stripping continuation bytes. Restructured to use a 256-byte staging buffer so truncation can be detected, then repair runs only in the truncation branch. Algorithm now matches Go CalConfig.Normalise() exactly: scan back over continuation bytes (up to 3), find the lead byte, derive expected sequence length, cut if incomplete. Covers all cases: orphaned continuation, orphaned lead, cut on lead byte. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
192 lines
12 KiB
Markdown
192 lines
12 KiB
Markdown
# 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.
|