Skip to content

refactor(firmware): name the comment-only protocol values so the client sync can read them (#346) - #347

Merged
mottihoresh merged 1 commit into
mainfrom
refactor/named-protocol-values-346
Oct 16, 2026
Merged

mottihoresh merged 1 commit into
mainfrom
refactor/named-protocol-values-346

Conversation

@mottihoresh

Copy link
Copy Markdown
Collaborator

Closes #346.

What

The client's npm run sync reads firmware values only from enum [class] X : T { NAME = literal } and constexpr 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.

# Value Declaration
1 Gateway is node 0 GATEWAY_NODE_ID (NodeStatus.h). Used at the PanelBridge self-row and at the CANProtocol master checks (NODE_ID != 0, canIdHb(0))
2 _NODE_STATUS row layout NODE_STATUS_ROW_BYTES (14) + NodeStatusField byte offsets
3 HB flags enum class HeartbeatFlag { BOFF, EPVF }
4 ESR split HB_ESR_TEC_SHIFT / HB_ESR_REC_SHIFT
5 Die temp unavailable DIE_TEMP_UNAVAILABLE = -128
6 _NODE_TELEM framing NODE_TELEM_ROW_BYTES (9) + NodeTelemField
7 TELEM page layouts PduRailField / PduStatusField. The PDU packs its frames through them
8 NTC unavailable NTC_TEMP_UNAVAILABLE = -32768
9 Calibration payload offsets AxisCal.h: one offset enum for every type that has a payload (HELLO_ACK, CAL_DATA + record, SESSION_OPEN, SESSION_ACK, STREAM_SELECT, COMMIT, RESET, ACK, NACK, RAW) + CAL_DATA_REC_BYTES. SimGateway reads and writes every payload through them

Design choices

  • Formatting: the _NODE_STATUS / _NODE_TELEM rows are still built with snprintf. Static asserts tie each format string to its row enum, so moving an enum value fails the build until the format string is updated.
  • Sentinels: written as negative literals (-128, -32768), each with a static assert that it equals INT8_MIN / INT16_MIN. One name per sentinel.
  • ID, not NODE_ID: the row-field enums name the node-id field ID because every node build defines NODE_ID as a macro.
  • Item 9 scope: I added offset enums for the one-byte request payloads and ACK too, beyond the six types the issue lists, so the client needs no hand-typed offset at all.

No wire or behaviour change — proof

I built 20 envs on main (d29433a), then again on this branch, and compared the .bin files: all 20 are byte-identical.

  • Envs: E2E SimGateway / PanelBridge / PanelGroup, PDU_Center, Center_Armament, plus the node_status, node_telem, axis_cal_* (4), heartbeat_payload, health_payload, health_flags, node_status_service, pdu_math, board_thermal, rail_monitor, telem_schedule and adc_clock tests.
  • I checked first that a clean rebuild of unchanged source reproduces the same binary.
  • I also built the first env of every Tests/Panels/Examples project (34/34 OK) to catch macro collisions.

Docs (same PR)

  • FirmwarePlan 02 / 03 / 04 and TechSpec CANProtocol / NodeStatus / PanelBridge / PowerMonitor / STM32Board / SimGateway now name the declarations.
  • docs/firmware/build-flags.md had two CANProtocol.cpp line citations that this change shifted by two lines. I updated them, and check_doc_refs.py is clean.
  • Nothing else in the published docs needs to change: the wire format is the same.

Client follow-up (SkyHawkClient R1)

  • constexprNum must accept a leading - for the two sentinels.
  • The new enums and constants can be generated from NodeStatus.h and AxisCal.h. I checked that every new declaration matches the current parseEnum / constexprNum regexes. The only exception is the - sign above.

…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.
@mottihoresh mottihoresh added this to the Firmware v1.0.0 milestone Oct 16, 2026
@mottihoresh mottihoresh added the firmware Firmware implementation work label Oct 16, 2026
@mottihoresh
mottihoresh merged commit a78dd90 into main Oct 16, 2026
13 checks passed
@mottihoresh
mottihoresh deleted the refactor/named-protocol-values-346 branch October 16, 2026 23:49
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

firmware Firmware implementation work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(firmware): name the comment-only protocol values so the client sync can read them

1 participant