From 76e777f5af04cf3a2774866cc2d2a67ad9645202 Mon Sep 17 00:00:00 2001 From: pgdalmeida Date: Fri, 11 Sep 2026 21:28:18 +0200 Subject: [PATCH] Add hardware.ini assembly config, build identity in the header, and ;prev calibration history Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018GsFaaLUTCYy3ZjaNTJzMv --- CMakeLists.txt | 19 ++++ config/hardware.example.ini | 79 +++++++++++++++ include/fgc/Config.h | 67 ++++++++++++- include/fgc/Paths.h | 22 +++++ main.cpp | 16 ++- src/core/Application.cpp | 42 +++++++- src/core/Config.cpp | 192 +++++++++++++++++++++++++++++++++--- src/core/Paths.cpp | 28 ++++++ tests/test_config.cpp | 181 +++++++++++++++++++++++++++++++++ tests/test_paths.cpp | 40 ++++++++ 10 files changed, 666 insertions(+), 20 deletions(-) create mode 100644 config/hardware.example.ini diff --git a/CMakeLists.txt b/CMakeLists.txt index 82e9e81..b325746 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -35,6 +35,22 @@ find_package(Boost REQUIRED CONFIG COMPONENTS program_options) find_package(PkgConfig REQUIRED) pkg_check_modules(JXL REQUIRED IMPORTED_TARGET libjxl libjxl_threads) +# --------------------------------------------------------------------------- +# Build identity. deploy.sh computes `describe --always --dirty` and passes it in +# the environment (GIT_REV=... cmake ...), so a deployed binary can name the +# revision it came from in the TUI header. A plain local build has no GIT_REV and +# falls back to "unknown", exactly as firmware/src/main.cpp does. +# +# Read at CONFIGURE time: `cmake --build` alone keeps whatever value the last +# configure saw. deploy.sh reconfigures on every deploy, so the deployed path is +# always fresh. +# --------------------------------------------------------------------------- +if(DEFINED ENV{GIT_REV} AND NOT "$ENV{GIT_REV}" STREQUAL "") + set(FGC_GIT_REV "$ENV{GIT_REV}") +else() + set(FGC_GIT_REV "unknown") +endif() + # --------------------------------------------------------------------------- # fgc_core: SDK-independent core (config, path resolution, and - as the # refactor proceeds - logging, the capture scheduler and the I/O interfaces). @@ -78,6 +94,8 @@ target_include_directories(fgc_core PUBLIC ${CMAKE_SOURCE_DIR} # for the bundled ini.h ) target_link_libraries(fgc_core PUBLIC Threads::Threads) +# PUBLIC so the application and the tests both see the same build identity. +target_compile_definitions(fgc_core PUBLIC FGC_GIT_REV="${FGC_GIT_REV}") # --------------------------------------------------------------------------- # Application executable. Camera image pipeline + serial controller always @@ -170,4 +188,5 @@ message(STATUS " WITH_VIMBA : ${WITH_VIMBA}") message(STATUS " WITH_MQTT : ${WITH_MQTT}") message(STATUS " WITH_TUI : ${WITH_TUI}") message(STATUS " tests : ${BUILD_TESTING}") +message(STATUS " build id : ${FGC_GIT_REV}") message(STATUS "------------------------------------------------------------") diff --git a/config/hardware.example.ini b/config/hardware.example.ini new file mode 100644 index 0000000..cf8d7a3 --- /dev/null +++ b/config/hardware.example.ini @@ -0,0 +1,79 @@ +; =========================================================================== +; hardware.ini - what this machine IS. +; +; Written once when the tower is assembled. Changing anything here means the +; hardware itself changed: a different board, a sensor fitted or removed, a +; cable moved to another port. Operational settings you tune to get better +; images - capture interval, camera profiles, scan grid, output paths - live in +; config.ini instead, and so does calibration, which is redone from time to time. +; +; WHERE THIS FILE GOES +; /etc/fire_gimbal_control/hardware.ini +; +; That is beside credentials.env, and it is deliberately OUTSIDE the directory +; deploy.sh rsyncs: a deploy cannot overwrite or delete what it never touches. +; The program looks, in order, at: +; --hardware | $FGC_HARDWARE | /etc/... | beside config.ini +; +; Keys here override the same keys in config.ini, and any that are still set in +; both are named in a warning at startup so the stale copy can be deleted. +; =========================================================================== + +; --- Which computer ------------------------------------------------------- +[Board] +; Free-form, but keep it matching a directory under hardware/ so the setup kit +; for this board is easy to find. +model = lattepanda-3-delta +; PlatformIO env used to build the motor firmware for this board's MCU. +; lattepanda-3-delta / sigma -> leonardo (ATmega32U4, 5V logic) +; lattepanda-iota -> iota_rp2040 (RP2040, 3.3V logic) +firmware_env = leonardo + +; --- What is fitted ------------------------------------------------------- +; This is a DECLARATION, and its whole purpose is that startup can compare it +; with what it actually finds. The program already copes gracefully when a part +; is missing - the pitch axis, the IMU, the ambient sensor - which is correct for +; a rig built without them, and indistinguishable from a fault on a rig that has +; them. Saying what was assembled is what tells those two apart. +[Hardware] +; 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. +imu = none +; Ambient temperature/humidity sensor: a model name, or 'none'. +env = sht41 +; The [Camera.] blocks expected to be present and found. +cameras = rgb + +; --- Where to find it ----------------------------------------------------- +; Device bindings. These follow the hardware, not the tower, so they belong here +; rather than in config.ini. + +[Serial] +; The motor controller MCU. Prefer the stable by-id symlink over /dev/ttyACM0, +; which is assigned in enumeration order and moves when USB devices change. +; Leonardo : /dev/serial/by-id/usb-Arduino_LLC_Arduino_Leonardo-if00 +device = /dev/serial/by-id/usb-Arduino_LLC_Arduino_Leonardo-if00 +baud = 115200 + +[IMU] +; Only read when [Hardware] imu is not 'none'. On the LattePanda 3 Delta the MTi +; is on the RS-232 header, which is a hardware UART: a stable /dev/ttyS* node, +; NOT a USB device and NOT /dev/ttyUSB0-3 (those are the modem). +device = /dev/ttyS0 + +[Env] +; Only read when [Hardware] env is not 'none'. This is the board's OWN native +; I2C bus, not the motor controller's serial link. The bus number varies by +; board - it is /dev/i2c-2 on the 3 Delta, not the /dev/i2c-1 default. +; +; Two traps when finding it: i2cdetect false-negatives on the SHT4x (probe with +; a real measurement instead), and an SMBus-only adapter can never work at all +; (i2cdetect -F N reports 'I2C = no'). +i2c_device = /dev/i2c-2 +i2c_addr = 0x44 + +[Camera.rgb] +; The serial of the physical camera fitted to this tower. A USB DEV_ id, or a +; GigE IP address. +id = DEV_1AB22C049CD1 diff --git a/include/fgc/Config.h b/include/fgc/Config.h index a622c8c..38ff0ff 100644 --- a/include/fgc/Config.h +++ b/include/fgc/Config.h @@ -19,6 +19,43 @@ struct GeneralConfig { bool debug = false; // print motor telemetry each loop tick }; +// [Board]: which computer this is. Assembly data - it changes only when the +// machine is physically replaced, so it belongs in hardware.ini, not config.ini. +struct BoardConfig { + std::string model; // e.g. "lattepanda-3-delta", "lattepanda-iota" + std::string firmware_env; // PlatformIO env for the motor MCU ("leonardo", ...) +}; + +// [Hardware]: what is actually fitted to THIS rig. +// +// The point of declaring it is that the rest of the program degrades gracefully +// when something is missing - TestRunner skips the pitch and IMU leaves, CamEvent +// falls back to encoder pitch, CaptureScheduler sends the single-axis MOVE form - +// which is right for a rig that genuinely lacks them, and indistinguishable from a +// FAILURE on a rig that does not. Declaring what was assembled is what lets +// startup tell those two apart. +struct HardwareConfig { + // Empty means "not declared": nothing was stated, so nothing is reconciled. + // This is the state every existing tower is in until it gets a hardware.ini. + std::vector axes; // e.g. {"yaw", "pitch"} + std::string imu; // sensor model, or "none" + std::string env; // sensor model, or "none" + std::vector cameras; // the [Camera.] blocks expected to exist + + // Was a hardware.ini actually read? False on every tower that has not been + // migrated yet, and the switch that keeps this change invisible on rollout. + bool declared = false; + // Where it came from, for logging and for `test hardware` to cite. + std::string source_path; + + // True when `name` is listed in axes. An UNDECLARED rig answers true for + // everything: with nothing stated there is nothing to contradict, and callers + // must keep behaving exactly as they did before. + bool hasAxis(const std::string& name) const; + // "yaw, pitch" - for log lines and test output. + std::string axesLabel() const; +}; + struct NetworkConfig { std::string broker_ip = "127.0.0.1"; // MQTT broker host/IP, or a full tcp://... URI int mqtt_port = 1883; // ignored when broker_ip carries its own port @@ -177,6 +214,8 @@ struct AppConfig { SerialConfig serial; CameraConfig camera; PathsConfig paths; + BoardConfig board; // [Board] from hardware.ini + HardwareConfig hardware; // [Hardware] from hardware.ini FeaturesConfig features; LoggingConfig logging; UiConfig ui; // [UI] terminal dashboard toggle @@ -205,7 +244,14 @@ public: // Reads the INI file at `path` (via the bundled inih parser), maps it into // a typed AppConfig, applies environment overrides, and validates it. // Throws std::runtime_error with a clear message on failure. - static AppConfig loadFromFile(const std::string& path); + // + // `hardware_path`, when given, is read AFTER the config and its keys win. It + // carries the assembly-time description of the machine ([Board], [Hardware], + // and the device bindings), which outlives any amount of operational tuning. + // Passing "" keeps the historical behaviour exactly: everything comes from + // config.ini and `hardware.declared` stays false. + static AppConfig loadFromFile(const std::string& path, + const std::string& hardware_path = ""); // Same mapping/validation logic, but from an already-parsed key->value map // ("Section.name" => value). Exposed for unit testing without file IO. @@ -216,14 +262,29 @@ public: // existing keys in that section are replaced in place (comments/order preserved), // and any missing ones are appended in a `[section]` block at the end. Pure (no // I/O) so it is unit-testable. +// +// With a non-empty `provenance`, a key whose value actually CHANGES also gets its +// previous value kept above it as a comment, so a calibration can be reverted by +// hand: +// +// ;prev yaw_heading_at_center = 0.0 ; 2026-09-11T14:32Z, gimbal heading setup +// yaw_heading_at_center = 127.4 +// +// Exactly one generation is kept - an existing `;prev` for a changing key is +// replaced, not appended to - and unchanged keys are left completely alone, so +// the file grows once and then holds steady. std::string updateIniSectionKeys(const std::string& contents, const std::string& section, - const std::vector>& kv); + const std::vector>& kv, + const std::string& provenance = ""); // Persist the live `[Motor]` geometry (counts_per_deg / center_count / // deg_at_center for both axes, plus the endstop margins) back into the INI file // at `path`. Written via a temp file and rename, so an interrupted save cannot // leave a truncated config behind. Returns true on success. +// +// `provenance` names what produced these values (e.g. "2026-09-11T14:32Z, gimbal +// heading setup"); when given, superseded values are kept as `;prev` comments. bool saveMotorGeometry(const std::string& path, const Geometry& geo, long yaw_margin, - long pitch_margin); + long pitch_margin, const std::string& provenance = ""); } // namespace fgc diff --git a/include/fgc/Paths.h b/include/fgc/Paths.h index ad7f0e3..19cea78 100644 --- a/include/fgc/Paths.h +++ b/include/fgc/Paths.h @@ -22,6 +22,28 @@ std::vector configSearchPaths(const std::string& cliArg = ""); // readable file wins. Returns nullopt if none is found. std::optional resolveConfigPath(const std::string& cliArg = ""); +// The ordered list of locations searched for hardware.ini - the assembly-time +// description of THIS machine (which board, which sensors are fitted, which +// device nodes they are on). Deliberately NOT derived from the config.ini path: +// +// --hardware explicit, for dev and testing +// $FGC_HARDWARE the service environment +// /etc/fire_gimbal_control/hardware.ini the normal answer on a tower +// /hardware.ini so a dev checkout just works +// +// /etc is where credentials.env already lives: root-owned, per-device, and +// OUTSIDE the deploy tree, so an rsync cannot overwrite or delete it. Keeping the +// assembly description there makes "a deploy never touches it" a property of +// where the file is, not of an exclude rule someone has to maintain. +std::vector hardwareSearchPaths(const std::string& cliArg = "", + const std::string& configPath = ""); + +// Resolve hardware.ini using hardwareSearchPaths(); first existing file wins. +// Returns nullopt when there is none - which is not an error: the program then +// falls back to reading the assembly keys from config.ini as it always has. +std::optional resolveHardwarePath(const std::string& cliArg = "", + const std::string& configPath = ""); + // Default image output directory when none is configured // ($XDG_DATA_HOME/fire_gimbal_control/images, else ~/.local/share/...). std::string defaultOutputDir(); diff --git a/main.cpp b/main.cpp index 1405667..45dff65 100644 --- a/main.cpp +++ b/main.cpp @@ -16,6 +16,9 @@ int main(int argc, char* argv[]) { desc.add_options() ("help,h", "show this help") ("config,c", po::value(), "path to config.ini") + ("hardware", po::value(), + "path to hardware.ini (default: $FGC_HARDWARE, /etc/fire_gimbal_control/hardware.ini, " + "or one beside config.ini)") ("init,i", po::bool_switch(), "home the gimbal on startup (required: nothing is homed across a power cycle)") ("start,s", po::bool_switch(), "start capture automatically") ("demo,d", po::bool_switch(), "demo mode (copy placeholder image instead of encoding)") @@ -56,9 +59,20 @@ int main(int argc, char* argv[]) { return 1; } + // hardware.ini describes the machine itself and is optional: a tower that has + // not been migrated yet simply has none, and everything comes from config.ini + // exactly as before. An explicit --hardware that does not exist IS an error, + // though - it means someone expected an assembly description to be in force. + const std::string cli_hw = vm.count("hardware") ? vm["hardware"].as() : ""; + auto hw_path = paths::resolveHardwarePath(cli_hw, *cfg_path); + if (!cli_hw.empty() && !hw_path) { + std::cerr << "No hardware.ini at --hardware " << cli_hw << "\n"; + return 1; + } + AppConfig cfg; try { - cfg = ConfigLoader::loadFromFile(*cfg_path); + cfg = ConfigLoader::loadFromFile(*cfg_path, hw_path.value_or("")); } catch (const std::exception& e) { std::cerr << "Config error (" << *cfg_path << "): " << e.what() << "\n"; return 1; diff --git a/src/core/Application.cpp b/src/core/Application.cpp index 0b7940b..3bf445f 100644 --- a/src/core/Application.cpp +++ b/src/core/Application.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -81,6 +82,24 @@ long long nowEpochMs() { .count(); } +// "2026-09-11T14:32Z" - UTC, minute resolution. Stamped into the `;prev` comments +// saveMotorGeometry leaves behind, where the point is telling two calibrations +// apart later, not sub-second precision. UTC because towers are read remotely and +// a local-time stamp says nothing without also knowing the host's zone. +std::string utcStampNow() { + const std::time_t t = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now()); + std::tm tm{}; +#if defined(_WIN32) + gmtime_s(&tm, &t); +#else + gmtime_r(&t, &tm); +#endif + char b[32]; + std::snprintf(b, sizeof(b), "%04d-%02d-%02dT%02d:%02dZ", tm.tm_year + 1900, tm.tm_mon + 1, + tm.tm_mday, tm.tm_hour, tm.tm_min); + return std::string(b); +} + // True if `dir` can be created and a probe file written there. A successful // create_directories alone isn't enough (the dir may exist read-only), so we // actually write+remove a probe file. @@ -219,6 +238,9 @@ struct Application::Impl { std::vector last_calib_summary; long long last_calib_ts = 0; // epoch ms bool calib_save_pending_ = false; // awaiting y/n to persist to config + // Which command produced the pending values, recorded into the `;prev` + // comment when they are saved so the file says what superseded what. + std::string calib_save_source_; // Hardware self-test (`test` command) results, for the activity strip. TestResultView last_test_view; @@ -607,7 +629,9 @@ struct Application::Impl { const bool mock_serial = opts.mock_serial.value_or(cfg.features.mock_serial); s.header.tower = cfg.general.tower_name; - s.header.build = "dev"; + // Set by CMake from $GIT_REV, which deploy.sh passes in; "unknown" for a + // plain local build. This is the TUI header's build id. + s.header.build = FGC_GIT_REV; s.header.uptime_ms = std::chrono::duration_cast( std::chrono::steady_clock::now() - start_time).count(); s.header.live = !mock_serial; @@ -1254,6 +1278,10 @@ struct Application::Impl { y.deg_at_center = r.deg_at_center; applyYawMapChange(); + // The landmark wizard and a bare `gimbal heading fit` land here alike; name + // whichever it was before the flag is cleared. + calib_save_source_ = heading_setup_active_ ? "gimbal heading setup" + : "gimbal heading fit"; heading_setup_active_ = false; // Offer to persist, through the same activity-strip prompt the pitch setup // uses - the operator answers y/n without leaving the dashboard. @@ -1490,12 +1518,19 @@ struct Application::Impl { } // Persist the live (just-calibrated) [Motor] geometry back into config.ini. + // Superseded values are kept above their key as `;prev` comments, stamped with + // when and what replaced them, so a calibration that turns out worse than the + // one it replaced can be reverted by uncommenting. void saveCalibration() { if (!calib_save_pending_) return; calib_save_pending_ = false; + const std::string provenance = + utcStampNow() + ", " + + (calib_save_source_.empty() ? std::string("calibration") : calib_save_source_); if (saveMotorGeometry(opts.config_path, cfg.geometry, cfg.yaw_soft_margin_counts, - cfg.pitch_soft_margin_counts)) - LOG_INFO << "[Motor] calibration saved as the new default in " << opts.config_path; + cfg.pitch_soft_margin_counts, provenance)) + LOG_INFO << "[Motor] calibration saved as the new default in " << opts.config_path + << " (previous values kept as ';prev' comments)"; else LOG_WARN << "could not write [Motor] calibration to " << opts.config_path; } @@ -1723,6 +1758,7 @@ struct Application::Impl { last_calib_view = v; last_calib_ts = nowEpochMs(); // Offer to persist it as the new default (answered via the activity strip). + calib_save_source_ = "gimbal pitch setup"; if (rep.valid && !opts.config_path.empty()) calib_save_pending_ = true; } } diff --git a/src/core/Config.cpp b/src/core/Config.cpp index f7d6a1e..ad82a11 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -98,6 +98,33 @@ bool getBool(const std::map& kv, const std::string& ke throw std::runtime_error("Config key '" + key + "' must be a boolean, got '" + v + "'"); } +// "yaw, pitch" -> {"yaw","pitch"}. Lowercased and trimmed; empty entries dropped, +// so trailing commas and stray spacing are not errors an operator has to hunt. +std::vector splitList(const std::string& s) { + std::vector out; + std::string cur; + auto flush = [&] { + size_t a = cur.find_first_not_of(" \t"); + if (a == std::string::npos) { cur.clear(); return; } + size_t b = cur.find_last_not_of(" \t"); + std::string t = cur.substr(a, b - a + 1); + for (char& c : t) c = static_cast(std::tolower((unsigned char)c)); + if (!t.empty()) out.push_back(t); + cur.clear(); + }; + for (char c : s) { + if (c == ',') flush(); + else cur += c; + } + flush(); + return out; +} + +std::string lowered(std::string s) { + for (char& c : s) c = static_cast(std::tolower((unsigned char)c)); + return s; +} + std::string envOr(const char* name, const std::string& fallback) { const char* v = std::getenv(name); return (v && *v) ? std::string(v) : fallback; @@ -210,6 +237,20 @@ AppConfig ConfigLoader::fromMap(const std::map& kv) { cfg.general.image_interval = getInt(kv, "General.image_interval", cfg.general.image_interval); cfg.general.debug = getBool(kv, "General.debug", cfg.general.debug); + // [Board] / [Hardware]: normally from hardware.ini, but read from the same + // merged map, so a config.ini carrying them still works during migration. + cfg.board.model = get(kv, "Board.model", cfg.board.model); + cfg.board.firmware_env = get(kv, "Board.firmware_env", cfg.board.firmware_env); + + cfg.hardware.axes = splitList(get(kv, "Hardware.axes")); + cfg.hardware.cameras = splitList(get(kv, "Hardware.cameras")); + cfg.hardware.imu = lowered(get(kv, "Hardware.imu")); + cfg.hardware.env = lowered(get(kv, "Hardware.env")); + for (const auto& a : cfg.hardware.axes) + if (a != "yaw" && a != "pitch") + throw std::runtime_error("Hardware.axes: unknown axis '" + a + + "' (expected yaw and/or pitch)"); + cfg.network.broker_ip = get(kv, "Network.zkms_server_ip", cfg.network.broker_ip); cfg.network.mqtt_port = getInt(kv, "Network.mqtt_port", cfg.network.mqtt_port); if (cfg.network.mqtt_port < 1 || cfg.network.mqtt_port > 65535) @@ -430,14 +471,56 @@ AppConfig ConfigLoader::fromMap(const std::map& kv) { return cfg; } -AppConfig ConfigLoader::loadFromFile(const std::string& path) { +bool HardwareConfig::hasAxis(const std::string& name) const { + // An undeclared rig answers true for everything - see the header. + if (axes.empty()) return true; + return std::find(axes.begin(), axes.end(), name) != axes.end(); +} + +std::string HardwareConfig::axesLabel() const { + std::string s; + for (const auto& a : axes) s += (s.empty() ? "" : ", ") + a; + return s; +} + +AppConfig ConfigLoader::loadFromFile(const std::string& path, const std::string& hardware_path) { std::map kv; int rc = ini_parse(path.c_str(), iniHandler, &kv); if (rc < 0) throw std::runtime_error("Cannot read config file: " + path); if (rc > 0) throw std::runtime_error("Parse error in config file " + path + " at line " + std::to_string(rc)); + + // hardware.ini is layered ON TOP, so the assembly description wins over a + // config.ini that still carries the same keys from before the split. Any such + // leftover is named once, because it is a migration leftover to delete rather + // than a setting that is quietly doing nothing. + std::vector shadowed; + if (!hardware_path.empty()) { + std::map hw; + int hrc = ini_parse(hardware_path.c_str(), iniHandler, &hw); + if (hrc < 0) throw std::runtime_error("Cannot read hardware file: " + hardware_path); + if (hrc > 0) + throw std::runtime_error("Parse error in hardware file " + hardware_path + + " at line " + std::to_string(hrc)); + for (const auto& [k, v] : hw) { + if (auto it = kv.find(k); it != kv.end() && it->second != v) shadowed.push_back(k); + kv[k] = v; + } + } + AppConfig cfg = fromMap(kv); + + if (!hardware_path.empty()) { + cfg.hardware.declared = true; + cfg.hardware.source_path = hardware_path; + LOG_INFO << "hardware: " << hardware_path + << (cfg.board.model.empty() ? "" : " board " + cfg.board.model) + << (cfg.hardware.axes.empty() ? "" : " axes " + cfg.hardware.axesLabel()); + for (const auto& k : shadowed) + LOG_WARN << "config.ini still sets " << k + << ", which hardware.ini overrides -- delete it from config.ini"; + } // Anchor a relative scan grid_file to the config file's directory so it resolves // no matter which working directory the process is launched from (the config // itself is found via a search path that may be unrelated to the cwd). @@ -463,10 +546,80 @@ std::string trimmed(const std::string& s) { size_t b = s.find_last_not_of(ws); return s.substr(a, b - a + 1); } + +// Marker for a superseded value, kept so a calibration can be reverted by hand. +// It is a comment, so inih ignores it on the next read, but it is distinctive +// enough for updateIniSectionKeys to find its own earlier markers and replace +// them rather than pile them up. +const std::string kPrevMarker = ";prev "; + +// The value inih would actually parse from the right-hand side of `key = value`: +// everything up to an inline comment, which is a ';' PRECEDED BY WHITESPACE (see +// ini_find_chars_or_comment in ini.c, with INI_ALLOW_INLINE_COMMENTS=1 and +// INI_INLINE_COMMENT_PREFIXES=";"). Mirroring that rule matters here: "did this +// value change?" has to compare what the program read, not the raw line, or a +// key carrying a trailing comment would look changed on every save. +std::string iniValueOf(const std::string& rhs) { + bool was_space = false; + for (size_t i = 0; i < rhs.size(); ++i) { + if (was_space && rhs[i] == ';') return trimmed(rhs.substr(0, i)); + was_space = std::isspace(static_cast(rhs[i])) != 0; + } + return trimmed(rhs); +} + +// If `t` (already trimmed) is a ";prev = ..." marker, return , else "". +std::string prevMarkerKey(const std::string& t) { + const size_t n = kPrevMarker.size(); + if (t.compare(0, n, kPrevMarker) != 0) return ""; + const size_t eq = t.find('=', n); + if (eq == std::string::npos) return ""; + return trimmed(t.substr(n, eq - n)); +} } // namespace std::string updateIniSectionKeys(const std::string& contents, const std::string& section, - const std::vector>& kv) { + const std::vector>& kv, + const std::string& provenance) { + // Which keys are actually changing, and what they hold now. + // + // Only a changing key gets a ";prev" line, and only its marker is replaced: + // a key whose value is unchanged keeps the history it has, because that still + // names the value before the current one. This also keeps the file from + // growing on every save - saveMotorGeometry rewrites all eight keys each time, + // and without this filter seven of them would be noise. + // + // Computed in a first pass because a key's ";prev" marker sits ABOVE its value + // line, so the writing pass meets the marker before it has seen whether the + // value differs. + std::vector changed(kv.size(), false); + std::vector old_value(kv.size()); + { + std::istringstream scan(contents); + std::string line, cur; + std::vector seen(kv.size(), false); + while (std::getline(scan, line)) { + const std::string t = trimmed(line); + if (t.size() >= 2 && t.front() == '[' && t.back() == ']') { + cur = trimmed(t.substr(1, t.size() - 2)); + continue; + } + if (cur != section || t.empty() || t[0] == ';' || t[0] == '#') continue; + const size_t eq = line.find('='); + if (eq == std::string::npos) continue; + const std::string key = trimmed(line.substr(0, eq)); + for (size_t i = 0; i < kv.size(); ++i) { + if (!seen[i] && key == kv[i].first) { + seen[i] = true; + old_value[i] = iniValueOf(line.substr(eq + 1)); + changed[i] = old_value[i] != kv[i].second; + break; + } + } + } + } + const bool keep_history = !provenance.empty(); + std::vector written(kv.size(), false); std::istringstream in(contents); std::ostringstream out; @@ -475,15 +628,28 @@ std::string updateIniSectionKeys(const std::string& contents, const std::string& const std::string t = trimmed(line); if (t.size() >= 2 && t.front() == '[' && t.back() == ']') { cur = trimmed(t.substr(1, t.size() - 2)); - } else if (cur == section && !t.empty() && t[0] != ';' && t[0] != '#') { - const size_t eq = line.find('='); - if (eq != std::string::npos) { - const std::string key = trimmed(line.substr(0, eq)); - for (size_t i = 0; i < kv.size(); ++i) { - if (!written[i] && key == kv[i].first) { - line = kv[i].first + " = " + kv[i].second; - written[i] = true; - break; + } else if (cur == section) { + // An earlier ";prev" for a key that is changing now is dropped here + // and re-emitted below, so exactly one generation is ever kept. + const std::string pk = prevMarkerKey(t); + if (!pk.empty()) { + bool supersede = false; + for (size_t i = 0; i < kv.size(); ++i) + if (pk == kv[i].first && changed[i]) { supersede = true; break; } + if (supersede) continue; + } else if (!t.empty() && t[0] != ';' && t[0] != '#') { + const size_t eq = line.find('='); + if (eq != std::string::npos) { + const std::string key = trimmed(line.substr(0, eq)); + for (size_t i = 0; i < kv.size(); ++i) { + if (!written[i] && key == kv[i].first) { + if (keep_history && changed[i]) + out << kPrevMarker << kv[i].first << " = " << old_value[i] + << " ; " << provenance << "\n"; + line = kv[i].first + " = " + kv[i].second; + written[i] = true; + break; + } } } } @@ -503,7 +669,7 @@ std::string updateIniSectionKeys(const std::string& contents, const std::string& } bool saveMotorGeometry(const std::string& path, const Geometry& geo, long yaw_margin, - long pitch_margin) { + long pitch_margin, const std::string& provenance) { if (path.empty()) return false; std::ifstream f(path); if (!f) return false; @@ -529,7 +695,7 @@ bool saveMotorGeometry(const std::string& path, const Geometry& geo, long yaw_ma {"pitch_angle_at_center", num(geo.pitch.deg_at_center)}, {"pitch_soft_margin_counts", std::to_string(pitch_margin)}, }; - const std::string updated = updateIniSectionKeys(buf.str(), "Motor", kv); + const std::string updated = updateIniSectionKeys(buf.str(), "Motor", kv, provenance); // Write a sibling temp file and rename over the original: a truncating write // that dies partway leaves config.ini destroyed, and three commands rewrite diff --git a/src/core/Paths.cpp b/src/core/Paths.cpp index d1783ad..6c49316 100644 --- a/src/core/Paths.cpp +++ b/src/core/Paths.cpp @@ -92,6 +92,34 @@ std::optional resolveConfigPath(const std::string& cliArg) { return std::nullopt; } +std::vector hardwareSearchPaths(const std::string& cliArg, + const std::string& configPath) { + std::vector paths; + if (!cliArg.empty()) paths.push_back(expandUser(cliArg)); + + if (const char* env = std::getenv("FGC_HARDWARE"); env && *env) + paths.push_back(expandUser(env)); + + paths.push_back("/etc/fire_gimbal_control/hardware.ini"); + + // Beside config.ini, so a checkout with a local config.ini picks up a local + // hardware.ini without needing root or an env var. + if (!configPath.empty()) { + const fs::path dir = fs::path(expandUser(configPath)).parent_path(); + paths.push_back((dir.empty() ? fs::path(".") : dir) / "hardware.ini"); + } + return paths; +} + +std::optional resolveHardwarePath(const std::string& cliArg, + const std::string& configPath) { + std::error_code ec; + for (const auto& p : hardwareSearchPaths(cliArg, configPath)) { + if (fs::is_regular_file(p, ec)) return p; + } + return std::nullopt; +} + std::string defaultOutputDir() { std::string base = envOr("XDG_DATA_HOME", expandUser("~/.local/share")); return (fs::path(base) / "fire_gimbal_control" / "images").string(); diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 3bf9d86..be92122 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -12,6 +12,20 @@ using namespace fgc; +namespace { +// How many times `needle` appears in `hay`. Used to assert that calibration +// history replaces itself rather than accumulating. +int countOccurrences(const std::string& hay, const std::string& needle) { + int n = 0; + size_t i = hay.find(needle); + while (i != std::string::npos) { + ++n; + i = hay.find(needle, i + needle.size()); + } + return n; +} +} // namespace + TEST_CASE("ConfigLoader maps and defaults typed values") { unsetenv("FGC_MQTT_USER"); unsetenv("FGC_MQTT_PW"); @@ -260,6 +274,90 @@ TEST_CASE("updateIniSectionKeys appends keys missing from the section") { CHECK(out.find("pitch_center_count = 7") != std::string::npos); } +TEST_CASE("updateIniSectionKeys keeps the superseded value as a ';prev' comment") { + const std::string in = + "[Motor]\n" + "; the heading the gimbal points at when parked\n" + "yaw_heading_at_center = 0.0\n" + "yaw_center_count = 500000\n"; + + auto out = updateIniSectionKeys(in, "Motor", + {{"yaw_heading_at_center", "127.4"}, + {"yaw_center_count", "500000"}}, // unchanged + "2026-09-11T14:32Z, gimbal heading setup"); + + SUBCASE("the changed key keeps its old value, stamped") { + CHECK(out.find(";prev yaw_heading_at_center = 0.0") != std::string::npos); + CHECK(out.find("2026-09-11T14:32Z, gimbal heading setup") != std::string::npos); + CHECK(out.find("yaw_heading_at_center = 127.4") != std::string::npos); + } + SUBCASE("the history sits directly above the value it superseded") { + CHECK(out.find(";prev yaw_heading_at_center = 0.0 ; 2026-09-11T14:32Z, gimbal " + "heading setup\nyaw_heading_at_center = 127.4") != std::string::npos); + } + SUBCASE("an UNCHANGED key gets no history - otherwise every save adds noise") { + CHECK(out.find(";prev yaw_center_count") == std::string::npos); + } + SUBCASE("hand-written comments survive") { + CHECK(out.find("; the heading the gimbal points at when parked") != std::string::npos); + } +} + +TEST_CASE("updateIniSectionKeys keeps exactly one generation of history") { + // Saving twice must REPLACE the marker, not stack markers up: the file has to + // grow once and then hold steady however often the gimbal is recalibrated. + std::string s = "[Motor]\nyaw_heading_at_center = 0.0\n"; + s = updateIniSectionKeys(s, "Motor", {{"yaw_heading_at_center", "10"}}, "t1, first"); + s = updateIniSectionKeys(s, "Motor", {{"yaw_heading_at_center", "20"}}, "t2, second"); + s = updateIniSectionKeys(s, "Motor", {{"yaw_heading_at_center", "30"}}, "t3, third"); + + CHECK(countOccurrences(s, ";prev yaw_heading_at_center") == 1); + CHECK(s.find(";prev yaw_heading_at_center = 20") != std::string::npos); // the one before + CHECK(s.find("yaw_heading_at_center = 30") != std::string::npos); + CHECK(s.find("= 0.0") == std::string::npos); // the oldest is gone + CHECK(s.find("t3, third") != std::string::npos); + CHECK(s.find("t1, first") == std::string::npos); +} + +TEST_CASE("updateIniSectionKeys history respects inline comments and unchanged values") { + // inih treats ' ;' as an inline comment, so the stored value is "983.33", not + // "983.33 ; measured". Comparing raw text instead would call this key changed + // on every save and bury the file in useless history. + const std::string in = "[Motor]\nyaw_counts_per_deg = 983.33 ; measured on the rig\n"; + + SUBCASE("same value behind an inline comment counts as unchanged") { + auto out = updateIniSectionKeys(in, "Motor", {{"yaw_counts_per_deg", "983.33"}}, "t, c"); + CHECK(out.find(";prev") == std::string::npos); + } + SUBCASE("a real change records only the parsed value, not the comment") { + auto out = updateIniSectionKeys(in, "Motor", {{"yaw_counts_per_deg", "1000"}}, "t, c"); + CHECK(out.find(";prev yaw_counts_per_deg = 983.33 ; t, c") != std::string::npos); + } +} + +TEST_CASE("updateIniSectionKeys writes no history without a provenance") { + // The default keeps the old behaviour, so callers that have nothing useful to + // say about where the values came from do not litter the file. + const std::string in = "[Motor]\nyaw_center_count = 1\n"; + auto out = updateIniSectionKeys(in, "Motor", {{"yaw_center_count", "2"}}); + CHECK(out.find(";prev") == std::string::npos); + CHECK(out.find("yaw_center_count = 2") != std::string::npos); +} + +TEST_CASE("a ';prev' comment does not survive a reload as a real key") { + // The marker has to be inert to inih, or reverting by uncommenting would be + // pointless: the stale value would already be in force. + std::string s = "[General]\ntower_name = T\n[Motor]\nyaw_center_count = 1\n"; + s = updateIniSectionKeys(s, "Motor", {{"yaw_center_count", "2"}}, "t, c"); + REQUIRE(s.find(";prev yaw_center_count = 1") != std::string::npos); + + const std::string path = "test_prev_marker_reload.ini"; + { std::ofstream(path) << s; } + auto cfg = ConfigLoader::loadFromFile(path); + CHECK(cfg.geometry.yaw.center_count == 2); + std::filesystem::remove(path); +} + TEST_CASE("saveMotorGeometry round-trips the full per-axis map") { // Write a starting config, save a calibrated Geometry into it, reload, and // confirm every [Motor] parameter survives, while another section is left @@ -401,6 +499,89 @@ TEST_CASE("[IMU] x/y/z describe the mount and default to no rotation") { CHECK_FALSE(c.imu.mount.identity()); } +TEST_CASE("hardware.ini layers over config.ini and wins") { + const std::string cfg_path = "test_hw_layer_config.ini"; + const std::string hw_path = "test_hw_layer_hardware.ini"; + { + std::ofstream c(cfg_path); + c << "[General]\ntower_name = NorthRidge\nimage_interval = 3\n" + "[Camera.rgb]\nid = OLD_ID\nprofile_dir = /tmp/p\n" + "[Serial]\ndevice = /dev/ttyACM0\n"; + std::ofstream h(hw_path); + h << "[Board]\nmodel = lattepanda-iota\nfirmware_env = iota_rp2040\n" + "[Hardware]\naxes = yaw, pitch\nimu = none\nenv = sht41\ncameras = rgb\n" + "[Serial]\ndevice = /dev/serial/by-id/usb-RP2040-if00\n" + "[Camera.rgb]\nid = NEW_ID\n"; + } + auto c = ConfigLoader::loadFromFile(cfg_path, hw_path); + + SUBCASE("assembly data arrives from hardware.ini") { + CHECK(c.board.model == "lattepanda-iota"); + CHECK(c.board.firmware_env == "iota_rp2040"); + CHECK(c.hardware.axes == std::vector{"yaw", "pitch"}); + CHECK(c.hardware.imu == "none"); + CHECK(c.hardware.env == "sht41"); + CHECK(c.hardware.cameras == std::vector{"rgb"}); + CHECK(c.hardware.declared); + CHECK(c.hardware.source_path == hw_path); + } + SUBCASE("bindings in hardware.ini override the ones left in config.ini") { + CHECK(c.serial.device == "/dev/serial/by-id/usb-RP2040-if00"); + REQUIRE(c.camera.cameras.size() == 1); + CHECK(c.camera.cameras[0].id == "NEW_ID"); + } + SUBCASE("operational settings stay with config.ini") { + CHECK(c.general.tower_name == "NorthRidge"); + CHECK(c.general.image_interval == 3); + } + std::filesystem::remove(cfg_path); + std::filesystem::remove(hw_path); +} + +TEST_CASE("without a hardware.ini nothing changes and nothing is declared") { + // The migration guarantee: an un-migrated tower must behave exactly as before. + const std::string path = "test_hw_absent.ini"; + { + std::ofstream c(path); + c << "[General]\ntower_name = Old\n" + "[Camera.rgb]\nid = D\nprofile_dir = /tmp/p\n" + "[Serial]\ndevice = /dev/ttyACM0\n"; + } + auto c = ConfigLoader::loadFromFile(path); + CHECK_FALSE(c.hardware.declared); + CHECK(c.hardware.axes.empty()); + CHECK(c.serial.device == "/dev/ttyACM0"); + CHECK(c.general.tower_name == "Old"); + + // An undeclared rig must not answer "no" to any axis, or every caller that + // asks would start refusing work on towers that have not been migrated. + CHECK(c.hardware.hasAxis("yaw")); + CHECK(c.hardware.hasAxis("pitch")); + std::filesystem::remove(path); +} + +TEST_CASE("Hardware.axes parsing and membership") { + auto c = ConfigLoader::fromMap({{"Camera.rgb.id", "D"}, + {"Camera.rgb.profile_dir", "/tmp/p"}, + {"Hardware.axes", " YAW ,, pitch ,"}}); + SUBCASE("case, spacing and stray commas are tolerated") { + CHECK(c.hardware.axes == std::vector{"yaw", "pitch"}); + CHECK(c.hardware.axesLabel() == "yaw, pitch"); + } + SUBCASE("a declared yaw-only rig answers no to pitch") { + auto y = ConfigLoader::fromMap({{"Camera.rgb.id", "D"}, + {"Camera.rgb.profile_dir", "/tmp/p"}, + {"Hardware.axes", "yaw"}}); + CHECK(y.hardware.hasAxis("yaw")); + CHECK_FALSE(y.hardware.hasAxis("pitch")); + } + SUBCASE("an unknown axis is a typo worth refusing, not ignoring") { + CHECK_THROWS(ConfigLoader::fromMap({{"Camera.rgb.id", "D"}, + {"Camera.rgb.profile_dir", "/tmp/p"}, + {"Hardware.axes", "yaw, roll"}})); + } +} + TEST_CASE("A mount angle that cannot be a rotation is rejected") { auto with = [](const std::string& k, const std::string& v) { return ConfigLoader::fromMap({{"Camera.rgb.id", "D"}, diff --git a/tests/test_paths.cpp b/tests/test_paths.cpp index 62931ce..51b93a0 100644 --- a/tests/test_paths.cpp +++ b/tests/test_paths.cpp @@ -23,3 +23,43 @@ TEST_CASE("configSearchPaths honours the CLI argument first") { REQUIRE(!p.empty()); CHECK(p.front() == "/explicit/config.ini"); } + +TEST_CASE("hardwareSearchPaths looks in the documented order") { + unsetenv("FGC_HARDWARE"); + auto p = paths::hardwareSearchPaths("", "/opt/fgc/config.ini"); + + auto at = [&](const std::string& s) { + for (size_t i = 0; i < p.size(); ++i) + if (p[i] == s) return static_cast(i); + return -1; + }; + // /etc is the normal answer on a tower - root-owned, per-device, and OUTSIDE + // the deploy tree - so it is searched before a checkout-local file. + REQUIRE(at("/etc/fire_gimbal_control/hardware.ini") >= 0); + REQUIRE(at("/opt/fgc/hardware.ini") >= 0); + CHECK(at("/etc/fire_gimbal_control/hardware.ini") < at("/opt/fgc/hardware.ini")); + + SUBCASE("an explicit argument outranks everything") { + auto q = paths::hardwareSearchPaths("/tmp/mine.ini", "/opt/fgc/config.ini"); + CHECK(q.front() == "/tmp/mine.ini"); + } + SUBCASE("the environment outranks /etc but not the argument") { + setenv("FGC_HARDWARE", "/tmp/env.ini", 1); + auto q = paths::hardwareSearchPaths("/tmp/cli.ini", "/opt/fgc/config.ini"); + REQUIRE(q.size() >= 2); + CHECK(q[0] == "/tmp/cli.ini"); + CHECK(q[1] == "/tmp/env.ini"); + unsetenv("FGC_HARDWARE"); + } + SUBCASE("with no config path there is simply no sibling candidate") { + auto q = paths::hardwareSearchPaths("", ""); + for (const auto& s : q) CHECK(s.find("/opt/fgc/") == std::string::npos); + } +} + +TEST_CASE("resolveHardwarePath returns nothing when there is no hardware.ini") { + // Not an error: every existing tower is in this state until it is migrated, + // and the program must go on reading everything from config.ini. + unsetenv("FGC_HARDWARE"); + CHECK_FALSE(paths::resolveHardwarePath("/nonexistent/nope.ini", "").has_value()); +}