From b5d1fac4cb7d084cd0196be8c93ccd0a78af2341 Mon Sep 17 00:00:00 2001 From: pgdalmeida Date: Fri, 18 Sep 2026 15:44:19 +0200 Subject: [PATCH] Declare axis speed in degrees per second, not VMAX --- CMakeLists.txt | 4 +- config/config.example.ini | 9 + config/hardware.example.ini | 11 ++ docs/configuration.md | 41 +++++ docs/hardware-config-plan.md | 29 +++- docs/modules-reference.md | 2 + docs/test-command.md | 47 ++++++ include/fgc/AxisSpeed.h | 72 ++++++++ include/fgc/Config.h | 14 ++ include/fgc/HardwareChecks.h | 71 ++++++++ include/fgc/TestRunner.h | 48 +++++- include/fgc/sensors/I2cBus.h | 6 + src/core/Application.cpp | 54 +++++- src/core/AxisSpeed.cpp | 73 +++++++++ src/core/Config.cpp | 3 + src/core/HardwareChecks.cpp | 128 +++++++++++++++ src/core/HelpText.cpp | 7 + src/core/TestRunner.cpp | 298 +++++++++++++++++++++++++++++++++- src/sensors/I2cBus.cpp | 5 + tests/CMakeLists.txt | 2 + tests/test_axisspeed.cpp | 113 +++++++++++++ tests/test_hardwarechecks.cpp | 89 ++++++++++ tests/test_testrunner.cpp | 41 +++++ 23 files changed, 1149 insertions(+), 18 deletions(-) create mode 100644 include/fgc/AxisSpeed.h create mode 100644 include/fgc/HardwareChecks.h create mode 100644 src/core/AxisSpeed.cpp create mode 100644 src/core/HardwareChecks.cpp create mode 100644 tests/test_axisspeed.cpp create mode 100644 tests/test_hardwarechecks.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 455cada..02f724d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -65,6 +65,8 @@ add_library(fgc_core STATIC src/core/Geometry.cpp src/core/TmcCurrent.cpp src/core/HardwarePresence.cpp + src/core/HardwareChecks.cpp + src/core/AxisSpeed.cpp src/core/ScanGrid.cpp src/core/TelemetryParser.cpp src/core/CaptureScheduler.cpp @@ -85,6 +87,7 @@ add_library(fgc_core STATIC src/core/Hash.cpp src/core/ChannelHistory.cpp src/core/ChannelTopics.cpp + src/sensors/I2cBus.cpp src/sensors/Sht41Protocol.cpp src/ui/UiSnapshot.cpp src/ui/Scroll.cpp @@ -111,7 +114,6 @@ set(FGC_SOURCES src/camera/ImagePipeline.cpp src/serial/SerialMotorController.cpp src/serial/MtiImuSource.cpp - src/sensors/I2cBus.cpp src/sensors/Sht41EnvSensor.cpp ) if(WITH_MQTT) diff --git a/config/config.example.ini b/config/config.example.ini index 147d9dc..fdac21a 100644 --- a/config/config.example.ini +++ b/config/config.example.ini @@ -243,6 +243,15 @@ mock_env = false ; here without code changes). profile = standard baseline_file = +; `test hardware camera` USB preconditions. Both are facts about the machine, not +; tuning. usbfs_memory_mb reverts to 16 on every reboot unless it is on the kernel +; cmdline, and frame delivery stalls below ~1000 (known-issues #19). +usbfs_min_mb = 1000 +; The camera port's usb3_lpm_permit node. The path differs per board, so it cannot +; be guessed: left empty, the LPM check reports "not checked" rather than passing. +; Find it with the procedure in known-issues.md, e.g. +; usb3_lpm_path = /sys/devices/.../usb2/2-0:1.0/usb2-port3/usb3_lpm_permit +usb3_lpm_path = [TestProfile.standard] reps = 5 ; repetitions for homing / friction / balance diff --git a/config/hardware.example.ini b/config/hardware.example.ini index b64d3ba..1b0b461 100644 --- a/config/hardware.example.ini +++ b/config/hardware.example.ini @@ -112,6 +112,17 @@ homing_current_ma = 200 ; ROUGH until somebody measures it. Every bearing inherits its error ; proportionally, until 'gimbal heading setup' supersedes it with a landmark fit. travel_deg = 340 +; Production move speed, IN DEGREES PER SECOND - not VMAX. +; +; A register value is not portable: VMAX 30000 is 21.8 deg/s on the Delta fleet +; and 6.0 deg/s on the IOTA, because their gear ratios differ by 3.6x. Both were +; carrying the same literal, which is how two towers ended up sweeping at +; completely different speeds while their configs looked identical. +; +; The host converts through yaw_counts_per_deg and sends SPEED after each home, +; so this follows the calibration automatically. An axis whose scale is not +; trustworthy is left on the firmware's default and the reason is logged. +scan_speed_deg_s = 8.0 [Camera.rgb] ; The serial of the physical camera fitted to this tower. A USB DEV_ id, or a diff --git a/docs/configuration.md b/docs/configuration.md index 4bccbfa..51dde38 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -74,6 +74,7 @@ Parsed and validated by `ConfigLoader` ([src/core/Config.cpp](../src/core/Config | `Motor.` | `r_sense_ohm` | float | — | Sense resistor on that axis's TMC5160 board. **hardware.ini.** Makes the milliamp figures below mean something | | `Motor.` | `run_current_ma` / `homing_current_ma` | float | — | Coil current (mA RMS) for normal motion, and while hunting an endstop. **hardware.ini** | | `Motor.` | `travel_deg` | float | — | Mechanical angle between the hard endstops. **hardware.ini.** Homing divides the measured travel by it to derive `_counts_per_deg` | +| `Motor.` | `scan_speed_deg_s` | float | `8.0` | Production move speed **in degrees per second**. **hardware.ini.** The host converts it to VMAX through `_counts_per_deg` and sends `SPEED`; a register value would not mean the same thing on two drivetrains | | `Scan` | `grid_file` | string | — | CSV of `yaw_deg,pitch_deg` waypoints; empty → generate | | `Scan` | `yaw_intervals` | int | `56` | Generated yaw positions across `[yaw_min_deg, yaw_max_deg]` | | `Scan` | `yaw_min_deg`/`yaw_max_deg` | float | `-90`/`90` | Generated yaw arc | @@ -663,6 +664,46 @@ host checks them **before** homing. If an axis declares a reduced homing current firmware has not adopted it, the host **refuses to home** rather than driving into a stop at full torque — the fire it is there to prevent. +### Why speed is declared in degrees per second + +`VMAX` is a register value, and what it *means* depends on the gear ratio: + +| | Tower 111 (Delta) | Tower 121 (IOTA) | +|---|---|---| +| `counts_per_deg` | 983.33 | 3552.87 | +| `VMAX 30000` is | **21.8 °/s** | **6.0 °/s** | + +Both towers were carrying the same literal `30000`, running at speeds differing by +3.6× — exactly their gear-ratio difference. A register value copied between towers +produces a silently different machine, which is why the speed is declared as the +physical quantity and converted per tower. + +The conversion is `v[counts/s] = VMAX × f_CLK / 2²⁴` with `f_CLK` = 12 MHz, divided +by `counts_per_deg`. That single factor suffices because the firmware keeps +`XACTUAL` (step space) and `XENC` (encoder space) on the **same scale** — see +`firmware/docs/hardware-notes.md`, *"Encoders track 1:1 but must be aligned to +XACTUAL"*. The 0.7153 counts/s per VMAX unit this predicts is within 2% of the 0.73 +measured across three homing runs on tower 121. + +The speed is sent at the end of the init sequence, **after** `counts_per_deg` has +been re-derived from the travel just homed, so it always uses the current scale. +What is logged is what the axis will actually do, which differs from the request by +the integer quantisation: + +``` +yaw speed: 8.00012 deg/s (VMAX 32897) -- 2941.18 counts/deg +``` + +An axis whose scale is not trustworthy is **not commanded at all** — the firmware +keeps its compile-time default and the reason is logged. The floor is physical: +the AS5047P emits 4000 steps/rev in ABI mode, so even an ungeared axis reads 11.1 +counts/deg, and anything below that is an uncalibrated axis still carrying +`AxisMap`'s default of 1.0 rather than a real drivetrain. + +> **This replaced two hardcoded literals** — `SPEED Y 50000` and `SPEED P 150000` +> — which contradicted the firmware's own tuned-down defaults and won. On the +> Delta fleet that was 36.4 °/s against an intended 21.8 °/s. + ### Deriving `counts_per_deg` from the homed travel `travel_deg` is a nameplate fact: the mechanical angle between the stops. Homing measures the diff --git a/docs/hardware-config-plan.md b/docs/hardware-config-plan.md index 7fa4f1c..b0f025a 100644 --- a/docs/hardware-config-plan.md +++ b/docs/hardware-config-plan.md @@ -384,7 +384,7 @@ a yaw-only tower, captures one elevation band, and reports itself healthy. | Today | After | |---|---| -| **Production speeds hardcoded in `Application.cpp:1040` contradict `firmware/src/config.h`** — and win | one value in `hardware.ini`, requested by the firmware | +| ~~Production speeds hardcoded in `Application.cpp` contradict `firmware/src/config.h` — and win~~ **fixed** | `[Motor.] scan_speed_deg_s` in `hardware.ini`, in degrees per second, converted to VMAX per tower | | Firmware `MOTOR_CONFIG` duplicates every drivetrain value the host also knows | firmware holds none of them until told | | Firmware `NUM_AXES` hardcoded; host infers axes from telemetry | generated from `[Hardware] axes` | | `[Features] enable_imu` / `enable_env` | derived from `imu` / `env`; the flags retire | @@ -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`). -247 tests pass, up from a 204 baseline. +269 tests pass, up from a 204 baseline. | § | Item | State | |----|------|-------| @@ -436,11 +436,19 @@ Branch `worktree-hardware-config` (worktree at `.claude/worktrees/hardware-confi | 4 | Resolution chain `--hardware` / `$FGC_HARDWARE` / `/etc/...` / beside `config.ini` | **done**, 2 tests | | 8 | `config/hardware.example.ini` documenting the shape | **done**, and the `hardware/lattepanda-iota/` setup kit now exists too (`install.sh`, `packages.txt`, three precondition files) | | 10 | Presence reconciliation (declared vs found) | **done**, 13 tests. `HardwarePresence.h` compares `[Hardware]` against what came up; warn-only by default, refusing under `[Features] strict_presence` with `--degraded` as the field-triage escape (decision 4) | -| 11 | `test hardware` | **not started** | -| 5 | Delete the `Application.cpp:1040` speed literals | **not started** — blocked on decision 2 | +| 11 | `test hardware` | **done**, 11 tests. Five leaves (`axes` / `motor` / `imu` / `env` / `camera`) under a new `hardware` subsystem; included in a bare `test`, and deliberately not gated on a homed gimbal. Nothing declared reports SKIP, never PASS | +| 5 | Delete the `Application.cpp` speed literals | **done**, 13 tests. Decision 2 answered: speed is declared as `[Motor.] 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 | -Nothing yet changes behaviour on a tower without a `hardware.ini`: absent is silent, and -`hardware.declared` stays false. Verified by running the binary both ways. +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. + +> **§5 is the exception, and it is deliberate.** `scan_speed_deg_s` defaults to **8.0 deg/s** and +> applies whether or not a `hardware.ini` exists, because the literals it replaced were themselves +> unconditional. **Every tower changes speed on upgrade**: the Delta fleet goes from 36.4 deg/s to +> 8.0, so a full revolution takes ~45 s instead of ~16.5 s. Check that against `image_interval` and +> the scan grid's waypoint count before deploying, and set `scan_speed_deg_s` per tower if 8.0 is +> not right for it. An axis whose `counts_per_deg` is untrustworthy is left on the firmware default +> instead. --- @@ -448,9 +456,12 @@ Nothing yet changes behaviour on a tower without a `hardware.ini`: absent is sil 1. **How do the files find each other?** Recommendation: a fixed `/etc/fire_gimbal_control/hardware.ini`, matching the existing `credentials.env` convention, rather than a pointer in `config.ini` (§4). -2. **The `SPEED Y 50000` / `SPEED P 150000` literals in `Application.cpp:1040` contradict the firmware's - tuned-down defaults and win.** Is the intended yaw production speed 30000 or 50000? Live on the fleet - now, and worth answering before the refactor (§5). +2. ~~**The `SPEED Y 50000` / `SPEED P 150000` literals contradict the firmware's tuned-down defaults and + win.** Is the intended yaw production speed 30000 or 50000?~~ **ANSWERED, and the question turned out to + be mis-framed.** 30000 was intended *for tower 111* — but a VMAX is not a portable quantity: the same + 30000 is 21.8 deg/s on the Delta and 6.0 deg/s on the IOTA. Speed is now declared in **degrees per + second** (`[Motor.] scan_speed_deg_s`, default 8.0) and converted per tower through + `counts_per_deg`. The literals were 36.4 deg/s. 3. **`NEEDCFG` — refuse to move, or fall back after a timeout?** Recommendation: refuse, with an explicit `USE DEFAULTS` for bench work. A timeout lets a slow host produce a silently mis-tuned tower (§6). 4. **How strict is reconciliation, and when?** Recommendation: warn-only through the fleet rollout, then diff --git a/docs/modules-reference.md b/docs/modules-reference.md index 94918ac..ecf309d 100644 --- a/docs/modules-reference.md +++ b/docs/modules-reference.md @@ -16,6 +16,8 @@ Per-file reference for the refactored tree, plus the shared data structures. | [include/fgc/HelpText.h](../include/fgc/HelpText.h), [src/core/HelpText.cpp](../src/core/HelpText.cpp) | Operator command catalog (`helpCatalog`) + `renderHelp` (console & TUI help) | | [include/fgc/TmcCurrent.h](../include/fgc/TmcCurrent.h), [src/core/TmcCurrent.cpp](../src/core/TmcCurrent.cpp) | Milliamps → TMC5160 `GLOBALSCALER`/`IRUN` register fields, using the sense resistor from `[Motor.]`. Pure arithmetic, so it is unit-tested; the host owns the conversion because only it knows the resistor | | [include/fgc/HardwarePresence.h](../include/fgc/HardwarePresence.h), [src/core/HardwarePresence.cpp](../src/core/HardwarePresence.cpp) | `reconcilePresence` (what `hardware.ini` declares vs what actually came up) + `presenceVerdict` (proceed / warn / refuse, per `strict_presence` and `--degraded`). Pure comparison, so the policy is unit-tested without a rig | +| [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/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/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 | diff --git a/docs/test-command.md b/docs/test-command.md index 46dde78..0ae7238 100644 --- a/docs/test-command.md +++ b/docs/test-command.md @@ -29,6 +29,7 @@ token that names a configured profile selects the profile at any position. | `test gimbal homing` | just homing | | `test imu drift` | just IMU yaw drift | | `test host` | host thermal/disk/memory/load (no hardware needed) | +| `test hardware` | is this rig the one `hardware.ini` declares? (no homing needed) | | `test gimbal encoder strict` | one leaf, `strict` profile | | `test strict` | everything, `strict` profile | @@ -52,16 +53,62 @@ While a test runs, interfering commands (moves, homing, a second test, capture, | `host` | `disk` | free space on the image partition (`statvfs`) | | `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` | `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 | Gimbal leaves require a homed gimbal (`home` first); IMU leaves require the IMU (`[Features] enable_imu`). `host` needs neither, so `test host` doubles as a fast non-intrusive smoke check. +**`hardware` deliberately does not need a homed gimbal either** — it reads telemetry +and `DUMP` and moves nothing, because it is what you run on a tower that has just +been installed and *before* you trust it enough to home it. + > Some metrics are most accurate with firmware support (a homing-report `HM` line > and a load-capture `LP` primitive); until those land the modules fall back to > sampling the `ST` telemetry stream and note it in the report. See > [test-roadmap.md](test-roadmap.md). +### What `test hardware` is for + +The other subsystems measure things and trend them. `hardware` asks a different +question — **is this rig the one `hardware.ini` says it is?** — so its metrics are +1/0 verdicts and the sentence that matters lives in the notes: + +``` +[hw_motor] FAIL 0 ms # firmware adopted the declared drivetrain + yaw_current = 0 FAIL + ; yaw_current: yaw: firmware did not echo gs/irun/irun_home -- it predates the + CURRENT command, so whatever hardware.ini asks for is NOT being applied. Re-flash it + ; pitch_current: not checked -- no [Motor.pitch] current declared +``` + +Three rules hold across every leaf: + +- **Nothing declared reports `SKIP`, never `PASS`.** A check that did not happen + must not look like one that succeeded — that is how a precondition regression + survives a green run. An un-migrated tower with no `hardware.ini` skips the lot + and still reports `all_pass`. +- **Unreadable is a failure.** An empty or unparseable `usbfs_memory_mb` fails + rather than passing by default, for the same reason. +- **The detail names the fix, not the symptom.** "SMBus-only, point `[Env] + i2c_device` at a real adapter" rather than "sensor not responding" — the two + look identical from the sensor's side, and the distinction is what + known-issues #2 cost to learn. + +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 +case that destroys an endstop. + +| Key | Default | For | +|---|---|---| +| `[Test] usbfs_min_mb` | `1000` | What `usbfs_memory_mb` must reach. It reverts to 16 on every reboot unless it is on the kernel cmdline | +| `[Test] usb3_lpm_path` | *(empty)* | The camera port's `usb3_lpm_permit` node. Differs per board, so it cannot be guessed; empty means the LPM check reports "not checked" rather than passing | + ## Profiles A profile is a named bundle of repetition counts, sampling windows and pass diff --git a/include/fgc/AxisSpeed.h b/include/fgc/AxisSpeed.h new file mode 100644 index 0000000..7b7de04 --- /dev/null +++ b/include/fgc/AxisSpeed.h @@ -0,0 +1,72 @@ +#pragma once + +#include + +namespace fgc { + +// Axis speed as a physical quantity, and its conversion to the TMC5160's VMAX. +// +// VMAX is a register value in internal units, and what it MEANS depends on the +// drivetrain: 30000 is 21.8 deg/s on the Delta fleet and 6.0 deg/s on the IOTA +// tower, because their gear ratios differ by 3.6x. Copying the number between +// towers therefore produces a silently different machine, which is exactly what +// happened - both carry the same literal. +// +// Declaring degrees per second instead makes the intent portable and checkable: +// it is the quantity that determines image smear and panorama period, and it can +// be reasoned about without knowing a gear ratio. +// +// The conversion needs only counts_per_deg, which the host already derives per +// tower from the homed travel. That works because the firmware keeps XACTUAL +// (step space) and XENC (encoder space) on the SAME scale - see +// firmware/docs/hardware-notes.md, "Encoders track 1:1 but must be aligned to +// XACTUAL". If that ever stops being true, this conversion breaks and the step +// scale becomes a separate calibration. + +// The TMC5160 ramp generator: v[counts/s] = VMAX * f_CLK / 2^24. +// +// 12 MHz is the internal oscillator's nominal frequency. Confirmed on the rig +// rather than taken on faith: the predicted 0.7153 counts/s per VMAX unit is +// within 2% of the 0.73 measured across three homing runs on tower 121. +constexpr double kTmcClockHz = 12'000'000.0; +constexpr double kTmcVmaxDiv = 16'777'216.0; // 2^24 +// VMAX is a 23-bit field. +constexpr long kTmcVmaxMax = 0x7FFFFF; + +// The fleet default, in the unit that means the same thing on every tower. +constexpr double kDefaultScanSpeedDegPerSec = 8.0; + +// Below this, counts_per_deg is not a drivetrain - it is an uncalibrated axis +// still carrying AxisMap's default of 1.0. +// +// The floor is physical, not arbitrary: the AS5047P in ABI mode emits 4000 steps +// per revolution (COMPONENTS.md section 7), so even an axis driven with NO gear +// reduction reads 11.1 counts/deg. Anything under that cannot have come from a +// real encoder on this rig. Converting a speed through such a scale yields a +// VMAX near zero - an axis that looks configured and never finishes a sweep - +// so it is refused rather than commanded. +constexpr double kMinPlausibleCountsPerDeg = 10.0; + +struct AxisSpeedPlan { + bool ok = false; + long vmax = 0; + double requested_deg_s = 0.0; + // What `vmax` actually delivers. VMAX is an integer, so this differs from the + // request by the quantisation - negligible at production speeds, but it is + // what the startup log should report, because it is what the axis does. + double actual_deg_s = 0.0; + bool clamped = false; + std::string error; // why, when !ok +}; + +// `counts_per_deg` may be negative to flip the direction sense (see Geometry.h); +// only its magnitude matters to a speed, so the sign is ignored rather than +// producing a negative VMAX. +AxisSpeedPlan planAxisSpeed(double deg_per_s, double counts_per_deg, + double clock_hz = kTmcClockHz); + +// The inverse, for reporting what a firmware default or an existing literal +// actually amounts to on this drivetrain. +double vmaxToDegPerSec(long vmax, double counts_per_deg, double clock_hz = kTmcClockHz); + +} // namespace fgc diff --git a/include/fgc/Config.h b/include/fgc/Config.h index ed1a4f0..084b961 100644 --- a/include/fgc/Config.h +++ b/include/fgc/Config.h @@ -77,6 +77,13 @@ struct AxisDrivetrainConfig { // key would silently discard every saved calibration. double travel_deg = 0.0; // 0 = undeclared -> nothing is derived + // Production move speed, as a PHYSICAL quantity. VMAX is a register value + // whose meaning depends on the gear ratio - 30000 is 21.8 deg/s on the Delta + // fleet and 6.0 deg/s on the IOTA - so declaring it in register units makes + // the number unportable and, in practice, wrong when copied between towers. + // The host converts through counts_per_deg and sends SPEED; see AxisSpeed.h. + double scan_speed_deg_s = 8.0; // kDefaultScanSpeedDegPerSec + // True when the homing current is genuinely lower than the run current, i.e. // when the reduction is load-bearing and homing without it risks the rig. bool reducesForHoming() const { @@ -246,6 +253,13 @@ struct TestConfig { std::string baseline_file; // empty => /test_baseline.txt std::map profiles; + // `test hardware camera` USB preconditions. Both are machine facts rather + // than tuning: the threshold is what frame delivery needs (known-issues #19), + // and the LPM node's path differs per board, so it cannot be guessed. Left + // empty, the LPM check reports "not checked" rather than passing. + int usbfs_min_mb = 1000; + std::string usb3_lpm_path; + const TestProfile* find(const std::string& name) const; // nullptr if absent }; diff --git a/include/fgc/HardwareChecks.h b/include/fgc/HardwareChecks.h new file mode 100644 index 0000000..991ed5a --- /dev/null +++ b/include/fgc/HardwareChecks.h @@ -0,0 +1,71 @@ +#pragma once + +#include + +namespace fgc { + +// The decisions behind `test hardware`, separated from the I/O that feeds them. +// +// Each function takes what was read - a sysfs string, an ioctl bitmask, a DUMP +// echo - and returns a verdict with the sentence an operator needs. Keeping them +// pure is what lets the thresholds be tested without a rig, which matters because +// several of them encode traps that cost real debugging time (known-issues #2 +// and #19) and must not drift silently. + +struct CheckResult { + bool ran = true; // false => nothing was declared, so nothing was checked + bool pass = true; + std::string detail; // one line; names the fix when it failed +}; + +// /sys/module/usbcore/parameters/usbfs_memory_mb. +// +// Reverts to 16 on every reboot unless persisted on the kernel cmdline, and +// frame delivery stalls below ~1000 regardless of application code +// (known-issues #19). A value that will not parse is a failure, not a pass: +// an unreadable node means the check did not happen. +CheckResult checkUsbfsMemory(const std::string& sysfs_value, int min_mb); + +// A usb3_lpm_permit node. U1/U2 link power management must be OFF on the +// camera's port or the camera logs frame drops under load; the per-device +// usb3_hardware_lpm_u* files are read-only, so this port-level node is the one +// that can actually be set (known-issues #19). +CheckResult checkUsb3Lpm(const std::string& permit_value); + +// The I2C_FUNCS ioctl bitmask for the bus the SHT41 is on. +// +// An SMBus-only adapter opens, binds its slave address, and then fails every +// transaction - which reads as a dead sensor rather than as the wrong bus +// (known-issues #2). `i2cdetect -F ` is the manual form of this check. +// +// The bit is spelled out rather than pulled from so this stays a +// pure module: I2C_FUNC_I2C is 0x00000001 and is ABI-stable. +constexpr unsigned long kI2cFuncI2c = 0x00000001UL; +CheckResult checkI2cFunctionality(unsigned long funcs); + +// What the host computed from hardware.ini for one axis. +struct DrivetrainExpectation { + bool declared = false; // hardware.ini carries a current for this axis + int global_scaler = 0; + int run_irun = 0; + int homing_irun = 0; +}; + +// 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. +struct DrivetrainEcho { + long global_scaler = -1; + long run_irun = -1; + long homing_irun = -1; + + bool absent() const { return global_scaler < 0 && run_irun < 0 && homing_irun < 0; } +}; + +// Did the firmware adopt what the host sent? Both registers are write-only on +// the TMC5160, so this echo is the only confirmation available - and without it +// a tower can run its whole life on limp-home defaults while reporting healthy. +CheckResult checkDrivetrainAdopted(const std::string& axis, const DrivetrainExpectation& want, + const DrivetrainEcho& got); + +} // namespace fgc diff --git a/include/fgc/TestRunner.h b/include/fgc/TestRunner.h index d6debe8..926d055 100644 --- a/include/fgc/TestRunner.h +++ b/include/fgc/TestRunner.h @@ -15,6 +15,10 @@ namespace fgc { class IMotorController; class IImuSource; +class IEnvSensor; +class ICameraSource; +struct HardwareConfig; +struct DrivetrainConfig; // Leaf-test selection bitmask. Subsystem masks are unions of their leaves. enum TestLeaf : uint32_t { @@ -30,14 +34,27 @@ enum TestLeaf : uint32_t { T_HOST_DISK = 1u << 9, T_HOST_MEMORY = 1u << 10, T_HOST_LOAD = 1u << 11, + // `test hardware`: is this rig the one hardware.ini says it is? These are + // preconditions rather than measurements - each either holds or does not, so + // they carry pass/fail metrics rather than values to trend. + T_HW_AXES = 1u << 12, + T_HW_MOTOR = 1u << 13, + T_HW_IMU = 1u << 14, + T_HW_ENV = 1u << 15, + T_HW_CAMERA = 1u << 16, }; constexpr uint32_t T_GIMBAL_ALL = T_GIMBAL_HOMING | T_GIMBAL_ENCODER | T_GIMBAL_FRICTION | T_GIMBAL_BACKLASH | T_GIMBAL_BALANCE; constexpr uint32_t T_IMU_ALL = T_IMU_CONFIG | T_IMU_HEALTH | T_IMU_DRIFT; constexpr uint32_t T_HOST_ALL = T_HOST_THERMAL | T_HOST_DISK | T_HOST_MEMORY | T_HOST_LOAD; -constexpr uint32_t T_ALL = T_GIMBAL_ALL | T_IMU_ALL | T_HOST_ALL; +constexpr uint32_t T_HW_ALL = T_HW_AXES | T_HW_MOTOR | T_HW_IMU | T_HW_ENV | T_HW_CAMERA; +constexpr uint32_t T_ALL = T_GIMBAL_ALL | T_IMU_ALL | T_HOST_ALL | T_HW_ALL; // Which leaf masks need real motor / IMU hardware (used by the caller to guard). +// Needs a HOMED gimbal, not merely a motor link: the caller refuses the whole +// run when it is not ready. The `test hardware` leaves are deliberately NOT +// here - they only read telemetry and DUMP, and they are the checks you want +// on a tower that has not been homed yet. constexpr uint32_t T_NEEDS_MOTOR = T_GIMBAL_ALL; constexpr uint32_t T_NEEDS_IMU = T_IMU_ALL; @@ -47,6 +64,26 @@ constexpr uint32_t T_NEEDS_IMU = T_IMU_ALL; bool resolveTestSelection(const std::string& subsystem, const std::string& test, uint32_t& mask, std::string& err); +// Everything the `test hardware` leaves need beyond the motor and IMU the runner +// already holds. Passed as one struct because the alternative is five more +// constructor parameters, and because every field is optional: a leaf whose +// input is absent reports "not run" rather than failing. +struct HardwareTestContext { + const HardwareConfig* declared = nullptr; // [Hardware], or null when undeclared + const DrivetrainConfig* drive = nullptr; // [Motor.], for the current echo + IEnvSensor* env = nullptr; + ICameraSource* camera = nullptr; + + std::vector configured_cameras; // [Camera.] blocks in open order + std::string i2c_device; // [Env] i2c_device, for the adapter-class check + double r_sense_ohm = 0.0; // to recompute what CURRENT should have sent + + // sysfs nodes, overridable so the checks are exercisable off a tower. + std::string usbfs_path = "/sys/module/usbcore/parameters/usbfs_memory_mb"; + std::string lpm_path; // empty => discovered from the camera's USB port, or skipped + int usbfs_min_mb = 1000; +}; + // Live progress for the activity strip (mirrors CalibProgress). struct TestProgress { bool running = false; @@ -64,7 +101,8 @@ struct TestProgress { class TestRunner { public: TestRunner(IMotorController& motor, IImuSource* imu, Geometry geo, - uint32_t selection, TestProfile profile, std::string disk_path); + uint32_t selection, TestProfile profile, std::string disk_path, + HardwareTestContext hw = {}); ~TestRunner(); bool start(); @@ -89,6 +127,11 @@ private: void testImuHealth(TestReport& r); void testImuDrift(TestReport& r); void testHost(TestReport& r); + void testHwAxes(TestReport& r); + void testHwMotor(TestReport& r); + void testHwImu(TestReport& r); + void testHwEnv(TestReport& r); + void testHwCamera(TestReport& r); IMotorController& motor_; IImuSource* imu_; @@ -96,6 +139,7 @@ private: uint32_t selection_; TestProfile profile_; std::string disk_path_; + HardwareTestContext hw_; std::thread thread_; std::atomic running_{false}; diff --git a/include/fgc/sensors/I2cBus.h b/include/fgc/sensors/I2cBus.h index 88db68e..a3ff87a 100644 --- a/include/fgc/sensors/I2cBus.h +++ b/include/fgc/sensors/I2cBus.h @@ -33,6 +33,12 @@ public: const std::string& device() const { return device_; } uint8_t address() const { return addr_; } + // The adapter's I2C_FUNCS bitmask, i.e. what class of transactions this bus + // can actually do. An SMBus-only adapter opens and binds exactly like a real + // one and then fails every transfer, so this is the difference between "the + // sensor is dead" and "this was never an I2C bus". Requires an open bus. + bool functionality(unsigned long& funcs) const; + private: std::string device_; uint8_t addr_; diff --git a/src/core/Application.cpp b/src/core/Application.cpp index 11dbf1f..953bb31 100644 --- a/src/core/Application.cpp +++ b/src/core/Application.cpp @@ -9,6 +9,7 @@ #include "fgc/HelpText.h" #include "fgc/Paths.h" #include "fgc/CameraProfile.h" +#include "fgc/AxisSpeed.h" #include "fgc/HardwarePresence.h" #include "fgc/TmcCurrent.h" #include "fgc/ICameraSource.h" @@ -1137,12 +1138,45 @@ struct Application::Impl { "counts_per_deg both skipped"; } } - // Production move speeds for subsequent MOVE commands. - motor->sendCommand("SPEED Y 50000"); - if (cfg.hardware.hasAxis("pitch")) motor->sendCommand("SPEED P 150000"); + // Production move speeds for subsequent MOVE commands, converted from the + // declared deg/s. Sent here rather than earlier because deriveCountsPerDeg() + // above has just refreshed the scale this conversion depends on. + sendProductionSpeeds(); LOG_INFO << "Gimbal init finished"; } + // Translate each axis's declared deg/s into the VMAX this drivetrain needs, + // and tell the firmware. + // + // The speed is declared as a physical quantity because a register value is + // not portable: the same VMAX is 21.8 deg/s on the Delta fleet and 6.0 deg/s + // on the IOTA, and both were carrying the same literal. What is logged is + // what the axis will actually do, not what was asked for - the two differ by + // the integer quantisation, and by more than that when a request is clamped. + void sendProductionSpeeds() { + auto one = [&](const char* name, char code, const AxisMap& map, + const AxisDrivetrainConfig& d) { + if (!cfg.hardware.hasAxis(name)) return; + const auto p = planAxisSpeed(d.scan_speed_deg_s, map.counts_per_deg); + if (!p.ok) { + // Leave the firmware on its compile-time default rather than + // commanding a speed derived from a scale we do not trust. + LOG_WARN << name << " speed: not set -- " << p.error + << "; the firmware keeps its built-in default"; + return; + } + motor->sendCommand(std::string("SPEED ") + code + " " + std::to_string(p.vmax)); + if (p.clamped) + LOG_WARN << name << " speed: " << d.scan_speed_deg_s + << " deg/s exceeds what VMAX can express on this drivetrain; clamped to " + << p.actual_deg_s << " deg/s"; + LOG_INFO << name << " speed: " << p.actual_deg_s << " deg/s (VMAX " << p.vmax + << ") -- " << map.counts_per_deg << " counts/deg"; + }; + one("yaw", 'Y', cfg.geometry.yaw, cfg.drivetrain.yaw); + one("pitch", 'P', cfg.geometry.pitch, cfg.drivetrain.pitch); + } + // Convert this tower's declared milliamps into TMC register fields, once. // The host owns this because hardware.ini is where the sense resistor lives; // the firmware is told registers and keeps working compile-time defaults. @@ -1971,8 +2005,20 @@ struct Application::Impl { stopCapture(); // free the serial link; don't fight the routine's moves LOG_INFO << "starting hardware test (profile '" << profile.name << "')"; + // What `test hardware` needs beyond the motor and IMU: the declaration to + // check against, and the subsystems that answer for themselves. + HardwareTestContext hw; + hw.declared = &cfg.hardware; + hw.drive = &cfg.drivetrain; + hw.env = env.get(); + 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.usbfs_min_mb = cfg.test.usbfs_min_mb; + hw.lpm_path = cfg.test.usb3_lpm_path; + tester = std::make_unique(*motor, imu.get(), cfg.geometry, mask, profile, - cfg.paths.output_dir); + cfg.paths.output_dir, std::move(hw)); tester->start(); } diff --git a/src/core/AxisSpeed.cpp b/src/core/AxisSpeed.cpp new file mode 100644 index 0000000..ef0b848 --- /dev/null +++ b/src/core/AxisSpeed.cpp @@ -0,0 +1,73 @@ +#include "fgc/AxisSpeed.h" + +#include + +namespace fgc { +namespace { + +std::string num(double v) { + char buf[32]; + std::snprintf(buf, sizeof(buf), "%.3g", v); + return buf; +} + +} // namespace + +double vmaxToDegPerSec(long vmax, double counts_per_deg, double clock_hz) { + const double cpd = std::fabs(counts_per_deg); + if (cpd <= 0.0 || clock_hz <= 0.0) return 0.0; + return (static_cast(vmax) * clock_hz / kTmcVmaxDiv) / cpd; +} + +AxisSpeedPlan planAxisSpeed(double deg_per_s, double counts_per_deg, double clock_hz) { + AxisSpeedPlan p; + p.requested_deg_s = deg_per_s; + + // A speed is a magnitude; the direction sense lives in counts_per_deg's sign + // and belongs to Geometry, not here. + const double cpd = std::fabs(counts_per_deg); + + if (deg_per_s <= 0.0) { + p.error = "speed must be > 0 deg/s, got " + num(deg_per_s); + return p; + } + if (cpd < kMinPlausibleCountsPerDeg) { + p.error = "counts_per_deg is " + num(counts_per_deg) + + ", which is not a calibrated drivetrain (an ungeared AS5047P alone reads 11.1) " + "-- calibrate the axis first ('gimbal heading setup'), or declare " + "[Motor.] travel_deg so homing can derive it"; + return p; + } + if (clock_hz <= 0.0) { + p.error = "TMC clock must be > 0 Hz"; + return p; + } + + const double counts_per_s = deg_per_s * cpd; + const double exact = counts_per_s * kTmcVmaxDiv / clock_hz; + + // Round rather than truncate: at low speeds truncation biases every axis slow, + // and at 1 count/s it would round to zero and stop the gimbal outright. + long vmax = std::lround(exact); + + if (vmax > kTmcVmaxMax) { + vmax = kTmcVmaxMax; + p.clamped = true; + } + if (vmax < 1) { + // Anything that rounds below one unit cannot be commanded at all. Failing + // is right: silently substituting the slowest possible speed would look + // like a working axis that never finishes a sweep. + p.error = num(deg_per_s) + " deg/s is below what VMAX can express on this drivetrain (" + + num(cpd) + " counts/deg); the slowest commandable speed is " + + num(vmaxToDegPerSec(1, cpd, clock_hz)) + " deg/s"; + return p; + } + + p.ok = true; + p.vmax = vmax; + p.actual_deg_s = vmaxToDegPerSec(vmax, cpd, clock_hz); + return p; +} + +} // namespace fgc diff --git a/src/core/Config.cpp b/src/core/Config.cpp index ee239d6..0732278 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -381,6 +381,8 @@ AppConfig ConfigLoader::fromMap(const std::map& kv) { // lands in that profile's open param map for the test modules to read. cfg.test.default_profile = get(kv, "Test.profile", cfg.test.default_profile); cfg.test.baseline_file = get(kv, "Test.baseline_file"); + cfg.test.usbfs_min_mb = getInt(kv, "Test.usbfs_min_mb", cfg.test.usbfs_min_mb); + cfg.test.usb3_lpm_path = get(kv, "Test.usb3_lpm_path"); if (!cfg.test.baseline_file.empty()) cfg.test.baseline_file = paths::expandUser(cfg.test.baseline_file); { @@ -439,6 +441,7 @@ AppConfig ConfigLoader::fromMap(const std::map& kv) { // An axis that names only a run current is not asking for a reduction. d.homing_current_ma = getDouble(kv, pre + "homing_current_ma", d.run_current_ma); d.travel_deg = getDouble(kv, pre + "travel_deg", d.travel_deg); + d.scan_speed_deg_s = getDouble(kv, pre + "scan_speed_deg_s", d.scan_speed_deg_s); }; axisDrivetrain("yaw", cfg.drivetrain.yaw); axisDrivetrain("pitch", cfg.drivetrain.pitch); diff --git a/src/core/HardwareChecks.cpp b/src/core/HardwareChecks.cpp new file mode 100644 index 0000000..9499329 --- /dev/null +++ b/src/core/HardwareChecks.cpp @@ -0,0 +1,128 @@ +#include "fgc/HardwareChecks.h" + +#include +#include +#include + +namespace fgc { +namespace { + +std::string trimmed(const std::string& s) { + const auto b = s.find_first_not_of(" \t\r\n"); + if (b == std::string::npos) return ""; + const auto e = s.find_last_not_of(" \t\r\n"); + return s.substr(b, e - b + 1); +} + +// A sysfs integer, or nullopt when the node was empty/unreadable/not a number. +bool asInt(const std::string& s, long& out) { + const std::string t = trimmed(s); + if (t.empty()) return false; + if (!std::all_of(t.begin(), t.end(), [](unsigned char c) { return std::isdigit(c); })) + return false; + try { + out = std::stol(t); + } catch (...) { + return false; + } + return true; +} + +} // namespace + +CheckResult checkUsbfsMemory(const std::string& sysfs_value, int min_mb) { + CheckResult r; + long mb = 0; + if (!asInt(sysfs_value, mb)) { + r.pass = false; + r.detail = "could not read usbfs_memory_mb -- expected a number at " + "/sys/module/usbcore/parameters/usbfs_memory_mb"; + return r; + } + r.pass = mb >= min_mb; + if (r.pass) { + r.detail = "usbfs_memory_mb " + std::to_string(mb); + } else { + r.detail = "usbfs_memory_mb is " + std::to_string(mb) + ", needs >= " + + std::to_string(min_mb) + + " -- frames will stall under load. It resets on every reboot unless " + "usbcore.usbfs_memory_mb=" + std::to_string(min_mb) + + " is on the kernel cmdline (known-issues #19)"; + } + return r; +} + +CheckResult checkUsb3Lpm(const std::string& permit_value) { + CheckResult r; + const std::string v = trimmed(permit_value); + if (v.empty()) { + r.pass = false; + r.detail = "could not read usb3_lpm_permit for the camera's port"; + return r; + } + r.pass = (v == "0"); + r.detail = r.pass ? "USB3 LPM off on the camera port" + : "USB3 LPM is permitted (usb3_lpm_permit=" + v + + ") -- write 0 and re-enumerate, and persist it with a udev rule; " + "the per-device usb3_hardware_lpm_u* nodes are read-only " + "(known-issues #19)"; + return r; +} + +CheckResult checkI2cFunctionality(unsigned long funcs) { + CheckResult r; + r.pass = (funcs & kI2cFuncI2c) != 0; + r.detail = r.pass ? "adapter supports real I2C transactions" + : "adapter is SMBus-only (I2C_FUNCS lacks I2C_FUNC_I2C) -- the SHT41 will " + "bind and then fail every read. Point [Env] i2c_device at a real I2C " + "adapter; 'i2cdetect -F ' is the manual check (known-issues #2)"; + return r; +} + +CheckResult checkDrivetrainAdopted(const std::string& axis, const DrivetrainExpectation& want, + const DrivetrainEcho& got) { + CheckResult r; + + // Nothing declared for this axis means nothing was sent, and the firmware's + // compile-time defaults are the intended configuration. There is no skew to + // find. + if (!want.declared) { + r.ran = false; + r.detail = "no [Motor." + axis + "] current declared; firmware defaults are in force"; + return r; + } + + if (got.absent()) { + r.pass = false; + r.detail = axis + + ": firmware did not echo gs/irun/irun_home -- it predates the CURRENT command, " + "so whatever hardware.ini asks for is NOT being applied. Re-flash it"; + return r; + } + + auto mismatch = [&](const char* field, int wanted, long saw) { + 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); + + if (r.pass) { + r.detail = axis + ": firmware has the declared current (GLOBALSCALER " + + std::to_string(want.global_scaler) + ", IRUN " + std::to_string(want.run_irun) + + ", homing " + std::to_string(want.homing_irun) + ")"; + } else { + r.detail = axis + ": " + r.detail + + " -- the CURRENT command did not land. A homing current that is not in force is " + "the case that damages the endstop"; + } + return r; +} + +} // namespace fgc diff --git a/src/core/HelpText.cpp b/src/core/HelpText.cpp index f323039..da21f59 100644 --- a/src/core/HelpText.cpp +++ b/src/core/HelpText.cpp @@ -120,6 +120,13 @@ const std::vector& helpCatalog() { "token names a config profile. Each run is compared to a pinned baseline", "and the previous run. Esc cancels.", "Example: test gimbal homing / test list / test baseline"}}, + {"test hardware [axes|motor|imu|env|camera]", + "Check the rig against what hardware.ini declares.", { + "Preconditions, not measurements: the declared axes appear in telemetry and", + "no others; the firmware echoes the drivetrain current hardware.ini asked for;", + "a declared IMU streams; a declared ambient sensor sits on a REAL I2C bus (not", + "SMBus-only) and its CRC checks out; the cameras opened and the USB", + "preconditions hold. Nothing declared reports 'not checked', never PASS."}}, {"gimbal pitch setup", "Calibrate PITCH steps<->degrees against gravity, using the IMU.", { "Auto-homes if needed, sweeps pitch dwelling at each step, fits, then", diff --git a/src/core/TestRunner.cpp b/src/core/TestRunner.cpp index c3edfdb..435feba 100644 --- a/src/core/TestRunner.cpp +++ b/src/core/TestRunner.cpp @@ -3,6 +3,12 @@ #include "fgc/Calibration.h" // unwrapNear #include "fgc/DiagParser.h" +#include "fgc/HardwareChecks.h" +#include "fgc/HardwarePresence.h" +#include "fgc/ICameraSource.h" +#include "fgc/IEnvSensor.h" +#include "fgc/TmcCurrent.h" +#include "fgc/sensors/I2cBus.h" #include "fgc/DumpParser.h" #include "fgc/HostMetrics.h" #include "fgc/IImuSource.h" @@ -10,6 +16,8 @@ #include "fgc/Logger.h" #include +#include +#include #include #include #include @@ -109,6 +117,15 @@ bool resolveTestSelection(const std::string& subsystem, const std::string& test, else if (test == "health") m = T_IMU_HEALTH; else if (test == "drift") m = T_IMU_DRIFT; else { err = "unknown imu test: " + test; return false; } + } else if (subsystem == "hardware") { + sub_ok = true; + if (test.empty()) m = T_HW_ALL; + else if (test == "axes") m = T_HW_AXES; + else if (test == "motor") m = T_HW_MOTOR; + else if (test == "imu") m = T_HW_IMU; + else if (test == "env") m = T_HW_ENV; + else if (test == "camera") m = T_HW_CAMERA; + else { err = "unknown hardware test: " + test; return false; } } else if (subsystem == "host") { sub_ok = true; if (test.empty()) m = T_HOST_ALL; @@ -125,9 +142,10 @@ bool resolveTestSelection(const std::string& subsystem, const std::string& test, } TestRunner::TestRunner(IMotorController& motor, IImuSource* imu, Geometry geo, - uint32_t selection, TestProfile profile, std::string disk_path) + uint32_t selection, TestProfile profile, std::string disk_path, + HardwareTestContext hw) : motor_(motor), imu_(imu), geo_(geo), selection_(selection), - profile_(std::move(profile)), disk_path_(std::move(disk_path)) {} + profile_(std::move(profile)), disk_path_(std::move(disk_path)), hw_(std::move(hw)) {} TestRunner::~TestRunner() { cancel_ = true; @@ -199,6 +217,11 @@ void TestRunner::run() { if (!cancelled() && (selection_ & T_IMU_HEALTH)) testImuHealth(r); if (!cancelled() && (selection_ & T_IMU_DRIFT)) testImuDrift(r); if (!cancelled() && (selection_ & T_HOST_ALL)) testHost(r); + if (!cancelled() && (selection_ & T_HW_AXES)) testHwAxes(r); + if (!cancelled() && (selection_ & T_HW_MOTOR)) testHwMotor(r); + if (!cancelled() && (selection_ & T_HW_IMU)) testHwImu(r); + if (!cancelled() && (selection_ & T_HW_ENV)) testHwEnv(r); + if (!cancelled() && (selection_ & T_HW_CAMERA)) testHwCamera(r); r.all_pass = true; for (const auto& s : r.sections) @@ -770,4 +793,275 @@ void TestRunner::testHost(TestReport& r) { } } +// --------------------------------------------------------------------------- +// `test hardware` - is this rig the one hardware.ini says it is? +// +// These differ from every other leaf: they measure nothing and trend nothing. +// Each is a precondition that either holds or does not, so each metric is a +// 1/0 with a threshold of 1, and the detail lives in the section notes where an +// operator reads it. + +namespace { + +// A pass/fail metric. Uses the same threshold machinery as the measured leaves +// so rollup(), formatting and comparison all keep working unchanged. +void verdict(TestSection& s, const std::string& name, const CheckResult& c) { + if (!c.ran) { + s.notes.push_back(name + ": not checked -- " + c.detail); + return; + } + auto& m = add(s, name, c.pass ? 1 : 0, "", /*lower_is_better=*/false); + thresh(m, 1); + s.notes.push_back(name + ": " + c.detail); +} + +std::string readSysfs(const std::string& path) { + if (path.empty()) return ""; + std::ifstream f(path); + if (!f) return ""; + std::string line; + std::getline(f, line); + return line; +} + +} // namespace + +void TestRunner::testHwAxes(TestReport& r) { + TestSection s; s.id = "hw_axes"; s.title = "declared axes vs telemetry"; + setProgress(s.id, 1, 1, "reading telemetry"); + + if (!hw_.declared || !hw_.declared->declared) { + s.notes.push_back("no hardware.ini in force; nothing is declared to check against"); + s.ran = false; + r.sections.push_back(std::move(s)); + return; + } + + // One ST line is enough: yaw is implied by any well-formed line, pitch by + // its own segment. + MotorTelemetry t = motor_.telemetry(); + FoundHardware f; + f.axes_known = t.yaw.state != AxisState::Unknown; + if (f.axes_known) { + f.axes.push_back("yaw"); + if (t.pitch_present) f.axes.push_back("pitch"); + } + + // Only the axis findings belong here; the sensors and cameras have their own + // leaves, and a declaration they disagree with would otherwise fail twice. + HardwareConfig axes_only; + axes_only.declared = true; + axes_only.source_path = hw_.declared->source_path; + axes_only.axes = hw_.declared->axes; + + const PresenceReport pr = reconcilePresence(axes_only, f); + CheckResult c; + c.pass = !pr.anyMissing() && !pr.anyUndeclared(); + if (c.pass) { + c.detail = "telemetry carries exactly the declared axes (" + axes_only.axesLabel() + ")"; + } else { + for (const auto& line : formatPresence(pr)) + c.detail += (c.detail.empty() ? "" : "; ") + line; + } + verdict(s, "axes_match", c); + + rollup(s); + r.sections.push_back(std::move(s)); +} + +void TestRunner::testHwMotor(TestReport& r) { + TestSection s; s.id = "hw_motor"; s.title = "firmware adopted the declared drivetrain"; + setProgress(s.id, 1, 1, "reading DUMP"); + + if (!hw_.drive) { + s.notes.push_back("no drivetrain declared; firmware defaults are in force"); + s.ran = false; + r.sections.push_back(std::move(s)); + return; + } + + motor_.sendCommand("DUMP"); + std::this_thread::sleep_for(250ms); + const DumpData d = parseDump(motor_.lastDump()); + if (r.fw.empty() && !d.build.empty()) r.fw = d.build; + if (!d.valid) { + CheckResult c; + c.pass = false; + c.detail = "no parseable DUMP came back, so what the firmware is running could not be " + "confirmed"; + verdict(s, "dump_readable", c); + rollup(s); + r.sections.push_back(std::move(s)); + return; + } + + auto check_axis = [&](const char* name, char code, const AxisDrivetrainConfig& cfg) { + DrivetrainExpectation want; + if (cfg.run_current_ma > 0.0 && cfg.r_sense_ohm > 0.0) { + const auto plan = planTmcCurrent(cfg.r_sense_ohm, cfg.run_current_ma, + cfg.homing_current_ma); + if (plan.ok) { + want.declared = true; + want.global_scaler = plan.global_scaler; + want.run_irun = plan.run_irun; + want.homing_irun = plan.homing_irun; + } + } + DrivetrainEcho got; + for (const auto& ax : d.axes) + if (ax.axis == code) { + got.global_scaler = ax.global_scaler; + got.run_irun = ax.run_irun; + got.homing_irun = ax.homing_irun; + } + verdict(s, std::string(name) + "_current", checkDrivetrainAdopted(name, want, got)); + }; + check_axis("yaw", 'Y', hw_.drive->yaw); + if (!hw_.declared || hw_.declared->hasAxis("pitch")) check_axis("pitch", 'P', hw_.drive->pitch); + + rollup(s); + r.sections.push_back(std::move(s)); +} + +void TestRunner::testHwImu(TestReport& r) { + TestSection s; s.id = "hw_imu"; s.title = "declared IMU is present and streaming"; + setProgress(s.id, 1, 1, "sampling"); + + const bool declared = + hw_.declared && hw_.declared->declared && !hw_.declared->imu.empty() && + hw_.declared->imu != "none"; + if (!declared) { + s.notes.push_back("no IMU declared"); + s.ran = false; + r.sections.push_back(std::move(s)); + return; + } + + CheckResult c; + if (!imu_) { + c.pass = false; + c.detail = "declared as '" + hw_.declared->imu + + "' but no IMU source was built -- check [Features] enable_imu and [IMU] device"; + } else if (!imu_->connected()) { + c.pass = false; + c.detail = "source exists but is not connected -- the port opened without a 0xFA-framed " + "stream arriving; check the baud and that it is the MTi's node"; + } else if (!imu_->sample()) { + c.pass = false; + c.detail = "connected but no sample within the staleness window -- the stream stalled"; + } else { + c.detail = "'" + hw_.declared->imu + "' streaming"; + } + verdict(s, "imu_streaming", c); + + rollup(s); + r.sections.push_back(std::move(s)); +} + +void TestRunner::testHwEnv(TestReport& r) { + TestSection s; s.id = "hw_env"; s.title = "declared ambient sensor, on a real I2C bus"; + setProgress(s.id, 1, 2, "probing bus"); + + const bool declared = + hw_.declared && hw_.declared->declared && !hw_.declared->env.empty() && + hw_.declared->env != "none"; + if (!declared) { + s.notes.push_back("no ambient sensor declared"); + s.ran = false; + r.sections.push_back(std::move(s)); + return; + } + + // The adapter class first: an SMBus-only bus makes every later symptom look + // like a dead sensor, so naming it first is what saves the debugging time. + if (hw_.i2c_device.empty()) { + CheckResult c; + c.pass = false; + c.detail = "declared as '" + hw_.declared->env + "' but [Env] i2c_device is empty"; + verdict(s, "i2c_adapter", c); + } else { + I2cBus bus(hw_.i2c_device, 0x44); + unsigned long funcs = 0; + CheckResult c; + if (!bus.open()) { + c.pass = false; + c.detail = "cannot open " + hw_.i2c_device + + " -- wrong bus number, or the user is not in the i2c group"; + } else if (!bus.functionality(funcs)) { + c.pass = false; + c.detail = "I2C_FUNCS ioctl failed on " + hw_.i2c_device; + } else { + c = checkI2cFunctionality(funcs); + c.detail = hw_.i2c_device + ": " + c.detail; + } + verdict(s, "i2c_adapter", c); + } + + setProgress(s.id, 2, 2, "reading sensor"); + CheckResult m; + if (!hw_.env) { + m.pass = false; + m.detail = "declared as '" + hw_.declared->env + + "' but no sensor was built -- check [Features] enable_env"; + } else if (auto sample = hw_.env->sample()) { + m.detail = "CRC-valid reading from " + hw_.env->name(); + } else { + m.pass = false; + m.detail = "no valid reading within the staleness window -- every transfer is failing, or " + "the CRC never checks out"; + } + verdict(s, "env_reading", m); + + rollup(s); + r.sections.push_back(std::move(s)); +} + +void TestRunner::testHwCamera(TestReport& r) { + TestSection s; s.id = "hw_camera"; s.title = "cameras present, and the USB preconditions"; + setProgress(s.id, 1, 2, "counting cameras"); + + // The USB preconditions are preconditions FOR A CAMERA. With none configured + // and no source built, there is nothing they could be protecting, and + // checking them would fail a dev box for a fault it cannot have. + if (hw_.configured_cameras.empty() && !hw_.camera) { + s.notes.push_back("no cameras configured; the USB preconditions have nothing to protect"); + s.ran = false; + r.sections.push_back(std::move(s)); + return; + } + + if (!hw_.configured_cameras.empty()) { + CheckResult c; + const int want = static_cast(hw_.configured_cameras.size()); + const int got = hw_.camera ? hw_.camera->cameraCount() : -1; + if (got < 0) { + c.ran = false; + c.detail = "no camera source in this run"; + } else if (got < want) { + c.pass = false; + c.detail = std::to_string(want) + " configured, only " + std::to_string(got) + + " opened -- one is not on the bus or is held by another process"; + } else { + c.detail = std::to_string(got) + " of " + std::to_string(want) + " opened"; + } + verdict(s, "cameras_found", c); + } + + setProgress(s.id, 2, 2, "USB preconditions"); + verdict(s, "usbfs_memory", checkUsbfsMemory(readSysfs(hw_.usbfs_path), hw_.usbfs_min_mb)); + + // LPM lives on a port node that differs per machine, so it is checked only + // when the path is known. Silence here means "not located", never "fine". + if (!hw_.lpm_path.empty()) { + verdict(s, "usb3_lpm", checkUsb3Lpm(readSysfs(hw_.lpm_path))); + } else { + s.notes.push_back( + "usb3_lpm: not checked -- no port node configured. Known-issues #19 has how to find " + "and persist it; it re-enables on every reboot without a udev rule"); + } + + rollup(s); + r.sections.push_back(std::move(s)); +} + } // namespace fgc diff --git a/src/sensors/I2cBus.cpp b/src/sensors/I2cBus.cpp index 375feaa..d0fd595 100644 --- a/src/sensors/I2cBus.cpp +++ b/src/sensors/I2cBus.cpp @@ -29,6 +29,11 @@ bool I2cBus::open() { return true; } +bool I2cBus::functionality(unsigned long& funcs) const { + if (fd_ < 0) return false; + return ::ioctl(fd_, I2C_FUNCS, &funcs) >= 0; +} + void I2cBus::close() { if (fd_ >= 0) { ::close(fd_); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 575bca1..162026a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -24,6 +24,8 @@ add_executable(fgc_tests test_config.cpp test_tmccurrent.cpp test_hardwarepresence.cpp + test_hardwarechecks.cpp + test_axisspeed.cpp test_cameraprofile.cpp test_camerasettingsxml.cpp test_hash.cpp diff --git a/tests/test_axisspeed.cpp b/tests/test_axisspeed.cpp new file mode 100644 index 0000000..df68878 --- /dev/null +++ b/tests/test_axisspeed.cpp @@ -0,0 +1,113 @@ +#include "doctest/doctest.h" +#include "fgc/AxisSpeed.h" + +using namespace fgc; + +namespace { +// The two real drivetrains. These numbers are the whole point of the module: +// the same VMAX means different things on them. +constexpr double kDelta = 983.33; // tower 111 yaw, counts/deg +constexpr double kIota = 3552.87; // tower 121 yaw, counts/deg (derived from the homed travel) +} // namespace + +TEST_CASE("the clock constant matches what was measured on the rig") { + // Three homing runs on tower 121 put it at ~0.73 counts/s per VMAX unit; the + // datasheet's f_CLK/2^24 predicts 0.7153. If this drifts, every speed does. + const double per_unit = vmaxToDegPerSec(1, 1.0); // counts/s for counts_per_deg == 1 + CHECK(per_unit == doctest::Approx(0.7153).epsilon(0.001)); + CHECK(per_unit == doctest::Approx(0.73).epsilon(0.03)); +} + +TEST_CASE("the same VMAX is a different speed on each tower") { + // This is the bug the module exists to remove: both towers carry 30000. + CHECK(vmaxToDegPerSec(30000, kDelta) == doctest::Approx(21.8).epsilon(0.01)); + CHECK(vmaxToDegPerSec(30000, kIota) == doctest::Approx(6.04).epsilon(0.01)); + + // And the host literal that was overriding the firmware on the Delta fleet. + CHECK(vmaxToDegPerSec(50000, kDelta) == doctest::Approx(36.4).epsilon(0.01)); +} + +TEST_CASE("the fleet default resolves on both drivetrains") { + auto d = planAxisSpeed(kDefaultScanSpeedDegPerSec, kDelta); + REQUIRE(d.ok); + CHECK(d.vmax == 10998); + CHECK(d.actual_deg_s == doctest::Approx(8.0).epsilon(0.001)); + + auto i = planAxisSpeed(kDefaultScanSpeedDegPerSec, kIota); + REQUIRE(i.ok); + CHECK(i.vmax == 39738); + CHECK(i.actual_deg_s == doctest::Approx(8.0).epsilon(0.001)); + + // Same physical speed, VMAX differing by the gear ratio - which is the whole + // argument for declaring deg/s. + CHECK(static_cast(i.vmax) / d.vmax == doctest::Approx(kIota / kDelta).epsilon(0.001)); +} + +TEST_CASE("a round trip is stable to the integer quantisation") { + for (double want : {2.0, 8.0, 21.8, 36.4}) { + auto p = planAxisSpeed(want, kIota); + REQUIRE(p.ok); + CHECK(p.actual_deg_s == doctest::Approx(want).epsilon(0.001)); + } +} + +TEST_CASE("direction sense belongs to Geometry, not to a speed") { + // counts_per_deg may be negative to flip the axis; VMAX must not go negative. + auto p = planAxisSpeed(8.0, -kDelta); + REQUIRE(p.ok); + CHECK(p.vmax > 0); + CHECK(p.vmax == planAxisSpeed(8.0, kDelta).vmax); +} + +TEST_CASE("an uncalibrated axis is refused, and the message says how to fix it") { + auto p = planAxisSpeed(8.0, 0.0); + CHECK_FALSE(p.ok); + CHECK(p.error.find("counts_per_deg") != std::string::npos); + CHECK(p.error.find("travel_deg") != std::string::npos); +} + +TEST_CASE("AxisMap's uncalibrated default is refused, not quietly obeyed") { + // The real bug this caught: an un-calibrated pitch sits at counts_per_deg + // 1.0, which is > 0 and so passes a naive guard. 8 deg/s through it yields + // VMAX 11 - an axis that looks configured and never finishes a sweep. + auto p = planAxisSpeed(8.0, 1.0); + CHECK_FALSE(p.ok); + CHECK(p.error.find("not a calibrated drivetrain") != std::string::npos); + + // The floor is the encoder's own ungeared resolution, so a real axis - even + // a direct-drive one - is never rejected. + CHECK(planAxisSpeed(8.0, 11.2).ok); +} + +TEST_CASE("a nonsensical speed is refused rather than silently substituted") { + CHECK_FALSE(planAxisSpeed(0.0, kDelta).ok); + CHECK_FALSE(planAxisSpeed(-5.0, kDelta).ok); +} + +TEST_CASE("a speed below one VMAX unit fails instead of creeping") { + // Substituting the slowest commandable speed would look like a working axis + // that never finishes a sweep. + auto p = planAxisSpeed(0.00001, kDelta); + CHECK_FALSE(p.ok); + CHECK(p.error.find("slowest commandable") != std::string::npos); +} + +TEST_CASE("a speed beyond the register is clamped, and says so") { + // 23-bit VMAX. On the IOTA's gear ratio this is reachable with a plausible + // typo, so it must clamp rather than wrap. + auto p = planAxisSpeed(5000.0, kIota); + REQUIRE(p.ok); + CHECK(p.clamped); + CHECK(p.vmax == kTmcVmaxMax); + // The reported speed is what the axis will really do, not what was asked. + CHECK(p.actual_deg_s < 5000.0); +} + +TEST_CASE("tower 121 cannot match tower 111's old speed, and the plan shows it") { + // 36.4 deg/s on the IOTA needs VMAX ~181k - expressible, but 6x its current + // 30000. Declaring deg/s is what makes that visible instead of mysterious. + auto p = planAxisSpeed(36.4, kIota); + REQUIRE(p.ok); + CHECK(p.vmax > 180000); + CHECK(p.vmax < 182000); +} diff --git a/tests/test_hardwarechecks.cpp b/tests/test_hardwarechecks.cpp new file mode 100644 index 0000000..25f3cbd --- /dev/null +++ b/tests/test_hardwarechecks.cpp @@ -0,0 +1,89 @@ +#include "doctest/doctest.h" +#include "fgc/HardwareChecks.h" + +using namespace fgc; + +TEST_CASE("usbfs_memory_mb is checked against the value frames actually need") { + CHECK(checkUsbfsMemory("1000", 1000).pass); + CHECK(checkUsbfsMemory("2048\n", 1000).pass); + + // The reboot default. This is the exact state that was found on the + // LattePanda and stalled frame delivery. + auto low = checkUsbfsMemory("16", 1000); + CHECK_FALSE(low.pass); + CHECK(low.detail.find("kernel cmdline") != std::string::npos); +} + +TEST_CASE("an unreadable usbfs node fails rather than passing by default") { + // A check that did not happen must not report PASS - that is how a silent + // precondition regression survives a green test run. + CHECK_FALSE(checkUsbfsMemory("", 1000).pass); + CHECK_FALSE(checkUsbfsMemory("not-a-number", 1000).pass); +} + +TEST_CASE("USB3 LPM must be off, and anything unreadable is a failure") { + CHECK(checkUsb3Lpm("0").pass); + CHECK(checkUsb3Lpm(" 0\n").pass); + + auto on = checkUsb3Lpm("1"); + CHECK_FALSE(on.pass); + CHECK(on.detail.find("re-enumerate") != std::string::npos); + + CHECK_FALSE(checkUsb3Lpm("").pass); +} + +TEST_CASE("an SMBus-only adapter is caught before it looks like a dead sensor") { + CHECK(checkI2cFunctionality(kI2cFuncI2c).pass); + CHECK(checkI2cFunctionality(kI2cFuncI2c | 0xF0).pass); + + auto smbus = checkI2cFunctionality(0xF0); // SMBus bits only + CHECK_FALSE(smbus.pass); + CHECK(smbus.detail.find("i2cdetect -F") != std::string::npos); +} + +TEST_CASE("an undeclared axis has no drivetrain skew to find") { + DrivetrainExpectation want; // declared == false + DrivetrainEcho got; // all -1 + auto r = checkDrivetrainAdopted("yaw", want, got); + CHECK_FALSE(r.ran); + CHECK(r.pass); +} + +TEST_CASE("a firmware that cannot echo the current is a version skew, not zero current") { + DrivetrainExpectation want{true, 80, 31, 7}; + DrivetrainEcho got; // all absent + auto r = checkDrivetrainAdopted("yaw", want, got); + CHECK(r.ran); + CHECK_FALSE(r.pass); + CHECK(r.detail.find("predates") != std::string::npos); + CHECK(r.detail.find("Re-flash") != std::string::npos); +} + +TEST_CASE("the IOTA tower's declared current, adopted") { + DrivetrainExpectation want{true, 80, 31, 7}; // 756 mA run, 189 mA homing at 75 mOhm + DrivetrainEcho got{80, 31, 7}; + auto r = checkDrivetrainAdopted("yaw", want, got); + CHECK(r.ran); + CHECK(r.pass); +} + +TEST_CASE("a homing current that did not land is named as the damaging case") { + // The exact failure that sheared the endstop: run current correct, homing + // current still at the firmware default. + DrivetrainExpectation want{true, 80, 31, 7}; + DrivetrainEcho got{80, 31, 31}; + auto r = checkDrivetrainAdopted("yaw", want, got); + CHECK_FALSE(r.pass); + CHECK(r.detail.find("homing IRUN wanted 7, firmware has 31") != std::string::npos); + CHECK(r.detail.find("endstop") != std::string::npos); +} + +TEST_CASE("every mismatched field is reported, not just the first") { + DrivetrainExpectation want{true, 80, 31, 7}; + DrivetrainEcho got{255, 16, 31}; + auto r = checkDrivetrainAdopted("yaw", want, got); + CHECK_FALSE(r.pass); + CHECK(r.detail.find("GLOBALSCALER") != std::string::npos); + CHECK(r.detail.find("IRUN wanted 31") != std::string::npos); + CHECK(r.detail.find("homing IRUN") != std::string::npos); +} diff --git a/tests/test_testrunner.cpp b/tests/test_testrunner.cpp index aaa88cd..be9f6ba 100644 --- a/tests/test_testrunner.cpp +++ b/tests/test_testrunner.cpp @@ -1,6 +1,8 @@ #include #include "fgc/TestRunner.h" + +#include #include "fgc/mock/MockImuSource.h" #include "fgc/mock/MockMotorController.h" @@ -60,6 +62,45 @@ TEST_CASE("resolveTestSelection maps subsystem/test tokens") { CHECK_FALSE(resolveTestSelection("gimbal", "bogus", m, err)); } +TEST_CASE("the hardware subsystem resolves, and is part of a bare 'test'") { + uint32_t m = 0; + std::string err; + CHECK(resolveTestSelection("hardware", "", m, err)); + CHECK(m == T_HW_ALL); + CHECK(resolveTestSelection("hardware", "axes", m, err)); + CHECK(m == T_HW_AXES); + CHECK(resolveTestSelection("hardware", "camera", m, err)); + CHECK(m == T_HW_CAMERA); + CHECK_FALSE(resolveTestSelection("hardware", "bogus", m, err)); + + // `test` with no arguments must include them, or the gate is only met by + // someone who knew to ask for it. + CHECK(resolveTestSelection("", "", m, err)); + CHECK((m & T_HW_ALL) == T_HW_ALL); + + // They must NOT need a homed gimbal: that guard aborts the whole run, and + // `test hardware` is precisely what you run on a tower before homing it. + CHECK((T_NEEDS_MOTOR & T_HW_ALL) == 0); +} + +TEST_CASE("hardware leaves report 'not checked' rather than PASS when nothing is declared") { + // The default context declares nothing, which is every un-migrated tower. + MockMotorController motor; + motor.start(); + TestRunner tr(motor, nullptr, unitGeometry(), T_HW_ALL, fastProfile(), "."); + auto rep = runToCompletion(tr); + REQUIRE(rep); + + for (const char* id : {"hw_axes", "hw_motor", "hw_imu", "hw_env"}) { + const auto it = std::find_if(rep->sections.begin(), rep->sections.end(), + [&](const TestSection& s) { return s.id == id; }); + REQUIRE(it != rep->sections.end()); + CHECK_FALSE(it->ran); // reported, but not counted as a pass + } + // An undeclared rig must not fail the run either. + CHECK(rep->all_pass); +} + TEST_CASE("TestRunner host-only run completes and produces sections") { MockMotorController motor; TestRunner tr(motor, nullptr, unitGeometry(), T_HOST_ALL, fastProfile(), ".");