From e2bdc8debf09961a8c7d2654d7d7958758eacef4 Mon Sep 17 00:00:00 2001 From: pgdalmeida Date: Mon, 21 Sep 2026 19:03:55 +0200 Subject: [PATCH] Trust the firmware's reset-cause name over a decode that cannot be right for both boards --- docs/modules-reference.md | 2 +- include/fgc/DumpParser.h | 12 ++++++++++++ src/core/DumpParser.cpp | 14 ++++++++++++-- tests/test_dumpparser.cpp | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/docs/modules-reference.md b/docs/modules-reference.md index ab09d40..ad9e25b 100644 --- a/docs/modules-reference.md +++ b/docs/modules-reference.md @@ -19,7 +19,7 @@ Per-file reference for the refactored tree, plus the shared data structures. | [include/fgc/HardwareChecks.h](../include/fgc/HardwareChecks.h), [src/core/HardwareChecks.cpp](../src/core/HardwareChecks.cpp) | The verdicts behind `test hardware`: `checkUsbfsMemory`, `checkUsb3Lpm`, `checkI2cFunctionality`, `checkDrivetrainAdopted`. Pure, so the thresholds that encode known-issues #2 and #19 cannot drift unnoticed | | [include/fgc/AxisSpeed.h](../include/fgc/AxisSpeed.h), [src/core/AxisSpeed.cpp](../src/core/AxisSpeed.cpp) | `planAxisSpeed` (deg/s -> TMC `VMAX` through `counts_per_deg`) + `vmaxToDegPerSec`. Pure arithmetic; refuses an uncalibrated scale rather than commanding a near-zero speed | | [include/fgc/FirmwareConfig.h](../include/fgc/FirmwareConfig.h), [src/core/FirmwareConfig.cpp](../src/core/FirmwareConfig.cpp) | `parseNeedLine` (firmware `NEED` line -> axis + keys) + `planFirmwareConfig` (-> the `SET` lines that answer it). Reports a key it cannot answer separately from one it does not recognise: incomplete `hardware.ini` vs. a firmware newer than the host | -| [include/fgc/DumpParser.h](../include/fgc/DumpParser.h), [src/core/DumpParser.cpp](../src/core/DumpParser.cpp) | `parseDump` firmware `DUMP` block → `DumpData` (per-axis state + decoded TMC registers/flags); `formatDump` | +| [include/fgc/DumpParser.h](../include/fgc/DumpParser.h), [src/core/DumpParser.cpp](../src/core/DumpParser.cpp) | `parseDump` firmware `DUMP` block → `DumpData` (per-axis state + decoded TMC registers/flags); `formatDump`. Prefers the firmware's own `reset=` name over any host-side decode - `mcusr` is an AVR bitmask on one board and a plain enum on the other, so a single table cannot be right for both | | [include/fgc/DiagParser.h](../include/fgc/DiagParser.h), [src/core/DiagParser.cpp](../src/core/DiagParser.cpp) | `parseDiag` firmware `DG` self-test stream → `DiagResult` (per-axis tests, PASS/FAIL); `formatDiag` | | [include/fgc/Calibration.h](../include/fgc/Calibration.h), [src/core/Calibration.cpp](../src/core/Calibration.cpp) | `linearFit` (least-squares, R²) + `circularMeanDeg` for the IMU-referenced calibration | | [include/fgc/PitchSetup.h](../include/fgc/PitchSetup.h), [src/core/PitchSetup.cpp](../src/core/PitchSetup.cpp) | `gimbal pitch setup` worker thread: home-if-needed → pitch sweep dwelling on the gravity-referenced IMU pitch → least-squares fit → park level; tunable via `CalibParams`; exposes `progress()`/`report()`/`takeResult()`. Yaw is not touched | diff --git a/include/fgc/DumpParser.h b/include/fgc/DumpParser.h index 55bdc56..30a3a7a 100644 --- a/include/fgc/DumpParser.h +++ b/include/fgc/DumpParser.h @@ -66,7 +66,19 @@ struct DumpData { bool valid = false; std::string build; long uptime_ms = 0; + // Why the MCU last restarted, as the firmware reported it. + // + // `mcusr` is the RAW value and means DIFFERENT THINGS per board: on AVR it is + // the MCUSR bitmask (bit 0 power-on, bit 1 external, ...); on RP2040 it is + // rp2040.getResetReason(), a plain enum where 3 is one value, not two bits. + // Decoding one as the other is silently wrong - it reported an RP2040 soft + // reset as "PORF (power-on), EXTRF (external)" on tower 121. + // + // So the firmware now NAMES it in the dump (`reset=`), and that text is + // authoritative because only the board knows its own encoding. reset_flags is + // the legacy AVR bit decode, kept for firmware that predates the field. unsigned mcusr = 0; + std::string reset_text; // empty => firmware did not name it long free_ram = 0; std::vector reset_flags; std::vector axes; diff --git a/src/core/DumpParser.cpp b/src/core/DumpParser.cpp index 44f4861..c43810b 100644 --- a/src/core/DumpParser.cpp +++ b/src/core/DumpParser.cpp @@ -119,6 +119,7 @@ DumpData parseDump(const std::string& block) { d.uptime_ms = asInt(get(hdr, "uptime", "0")); d.mcusr = static_cast(asInt(get(hdr, "mcusr", "0"))); d.free_ram = asInt(get(hdr, "free_ram", "0")); + d.reset_text = get(hdr, "reset"); d.reset_flags = bitsSet(d.mcusr, kMcusr); // Per-axis lines: "DUMP state=.." and "DUMP TMC ..". @@ -199,8 +200,17 @@ std::vector formatDump(const DumpData& d) { } { std::ostringstream os; - os << "reset cause (mcusr=0x" << std::hex << d.mcusr << std::dec << "): " - << join(d.reset_flags); + os << "reset cause (0x" << std::hex << d.mcusr << std::dec << "): "; + if (!d.reset_text.empty()) { + // The firmware named it, so no guessing is involved. + os << d.reset_text; + } else { + // Firmware predating the `reset=` field. The only decode available is + // the AVR one, and on a non-AVR board it is wrong - so say that it is + // an assumption rather than stating it as fact. + os << join(d.reset_flags) << " [assumed AVR MCUSR bits; firmware does " + "not name it]"; + } out.push_back(os.str()); } for (const auto& ax : d.axes) { diff --git a/tests/test_dumpparser.cpp b/tests/test_dumpparser.cpp index af4b6d0..afa39c5 100644 --- a/tests/test_dumpparser.cpp +++ b/tests/test_dumpparser.cpp @@ -223,3 +223,40 @@ TEST_CASE("the numeric state codes did not shift when NEEDCFG was added") { CHECK(one(4) == "ERROR"); CHECK(one(5) == "NEEDCFG"); } + +TEST_CASE("the firmware's own name for the reset cause wins over any decode") { + // mcusr means different things per board: an AVR MCUSR bitmask, or the + // RP2040's getResetReason() enum. 0x3 is "two bits set" on one and a single + // enum value on the other, so a host-side table cannot be right for both. + const std::string text = + "DUMP BEGIN build=x uptime=1 mcusr=0x3 reset=SOFT_RESET free_ram=4096\n" + "DUMP Y state=1 hsub=0 enabled=0 lim_neg=0 lim_pos=0 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"; + auto d = fgc::parseDump(text); + REQUIRE(d.valid); + CHECK(d.reset_text == "SOFT_RESET"); + + std::string out; + for (const auto& l : fgc::formatDump(d)) out += l + "\n"; + CHECK(out.find("SOFT_RESET") != std::string::npos); + // The AVR names must not appear when the firmware said what it was. + CHECK(out.find("PORF") == std::string::npos); + CHECK(out.find("EXTRF") == std::string::npos); +} + +TEST_CASE("without that field the AVR decode is labelled as an assumption") { + const std::string text = + "DUMP BEGIN build=old uptime=1 mcusr=0x3 free_ram=4096\n" + "DUMP Y state=1 hsub=0 enabled=0 lim_neg=0 lim_pos=0 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"; + auto d = fgc::parseDump(text); + REQUIRE(d.valid); + CHECK(d.reset_text.empty()); + + std::string out; + for (const auto& l : fgc::formatDump(d)) out += l + "\n"; + CHECK(out.find("PORF") != std::string::npos); // still decoded + CHECK(out.find("assumed AVR") != std::string::npos); // but not stated as fact +}