Check every declared value, not just the current

This commit is contained in:
pgdalmeida 2026-09-18 16:44:08 +02:00
parent a554a3fd9c
commit 842d95eeca
Signed by: pedro.almeida
GPG Key ID: D4A6C394DF13F1D7
12 changed files with 314 additions and 12 deletions

View File

@ -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`).
286 tests pass, up from a 204 baseline.
295 tests pass, up from a 204 baseline.
| § | Item | State |
|----|------|-------|
@ -440,6 +440,7 @@ Branch `worktree-hardware-config` (worktree at `.claude/worktrees/hardware-confi
| 5 | Delete the `Application.cpp` speed literals | **done**, 13 tests. Decision 2 answered: speed is declared as `[Motor.<axis>] scan_speed_deg_s` (default **8.0 deg/s**) and converted per tower through `counts_per_deg` (`AxisSpeed.h`). The literals sent 36.4 deg/s against an intended 21.8 |
| 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** | **written, NOT compile-verified.** `NEEDCFG` state (`ST` code `C`), `NEED` emitted with the banner, `SET <axis> <key> <value>`, `USE DEFAULTS`, and the init block split so the residual-motion stop still runs at boot. Parser verified locally (30 native tests, g++); the rest type-checks against stubs on both board branches but has never been built by PlatformIO or run on hardware |
| 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` |
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.

View File

@ -54,7 +54,7 @@ While a test runs, interfering commands (moves, homing, a second test, capture,
| `host` | `memory` | available RAM / swap (`/proc/meminfo`) |
| `host` | `load` | load average vs CPU count |
| `hardware` | `axes` | the axes `[Hardware] axes` declares appear in telemetry, **and no others** |
| `hardware` | `motor` | `DUMP` echoes the drivetrain current `hardware.ini` asked for — catches a stale flash or a tower left on limp-home defaults |
| `hardware` | `motor` | `DUMP` echoes **everything** `hardware.ini` declares — current, `steps_per_rev`, `enc_deviation`, `sw_mode` and all three speeds — catching a stale flash, a lost `SET`, or an axis that never left `NEEDCFG` |
| `hardware` | `imu` | a declared IMU is connected and a sample arrives |
| `hardware` | `env` | a declared ambient sensor sits on a **real I2C** adapter (not SMBus-only) and returns a CRC-valid reading |
| `hardware` | `camera` | every configured camera opened; `usbfs_memory_mb` and USB3 LPM hold |
@ -99,6 +99,13 @@ Three rules hold across every leaf:
look identical from the sensor's side, and the distinction is what
known-issues #2 cost to learn.
The `motor` leaf reports two metrics per axis. `<axis>_current` covers
`GLOBALSCALER`/`IRUN`/`homing IRUN`; `<axis>_declared` covers everything else the
host owns. Both treat an **absent** field as unconfirmable rather than as a
mismatch — comparing against a missing value would report "firmware has -1", which
reads as a wrong setting and sends someone to re-send a command that was never the
problem.
The `motor` leaf is the one worth running after every flash: `GLOBALSCALER` and
`IRUN` are **write-only** on the TMC5160, so the `DUMP` echo is the only evidence
a `CURRENT` command landed. A homing current that silently did not take is the

View File

@ -31,6 +31,17 @@ struct DumpAxis {
long global_scaler = -1;
long run_irun = -1;
long homing_irun = -1;
// The rest of what the host declares and the firmware is told. Same -1
// convention and the same reason: absent means a firmware that predates the
// config handshake, which is a version skew, not a value of zero.
long steps_per_rev = -1;
long enc_deviation = -1;
long homing_speed = -1;
long center_speed = -1;
long sw_mode = -1;
// Bitmask over the firmware's CfgKey: which required keys have arrived.
// -1 when absent.
long cfg_received = -1;
// Encoder-tracking health accumulated by the firmware hold corrector.
long hold_corrections = 0; // # of hold resyncs since firmware boot/RESET
long hold_peak_dev = 0; // largest deviation seen while holding (counts)

View File

@ -51,6 +51,26 @@ struct DrivetrainExpectation {
int homing_irun = 0;
};
// One value the host declared and the firmware should be holding. Used for the
// parameters the config handshake added, where "not declared" and "not echoed"
// both mean the check does not apply rather than that it failed.
struct DeclaredValue {
const char* name = "";
bool want = false; // hardware.ini declares it
long value = 0;
long got = -1; // as DUMP echoed it; -1 = absent
};
// Does the firmware hold what hardware.ini says? This is the generalisation of
// the old "did CENTER land?" check: with the drivetrain owned by the host, every
// declared value is one the firmware can be silently wrong about, and a tower
// running on stale or defaulted values looks identical to a correct one from the
// outside.
//
// `values` may be empty, which is how an axis with nothing declared reports
// "not checked" rather than passing.
CheckResult checkDeclaredValues(const std::string& axis, const DeclaredValue* values, int count);
// What DUMP echoed back. -1 means the field was absent, which is a firmware
// predating the CURRENT command - a version skew, not a current of zero, and
// the two must stay distinguishable.

View File

@ -76,6 +76,10 @@ struct HardwareTestContext {
std::vector<std::string> configured_cameras; // [Camera.<name>] blocks in open order
std::string i2c_device; // [Env] i2c_device, for the adapter-class check
// The degree maps, so a declared deg/s can be turned back into the VMAX the
// firmware should be holding.
AxisMap geo_yaw;
AxisMap geo_pitch;
double r_sense_ohm = 0.0; // to recompute what CURRENT should have sent
// sysfs nodes, overridable so the checks are exercisable off a tower.

View File

@ -32,9 +32,10 @@ public:
LOG_INFO << "[mock] motor controller started";
if (!needcfg_) return;
std::lock_guard<std::mutex> lock(mutex_);
static const char* kKeys[] = {"steps_per_rev", "global_scaler", "sw_mode",
"enc_deviation", "normal_speed", "homing_speed",
"center_speed", "center", "margin"};
static const char* kKeys[] = {"steps_per_rev", "global_scaler", "run_irun",
"homing_irun", "sw_mode", "enc_deviation",
"normal_speed", "homing_speed", "center_speed",
"center", "margin"};
need_lines_.clear();
unanswered_.clear();
for (const char* axis : {"Y", "P"}) {
@ -112,10 +113,34 @@ public:
std::ostringstream d;
d << "DUMP BEGIN build=mock uptime=0 mcusr=0x01 free_ram=4096\n";
auto axis = [&](char L, const AxisTelemetry& a) {
// In NEEDCFG mode, echo back what the host actually SET, so the
// whole round trip - NEED, SET, DUMP, `test hardware motor` -
// can be exercised with no firmware. Outside it, stay on the old
// fixed line: that is a firmware predating the handshake, and
// the host must keep telling the two apart.
std::string owned = " speed=200000 center=500000 soft_margin=5000";
if (needcfg_) {
const std::string ax(1, L);
auto val = [&](const char* k, const char* dflt) {
auto it = settings_.find(ax + "." + k);
return it == settings_.end() ? std::string(dflt) : it->second;
};
owned = " speed=" + val("normal_speed", "200000") +
" center=" + val("center", "500000") +
" soft_margin=" + val("margin", "5000") +
" steps_per_rev=" + val("steps_per_rev", "0") +
" enc_dev=" + val("enc_deviation", "0") +
" homing_speed=" + val("homing_speed", "0") +
" center_speed=" + val("center_speed", "0") +
" sw_mode=" + val("sw_mode", "0") +
" gs=" + val("global_scaler", "0") +
" irun=" + val("run_irun", "0") +
" irun_home=" + val("homing_irun", "0");
}
d << "DUMP " << L << " state=" << st
<< " hsub=0 enabled=" << (homed_ ? 1 : 0)
<< " lim_neg=0 lim_pos=1000000 hold_target=" << a.xenc
<< " speed=200000 center=500000 soft_margin=5000"
<< owned
<< " hold_corr=0 hold_peak=0 hold_cumul=0 has_encoder=1\n";
d << "DUMP " << L << " TMC GCONF=0x00000004 GSTAT=0x00000000"
<< " IOIN=0x30000008 TSTEP=0x000fffff RAMPMODE=0x00000000"

View File

@ -2111,6 +2111,8 @@ struct Application::Impl {
hw.camera = camera.get();
for (const auto* c : cfg.camera.active()) hw.configured_cameras.push_back(c->name);
hw.i2c_device = cfg.env.i2c_device;
hw.geo_yaw = cfg.geometry.yaw;
hw.geo_pitch = cfg.geometry.pitch;
hw.usbfs_min_mb = cfg.test.usbfs_min_mb;
hw.lpm_path = cfg.test.usb3_lpm_path;

View File

@ -165,6 +165,12 @@ DumpData parseDump(const std::string& block) {
ax.global_scaler = asInt(get(m, "gs", "-1"));
ax.run_irun = asInt(get(m, "irun", "-1"));
ax.homing_irun = asInt(get(m, "irun_home", "-1"));
ax.steps_per_rev = asInt(get(m, "steps_per_rev", "-1"));
ax.enc_deviation = asInt(get(m, "enc_dev", "-1"));
ax.homing_speed = asInt(get(m, "homing_speed", "-1"));
ax.center_speed = asInt(get(m, "center_speed", "-1"));
ax.sw_mode = asInt(get(m, "sw_mode", "-1"));
ax.cfg_received = asInt(get(m, "cfg_rx", "-1"), -1);
ax.hold_corrections = asInt(get(m, "hold_corr", "0"));
ax.hold_peak_dev = asInt(get(m, "hold_peak", "0"));
ax.hold_cumulative_dev = asInt(get(m, "hold_cumul", "0"));

View File

@ -100,18 +100,32 @@ CheckResult checkDrivetrainAdopted(const std::string& axis, const DrivetrainExpe
return r;
}
auto mismatch = [&](const char* field, int wanted, long saw) {
// A field is compared only when it was actually echoed. Comparing against -1
// would report "firmware has -1", which reads as a wrong value rather than
// as a field this firmware does not report - and sends someone to re-send a
// CURRENT that was never the problem.
int missing = 0;
auto compare = [&](const char* field, int wanted, long saw) {
if (saw < 0) { ++missing; return; }
if (saw == wanted) return;
r.pass = false;
if (!r.detail.empty()) r.detail += "; ";
r.detail += std::string(field) + " wanted " + std::to_string(wanted) + ", firmware has " +
std::to_string(saw);
};
if (got.global_scaler != want.global_scaler)
mismatch("GLOBALSCALER", want.global_scaler, got.global_scaler);
if (got.run_irun != want.run_irun) mismatch("IRUN", want.run_irun, got.run_irun);
if (got.homing_irun != want.homing_irun)
mismatch("homing IRUN", want.homing_irun, got.homing_irun);
compare("GLOBALSCALER", want.global_scaler, got.global_scaler);
compare("IRUN", want.run_irun, got.run_irun);
compare("homing IRUN", want.homing_irun, got.homing_irun);
if (missing > 0 && r.pass) {
r.pass = false;
r.detail = axis + ": firmware echoes only part of the current (" +
std::to_string(missing) +
" of 3 fields absent), so it cannot be confirmed -- it predates the full "
"CURRENT echo. Re-flash it";
return r;
}
if (r.pass) {
r.detail = axis + ": firmware has the declared current (GLOBALSCALER " +
@ -125,4 +139,49 @@ CheckResult checkDrivetrainAdopted(const std::string& axis, const DrivetrainExpe
return r;
}
CheckResult checkDeclaredValues(const std::string& axis, const DeclaredValue* values, int count) {
CheckResult r;
int checked = 0, absent = 0;
for (int i = 0; i < count; ++i) {
const DeclaredValue& v = values[i];
if (!v.want) continue; // nothing declared: nothing to be wrong about
if (v.got < 0) {
// The firmware does not echo this field at all, which is a version
// skew rather than a mismatch - and it means the value cannot be
// confirmed either way, so it must not read as a pass.
++absent;
continue;
}
++checked;
if (v.got != v.value) {
r.pass = false;
if (!r.detail.empty()) r.detail += "; ";
r.detail += std::string(v.name) + " wanted " + std::to_string(v.value) +
", firmware has " + std::to_string(v.got);
}
}
if (checked == 0 && absent == 0) {
r.ran = false;
r.detail = axis + ": nothing declared for this axis beyond the current";
return r;
}
if (absent > 0 && r.pass) {
r.pass = false;
r.detail = axis + ": the firmware does not echo " + std::to_string(absent) +
" declared value(s), so what it is running on cannot be confirmed -- it "
"predates the config handshake. Re-flash it";
return r;
}
if (!r.pass) {
r.detail = axis + ": " + r.detail +
" -- the firmware is not running what hardware.ini declares. A SET was lost, "
"or the axis never left NEEDCFG";
return r;
}
r.detail = axis + ": firmware holds all " + std::to_string(checked) + " declared values";
return r;
}
} // namespace fgc

View File

@ -3,6 +3,7 @@
#include "fgc/Calibration.h" // unwrapNear
#include "fgc/DiagParser.h"
#include "fgc/AxisSpeed.h"
#include "fgc/HardwareChecks.h"
#include "fgc/HardwarePresence.h"
#include "fgc/ICameraSource.h"
@ -815,6 +816,31 @@ void verdict(TestSection& s, const std::string& name, const CheckResult& c) {
s.notes.push_back(name + ": " + c.detail);
}
// Pull one axis's echo out of a DUMP, or -1 when the axis or field is absent.
long axisField(const DumpData& d, char code, long DumpAxis::*field) {
for (const auto& ax : d.axes)
if (ax.axis == code) return ax.*field;
return -1;
}
long echoSteps(const DumpData& d, char c) { return axisField(d, c, &DumpAxis::steps_per_rev); }
long echoDev(const DumpData& d, char c) { return axisField(d, c, &DumpAxis::enc_deviation); }
long echoSwMode(const DumpData& d, char c) { return axisField(d, c, &DumpAxis::sw_mode); }
long echoHomingSpeed(const DumpData& d, char c) { return axisField(d, c, &DumpAxis::homing_speed); }
long echoCenterSpeed(const DumpData& d, char c) { return axisField(d, c, &DumpAxis::center_speed); }
long echoSpeed(const DumpData& d, char c) { return axisField(d, c, &DumpAxis::speed); }
// The VMAX a declared deg/s becomes on this axis, or 0 when it cannot be formed.
// Mirrors Application's fallback: the live calibration when it is trustworthy,
// otherwise the one the declaration implies.
long speedVmax(double deg_s, const AxisDrivetrainConfig& d, const AxisMap& map) {
if (deg_s <= 0.0) return 0;
const double live = std::fabs(map.counts_per_deg);
const double cpd = live >= kMinPlausibleCountsPerDeg ? map.counts_per_deg
: d.preHomeCountsPerDeg();
const auto p = planAxisSpeed(deg_s, cpd);
return p.ok ? p.vmax : 0;
}
std::string readSysfs(const std::string& path) {
if (path.empty()) return "";
std::ifstream f(path);
@ -915,6 +941,24 @@ void TestRunner::testHwMotor(TestReport& r) {
got.homing_irun = ax.homing_irun;
}
verdict(s, std::string(name) + "_current", checkDrivetrainAdopted(name, want, got));
// Everything else the host declares and the firmware is told. Before the
// config handshake these lived only in the firmware's own table, where a
// stale or defaulted value was invisible from here.
const AxisMap& map = (code == 'Y') ? hw_.geo_yaw : hw_.geo_pitch;
const long want_normal = speedVmax(cfg.scan_speed_deg_s, cfg, map);
const DeclaredValue vals[] = {
{"steps_per_rev", cfg.steps_per_rev > 0, cfg.steps_per_rev, echoSteps(d, code)},
{"enc_deviation", cfg.enc_deviation > 0, cfg.enc_deviation, echoDev(d, code)},
{"sw_mode", cfg.wiring_declared, (long)cfg.sw_mode, echoSwMode(d, code)},
{"homing_speed", cfg.homing_speed_deg_s > 0,
speedVmax(cfg.homing_speed_deg_s, cfg, map), echoHomingSpeed(d, code)},
{"center_speed", cfg.center_speed_deg_s > 0,
speedVmax(cfg.center_speed_deg_s, cfg, map), echoCenterSpeed(d, code)},
{"normal_speed", want_normal > 0, want_normal, echoSpeed(d, code)},
};
verdict(s, std::string(name) + "_declared",
checkDeclaredValues(name, vals, (int)(sizeof(vals) / sizeof(vals[0]))));
};
check_axis("yaw", 'Y', hw_.drive->yaw);
if (!hw_.declared || hw_.declared->hasAxis("pitch")) check_axis("pitch", 'P', hw_.drive->pitch);

View File

@ -165,3 +165,39 @@ TEST_CASE("a DUMP from firmware without the current fields reports -1, not zero"
CHECK(d.axes[0].run_irun == -1);
CHECK(d.axes[0].homing_irun == -1);
}
TEST_CASE("the host-owned drivetrain values are read back from DUMP") {
const std::string text =
"DUMP BEGIN build=abc uptime=1 mcusr=0x01 free_ram=4096\n"
"DUMP Y state=2 hsub=0 enabled=1 lim_neg=0 lim_pos=1000000 hold_target=0 speed=39738 "
"center=500000 soft_margin=5000 gs=80 irun=31 irun_home=7 steps_per_rev=1210778 "
"enc_dev=1500 homing_speed=9958 center_speed=29873 sw_mode=0 cfg_rx=0xFFE "
"hold_corr=0 hold_peak=0 hold_cumul=0 has_encoder=1\n"
"DUMP END\n";
auto d = fgc::parseDump(text);
REQUIRE(d.valid);
REQUIRE(d.axes.size() == 1);
const auto& a = d.axes[0];
CHECK(a.steps_per_rev == 1210778);
CHECK(a.enc_deviation == 1500);
CHECK(a.homing_speed == 9958);
CHECK(a.center_speed == 29873);
CHECK(a.sw_mode == 0);
CHECK(a.cfg_received == 0xFFE);
}
TEST_CASE("a firmware predating the handshake leaves those fields absent, not zero") {
// -1 keeps "did not say" distinguishable from "said zero" - sw_mode 0 is a
// legitimate setting on the yaw axis, so zero-as-absent would be a real bug.
const std::string text =
"DUMP BEGIN build=old uptime=1 mcusr=0x01 free_ram=4096\n"
"DUMP Y state=2 hsub=0 enabled=1 lim_neg=0 lim_pos=1000000 hold_target=0 speed=200000 "
"center=500000 soft_margin=5000 hold_corr=0 hold_peak=0 hold_cumul=0 has_encoder=1\n"
"DUMP END\n";
auto d = fgc::parseDump(text);
REQUIRE(d.valid);
REQUIRE(d.axes.size() == 1);
CHECK(d.axes[0].steps_per_rev == -1);
CHECK(d.axes[0].sw_mode == -1);
CHECK(d.axes[0].cfg_received == -1);
}

View File

@ -87,3 +87,90 @@ TEST_CASE("every mismatched field is reported, not just the first") {
CHECK(r.detail.find("IRUN wanted 31") != std::string::npos);
CHECK(r.detail.find("homing IRUN") != std::string::npos);
}
// --- the generalised adoption check (config-plan A.6) -----------------------
TEST_CASE("an axis with nothing declared beyond the current is not checked") {
const DeclaredValue none[] = {{"steps_per_rev", false, 0, -1}};
auto r = checkDeclaredValues("yaw", none, 1);
CHECK_FALSE(r.ran);
CHECK(r.pass);
}
TEST_CASE("every declared value matching is a pass that says how many") {
const DeclaredValue v[] = {
{"steps_per_rev", true, 1210778, 1210778},
{"enc_deviation", true, 1500, 1500},
{"normal_speed", true, 39738, 39738},
};
auto r = checkDeclaredValues("yaw", v, 3);
CHECK(r.ran);
CHECK(r.pass);
CHECK(r.detail.find("all 3") != std::string::npos);
}
TEST_CASE("a firmware that cannot echo a declared value fails, and is named a skew") {
// Not a mismatch: the value cannot be confirmed either way, and a check that
// did not happen must never read as a pass.
const DeclaredValue v[] = {
{"steps_per_rev", true, 1210778, -1},
{"enc_deviation", true, 1500, -1},
};
auto r = checkDeclaredValues("yaw", v, 2);
CHECK(r.ran);
CHECK_FALSE(r.pass);
CHECK(r.detail.find("does not echo 2") != std::string::npos);
CHECK(r.detail.find("Re-flash") != std::string::npos);
}
TEST_CASE("a lost SET is caught, and every wrong field is named") {
// The failure mode the handshake introduces: most SETs land, one does not,
// and the axis runs on a value nobody declared.
const DeclaredValue v[] = {
{"steps_per_rev", true, 1210778, 1210778},
{"enc_deviation", true, 1500, 177000}, // stale Delta value
{"homing_speed", true, 9958, 12000}, // firmware default
};
auto r = checkDeclaredValues("yaw", v, 3);
CHECK_FALSE(r.pass);
CHECK(r.detail.find("enc_deviation wanted 1500, firmware has 177000") != std::string::npos);
CHECK(r.detail.find("homing_speed wanted 9958, firmware has 12000") != std::string::npos);
CHECK(r.detail.find("NEEDCFG") != std::string::npos);
}
TEST_CASE("an undeclared value is skipped even when the firmware echoes one") {
// hardware.ini not declaring it means the firmware default IS the intent.
const DeclaredValue v[] = {
{"steps_per_rev", true, 1210778, 1210778},
{"sw_mode", false, 0, 3},
};
auto r = checkDeclaredValues("yaw", v, 2);
CHECK(r.pass);
CHECK(r.detail.find("all 1") != std::string::npos);
}
TEST_CASE("a partly echoed current is a version skew, not a wrong value") {
// The bug this caught: comparing an absent field against -1 reported
// "firmware has -1", which reads as a wrong value and sends someone to
// re-send a CURRENT that was never the problem.
DrivetrainExpectation want{true, 80, 31, 7};
DrivetrainEcho got;
got.global_scaler = 80; // echoed
got.run_irun = -1; // not echoed
got.homing_irun = -1;
auto r = checkDrivetrainAdopted("yaw", want, got);
CHECK_FALSE(r.pass);
CHECK(r.detail.find("2 of 3 fields absent") != std::string::npos);
CHECK(r.detail.find("firmware has -1") == std::string::npos);
}
TEST_CASE("a real mismatch alongside an absent field still names the mismatch") {
DrivetrainExpectation want{true, 80, 31, 7};
DrivetrainEcho got;
got.global_scaler = 255; // wrong
got.run_irun = 31; // right
got.homing_irun = -1; // absent
auto r = checkDrivetrainAdopted("yaw", want, got);
CHECK_FALSE(r.pass);
CHECK(r.detail.find("GLOBALSCALER wanted 80, firmware has 255") != std::string::npos);
}