From 86f930a67137097ce08acd59a8eedd95511e6d08 Mon Sep 17 00:00:00 2001 From: pgdalmeida Date: Wed, 23 Sep 2026 18:52:44 +0200 Subject: [PATCH] Add a one-shot test command and a tool that edits the config files safely --- CMakeLists.txt | 9 + docs/tower-installation.md | 25 ++- include/fgc/Application.h | 11 + include/fgc/IniEdit.h | 56 +++++ include/fgc/mock/MockMotorController.h | 19 +- main.cpp | 20 ++ src/core/Application.cpp | 58 ++++- src/core/IniEdit.cpp | 118 ++++++++++ tests/CMakeLists.txt | 1 + tests/test_firmwareconfig.cpp | 40 ++++ tests/test_iniedit.cpp | 106 +++++++++ tools/fgc_config.cpp | 298 +++++++++++++++++++++++++ 12 files changed, 753 insertions(+), 8 deletions(-) create mode 100644 include/fgc/IniEdit.h create mode 100644 src/core/IniEdit.cpp create mode 100644 tests/test_iniedit.cpp create mode 100644 tools/fgc_config.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 0359561..2a6f643 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -59,6 +59,7 @@ endif() # --------------------------------------------------------------------------- add_library(fgc_core STATIC src/core/Config.cpp + src/core/IniEdit.cpp src/core/Paths.cpp src/core/SingleInstance.cpp src/core/Logger.cpp @@ -176,6 +177,14 @@ else() message(STATUS "WITH_TUI=OFF: building without the terminal dashboard (headless console only)") endif() +# --------------------------------------------------------------------------- +# fgc-config: writes and edits config.ini / hardware.ini, validating every +# result through the same loader as the program. Core-only, so it builds +# wherever fgc_core does; the tower installer depends on it. +# --------------------------------------------------------------------------- +add_executable(fgc-config tools/fgc_config.cpp) +target_link_libraries(fgc-config PRIVATE fgc_core) + # --------------------------------------------------------------------------- # Tests # --------------------------------------------------------------------------- diff --git a/docs/tower-installation.md b/docs/tower-installation.md index d922f6c..9ff519b 100644 --- a/docs/tower-installation.md +++ b/docs/tower-installation.md @@ -388,8 +388,13 @@ No new RPxxxx device found yet, waiting.. (x20, then it gives up) | `picotool reboot -f -u` | refused: no reset interface | - | | MicroPython REPL | **works** | n/a | -So drop into MicroPython's own REPL and tell it to enter the bootloader. Write -this to a file and run it with `~/.platformio/penv/bin/python` (it has pyserial): +So drop into MicroPython's own REPL and tell it to enter the bootloader. +`firmware/tools/flash.sh` (which `deploy.sh` runs on the tower) now does this by +itself when the port's by-id name says `MicroPython_Board`: `./deploy.sh` with +`SERIAL_PORT` set to that path should flash a factory board directly. `[untested]` +- no factory board has been through that path yet; the manual way, verified on +tower 121, is below. Write this to a file and run it with +`~/.platformio/penv/bin/python` (it has pyserial): ```python import serial, time @@ -904,6 +909,22 @@ table and lets the tower move while you work out what `hardware.ini` is missing. ## 11. Verification checklist +The checks that move nothing can be run as one command, which is what a script +should use rather than typing into the dashboard. Stop the app first: it holds +the serial port. + +```bash +scripts/fgc stop +GENICAM_GENTL64_PATH=/opt/VimbaX/cti ./build/fire_gimbal_control --no-mqtt --test hardware +echo $? # 0 PASS, 4 FAIL, 5 could not start (the log says why) +scripts/fgc start +``` + +It waits for the firmware to be talking and configured before it looks, so a +freshly flashed board is not failed for values the host had not sent yet. +`build/fgc-config check --config config.ini --hardware /etc/fire_gimbal_control/hardware.ini` +checks that the two files load together, and changes nothing. + | Check | How | Tower 121 | |---|---|---| | Key-only ssh | `ssh ggs@ hostname`, no prompt | ✅ | diff --git a/include/fgc/Application.h b/include/fgc/Application.h index 514b0ba..0ceb851 100644 --- a/include/fgc/Application.h +++ b/include/fgc/Application.h @@ -28,8 +28,19 @@ struct RuntimeOptions { std::string log_level; // empty => default std::string trace_categories; // comma list (serial,mqtt,camera,control,all); empty => unset std::string config_path; // resolved config.ini path (to save calibration back); empty => none + + // --test: run this one self-test selection (the words after `test` at the + // console, e.g. "hardware" or "hardware motor") once the motor link is up, + // then exit with its verdict. Empty => normal operation. For scripts - the + // installer above all - which need an answer, not a dashboard. + std::string run_test; }; +// Exit codes of a --test run, beside main()'s 1 (config), 2 (lock) and +// Application's 3 (declared hardware missing). +constexpr int kExitTestFail = 4; // the test ran and failed +constexpr int kExitTestNotRun = 5; // the test could not start; the log says why + // Owns the component lifecycle and the control loop. Builds the concrete // motor controller, control channel and camera source from config + options // (real or mock), wires them to the ImagePipeline and CaptureScheduler, and diff --git a/include/fgc/IniEdit.h b/include/fgc/IniEdit.h new file mode 100644 index 0000000..8a5a0b0 --- /dev/null +++ b/include/fgc/IniEdit.h @@ -0,0 +1,56 @@ +#pragma once + +// Editing the two INI files a tower is configured by - config.ini and +// hardware.ini - without breaking them. Pure functions on file contents, so +// every rule is unit-testable; tools/fgc_config.cpp does the file IO around them. +// +// The rules exist because each has already broken a real file: +// - inih SPLITS a line longer than its buffer instead of truncating it, and the +// tail then parses as a bogus line of its own (see ini.h INI_MAX_LINE). A +// generated comment of exactly 200 characters once took out a whole config. +// - A value left in config.ini that hardware.ini also sets is dead, and warned +// about at every start. A placeholder left there is worse: deploy.sh's guard +// greps the raw text, so even a commented-out DEV_0000000000 trips it. + +#include +#include +#include +#include +#include + +namespace fgc { + +// The longest line content inih reads whole. Its buffer is INI_MAX_LINE bytes +// and holds the newline and the terminator too. +std::size_t iniMaxLineContent(); + +// 1-based number of the first line too long for inih to read whole, or 0. +int firstOverlongIniLine(const std::string& contents); + +// "Camera.rgb.id" -> {"Camera.rgb", "id"}: the key is after the LAST dot, since +// section names may contain dots and key names never do. nullopt when there is +// no dot, or either side is empty. +std::optional> splitIniKey(const std::string& flat); + +// Every key the contents set, as "Section.key" -> value, exactly as the program +// reads them. Throws std::runtime_error naming the line if inih rejects it. +std::map parseIniString(const std::string& contents); + +// Set "Section.key" = value pairs. Grouped per section and applied through +// updateIniSectionKeys(), so comments and order survive, a missing key is +// appended, and a non-empty `provenance` keeps one ";prev" generation per +// changed key. Throws std::invalid_argument on a malformed key. +std::string applyIniSets(const std::string& contents, + const std::vector>& sets, + const std::string& provenance); + +// Replace each live `key = value` line whose "Section.key" is in `drops` with a +// one-line comment "; ", where note is the map's value. The VALUE is +// deliberately not kept: a dead placeholder in a comment still trips deploy.sh's +// raw-text guard, and a dead real value invites someone to "fix" the wrong file. +// Returns the new contents; `dropped` (if given) receives what was replaced. +std::string dropIniKeys(const std::string& contents, + const std::map& drops, + std::vector* dropped = nullptr); + +} // namespace fgc diff --git a/include/fgc/mock/MockMotorController.h b/include/fgc/mock/MockMotorController.h index ad6fa9d..cf1371d 100644 --- a/include/fgc/mock/MockMotorController.h +++ b/include/fgc/mock/MockMotorController.h @@ -122,7 +122,6 @@ public: // encoder positions track the mock's live state. // 5 = NEEDCFG, appended to the firmware enum so the older codes keep // their meanings. See motor.h. - const int st = (needcfg_ && !configured_) ? 5 : (homed_ ? 3 : 1); std::ostringstream d; d << "DUMP BEGIN build=mock uptime=0 mcusr=0x01 free_ram=4096"; if (needcfg_ && pins_.spi_sck >= 0) @@ -153,6 +152,7 @@ public: " irun=" + val("run_irun", "0") + " irun_home=" + val("homing_irun", "0"); } + const int st = (needcfg_ && !axisConfigured(L)) ? 5 : (homed_ ? 3 : 1); d << "DUMP " << L << " state=" << st << " hsub=0 enabled=" << (homed_ ? 1 : 0) << " lim_neg=0 lim_pos=1000000 hold_target=" << a.xenc @@ -205,8 +205,8 @@ public: MotorTelemetry telemetry() override { std::lock_guard lock(mutex_); - step(yaw_, yaw_target_); - step(pitch_, pitch_target_); + step('Y', yaw_, yaw_target_); + step('P', pitch_, pitch_target_); MotorTelemetry t; t.yaw = yaw_; t.pitch = pitch_; @@ -238,6 +238,15 @@ public: return settings_; } bool configured() const { return configured_; } + // One axis has every key it asked for. The real firmware leaves NEEDCFG per + // axis, so a yaw-only tower - which rightly never answers for pitch - still + // gets a configured yaw. + bool axisConfigured(char L) const { + const std::string pre = std::string(1, L) + "."; + for (const auto& k : unanswered_) + if (k.compare(0, pre.size(), pre) == 0) return false; + return true; + } bool connected() const override { return true; } @@ -251,7 +260,7 @@ private: static constexpr long kStep = 80000; // counts advanced per telemetry read - void step(AxisTelemetry& a, long target) { + void step(char L, AxisTelemetry& a, long target) { long d = target - a.xenc; if (d >= -kStep && d <= kStep) { a.xenc = target; @@ -263,7 +272,7 @@ private: a.xactual = a.xenc; // An axis waiting on its config says so, rather than looking like a // healthy un-homed one - that distinction is the whole point of the state. - a.state = (needcfg_ && !configured_) ? AxisState::NeedConfig + a.state = (needcfg_ && !axisConfigured(L)) ? AxisState::NeedConfig : (homed_ ? AxisState::Ready : AxisState::Reset); a.deviation = deviation_; } diff --git a/main.cpp b/main.cpp index b1c1dfb..a6c73af 100644 --- a/main.cpp +++ b/main.cpp @@ -70,6 +70,10 @@ int main(int argc, char* argv[]) { ("no-log-file", po::bool_switch(), "do not write a persistent log file this run") ("log-file-level", po::value(), "log-file verbosity, independent of --log-level (default debug)") + ("test", po::value(), + "run one self-test once the motor link is up, then exit: 0 PASS, 4 FAIL, 5 could " + "not start. Takes what follows `test` at the console, e.g. --test hardware or " + "--test \"hardware motor\". Headless; cannot be combined with --start") ("trace", po::value(), "verbatim wire trace, comma list: serial,mqtt,camera,control,all,none"); @@ -85,6 +89,18 @@ int main(int argc, char* argv[]) { std::cout << desc << "\n"; return 0; } + if (vm.count("test")) { + const std::string t = vm["test"].as(); + if (t.find_first_not_of(" \t") == std::string::npos) { + std::cerr << "Argument error: --test needs a selection, e.g. --test hardware\n"; + return 1; + } + if (vm["start"].as()) { + // Capture would move the gimbal and hold the serial link the test needs. + std::cerr << "Argument error: --test cannot be combined with --start\n"; + return 1; + } + } const std::string cli_cfg = vm.count("config") ? vm["config"].as() : ""; auto cfg_path = paths::resolveConfigPath(cli_cfg); @@ -200,6 +216,10 @@ int main(int argc, char* argv[]) { if (vm.count("log-level")) opts.log_level = vm["log-level"].as(); if (vm.count("trace")) opts.trace_categories = vm["trace"].as(); opts.config_path = *cfg_path; // so the setup commands can save back to it + if (vm.count("test")) { + opts.run_test = vm["test"].as(); + opts.use_tui = false; // a script wants lines on stdout, not a dashboard + } Application app(std::move(cfg), std::move(opts)); try { diff --git a/src/core/Application.cpp b/src/core/Application.cpp index d0bb0b1..b7fef7c 100644 --- a/src/core/Application.cpp +++ b/src/core/Application.cpp @@ -280,6 +280,11 @@ struct Application::Impl { bool heading_setup_active_ = false; std::atomic running{true}; + // --test: what run() returns, and where the one-shot test has got to. + int exit_code_ = 0; + bool oneshot_started_ = false; + std::chrono::steady_clock::time_point oneshot_since_{}; + std::chrono::steady_clock::time_point oneshot_link_ok_{}; std::mutex cmd_mutex; std::queue cmd_queue; @@ -2325,6 +2330,10 @@ struct Application::Impl { last_test_view.pass = r.all_pass; last_test_view.profile = r.profile; last_test_view.summary = last_test_summary; + if (!opts.run_test.empty()) { + exit_code_ = r.all_pass ? 0 : kExitTestFail; + running = false; + } } } unsigned seq = motor->diagSeq(); @@ -2482,6 +2491,49 @@ struct Application::Impl { } } + // --test: start the requested test once the firmware is talking AND has been + // told its drivetrain, then let pollBackgroundResults() end the run with the + // verdict. + // + // Not immediately at startup: the firmware's NEED is answered from inside the + // control loop, so a DUMP taken on the first tick can catch an axis still in + // NEEDCFG and fail `test hardware motor` on values nobody had sent yet - a + // false alarm that would teach people to ignore the test. A deadline stops a + // dead link from waiting forever; the test then runs anyway and reports the + // real failure rather than this function inventing one. + void driveOneShotTest() { + if (opts.run_test.empty() || oneshot_started_) return; + using namespace std::chrono; + const auto now = steady_clock::now(); + const MotorTelemetry tel = motor->telemetry(); + auto axisSettled = [](AxisState a) { + return a == AxisState::Reset || a == AxisState::Ready || a == AxisState::Error; + }; + // Pitch counts only where it is both expected and reporting: a yaw-only + // firmware has no P segment to wait for. + const AxisState st = tel.yaw.state; + const bool settled = + axisSettled(st) && (!cfg.hardware.hasAxis("pitch") || !tel.pitch_present || + axisSettled(tel.pitch.state)); + if (!settled) oneshot_link_ok_ = {}; + else if (oneshot_link_ok_ == steady_clock::time_point{}) oneshot_link_ok_ = now; + + const bool grace_over = settled && now - oneshot_link_ok_ >= seconds(1); + const bool timed_out = now - oneshot_since_ >= seconds(20); + if (!grace_over && !timed_out) return; + if (!grace_over) + LOG_WARN << "--test: the motor link did not settle within 20 s (an axis is still " + "booting, homing or waiting for its settings); running the test anyway"; + + oneshot_started_ = true; + handleTest("test " + opts.run_test); + if (!tester) { + LOG_ERROR << "--test: '" << opts.run_test << "' did not start (see above)"; + exit_code_ = kExitTestNotRun; + running = false; + } + } + void drainCommands() { std::queue local; { @@ -2669,8 +2721,10 @@ struct Application::Impl { std::signal(SIGTERM, handleShutdownSignal); LOG_INFO << "Entering control loop (type 'exit' to quit, or Ctrl-C)"; + oneshot_since_ = std::chrono::steady_clock::now(); while (running && !g_shutdown_requested.load()) { drainCommands(); + driveOneShotTest(); answerFirmwareNeeds(); // the firmware asks; hardware.ini answers pollBackgroundResults(); // apply calibration result / emit DIAG summary+log scheduler->tick(); @@ -2690,7 +2744,9 @@ struct Application::Impl { if (env) env->stop(); motor->stop(); channel->disconnect(); - return 0; + // A --test run interrupted before its verdict has no verdict to report. + if (!opts.run_test.empty() && running && exit_code_ == 0) exit_code_ = kExitTestNotRun; + return exit_code_; } }; diff --git a/src/core/IniEdit.cpp b/src/core/IniEdit.cpp new file mode 100644 index 0000000..95290bf --- /dev/null +++ b/src/core/IniEdit.cpp @@ -0,0 +1,118 @@ +#include "fgc/IniEdit.h" + +#include "fgc/Config.h" +#include "ini.h" + +#include +#include +#include + +namespace fgc { + +namespace { + +std::string trim(const std::string& s) { + const char* ws = " \t\r\n"; + const size_t a = s.find_first_not_of(ws); + if (a == std::string::npos) return ""; + const size_t b = s.find_last_not_of(ws); + return s.substr(a, b - a + 1); +} + +int collect(void* user, const char* section, const char* name, const char* value) { + auto* kv = static_cast*>(user); + (*kv)[std::string(section) + "." + name] = value ? value : ""; + return 1; +} + +} // namespace + +std::size_t iniMaxLineContent() { return INI_MAX_LINE - 2; } + +int firstOverlongIniLine(const std::string& contents) { + std::istringstream in(contents); + std::string line; + int n = 0; + while (std::getline(in, line)) { + ++n; + if (!line.empty() && line.back() == '\r') line.pop_back(); + if (line.size() > iniMaxLineContent()) return n; + } + return 0; +} + +std::optional> splitIniKey(const std::string& flat) { + const size_t dot = flat.rfind('.'); + if (dot == std::string::npos || dot == 0 || dot + 1 == flat.size()) return std::nullopt; + return std::make_pair(flat.substr(0, dot), flat.substr(dot + 1)); +} + +std::map parseIniString(const std::string& contents) { + std::map kv; + const int rc = ini_parse_string(contents.c_str(), collect, &kv); + if (rc != 0) + throw std::runtime_error("does not parse as INI at line " + std::to_string(rc)); + return kv; +} + +std::string applyIniSets(const std::string& contents, + const std::vector>& sets, + const std::string& provenance) { + // Per section, in first-mention order, so appended blocks come out in the + // order the caller listed them. + std::vector order; + std::map>> by_section; + for (const auto& [flat, value] : sets) { + const auto k = splitIniKey(flat); + if (!k) throw std::invalid_argument("'" + flat + "' is not Section.key"); + if (value.find('\n') != std::string::npos) + throw std::invalid_argument(flat + ": a value cannot span lines"); + if (!by_section.count(k->first)) order.push_back(k->first); + by_section[k->first].emplace_back(k->second, value); + } + std::string out = contents; + for (const auto& section : order) + out = updateIniSectionKeys(out, section, by_section[section], provenance); + return out; +} + +std::string dropIniKeys(const std::string& contents, + const std::map& drops, + std::vector* dropped) { + std::istringstream in(contents); + std::ostringstream out; + std::string line, section; + bool in_dropped_value = false; + while (std::getline(in, line)) { + const std::string t = trim(line); + // With INI_ALLOW_MULTILINE, an indented line continues the value above + // it. Once that key's line is gone, its continuation would attach itself + // to whichever key precedes it - so it goes too. + if (in_dropped_value && !line.empty() && std::isspace((unsigned char)line[0]) && + !t.empty() && t[0] != ';' && t[0] != '#') { + out << "; " << t << "\n"; + continue; + } + in_dropped_value = false; + if (t.size() >= 2 && t.front() == '[' && t.back() == ']') { + section = trim(t.substr(1, t.size() - 2)); + } else if (!t.empty() && t[0] != ';' && t[0] != '#') { + const size_t eq = line.find('='); + if (eq != std::string::npos) { + const std::string key = trim(line.substr(0, eq)); + const std::string flat = section + "." + key; + auto it = drops.find(flat); + if (it != drops.end()) { + out << "; " << key << " " << it->second << "\n"; + if (dropped) dropped->push_back(flat); + in_dropped_value = true; + continue; + } + } + } + out << line << "\n"; + } + return out.str(); +} + +} // namespace fgc diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e11fa24..67c66ec 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -22,6 +22,7 @@ add_executable(fgc_tests test_paths.cpp test_singleinstance.cpp test_config.cpp + test_iniedit.cpp test_tmccurrent.cpp test_hardwarepresence.cpp test_hardwarechecks.cpp diff --git a/tests/test_firmwareconfig.cpp b/tests/test_firmwareconfig.cpp index af6e890..a208f48 100644 --- a/tests/test_firmwareconfig.cpp +++ b/tests/test_firmwareconfig.cpp @@ -263,3 +263,43 @@ TEST_CASE("RESET is not a way out of NEEDCFG") { CHECK_FALSE(m.telemetry().yaw.ready()); CHECK(m.telemetry().yaw.state == AxisState::NeedConfig); } + +TEST_CASE("the mock leaves NEEDCFG per axis, as the firmware does") { + // A yaw-only tower never answers for pitch - the host refuses to configure + // an axis the tower does not declare. Yaw must still report configured, or + // anything waiting on it (--test, above all) waits for nothing. + MockMotorController m(/*needcfg=*/true); + m.start(); + auto lines = m.takeNeedLines(); + REQUIRE(lines.size() == 2); + + AxisConfigSource src; + src.have_current = true; + src.global_scaler = 80; + src.center_count = 500000; + src.soft_margin = 5000; + src.have_travel = true; + src.steps_per_rev = 1210778; + src.have_deviation = true; + src.enc_deviation = 1500; + src.normal_vmax = 39738; + src.homing_vmax = 30000; + src.center_vmax = 25000; + src.have_wiring = true; + + auto yaw = parseNeedLine(lines[0]); + REQUIRE(yaw); + for (const auto& cmd : planFirmwareConfig(*yaw, src).sets) m.sendCommand(cmd); + + CHECK(m.axisConfigured('Y')); + CHECK_FALSE(m.axisConfigured('P')); + CHECK_FALSE(m.configured()); // "every axis" keeps its meaning + const auto t = m.telemetry(); + CHECK(t.yaw.state == AxisState::Reset); + CHECK(t.pitch.state == AxisState::NeedConfig); + + m.sendCommand("DUMP"); + const std::string d = m.lastDump(); + CHECK(d.find("DUMP Y state=1") != std::string::npos); + CHECK(d.find("DUMP P state=5") != std::string::npos); +} diff --git a/tests/test_iniedit.cpp b/tests/test_iniedit.cpp new file mode 100644 index 0000000..0dfb4dc --- /dev/null +++ b/tests/test_iniedit.cpp @@ -0,0 +1,106 @@ +#include + +#include "fgc/IniEdit.h" + +#include +#include + +using namespace fgc; + +TEST_CASE("a Section.key splits on its LAST dot, since sections carry dots") { + auto k = splitIniKey("Camera.rgb.id"); + REQUIRE(k); + CHECK(k->first == "Camera.rgb"); + CHECK(k->second == "id"); + CHECK_FALSE(splitIniKey("nodot")); + CHECK_FALSE(splitIniKey(".key")); + CHECK_FALSE(splitIniKey("Section.")); +} + +TEST_CASE("the line limit is the one inih actually reads whole") { + // 200-byte buffer, minus the newline and the terminator. + CHECK(iniMaxLineContent() == 198); + const std::string ok(198, 'x'); + const std::string too_long(199, 'x'); + CHECK(firstOverlongIniLine("a = 1\n" + ok + "\n") == 0); + CHECK(firstOverlongIniLine("a = 1\n" + too_long + "\nb = 2\n") == 2); + // A CRLF file is measured without its \r. + CHECK(firstOverlongIniLine(ok + "\r\n") == 0); +} + +TEST_CASE("parseIniString reads keys as the program does, and names a bad line") { + auto kv = parseIniString("[General]\ntower_name = t1 ; note\n[Camera.rgb]\nid = D\n"); + CHECK(kv.at("General.tower_name") == "t1"); + CHECK(kv.at("Camera.rgb.id") == "D"); + CHECK_THROWS_WITH(parseIniString("[General]\nno equals sign here\n"), + doctest::Contains("line 2")); +} + +TEST_CASE("applyIniSets edits in place, appends what is missing, keeps comments") { + const std::string in = + "; header\n" + "[General]\n" + "; the name\n" + "tower_name = ExampleTower\n" + "[Network]\n" + "zkms_server_ip = 127.0.0.1\n"; + const std::string out = applyIniSets( + in, {{"General.tower_name", "north"}, {"Motor.yaw_center_count", "1000000"}}, ""); + CHECK(out.find("; header\n") == 0); + CHECK(out.find("; the name\ntower_name = north\n") != std::string::npos); + CHECK(out.find("ExampleTower") == std::string::npos); + CHECK(out.find("[Motor]\nyaw_center_count = 1000000\n") != std::string::npos); + CHECK(parseIniString(out).at("Network.zkms_server_ip") == "127.0.0.1"); +} + +TEST_CASE("applyIniSets keeps one ;prev generation when given a provenance") { + const std::string in = "[Serial]\ndevice = /dev/serial/by-id/old\n"; + const std::string out = + applyIniSets(in, {{"Serial.device", "/dev/serial/by-id/new"}}, "install-tower"); + CHECK(out.find(";prev device = /dev/serial/by-id/old") != std::string::npos); + CHECK(parseIniString(out).at("Serial.device") == "/dev/serial/by-id/new"); +} + +TEST_CASE("applyIniSets refuses a malformed key or a multi-line value") { + CHECK_THROWS_AS(applyIniSets("", {{"nodot", "1"}}, ""), std::invalid_argument); + CHECK_THROWS_AS(applyIniSets("", {{"A.b", "1\nc = 2"}}, ""), std::invalid_argument); +} + +TEST_CASE("dropIniKeys comments a key out WITHOUT its value") { + // The placeholder must not survive even as a comment: deploy.sh's guard greps + // the raw file for DEV_0000000000 and would refuse to run the tower. + const std::string in = + "[Camera.rgb]\n" + "enabled = true\n" + "id = DEV_0000000000 ; USB DEV_ id\n" + "[Serial]\n" + "device = /dev/ttyACM0\n"; + std::vector dropped; + const std::string out = dropIniKeys( + in, {{"Camera.rgb.id", "is set in hardware.ini"}, {"Serial.device", "is set elsewhere"}}, + &dropped); + CHECK(out.find("DEV_0000000000") == std::string::npos); + CHECK(out.find("; id is set in hardware.ini\n") != std::string::npos); + CHECK(out.find("; device is set elsewhere\n") != std::string::npos); + CHECK(dropped == std::vector{"Camera.rgb.id", "Serial.device"}); + const auto kv = parseIniString(out); + CHECK(kv.count("Camera.rgb.id") == 0); + CHECK(kv.at("Camera.rgb.enabled") == "true"); +} + +TEST_CASE("dropIniKeys matches the section too, not just the key name") { + const std::string in = "[IMU]\ndevice = /dev/ttyS0\n[Serial]\ndevice = /dev/ttyACM0\n"; + const auto kv = parseIniString(dropIniKeys(in, {{"Serial.device", "gone"}})); + CHECK(kv.at("IMU.device") == "/dev/ttyS0"); + CHECK(kv.count("Serial.device") == 0); +} + +TEST_CASE("dropIniKeys takes a dropped key's continuation lines with it") { + // inih joins an indented line onto the value above it. Left behind, it would + // attach itself to whatever key now precedes it. + const std::string in = "[A]\nkeep = 1\ngone = first\n second\nafter = 2\n"; + const auto kv = parseIniString(dropIniKeys(in, {{"A.gone", "x"}})); + CHECK(kv.at("A.keep") == "1"); + CHECK(kv.at("A.after") == "2"); + CHECK(kv.count("A.gone") == 0); +} diff --git a/tools/fgc_config.cpp b/tools/fgc_config.cpp new file mode 100644 index 0000000..2bf9bb5 --- /dev/null +++ b/tools/fgc_config.cpp @@ -0,0 +1,298 @@ +// fgc-config - write and edit a tower's config.ini and hardware.ini so that +// what lands on disk is always a file fire_gimbal_control will accept. +// +// fgc-config seed --example --out +// [--hardware ] [--set Section.key=value]... +// [--drop Section.key=note]... [--dry-run] +// fgc-config set --config [--hardware ] +// --set Section.key=value... [--provenance ] [--dry-run] +// fgc-config get Section.key +// fgc-config check --config [--hardware ] +// +// seed makes a NEW config.ini from the example: applies the --set values, and +// replaces every key that hardware.ini sets with a one-line comment +// saying so. Refuses if the output exists - a tower's config.ini holds +// its calibration, and is edited with `set`, never regenerated. +// set edits one of the two files in place: comments and order are kept, and +// each changed key keeps its previous value as a ";prev" comment. +// get prints a value as the program reads it (nothing, exit 1, if unset). +// check validates the pair and changes nothing. +// +// Every write is validated FIRST, by loading the candidate through the same +// ConfigLoader the program uses (with the other file layered as it will be), +// and by refusing any line too long for inih to read whole. Only then is it +// renamed into place, so a failed write leaves the old file untouched. +// --dry-run prints the candidate instead of writing it; the installer diffs it. +// +// Exit: 0 ok, 1 usage or refused, 2 the result would not load. + +#include "fgc/Config.h" +#include "fgc/IniEdit.h" + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace fs = std::filesystem; +using namespace fgc; + +namespace { + +constexpr int kUsage = 1; +constexpr int kInvalid = 2; + +int usage(const std::string& why) { + if (!why.empty()) std::cerr << "fgc-config: " << why << "\n"; + std::cerr << "usage:\n" + " fgc-config seed --example E --out O [--hardware H] [--set S.k=v]... " + "[--drop S.k=note]... [--dry-run]\n" + " fgc-config set FILE --config C [--hardware H] --set S.k=v... " + "[--provenance P] [--dry-run]\n" + " fgc-config get FILE S.k\n" + " fgc-config check --config C [--hardware H]\n"; + return kUsage; +} + +bool readFile(const std::string& path, std::string& out) { + std::ifstream f(path, std::ios::binary); + if (!f) return false; + std::ostringstream ss; + ss << f.rdbuf(); + out = ss.str(); + return true; +} + +bool writeFile(const std::string& path, const std::string& text) { + std::ofstream f(path, std::ios::binary | std::ios::trunc); + f << text; + f.flush(); + return static_cast(f); +} + +bool splitAssign(const std::string& s, std::pair& out) { + const size_t eq = s.find('='); + if (eq == std::string::npos || eq == 0) return false; + out = {s.substr(0, eq), s.substr(eq + 1)}; + return true; +} + +struct Args { + std::string cmd, file, example, out, config, hardware, provenance, key; + std::vector> sets; + std::map drops; + bool dry_run = false; +}; + +// Load the pair the way the program will. `cfg`/`hw` are the paths to load; the +// candidate being written is passed in their place. Returns false and prints why. +bool loads(const std::string& cfg, const std::string& hw) { + try { + (void)ConfigLoader::loadFromFile(cfg, hw); + return true; + } catch (const std::exception& e) { + std::cerr << "fgc-config: the result would not load: " << e.what() << "\n"; + return false; + } +} + +bool longLinesOk(const std::string& text, const std::string& what) { + if (int n = firstOverlongIniLine(text)) { + std::cerr << "fgc-config: " << what << " line " << n << " is longer than " + << iniMaxLineContent() + << " characters; inih would split it and misread the rest of the file\n"; + return false; + } + return true; +} + +// A sibling temp file, so the final rename stays on one filesystem and is atomic. +std::string tempBeside(const std::string& path) { + return (fs::path(path).parent_path() / ("." + fs::path(path).filename().string() + + ".fgc-config.tmp")).string(); +} + +// Validate `text` as the new `target`, then write it. `other` is the file layered +// with it (hardware.ini for a config.ini, config.ini for a hardware.ini), and +// `target_is_hw` says which role the target plays. +int commit(const std::string& target, const std::string& text, const std::string& other, + bool target_is_hw, bool dry_run) { + if (!longLinesOk(text, target)) return kInvalid; + const std::string tmp = tempBeside(target); + if (!writeFile(tmp, text)) { + std::cerr << "fgc-config: cannot write " << tmp << "\n"; + return kUsage; + } + const bool ok = target_is_hw ? loads(other, tmp) : loads(tmp, other); + if (!ok || dry_run) { + std::error_code ec; + fs::remove(tmp, ec); + if (ok) std::cout << text; + return ok ? 0 : kInvalid; + } + std::error_code ec; + if (fs::exists(target)) + fs::permissions(tmp, fs::status(target).permissions(), ec); + else + fs::permissions(tmp, fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_read | fs::perms::others_read, ec); + fs::rename(tmp, target, ec); + if (ec) { + std::cerr << "fgc-config: cannot replace " << target << ": " << ec.message() << "\n"; + fs::remove(tmp, ec); + return kUsage; + } + std::cerr << "fgc-config: wrote " << target << "\n"; + return 0; +} + +int cmdSeed(const Args& a) { + if (a.example.empty() || a.out.empty()) return usage("seed needs --example and --out"); + if (fs::exists(a.out) && !a.dry_run) { + std::cerr << "fgc-config: " << a.out << " exists. A tower's config.ini holds its " + "calibration: edit it with `fgc-config set`, never regenerate it\n"; + return kUsage; + } + std::string text; + if (!readFile(a.example, text)) return usage("cannot read " + a.example); + + std::map drops = a.drops; + if (!a.hardware.empty()) { + std::string hw; + if (!readFile(a.hardware, hw)) return usage("cannot read " + a.hardware); + try { + const auto hw_kv = parseIniString(hw); + for (const auto& [k, v] : hw_kv) { + (void)v; + drops.emplace(k, "is set in hardware.ini, which wins over this file"); + } + // The enable flags exist for towers WITHOUT a hardware.ini. Where the + // file declares the sensor, the declaration decides and a flag beside + // it is warned about at every start, so a new tower does not get one. + if (hw_kv.count("Hardware.imu")) + drops.emplace("Features.enable_imu", "follows [Hardware] imu in hardware.ini"); + if (hw_kv.count("Hardware.env")) + drops.emplace("Features.enable_env", "follows [Hardware] env in hardware.ini"); + } catch (const std::exception& e) { + std::cerr << "fgc-config: " << a.hardware << " " << e.what() << "\n"; + return kInvalid; + } + } + std::vector dropped; + text = dropIniKeys(text, drops, &dropped); + try { + text = applyIniSets(text, a.sets, /*provenance=*/""); + } catch (const std::exception& e) { + return usage(e.what()); + } + for (const auto& k : dropped) + std::cerr << "fgc-config: commented out " << k << " (" << drops.at(k) << ")\n"; + // The candidate is validated beside its final location, so profile_dir and + // other relative paths resolve exactly as they will for the program. + return commit(a.out, text, a.hardware, /*target_is_hw=*/false, a.dry_run); +} + +int cmdSet(const Args& a) { + if (a.file.empty() || a.config.empty()) return usage("set needs FILE and --config"); + if (a.sets.empty()) return usage("set needs at least one --set"); + std::error_code ec; + const bool is_cfg = fs::equivalent(a.file, a.config, ec); + const bool is_hw = !a.hardware.empty() && fs::equivalent(a.file, a.hardware, ec); + if (!is_cfg && !is_hw) return usage("FILE must be the --config or the --hardware file"); + + std::string text; + if (!readFile(a.file, text)) return usage("cannot read " + a.file); + try { + text = applyIniSets(text, a.sets, a.provenance); + } catch (const std::exception& e) { + return usage(e.what()); + } + return is_hw ? commit(a.hardware, text, a.config, true, a.dry_run) + : commit(a.config, text, a.hardware, false, a.dry_run); +} + +int cmdGet(const Args& a) { + if (a.file.empty() || a.key.empty()) return usage("get needs FILE and Section.key"); + std::string text; + if (!readFile(a.file, text)) return usage("cannot read " + a.file); + try { + const auto kv = parseIniString(text); + auto it = kv.find(a.key); + if (it == kv.end()) return kUsage; + std::cout << it->second << "\n"; + return 0; + } catch (const std::exception& e) { + std::cerr << "fgc-config: " << a.file << " " << e.what() << "\n"; + return kInvalid; + } +} + +int cmdCheck(const Args& a) { + if (a.config.empty()) return usage("check needs --config"); + for (const std::string& f : {a.config, a.hardware}) { + if (f.empty()) continue; + std::string text; + if (!readFile(f, text)) return usage("cannot read " + f); + if (!longLinesOk(text, f)) return kInvalid; + } + if (!loads(a.config, a.hardware)) return kInvalid; + std::cerr << "fgc-config: " << a.config << (a.hardware.empty() ? "" : " + " + a.hardware) + << " load cleanly\n"; + return 0; +} + +} // namespace + +int main(int argc, char** argv) { + if (argc < 2) return usage(""); + Args a; + a.cmd = argv[1]; + std::vector positional; + for (int i = 2; i < argc; ++i) { + const std::string s = argv[i]; + auto value = [&](std::string& dst) { + if (i + 1 >= argc) return false; + dst = argv[++i]; + return true; + }; + std::pair kv; + std::string v; + if (s == "--example") { if (!value(a.example)) return usage(s + " needs a value"); } + else if (s == "--out") { if (!value(a.out)) return usage(s + " needs a value"); } + else if (s == "--config") { if (!value(a.config)) return usage(s + " needs a value"); } + else if (s == "--hardware") { if (!value(a.hardware)) return usage(s + " needs a value"); } + else if (s == "--provenance") { if (!value(a.provenance)) return usage(s + " needs a value"); } + else if (s == "--dry-run") { a.dry_run = true; } + else if (s == "--set") { + if (!value(v) || !splitAssign(v, kv)) return usage("--set takes Section.key=value"); + a.sets.push_back(kv); + } else if (s == "--drop") { + if (!value(v) || !splitAssign(v, kv)) return usage("--drop takes Section.key=note"); + a.drops[kv.first] = kv.second; + } else if (s.rfind("--", 0) == 0) { + return usage("unknown option " + s); + } else { + positional.push_back(s); + } + } + if (a.cmd == "seed" || a.cmd == "check") { + if (!positional.empty()) return usage("unexpected argument " + positional[0]); + } else if (a.cmd == "set") { + if (positional.size() != 1) return usage("set takes exactly one FILE"); + a.file = positional[0]; + } else if (a.cmd == "get") { + if (positional.size() != 2) return usage("get takes FILE and Section.key"); + a.file = positional[0]; + a.key = positional[1]; + } + + if (a.cmd == "seed") return cmdSeed(a); + if (a.cmd == "set") return cmdSet(a); + if (a.cmd == "get") return cmdGet(a); + if (a.cmd == "check") return cmdCheck(a); + return usage("unknown command '" + a.cmd + "'"); +}