diff --git a/config/config.example.ini b/config/config.example.ini index fdac21a..b204de0 100644 --- a/config/config.example.ini +++ b/config/config.example.ini @@ -78,8 +78,9 @@ baud = 115200 ; (an onboard hardware UART), so this is a stable /dev/ttyS* node - NOT a USB ; device and NOT /dev/ttyUSB0-3 (those are the Quectel modem). Find it with ; ls /dev/ttyS* ; dmesg | grep -iE 'ttyS|LPSS|HSUART' (often /dev/ttyS4) -; Enable with [Features] enable_imu = true. The host reconfigures the MTi to a -; Euler + calibrated 100 Hz stream at startup. +; Enable by declaring it in hardware.ini as [Hardware] imu = ; the legacy +; [Features] enable_imu applies only on a tower with no hardware.ini. The host +; reconfigures the MTi to a Euler + calibrated 100 Hz stream at startup. device = baud = 115200 @@ -145,8 +146,9 @@ z = 0 ; Ambient temperature/humidity sensor (Adafruit SHT41), wired to the ; LattePanda's OWN native I2C bus - NOT the Arduino/motor serial link. Bus ; number varies by board; confirm with `i2cdetect -y N` (SHT41 shows up at -; 0x44). Enable with [Features] enable_env = true (or run with --mock-env, -; which implies it). i2c_addr takes decimal or 0x-prefixed hex. +; 0x44). Enable by declaring it in hardware.ini as [Hardware] env = ; the +; legacy [Features] enable_env applies only on a tower with no hardware.ini (or +; run with --mock-env, which implies it). i2c_addr takes decimal or 0x-hex. i2c_device = /dev/i2c-1 i2c_addr = 0x44 period_ms = 2000 @@ -227,11 +229,17 @@ enable_mqtt = true ; no profile XMLs required. For bringing a tower up one subsystem at a time. enable_camera = true enable_serial = true +; LEGACY. On a tower with a hardware.ini these are derived from [Hardware] imu +; and [Hardware] env - declaring a sensor there is what enables it, and a flag +; here that contradicts the declaration is warned about and ignored. They remain +; only for towers that have not been migrated. enable_imu = false +enable_env = false +; The mocks are NOT legacy: substituting a simulated device is a choice about +; this run, not a statement about what is bolted to the tower, so it stays here. mock_camera = false mock_serial = false mock_imu = false -enable_env = false mock_env = false [Test] diff --git a/config/hardware.example.ini b/config/hardware.example.ini index 1b0b461..157a038 100644 --- a/config/hardware.example.ini +++ b/config/hardware.example.ini @@ -39,8 +39,13 @@ firmware_env = leonardo ; Axes this gimbal has. A yaw-only rig says: axes = yaw axes = yaw, pitch ; Orientation sensor: a model name, or 'none'. Most of the fleet has none. +; +; Naming a model here is what ENABLES it - there is no second key to remember. +; [Features] enable_imu still exists for towers with no hardware.ini, but where +; this file declares a sensor the declaration wins, and a flag contradicting it +; is warned about and ignored. imu = none -; Ambient temperature/humidity sensor: a model name, or 'none'. +; Ambient temperature/humidity sensor: a model name, or 'none'. Same rule. env = sht41 ; The [Camera.] blocks expected to be present and found. cameras = rgb diff --git a/docs/configuration.md b/docs/configuration.md index 51dde38..67bdf3c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -49,11 +49,11 @@ Parsed and validated by `ConfigLoader` ([src/core/Config.cpp](../src/core/Config | `Features` | `enable_mqtt` | bool | `true` | Use MQTT (vs null channel) | | `Features` | `enable_camera` | bool | `true` | (reserved) | | `Features` | `enable_serial` | bool | `true` | (reserved) | -| `Features` | `enable_imu` | bool | `false` | Use the Xsens MTi orientation/IMU | +| `Features` | `enable_imu` | bool | `false` | **Legacy.** Derived from `[Hardware] imu` wherever that is declared; kept only for towers without a `hardware.ini`. A flag contradicting the declaration is warned about and ignored | | `Features` | `mock_camera` | bool | `false` | Use the simulated camera | | `Features` | `mock_serial` | bool | `false` | Use the simulated motor controller | | `Features` | `mock_imu` | bool | `false` | Use the simulated IMU instead of the MTi | -| `Features` | `enable_env` | bool | `false` | Use the ambient temp/humidity sensor (SHT41) | +| `Features` | `enable_env` | bool | `false` | **Legacy.** Derived from `[Hardware] env` wherever that is declared; same rule as `enable_imu` | | `Features` | `mock_env` | bool | `false` | Use the simulated env sensor instead of the SHT41 | | `Features` | `strict_presence` | bool | `false` | Refuse to start when `hardware.ini` declares a subsystem this rig does not have. Off during the fleet rollout; turn it on per tower once that file is known good. `--degraded` overrides it | | `IMU` | `device` | string | — | MTi serial device (see `[IMU]` note); required when `enable_imu` | @@ -401,6 +401,26 @@ generic `IEnvSensor` interface ([include/fgc/IEnvSensor.h](../include/fgc/IEnvSensor.h)), so swapping in a different ambient sensor later means adding a new backend, not touching `Application`, the UI, or MQTT. +### One key, not two: declaring a sensor is what enables it + +`[Hardware] imu` and `[Hardware] env` name what is fitted. `[Features] enable_imu` +and `enable_env` used to say the same thing a second time, and the two could +disagree — declare `imu = mti-630`, forget the flag, and the program would report +**missing hardware** for what was a missing config key. + +The declaration now wins wherever it exists: + +| `[Hardware] imu` | `[Features] enable_imu` | Result | +|---|---|---| +| *(absent)* | `true` / `false` | the flag governs, exactly as before — every un-migrated tower | +| `mti-630` | *(absent)* | **enabled.** No second key is needed | +| `mti-630` | `false` | **enabled**, and the clash is warned about so the stale key can be deleted | +| `none` | `true` | **disabled.** A rig that says it has no IMU cannot be argued into having one | + +`mock_imu` / `mock_env` / `mock_camera` / `mock_serial` stay in `config.ini` and are +untouched by this: substituting a simulated device is an operational choice about +this *run*, not a statement about what is bolted to the tower. + ### Declared vs found — what `hardware.ini` is checked against Everything in this program degrades gracefully: `TestRunner` skips the pitch and IMU leaves, `CamEvent` diff --git a/docs/hardware-config-plan.md b/docs/hardware-config-plan.md index b0f025a..7fe269a 100644 --- a/docs/hardware-config-plan.md +++ b/docs/hardware-config-plan.md @@ -387,7 +387,7 @@ a yaw-only tower, captures one elevation band, and reports itself healthy. | ~~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 | +| ~~`[Features] enable_imu` / `enable_env`~~ **fixed** | derived from `[Hardware] imu` / `env`; the flags remain only for towers without a `hardware.ini`, and a flag that contradicts the declaration is warned about and ignored | `mock_camera` / `mock_serial` / `mock_imu` / `mock_env` stay in `config.ini` — dev-box substitution is operational, and it is what lets all of this be exercised with no hardware. @@ -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`). -269 tests pass, up from a 204 baseline. +273 tests pass, up from a 204 baseline. | § | Item | State | |----|------|-------| diff --git a/include/fgc/Config.h b/include/fgc/Config.h index 084b961..9375fd7 100644 --- a/include/fgc/Config.h +++ b/include/fgc/Config.h @@ -190,6 +190,33 @@ struct FeaturesConfig { bool strict_presence = false; }; +// Where a sensor's on/off state came from. [Hardware] imu / env DECLARE what is +// fitted; [Features] enable_imu / enable_env are the older flags that said the +// same thing a second time, and could disagree with it. +enum class SensorEnableSource { + Declaration, // [Hardware] imu / env said so - the new, single source + Flag, // no declaration, but the legacy flag was written explicitly + Default, // neither: the built-in default +}; + +struct SensorEnable { + bool enabled = false; + SensorEnableSource source = SensorEnableSource::Default; + // The legacy flag was present AND contradicted the declaration. The + // declaration wins; this exists so the stale key can be named and deleted + // rather than silently ignored. + bool conflict = false; +}; + +// Resolve one sensor's enablement. +// +// The declaration wins wherever it exists, which is what retires the flag: a rig +// that says `imu = mti-630` needs no second key agreeing with it, and a rig that +// says `imu = none` cannot be contradicted into enabling one. With nothing +// declared - every tower until it is migrated - the flag governs exactly as +// before. +SensorEnable resolveSensorEnable(const std::string& declared, bool flag_value, bool flag_present); + // [IMU]: Xsens MTi connected over the LattePanda's RS-232 UART (a hardware // /dev/ttyS* node, stable across reboots — not a USB device). struct ImuConfig { diff --git a/src/core/Application.cpp b/src/core/Application.cpp index 953bb31..9b4fd0a 100644 --- a/src/core/Application.cpp +++ b/src/core/Application.cpp @@ -607,7 +607,7 @@ struct Application::Impl { return; } if (!imu) { - LOG_WARN << "no IMU (needs [Features] enable_imu, or mock_imu)"; + LOG_WARN << "no IMU (declare [Hardware] imu, or use --mock-imu)"; return; } @@ -1836,7 +1836,7 @@ struct Application::Impl { // gravity, and nothing else on the tower can measure it. Yaw does not - // `gimbal heading setup` uses landmarks instead. if (!imu) { - LOG_WARN << "gimbal pitch setup requires the IMU (set [Features] enable_imu); " + LOG_WARN << "gimbal pitch setup requires the IMU (declare [Hardware] imu); " "yaw does not - use 'gimbal heading setup'"; return; } @@ -1984,7 +1984,7 @@ struct Application::Impl { } // IMU leaves need an IMU; drop them (with a warning) if absent. if ((mask & T_NEEDS_IMU) && !imu) { - LOG_WARN << "test: IMU tests skipped (set [Features] enable_imu)"; + LOG_WARN << "test: IMU tests skipped (declare [Hardware] imu)"; mask &= ~T_NEEDS_IMU; } // Motor leaves need a homed gimbal. diff --git a/src/core/Config.cpp b/src/core/Config.cpp index 0732278..cac6106 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -353,6 +353,29 @@ AppConfig ConfigLoader::fromMap(const std::map& kv) { cfg.features.strict_presence = getBool(kv, "Features.strict_presence", cfg.features.strict_presence); + // [Hardware] imu / env are the declaration; enable_imu / enable_env are the + // older flags that said it a second time. Resolve them into one answer here, + // once, so nothing downstream has to know there were ever two keys - and so a + // rig cannot declare a sensor and then be told it is disabled. + { + auto settle = [&](const char* what, const std::string& declared, const char* flag_key, + bool& out) { + const SensorEnable e = + resolveSensorEnable(declared, out, kv.count(flag_key) > 0); + if (e.conflict) + LOG_WARN << "[Hardware] " << what << " = '" << declared << "' and [" << flag_key + << "] disagree; the declaration wins (" << what << " " + << (e.enabled ? "enabled" : "disabled") + << ") -- delete the flag, it is what hardware.ini is for"; + else if (e.source == SensorEnableSource::Declaration && kv.count(flag_key)) + LOG_INFO << "[" << flag_key << "] is redundant: [Hardware] " << what + << " already declares it"; + out = e.enabled; + }; + settle("imu", cfg.hardware.imu, "Features.enable_imu", cfg.features.enable_imu); + settle("env", cfg.hardware.env, "Features.enable_env", cfg.features.enable_env); + } + cfg.imu.device = get(kv, "IMU.device", cfg.imu.device); cfg.imu.baud = static_cast(getInt(kv, "IMU.baud", cfg.imu.baud)); @@ -525,6 +548,21 @@ AppConfig ConfigLoader::fromMap(const std::map& kv) { return cfg; } +SensorEnable resolveSensorEnable(const std::string& declared, bool flag_value, bool flag_present) { + SensorEnable r; + if (declared.empty()) { + // Nothing stated, so the legacy flag still governs. This is every tower + // that has not been migrated, and it must behave exactly as before. + r.enabled = flag_value; + r.source = flag_present ? SensorEnableSource::Flag : SensorEnableSource::Default; + return r; + } + r.enabled = (declared != "none"); + r.source = SensorEnableSource::Declaration; + r.conflict = flag_present && (flag_value != r.enabled); + return r; +} + bool HardwareConfig::hasAxis(const std::string& name) const { // An undeclared rig answers true for everything - see the header. if (axes.empty()) return true; diff --git a/src/core/HardwarePresence.cpp b/src/core/HardwarePresence.cpp index 459ea36..824137c 100644 --- a/src/core/HardwarePresence.cpp +++ b/src/core/HardwarePresence.cpp @@ -79,7 +79,8 @@ PresenceReport reconcilePresence(const HardwareConfig& declared, const FoundHard if (names(declared.imu) && !found.imu) missing(r, "imu", "declared as '" + declared.imu + - "' but no IMU came up -- check [IMU] device, and that enable_imu is set"); + "' but no IMU came up -- check [IMU] device; declaring it here is what " + "enables it"); if (stated(declared.imu) && !names(declared.imu) && found.imu) undeclared(r, "imu", "running, but [Hardware] imu says none -- the declaration is stale"); diff --git a/src/core/PitchSetup.cpp b/src/core/PitchSetup.cpp index 9f5cc66..6087dbe 100644 --- a/src/core/PitchSetup.cpp +++ b/src/core/PitchSetup.cpp @@ -144,7 +144,7 @@ void PitchSetupRoutine::run() { // 1. Preconditions. if (!imu_.connected()) { - LOG_WARN << "pitch setup aborted: IMU not connected (set [Features] enable_imu)"; + LOG_WARN << "pitch setup aborted: IMU not connected (declare [Hardware] imu)"; return; } diff --git a/src/core/TestRunner.cpp b/src/core/TestRunner.cpp index 435feba..39c26dc 100644 --- a/src/core/TestRunner.cpp +++ b/src/core/TestRunner.cpp @@ -941,7 +941,8 @@ void TestRunner::testHwImu(TestReport& r) { 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"; + "' but no IMU source was built -- check [IMU] device; the declaration itself is " + "what enables it"; } else if (!imu_->connected()) { c.pass = false; c.detail = "source exists but is not connected -- the port opened without a 0xFA-framed " @@ -1002,7 +1003,8 @@ void TestRunner::testHwEnv(TestReport& r) { if (!hw_.env) { m.pass = false; m.detail = "declared as '" + hw_.declared->env + - "' but no sensor was built -- check [Features] enable_env"; + "' but no sensor was built -- check [Env] i2c_device; the declaration itself is " + "what enables it"; } else if (auto sample = hw_.env->sample()) { m.detail = "CRC-valid reading from " + hw_.env->name(); } else { diff --git a/src/ui/TuiUi.cpp b/src/ui/TuiUi.cpp index 23808df..24347e8 100644 --- a/src/ui/TuiUi.cpp +++ b/src/ui/TuiUi.cpp @@ -1058,7 +1058,7 @@ Element sensorsDetailPanel(const ImuView& v, const EnvView& e, ScrollState& st) | color(Color::Magenta)); if (!v.present) { body.push_back(text("(no IMU data)") | color(Color::Red) | bold); - body.push_back(text("check [Features] enable_imu and [IMU] device, or --mock-imu") | dim); + body.push_back(text("declare [Hardware] imu and set [IMU] device, or --mock-imu") | dim); } else { body.push_back(hbox({ text("ORIENTATION") | dim | size(WIDTH, EQUAL, 14), @@ -1103,7 +1103,7 @@ Element sensorsDetailPanel(const ImuView& v, const EnvView& e, ScrollState& st) if (!e.enabled) { body.push_back(text("(env sensor disabled)") | dim); body.push_back( - text("enable with [Features] enable_env and [Env] i2c_device, or --mock-env") + text("declare [Hardware] env and set [Env] i2c_device, or --mock-env") | dim); } else if (!e.present) { body.push_back(text("(no reading)") | color(Color::Red) | bold); diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 274cc62..3a5272a 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -691,3 +691,47 @@ TEST_CASE("non-numeric values still compare as text") { {{"device", "/dev/ttyUSB0"}}, "a save"); CHECK(out.find(";prev device = /dev/ttyACM0") != std::string::npos); } + +// --- [Hardware] imu/env retiring [Features] enable_imu/enable_env (plan A.12) --- + +TEST_CASE("an undeclared rig still obeys the legacy flag exactly as before") { + // Every tower until it is migrated. Behaviour must not change. + auto on = resolveSensorEnable("", true, true); + CHECK(on.enabled); + CHECK(on.source == SensorEnableSource::Flag); + CHECK_FALSE(on.conflict); + + auto off = resolveSensorEnable("", false, true); + CHECK_FALSE(off.enabled); + + auto absent = resolveSensorEnable("", false, false); + CHECK_FALSE(absent.enabled); + CHECK(absent.source == SensorEnableSource::Default); +} + +TEST_CASE("a declared sensor needs no second key agreeing with it") { + // The trap this removes: declare the IMU, forget enable_imu, and presence + // reconciliation reports missing HARDWARE for what is a missing flag. + auto e = resolveSensorEnable("mti-630", /*flag_value=*/false, /*flag_present=*/false); + CHECK(e.enabled); + CHECK(e.source == SensorEnableSource::Declaration); + CHECK_FALSE(e.conflict); +} + +TEST_CASE("'none' cannot be contradicted into enabling a sensor") { + auto e = resolveSensorEnable("none", /*flag_value=*/true, /*flag_present=*/true); + CHECK_FALSE(e.enabled); + CHECK(e.source == SensorEnableSource::Declaration); + CHECK(e.conflict); // named, so the stale key can be deleted +} + +TEST_CASE("a declaration beats a flag that disagrees, and the clash is reported") { + auto e = resolveSensorEnable("sht41", /*flag_value=*/false, /*flag_present=*/true); + CHECK(e.enabled); + CHECK(e.conflict); + + // Agreeing is not a conflict, merely redundant. + auto agree = resolveSensorEnable("sht41", true, true); + CHECK(agree.enabled); + CHECK_FALSE(agree.conflict); +}