Repository navigation
refactor(firmware): name the comment-only protocol values so the client sync can read them (#346) - #347
Merged
Conversation
…nt sync can read them (#346) Nine protocol facts lived only in comments, bare literals or a format string, so the client's sync-a4ec.ts could not generate them. Each is now a named enum or constexpr the parser reads, and firmware uses the names: - NodeStatus.h: GATEWAY_NODE_ID, NODE_STATUS_ROW_BYTES + NodeStatusField, NODE_TELEM_ROW_BYTES + NodeTelemField, HeartbeatFlag, HB_ESR_TEC/REC_SHIFT, DIE_TEMP_UNAVAILABLE, PduRailField / PduStatusField, NTC_TEMP_UNAVAILABLE - AxisCal.h: payload offset enums for every calibration type with a payload, plus CAL_DATA_REC_BYTES The _NODE_STATUS / _NODE_TELEM snprintf formats stay; static_asserts hold them to the row enums. Sentinels are literals (-128 / -32768) asserted equal to INT8_MIN / INT16_MIN. Row enums use ID rather than NODE_ID, which every node build defines. No wire or behaviour change: all 20 compared envs (bridge, gateway, PanelGroup, PDU, and the node-status / telem / calibration / heartbeat / PDU tests) build to binaries byte-identical with main.
Closed
3 of 4 tasks
mottihoresh
added a commit
to OpenSkyHawk/SkyHawkClient
that referenced
this pull request
Oct 17, 2026
R1.5 of the structure rewrite. Firmware PR OpenSkyHawk/OpenSkyhawk#347 gave names to nine protocol values that used to exist only as comments or bare literals. This PR syncs those names and swaps the client's hand-typed copies for them (plan §3.3 rows B2–B9 and B17). Nothing changes in behaviour: no UI, wording or output differs. ## 1. Pin bump: d29433ab → a78dd902 `PIN.commit` now points at `a78dd90276fffb820b64ba74b82f354ab167348a`, the #347 merge. It is the only firmware commit between the two pins. The pin was bumped and `npm run sync` run in a commit of its own (c150ca8), before any generator change. That commit touches exactly one line in each of the 8 generated files: the `// Source: … @ <commit>` banner. `node-names.generated.ts` also has `NODE_REGISTRY_SOURCE.commit`. Every value is identical, and the registry `date` stays 2026-10-16. ``` scripts/sync-a4ec.ts | 2 +- src/shared/firmware/*.generated.ts (8 files) | 1+/1- each (node-names 2+/2-) ``` ## 2. What the sync now generates These are additions to the existing generators. No new script. `constexprNum` accepts a leading `-` on its first term, which the two signed sentinels need. A new `parseOffsets` reads an offset enum and checks two things: the offsets are strictly ascending, and each lies inside the row or payload it indexes. A relayout the decoders can't read field by field fails the sync instead of mis-decoding. **`node-status.generated.ts`** (from `NodeStatus.h`): - `GATEWAY_NODE_ID` = 0 - `NODE_STATUS_ROW_BYTES` = 14 and `NODE_STATUS_FIELD` {ID 0, PRESENT 1, HB_FLAGS 2, UPTIME 3, RX_COUNT 5, ESR 7, DIE_TEMP_C 9, HEALTH_FLAGS 10, FAULT_MASK 11, FAULT_ID 12, NODE_KIND 13} - `HEARTBEAT_FLAGS` {BOFF 0x1, EPVF 0x2} - `HB_ESR_TEC_SHIFT` = 0 and `HB_ESR_REC_SHIFT` = 8 - `DIE_TEMP_UNAVAILABLE` = -128 - `NODE_TELEM_ROW_BYTES` = 9 and `NODE_TELEM_FIELD` {ID 0, FRAME 1} - `PDU_RAIL_FIELD` {FLAGS 1, V_AVG_MV 2, V_MIN_MV 4, I_AVG_MA 6} - `PDU_STATUS_FIELD` {FLAGS 1, NTC_TEMP_DC 2, I12_PEAK_MA 4, I5_PEAK_MA 6} - `NTC_TEMP_UNAVAILABLE` = -32768 **`axis-cal.generated.ts`** (from `AxisCal.h`): - `CAL_PROTO.dataRecBytes` = 10, from `CAL_DATA_REC_BYTES`. - `CAL_HELLO_ACK_FIELD`, `CAL_DATA_FIELD`, `CAL_DATA_REC_FIELD`, `CAL_SESSION_OPEN_FIELD`, `CAL_SESSION_ACK_FIELD`, `CAL_STREAM_SELECT_FIELD`, `CAL_COMMIT_FIELD`, `CAL_RESET_FIELD`, `CAL_ACK_FIELD`, `CAL_NACK_FIELD`, `CAL_RAW_FIELD`. The type prefix is stripped from each entry, so `CAL_COMMIT_MIN` becomes `CAL_COMMIT_FIELD.MIN`. The sync fails if an entry lacks the expected prefix. Each table is bounded by its type's synced payload length; the per-axis record table is bounded by `CAL_DATA_REC_BYTES`. Every table is an `as const` name-keyed object, so a misspelt field name doesn't compile. Each entry keeps the firmware's trailing comment (`// u16: AxisCal::min`). `reference.test.ts` pins every new value at today's literal, so a future firmware relayout fails there. ## 3. Client sites replaced | Name | Client site replaced | How equality was proven | |---|---|---| | `GATEWAY_NODE_ID` (B2) | `shared/nodes.ts` `BRIDGE_NODE_ID = 0` (export kept, now `= GATEWAY_NODE_ID`); `tabs/settings/settings-view.ts` `nodeId === 0` | 0 === 0. `registryOnBus` base vs head over node IDs 0–70 × 4 models × link up/down. Settings › About › registry render identical | | `NODE_STATUS_ROW_BYTES` + `NODE_STATUS_FIELD` (B3) | `nodes.ts` `HEX_LEN = 28`, the `{28}` regex, and every `f(i, n)` hex slice | `parseNodeStatus` base vs head, `toStrictEqual`, over **89,949 rows** (below) | | `HEARTBEAT_FLAGS` (B4) | `nodes.ts` `flags & 0x01` / `& 0x02` | same corpus, all 256 HB_FLAGS bytes | | `HB_ESR_TEC_SHIFT` / `HB_ESR_REC_SHIFT` (B5) | `nodes.ts` `esr & 0xff` / `(esr >> 8) & 0xff` | same corpus, all 65,536 ESR values | | `DIE_TEMP_UNAVAILABLE` (B6) | `nodes.ts` `traw === 0x80` | all 256 die-temp bytes, base vs head (sign convention below) | | `NODE_TELEM_ROW_BYTES` + `NODE_TELEM_FIELD` (B7) | `node-telem.ts` `HEX_LEN = 18`, the `{18}` regex, `slice(0, 2)`, the frame-byte slice `2 + i*2`, `slice(4)` for the unknown-page body | `parseNodeTelem` base vs head, `toStrictEqual`, over **87,133 rows** (below) | | `PDU_RAIL_FIELD` / `PDU_STATUS_FIELD` (B8) | `node-telem.ts` `b(1)`, `u16(2/4/6)`, `i16(2/6)` | same corpus, both rail pages at the u16/i16 extremes | | `NTC_TEMP_UNAVAILABLE` (B9) | `node-telem.ts` `NTC_UNAVAILABLE = -0x8000` | -0x8000 === -32768. All 65,536 NTC words on the status page, base vs head | | `CAL_HELLO_ACK_FIELD` (B17) | `calibration.ts` `decodeHelloAck` `p[0..5]` | every decoder, base vs head, payload lengths 0–90 (short, exact and long) × 7 payloads each (all-0, all-0xff, all-0x80, all-0x7f, random), plus 2,000 random 82-byte payloads | | `CAL_DATA_FIELD` + `CAL_DATA_REC_FIELD` + `CAL_PROTO.dataRecBytes` (B17) | `decodeCalData` `p[0]`, `p[1]`, `2 + i * 10`, `o + 2/4/6/8` | same | | `CAL_SESSION_ACK_FIELD` (B17) | `decodeSessionAck` `u16(p, 0)`, `u16(p, 2)`, `p[4]` | same | | `CAL_ACK_FIELD` / `CAL_NACK_FIELD` / `CAL_RAW_FIELD` (B17) | `decodeAck` `p[0]`; `decodeNack` `p[0..2]`; `decodeRaw` `p[0]`, `u16(p, 1/3)` | same | | `CAL_COMMIT_FIELD` (B17) | `encodeCommit` `p[0]`, `put(1/3/5/7)`, and `new Uint8Array(9)` → `CAL_PAYLOAD_LEN[COMMIT]` (already synced, already pinned at 9) | base vs head bytes: 6 seqs × 5 idx × 12 values (incl. 0x10000, -1, 1.5, NaN), plus 2,000 random axes per seq. Demux round-trip identical | | `CAL_SESSION_OPEN_FIELD` / `CAL_STREAM_SELECT_FIELD` / `CAL_RESET_FIELD` (B17) | `one(type, seq, byte)` with its implicit offset 0 (`Uint8Array.from([byte])`) | base vs head bytes: 6 seqs × 20 byte values (incl. -1, 256, 1.5, NaN, ±Infinity, 1e9). `one` now sizes the payload `at + 1`, so if the field ever moved off byte 0, buildFrame's length check would throw rather than send a zero | **Status-row corpus (89,949 rows):** - every hex literal in the repo's test files and fixtures - `statusRow()` for node IDs 0–255 × present 0/1 × kinds 0–3, 4 and 0xff - each field at its min/max, with the rest all-min and all-max - every die-temp byte, every ESR value, and every byte of flags, present and hFlags - 20,000 random mixed-case rows - malformed rows: lengths 26/27/29/30, whitespace, non-hex, `0x`, `+`/`-` prefix, Arabic-Indic digit `isLegacyNodeStatus` was compared over the same rows. **Telemetry corpus (87,133 rows):** - every fixture literal - node IDs 0–255 × pages 0–4 - every page byte - every NTC int16 - rail extremes - 20,000 random rows - malformed rows The proof tests were mutation-checked. With `esr >> 7` and `p[F.DETAIL]` swapped in for the reason byte, they fail. They are throwaway files and are not committed. **Render compare:** each view is fed rows decoded through the decoders: a gateway row, panels with bus-off/error-passive, the die-temp sentinel, -40 °C and overheat+fault, a PDU, a removed node, rail/status/unknown TELEM pages including the NTC sentinel and an INT16_MIN current, and decoded CAL_DATA / SESSION_ACK / RAW. I dumped the HTML of 8 views: Cockpit, the Cockpit panel sheet, the Cockpit PDU sheet, Connection, HID, the Calibration dialog, Settings › About, and the Settings registry dialog. Head was dumped twice to confirm the output is deterministic. Then the four changed source files were swapped back to base and the views dumped again. `diff -r` base vs head: **identical**. ## 4. Sign conventions - **B6, die temperature.** The firmware value is signed int8 -128. The row carries its two's-complement byte, 0x80. The decoder already converted the byte to signed (`traw < 128 ? traw : traw - 256`). It now converts first and compares the signed value with `DIE_TEMP_UNAVAILABLE`. That is the same test as the old `traw === 0x80`, because the signed value is -128 exactly when the byte is 0x80. Proven for all 256 byte values. `reference.test` also pins `DIE_TEMP_UNAVAILABLE & 0xff === 0x80`. - **B9, NTC.** `i16()` already produced the signed value, and the old constant was `-0x8000`, so `NTC_TEMP_UNAVAILABLE` (-32768) drops straight in. Proven for all 65,536 words. `reference.test` pins `& 0xffff === 0x8000`. The `i16` sign conversion (`v >= 0x8000 ? v - 0x10000 : v`) is generic int16 handling, not the sentinel, so it stays. ## 5. What stays hand-typed, and why - `V2_HEX_LEN = 26` in `nodes.ts`. The legacy v2 row is detected client-side, only to say "this bridge is too old". The firmware no longer describes it, so there is nothing to sync from. A comment now says so. - Hat direction labels (CTR N NE … NW): client wording. - The TELEM page byte, read as `b(0)`. The firmware names no field for it; `NodeTelemField` says "TelemPage at FRAME + 0". - The calibration envelope offsets in `buildFrame` / `classify` (type @4, seq @5, len @6–7, payload @8). The firmware has no envelope-field enum, and they were outside B17. - Out of scope per the brief: B18 (CRC parameters; the vector test is kept) and B21 (hysteresis text). Cockpit-adopt's Gateway wording is untouched. ## 6. Grep proof Each pattern was grepped over `src/shared/nodes.ts`, `node-telem.ts`, `calibration.ts` and `renderer/src/tabs/settings/settings-view.ts`. Every count is 0: ``` 0 BRIDGE_NODE_ID = 0 0 nodeId === 0 0 HEX_LEN = (28|18) 0 \{28\}|\{18\} 0 f\([0-9]+, [0-9]\) 0 flags & 0x0[12] 0 esr >> 8|esr & 0xff 0 traw === 0x80 0 NTC_UNAVAILABLE|-0x8000 0 hex\.slice\(4\)|slice\(2 \+ i 0 b\([1-7]\)|u16\([2-6]\)|i16\([2-6]\) 0 p\[[0-9]\] 0 u16\(p, [0-9]|o \+ [0-9]|2 \+ i \* 10 0 put\([0-9] 0 new Uint8Array\(9\) 0 Uint8Array\.from\(\[byte\]\) ``` A repo-wide search for `0x80`, `0x8000`, `INT16_MIN`, `=== 0` on node IDs and `>> 8) & 0xff` outside tests and fixtures found only generic byte handling: LE writes, CRC, int16 sign conversion, and dcsbios export frames. ## Verification - `npm run sync` is idempotent at the new pin: the diff is byte-identical before and after a re-run. That is the CI freshness gate. - `npm run typecheck`: clean. - `npm run lint`: 0 errors. No warnings in the changed files. - `npm run lint:deps`: 0 errors. - `npx vitest run`: 62 files, 856 tests. That is 852 plus 4 new reference pins. Existing assertions are unchanged. - `npm run build`: passes. Refs #98 Refs OpenSkyHawk/OpenSkyhawk#346
mottihoresh
added a commit
to OpenSkyHawk/SkyHawkClient
that referenced
this pull request
Oct 17, 2026
…resentation-only renderer (#129) The structure rewrite (#101), integrated on `refactor/structure` through 27 squash-merged PRs (#102–#128). **Structure only. No new functionality and no intended behaviour change.** Every behaviour fix waits on the fix track in #101 and lands on `main` after this merge. **Merge with a merge commit** (not squash), so each step stays on `main` and can be bisected. ## What changed - **Process wall:** `src/main` decides, `src/renderer` only presents, `src/shared` holds the per-topic IPC contracts, the generated firmware reference and the formatting helpers, and `src/preload` holds the bridge. Lint enforces the wall. - **Topics:** `app`, `gateway`, `dcs`, `nodes`, `hid`, `hid/calibration`, `events`, `config`. Each has an `index.ts` as its only door, and the allowed import directions are enforced. - **Main is split into one owner per job:** - `GatewayConnection`, `DeviceLineTap`, `SerialTrafficMonitor` - `NodeHub` (roster poll + Gateway health), `MappingStore` - `HidService`, `HidCalibrationService` (it hosts the calibration workflow) - `EventJournal` - `DcsConnection`, `DcsRelay`, `StreamLog`, `SessionRecorder` - `SettingsService` (one zustand store, handed to services) - `AppServices` does wiring only. DeviceLink and Session are gone. - **The renderer decides nothing.** The calibration workflow, the cockpit snapshot, and the link, relay, HID and settings facts are all pushed from main. Where screens disagreed before, each screen's version now sits side by side in its owner; unifying them is the fix track. Each topic has its own store, and `app/ipc-adapter.ts` is the only `window.skyhawk` reader. - **Firmware values come only from `npm run sync`.** That includes the 9 values OpenSkyHawk/OpenSkyhawk#347 named. - **`push:batch`:** each main turn reaches the renderer as one message and one commit. - **`ARCHITECTURE.md`** is the contributor guide. ## How "no behaviour change" was checked, PR by PR - Each PR's tests passed with their assertions unchanged. The suite grew from 698 to 1131, including 35 component tests and the seam tests, and every new test was proven able to fail. - A side-effect trace of main (pushes, serial, files, timers, return values; up to 48 scenarios and about 11,800 events) matched base byte for byte. - A render comparison (up to about 2,050 DOM states across every component-test file plus extra flows) matched base in sequence. - A built-bundle boot probe (app events, IPC registration order, files written) matched base. - An async-IPC audit replayed every user action with IPC delayed. It found frames where part of the screen had updated and a related part hadn't; these were fixed (#125, #127), and a re-audit found no new ones. Lint, boundary lint, typecheck (3 projects) and build are clean, with every rule at error and 0 warnings. ## Before merging - [x] Hardware check on the real Gateway at 19cdcd9 (2026-10-17): Gateway connects with Health, board adopted, CAN state/TEC/REC/MCU temp shown, Replay with Drive plays a recorded capture to the Gateway (serial traffic confirmed), mode switching clean. - [ ] Live DCS check on the DCS computer: can follow after the merge. ## After merging - Fix track and parked-for-discussion items: see #101.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #346.
What
The client's
npm run syncreads firmware values only fromenum [class] X : T { NAME = literal }andconstexpr T NAME = literal. Nine protocol facts weren't written that way, so the client hand-typed them. Each one now has a name, and the firmware uses that name in place of the literal.GATEWAY_NODE_ID(NodeStatus.h). Used at the PanelBridge self-row and at the CANProtocol master checks (NODE_ID != 0,canIdHb(0))_NODE_STATUSrow layoutNODE_STATUS_ROW_BYTES(14) +NodeStatusFieldbyte offsetsenum class HeartbeatFlag { BOFF, EPVF }HB_ESR_TEC_SHIFT/HB_ESR_REC_SHIFTDIE_TEMP_UNAVAILABLE = -128_NODE_TELEMframingNODE_TELEM_ROW_BYTES(9) +NodeTelemFieldPduRailField/PduStatusField. The PDU packs its frames through themNTC_TEMP_UNAVAILABLE = -32768CAL_DATA_REC_BYTES. SimGateway reads and writes every payload through themDesign choices
_NODE_STATUS/_NODE_TELEMrows are still built withsnprintf. Static asserts tie each format string to its row enum, so moving an enum value fails the build until the format string is updated.-128,-32768), each with a static assert that it equalsINT8_MIN/INT16_MIN. One name per sentinel.ID, notNODE_ID: the row-field enums name the node-id fieldIDbecause every node build definesNODE_IDas a macro.No wire or behaviour change — proof
I built 20 envs on
main(d29433a), then again on this branch, and compared the.binfiles: all 20 are byte-identical.node_status,node_telem,axis_cal_*(4),heartbeat_payload,health_payload,health_flags,node_status_service,pdu_math,board_thermal,rail_monitor,telem_scheduleandadc_clocktests.Docs (same PR)
docs/firmware/build-flags.mdhad twoCANProtocol.cppline citations that this change shifted by two lines. I updated them, andcheck_doc_refs.pyis clean.Client follow-up (SkyHawkClient R1)
constexprNummust accept a leading-for the two sentinels.parseEnum/constexprNumregexes. The only exception is the-sign above.