From 041ad2a18f209d201d2ce757a2b466a8df05e1ed Mon Sep 17 00:00:00 2001 From: pgdalmeida Date: Fri, 18 Sep 2026 17:15:16 +0200 Subject: [PATCH] Understand the NEEDCFG state instead of reading it as a dead link --- docs/configuration.md | 4 ++++ docs/hardware-config-plan.md | 3 ++- docs/modules-reference.md | 2 +- include/fgc/IMotorController.h | 8 +++++++- include/fgc/mock/MockMotorController.h | 9 +++++++-- src/core/DumpParser.cpp | 4 ++++ src/core/TelemetryParser.cpp | 1 + src/core/TestRunner.cpp | 3 +++ src/ui/UiSnapshot.cpp | 4 ++++ tests/test_dumpparser.cpp | 22 ++++++++++++++++++++++ tests/test_telemetry.cpp | 21 +++++++++++++++++++++ 11 files changed, 76 insertions(+), 5 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 9384019..b9f60fb 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -723,6 +723,10 @@ Three rules, each protecting against a different silent failure: - **A partial answer does not unlock the axis.** Most keys landing is the common case when a declaration is nearly complete, and it must not look like success. +> An axis waiting on its config reports `NEEDCFG` in the dashboard and `C` in the +> `ST` line — deliberately not red, and deliberately not "no telemetry": the link +> is fine and the axis is talking, it just has not been told what it is driving. +> > **Status: the host half only.** The host answers `NEED` and the responder is > verified end to end, but the firmware that *asks* is not written yet. Set > `[Features] mock_needcfg = 1` to make the simulated controller run the diff --git a/docs/hardware-config-plan.md b/docs/hardware-config-plan.md index 4512431..b057939 100644 --- a/docs/hardware-config-plan.md +++ b/docs/hardware-config-plan.md @@ -426,7 +426,7 @@ Phase 1 is deployable on its own. Phase 2 requires it. ## Progress Branch `worktree-hardware-config` (worktree at `.claude/worktrees/hardware-config`). -295 tests pass, up from a 204 baseline. +298 tests pass, up from a 204 baseline. | § | Item | State | |----|------|-------| @@ -442,6 +442,7 @@ Branch `worktree-hardware-config` (worktree at `.claude/worktrees/hardware-confi | 6 | Config handshake — **host half** | **done**, 13 tests. `FirmwareConfig.h` parses `NEED` and plans the `SET` lines; unanswered keys and unknown keys are reported separately. `[Features] mock_needcfg` makes the simulated controller run the handshake, so the host half is verified with no firmware. **The firmware half is not written** | | 6 | Config handshake — **firmware half** | **done and built** on both targets 2026-09-18 (iota_rp2040 and leonardo, `./deploy.sh --compile-only`); behaviour still unproven on a running rig. `NEEDCFG` state (`ST` code `C`), `NEED` emitted with the banner, `SET `, `USE DEFAULTS`, and the init block split so the residual-motion stop still runs at boot. 33 native tests pass under `pio test -e native`. **Behaviour on a running rig is the one check still outstanding** | | 6 | Adoption check generalised | **done**, 9 tests. `DUMP` echoes every host-owned value plus a `cfg_rx` bitmask; `test hardware motor` checks all of them, and reports an absent field as unconfirmable rather than as a mismatch against -1. Round trip NEED -> SET -> DUMP -> PASS verified against `mock_needcfg` | +| 6 | `NEEDCFG` understood host-side | **done**, 5 tests. `AxisState::NeedConfig` parsed from `ST` `C`, named in `DUMP` and the dashboard, and counted as telemetry arriving rather than as a dead link. The firmware enum APPENDS the state (5) rather than inserting it — inserting renumbered every later state and made a READY axis read back as ERROR | Presence reconciliation (§10) changes nothing on a tower without a `hardware.ini`: absent is silent and `hardware.declared` stays false. Verified by running the binary both ways. diff --git a/docs/modules-reference.md b/docs/modules-reference.md index 20e3bc4..ab09d40 100644 --- a/docs/modules-reference.md +++ b/docs/modules-reference.md @@ -86,7 +86,7 @@ Per-file reference for the refactored tree, plus the shared data structures. ### `MotorTelemetry` / `AxisTelemetry` ([IMotorController.h](../include/fgc/IMotorController.h)) Per-axis `yaw` and `pitch` segments (`pitch_present` flag), each an `AxisTelemetry`: `state` -(`AxisState` B/R/H/A/E), `xactual`, `xenc` (encoder counts), `drv_status`, `sg`/`cs`/`pwm`, and flags +(`AxisState` B/R/H/A/E/C — the last being NEEDCFG, a firmware waiting to be told its drivetrain), `xactual`, `xenc` (encoder counts), `drv_status`, `sg`/`cs`/`pwm`, and flags `standstill`/`stall`/`deviation`/`overtemp`/`endstop_l`/`endstop_r`, with `moving()`/`ready()` helpers. Parsed from the firmware `ST Y:...[ P:...]` line; degrees are derived via `Geometry`. diff --git a/include/fgc/IMotorController.h b/include/fgc/IMotorController.h index c46411c..f39929e 100644 --- a/include/fgc/IMotorController.h +++ b/include/fgc/IMotorController.h @@ -6,7 +6,13 @@ namespace fgc { // Per-axis lifecycle state, from the firmware ST line's state char (B/R/H/A/E). -enum class AxisState { Boot, Reset, Homing, Ready, Error, Unknown }; +// Per-axis lifecycle, from the firmware ST line's state char. +// B Boot R Reset H Homing A reAdy E Error C NeedConfig +// +// NeedConfig means the firmware has been told nothing about the drivetrain it is +// driving and is refusing to move until the host answers its NEED lines. It is +// NOT a fault and NOT "no telemetry": the link is fine and the axis is talking. +enum class AxisState { Boot, Reset, Homing, Ready, Error, NeedConfig, Unknown }; // One axis segment of a firmware `ST` status line: // ST Y:,,,,,,, [P:...] diff --git a/include/fgc/mock/MockMotorController.h b/include/fgc/mock/MockMotorController.h index 49f851c..63264b9 100644 --- a/include/fgc/mock/MockMotorController.h +++ b/include/fgc/mock/MockMotorController.h @@ -109,7 +109,9 @@ public: // and main.cpp) so the host-side DumpParser is exercised without // hardware. Register values are illustrative, not simulated; the // encoder positions track the mock's live state. - const int st = homed_ ? 3 : 1; // READY : RESET + // 5 = NEEDCFG, appended to the firmware enum so the older codes keep + // their meanings. See motor.h. + const int st = (needcfg_ && !configured_) ? 5 : (homed_ ? 3 : 1); std::ostringstream d; d << "DUMP BEGIN build=mock uptime=0 mcusr=0x01 free_ram=4096\n"; auto axis = [&](char L, const AxisTelemetry& a) { @@ -240,7 +242,10 @@ private: a.standstill = false; } a.xactual = a.xenc; - a.state = homed_ ? AxisState::Ready : AxisState::Reset; + // An axis waiting on its config says so, rather than looking like a + // healthy un-homed one - that distinction is the whole point of the state. + a.state = (needcfg_ && !configured_) ? AxisState::NeedConfig + : (homed_ ? AxisState::Ready : AxisState::Reset); a.deviation = deviation_; } diff --git a/src/core/DumpParser.cpp b/src/core/DumpParser.cpp index 322b117..44f4861 100644 --- a/src/core/DumpParser.cpp +++ b/src/core/DumpParser.cpp @@ -16,6 +16,10 @@ const char* stateName(int s) { case 2: return "HOMING"; case 3: return "READY"; case 4: return "ERROR"; + // Appended in the firmware enum rather than placed in lifecycle order, + // because these numbers are on the wire: inserting it would renumber + // every later state and a READY axis would read back as ERROR. + case 5: return "NEEDCFG"; default: return "?"; } } diff --git a/src/core/TelemetryParser.cpp b/src/core/TelemetryParser.cpp index cd12bdd..2c2f214 100644 --- a/src/core/TelemetryParser.cpp +++ b/src/core/TelemetryParser.cpp @@ -29,6 +29,7 @@ AxisState stateFromChar(char c) { case 'H': case 'h': return AxisState::Homing; case 'A': case 'a': return AxisState::Ready; case 'E': case 'e': return AxisState::Error; + case 'C': case 'c': return AxisState::NeedConfig; default: return AxisState::Unknown; } } diff --git a/src/core/TestRunner.cpp b/src/core/TestRunner.cpp index 6531bc3..0088b9a 100644 --- a/src/core/TestRunner.cpp +++ b/src/core/TestRunner.cpp @@ -867,6 +867,9 @@ void TestRunner::testHwAxes(TestReport& r) { // its own segment. MotorTelemetry t = motor_.telemetry(); FoundHardware f; + // NEEDCFG counts as telemetry arriving: the link is fine and the axis is + // talking, it just has not been told what it is driving. Treating it as "no + // telemetry" would blame the serial port for a configuration gap. f.axes_known = t.yaw.state != AxisState::Unknown; if (f.axes_known) { f.axes.push_back("yaw"); diff --git a/src/ui/UiSnapshot.cpp b/src/ui/UiSnapshot.cpp index 150fa2e..85e4a16 100644 --- a/src/ui/UiSnapshot.cpp +++ b/src/ui/UiSnapshot.cpp @@ -13,6 +13,7 @@ const char* axisStateLabel(AxisState s) { case AxisState::Homing: return "HOMING"; case AxisState::Ready: return "READY"; case AxisState::Error: return "ERROR"; + case AxisState::NeedConfig: return "NEEDCFG"; case AxisState::Unknown: default: return "----"; } @@ -23,6 +24,9 @@ UiColor axisStateColor(AxisState s) { case AxisState::Ready: case AxisState::Homing: return UiColor::Cyan; case AxisState::Error: return UiColor::Red; + // Not red: nothing is broken, the firmware is waiting to be told what it + // is driving. Yellow so it reads as "needs you", not as a fault. + case AxisState::NeedConfig: return UiColor::Yellow; case AxisState::Boot: case AxisState::Reset: return UiColor::Dim; case AxisState::Unknown: diff --git a/tests/test_dumpparser.cpp b/tests/test_dumpparser.cpp index a81d53b..af4b6d0 100644 --- a/tests/test_dumpparser.cpp +++ b/tests/test_dumpparser.cpp @@ -201,3 +201,25 @@ TEST_CASE("a firmware predating the handshake leaves those fields absent, not ze CHECK(d.axes[0].sw_mode == -1); CHECK(d.axes[0].cfg_received == -1); } + +TEST_CASE("the numeric state codes did not shift when NEEDCFG was added") { + // NEEDCFG is appended (5), not inserted in lifecycle order, because these + // numbers are on the wire. Had it gone in at 1, a READY axis (which would + // have become 4) would read back as ERROR - a healthy tower reporting a + // fault in every dump, and an old firmware misread by a new host. + auto one = [](int n) { + const std::string text = + "DUMP BEGIN build=x uptime=1 mcusr=0x01 free_ram=1\n" + "DUMP Y state=" + std::to_string(n) + + " hsub=0 enabled=1 lim_neg=0 lim_pos=1 hold_target=0 speed=1 center=0 " + "soft_margin=0 hold_corr=0 hold_peak=0 hold_cumul=0 has_encoder=1\n" + "DUMP END\n"; + return fgc::parseDump(text).axes.at(0).state_name; + }; + CHECK(one(0) == "BOOT"); + CHECK(one(1) == "RESET"); + CHECK(one(2) == "HOMING"); + CHECK(one(3) == "READY"); + CHECK(one(4) == "ERROR"); + CHECK(one(5) == "NEEDCFG"); +} diff --git a/tests/test_telemetry.cpp b/tests/test_telemetry.cpp index f9b8768..c91ded2 100644 --- a/tests/test_telemetry.cpp +++ b/tests/test_telemetry.cpp @@ -82,3 +82,24 @@ TEST_CASE("parseAxisSegment reads the encoder-deviation flag 'd'") { CHECK(e->stall == true); CHECK(e->deviation == true); } + +TEST_CASE("the NEEDCFG state char is understood, not read as unknown") { + // The firmware refuses to move until the host answers its NEED lines, and + // says so with 'C'. A host that did not know the code would map it to + // Unknown, which downstream reads as "no telemetry arrived" - blaming the + // serial link for a configuration gap. + auto t = fgc::parseTelemetryLine("ST Y:C,0,0,00000000,0,0,0,S"); + REQUIRE(t); + CHECK(t->yaw.state == fgc::AxisState::NeedConfig); + CHECK_FALSE(t->yaw.state == fgc::AxisState::Unknown); + + auto lower = fgc::parseTelemetryLine("ST Y:c,0,0,00000000,0,0,0,S"); + REQUIRE(lower); + CHECK(lower->yaw.state == fgc::AxisState::NeedConfig); +} + +TEST_CASE("an axis in NEEDCFG is not ready") { + auto t = fgc::parseTelemetryLine("ST Y:C,0,0,00000000,0,0,0,S"); + REQUIRE(t); + CHECK_FALSE(t->yaw.ready()); +}