From ffaa41ac2fa91dcf59a84292759f15bf0e07017c Mon Sep 17 00:00:00 2001 From: pgdalmeida Date: Thu, 17 Sep 2026 17:58:45 +0200 Subject: [PATCH] Read the drivetrain per axis from hardware.ini, and compare INI values numerically --- config/hardware.example.ini | 32 +++++++++++++ include/fgc/Config.h | 39 +++++++++++++++ src/core/Config.cpp | 87 +++++++++++++++++++++++++++++++-- tests/test_config.cpp | 95 +++++++++++++++++++++++++++++++++++++ 4 files changed, 248 insertions(+), 5 deletions(-) diff --git a/config/hardware.example.ini b/config/hardware.example.ini index cf8d7a3..ec9fca7 100644 --- a/config/hardware.example.ini +++ b/config/hardware.example.ini @@ -73,6 +73,38 @@ device = /dev/ttyS0 i2c_device = /dev/i2c-2 i2c_addr = 0x44 +; --- What it is made of --------------------------------------------------- +; The drivetrain, one block per axis. A different motor, gearbox or driver board +; changes these; nothing an operator tunes belongs here. +[Motor.yaw] +; Sense resistor fitted to THIS axis's TMC5160 board. It is what makes the +; milliamp figures below mean anything, which is why the host - not the firmware - +; converts them into GLOBALSCALER/IRUN register values and sends those. +r_sense_ohm = 0.075 +; Milliamps RMS per phase. run_current_ma is normal motion; homing_current_ma +; applies ONLY while hunting for an endstop. +; +; The two differ when the stop cannot survive full torque. On the IOTA tower the +; endstop is a screw head meeting another screw head, and the high gear ratio let +; the motor shear one past the other - it ran 1.8 turns past the stop without the +; encoder ever falling behind. At 189 mA it homes against the same screw cleanly. +; +; The startup log prints what the registers ACTUALLY deliver, which is not +; always what you asked for; believe the log, not this file. +run_current_ma = 756 +homing_current_ma = 200 +; Mechanical angle between the hard endstops. A NAMEPLATE FACT: homing measures +; the travel in counts, and dividing by this yields [Motor] yaw_counts_per_deg in +; config.ini, offered for saving at the y/n prompt. +; +; Deliberately a different key, in a different file, from counts_per_deg itself: +; hardware.ini is merged OVER config.ini and wins, so sharing a key would +; silently discard every calibration the save path writes back. +; +; ROUGH until somebody measures it. Every bearing inherits its error +; proportionally, until 'gimbal heading setup' supersedes it with a landmark fit. +travel_deg = 340 + [Camera.rgb] ; The serial of the physical camera fitted to this tower. A USB DEV_ id, or a ; GigE IP address. diff --git a/include/fgc/Config.h b/include/fgc/Config.h index 38ff0ff..5aa2141 100644 --- a/include/fgc/Config.h +++ b/include/fgc/Config.h @@ -56,6 +56,40 @@ struct HardwareConfig { std::string axesLabel() const; }; +// [Motor.] - what this tower's drivetrain IS, one block per axis. These +// change only when the motor, gearbox or driver board changes, so they belong in +// hardware.ini beside the rest of the assembly description. +// +// Currents are milliamps RMS per phase, NOT register values: the conversion needs +// the sense resistor fitted to this particular driver board, so the host owns it +// and sends the firmware the registers it computed (see TmcCurrent.h). +struct AxisDrivetrainConfig { + bool declared = false; // any [Motor.] key present at all + double r_sense_ohm = 0.0; // sense resistor on this axis's TMC board + double run_current_ma = 0.0; // 0 = undeclared -> nothing is sent + double homing_current_ma = 0.0; // defaults to run_current_ma + + // Mechanical angle between the hard endstops. A nameplate fact, not a + // calibration: homing measures the travel in counts, and dividing by this + // yields [Motor] _counts_per_deg, which lives in config.ini and is + // saved with history. Keeping them in different files AND different keys is + // deliberate - hardware.ini is merged over config.ini and wins, so a shared + // key would silently discard every saved calibration. + double travel_deg = 0.0; // 0 = undeclared -> nothing is derived + + // True when the homing current is genuinely lower than the run current, i.e. + // when the reduction is load-bearing and homing without it risks the rig. + bool reducesForHoming() const { + return run_current_ma > 0.0 && homing_current_ma > 0.0 && + homing_current_ma < run_current_ma; + } +}; + +struct DrivetrainConfig { + AxisDrivetrainConfig yaw; + AxisDrivetrainConfig pitch; +}; + 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 @@ -220,6 +254,11 @@ struct AppConfig { LoggingConfig logging; UiConfig ui; // [UI] terminal dashboard toggle Geometry geometry; // [Motor] degrees<->counts maps (yaw + pitch) + DrivetrainConfig drivetrain; // [Motor.] from hardware.ini + // [Motor] how long a full two-end homing may take before the host calls it + // failed. A property of the drivetrain, not of this program: a high gear + // ratio searching at a gentle speed takes minutes. + int homing_timeout_s = 65; // [Motor] clearance kept from each hard endstop, in counts. Sent to the // firmware (MARGIN), which enforces it; kept in counts rather than degrees // because it must apply at homing time, before any degree map is trusted. diff --git a/src/core/Config.cpp b/src/core/Config.cpp index ad82a11..e27ef46 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -419,6 +419,27 @@ AppConfig ConfigLoader::fromMap(const std::map& kv) { cfg.pitch_soft_margin_counts = getLong(kv, "Motor.pitch_soft_margin_counts", cfg.pitch_soft_margin_counts); cfg.enc_error_warn_counts = getLong(kv, "Motor.enc_error_warn_counts", cfg.enc_error_warn_counts); + cfg.homing_timeout_s = getInt(kv, "Motor.homing_timeout_s", cfg.homing_timeout_s); + + // [Motor.]: this tower's drivetrain, normally from hardware.ini. Note + // the section is DOTTED - these are deliberately different keys from the flat + // [Motor] ones above, because hardware.ini is merged over config.ini and + // wins, so a shared key would silently discard the calibration that the save + // path writes back to config.ini. + auto axisDrivetrain = [&kv](const char* axis, AxisDrivetrainConfig& d) { + const std::string pre = std::string("Motor.") + axis + "."; + for (const auto& [k, v] : kv) { + (void)v; + if (k.rfind(pre, 0) == 0) { d.declared = true; break; } + } + d.r_sense_ohm = getDouble(kv, pre + "r_sense_ohm", d.r_sense_ohm); + d.run_current_ma = getDouble(kv, pre + "run_current_ma", d.run_current_ma); + // An axis that names only a run current is not asking for a reduction. + d.homing_current_ma = getDouble(kv, pre + "homing_current_ma", d.run_current_ma); + d.travel_deg = getDouble(kv, pre + "travel_deg", d.travel_deg); + }; + axisDrivetrain("yaw", cfg.drivetrain.yaw); + axisDrivetrain("pitch", cfg.drivetrain.pitch); // [Scan]: scan-grid source (explicit CSV file, else generated). cfg.scan.grid_file = get(kv, "Scan.grid_file", cfg.scan.grid_file); @@ -450,6 +471,27 @@ AppConfig ConfigLoader::fromMap(const std::map& kv) { throw std::runtime_error("Motor.yaw_soft_margin_counts must be >= 0"); if (cfg.pitch_soft_margin_counts < 0) throw std::runtime_error("Motor.pitch_soft_margin_counts must be >= 0"); + if (cfg.homing_timeout_s <= 0) + throw std::runtime_error("Motor.homing_timeout_s must be > 0"); + + // [Motor.]. Currents are only meaningful alongside the sense resistor + // that scales them, and a current without one would be computed against a + // guess - so demand them together rather than inventing a default resistor. + auto checkDrivetrain = [](const char* axis, const AxisDrivetrainConfig& d) { + const std::string sec = std::string("Motor.") + axis; + if (d.run_current_ma > 0.0 && d.r_sense_ohm <= 0.0) + throw std::runtime_error(sec + ".run_current_ma needs " + sec + + ".r_sense_ohm (the sense resistor on that driver " + "board) to mean anything"); + if (d.run_current_ma < 0.0 || d.homing_current_ma < 0.0) + throw std::runtime_error(sec + " currents must be >= 0"); + if (d.travel_deg < 0.0 || d.travel_deg > 720.0) + throw std::runtime_error(sec + ".travel_deg must be within (0, 720]; the " + "direction of travel is carried by the sign of " + "counts_per_deg, not by this angle"); + }; + checkDrivetrain("yaw", cfg.drivetrain.yaw); + checkDrivetrain("pitch", cfg.drivetrain.pitch); // Keys from the pre-centre-anchoring layout. Silently ignoring them would // leave an operator convinced they had configured a heading that is in fact @@ -458,15 +500,22 @@ AppConfig ConfigLoader::fromMap(const std::map& kv) { "Motor.yaw_zero_count", "Motor.pitch_zero_count", "Motor.yaw_min_deg", "Motor.yaw_max_deg", "Motor.pitch_min_deg", "Motor.pitch_max_deg", "Motor.yaw_home_zero", + // Drivetrain keys written under [Motor] instead of [Motor.]. They + // flatten to a name nothing reads, so without this they would be ignored + // in total silence - and an operator would believe they had reduced the + // homing current when they had not. + "Motor.r_sense_ohm", "Motor.run_current_ma", + "Motor.homing_current_ma", "Motor.travel_deg", }; std::string stale; for (const char* k : kRemovedMotorKeys) if (kv.count(k)) stale += (stale.empty() ? "" : ", ") + std::string(k + 6); if (!stale.empty()) - LOG_WARN << "[Motor] ignoring keys removed in the centre-anchored layout: " << stale - << " -- travel limits are now derived from homing, and the heading " - "origin comes from yaw_heading_at_center (set it with " - "'gimbal heading setup')"; + LOG_WARN << "[Motor] ignoring keys nothing reads: " << stale + << " -- travel limits are derived from homing, the heading origin " + "comes from yaw_heading_at_center (set it with 'gimbal heading " + "setup'), and drivetrain keys belong under [Motor.yaw] / " + "[Motor.pitch], not [Motor]"; return cfg; } @@ -559,6 +608,34 @@ const std::string kPrevMarker = ";prev "; // 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. +// Do these two INI values denote the same NUMBER, however they are spelled? +// +// The history filter below compares what is in the file against what is about to +// be written, and the writer formats doubles with "%.10g" - so a hand-written +// "0.0" is rewritten as "0", and a plain string comparison calls that a change. +// It then records a ";prev" line whose "previous" value is the value it still +// has, stamped with the provenance of whatever save happened to be running. +// +// That is worse than cosmetic noise: ";prev" deliberately keeps exactly ONE +// generation, so a spurious entry DISPLACES the real previous value it exists to +// preserve, and attributes an untouched key to a calibration that never wrote it. +// +// Falls back to comparing text whenever either side is not a plain number, which +// is what keys like a device path need. +bool sameIniValue(const std::string& a, const std::string& b) { + if (a == b) return true; + auto asNumber = [](const std::string& s, double& out) { + if (s.empty()) return false; + char* end = nullptr; + out = std::strtod(s.c_str(), &end); + if (end == s.c_str()) return false; // nothing numeric at all + while (*end && std::isspace(static_cast(*end))) ++end; + return *end == '\0'; // trailing junk: not a number + }; + double x = 0.0, y = 0.0; + return asNumber(a, x) && asNumber(b, y) && x == y; +} + std::string iniValueOf(const std::string& rhs) { bool was_space = false; for (size_t i = 0; i < rhs.size(); ++i) { @@ -612,7 +689,7 @@ std::string updateIniSectionKeys(const std::string& contents, const std::string& 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; + changed[i] = !sameIniValue(old_value[i], kv[i].second); break; } } diff --git a/tests/test_config.cpp b/tests/test_config.cpp index be92122..274cc62 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -596,3 +596,98 @@ TEST_CASE("A mount angle that cannot be a rotation is rejected") { CHECK_NOTHROW(with("IMU.z", "-360")); CHECK_NOTHROW(with("IMU.z", "360")); } + +TEST_CASE("[Motor.] drivetrain keys are parsed per axis") { + auto cfg = ConfigLoader::fromMap({ + {"General.tower_name", "t"}, + {"Motor.yaw.r_sense_ohm", "0.075"}, + {"Motor.yaw.run_current_ma", "756"}, + {"Motor.yaw.homing_current_ma", "200"}, + {"Motor.yaw.travel_deg", "340"}, + }); + CHECK(cfg.drivetrain.yaw.declared); + CHECK(cfg.drivetrain.yaw.r_sense_ohm == doctest::Approx(0.075)); + CHECK(cfg.drivetrain.yaw.run_current_ma == doctest::Approx(756.0)); + CHECK(cfg.drivetrain.yaw.homing_current_ma == doctest::Approx(200.0)); + CHECK(cfg.drivetrain.yaw.travel_deg == doctest::Approx(340.0)); + CHECK(cfg.drivetrain.yaw.reducesForHoming()); + // The other axis stays undeclared, so nothing is sent for it. + CHECK_FALSE(cfg.drivetrain.pitch.declared); +} + +TEST_CASE("an axis that names only a run current is not asking for a reduction") { + auto cfg = ConfigLoader::fromMap({ + {"General.tower_name", "t"}, + {"Motor.yaw.r_sense_ohm", "0.075"}, + {"Motor.yaw.run_current_ma", "756"}, + }); + CHECK(cfg.drivetrain.yaw.homing_current_ma == doctest::Approx(756.0)); + CHECK_FALSE(cfg.drivetrain.yaw.reducesForHoming()); +} + +TEST_CASE("[Motor.yaw] travel_deg does not shadow [Motor] yaw_counts_per_deg") { + // The whole reason the travel angle is a separate key in a separate section: + // hardware.ini is merged OVER config.ini and wins, so if the derived + // calibration shared a key with the nameplate fact, every saved calibration + // would be silently discarded on the next start. + auto cfg = ConfigLoader::fromMap({ + {"General.tower_name", "t"}, + {"Motor.yaw_counts_per_deg", "3561.1"}, // calibration, from config.ini + {"Motor.yaw.travel_deg", "340"}, // nameplate, from hardware.ini + }); + CHECK(cfg.geometry.yaw.counts_per_deg == doctest::Approx(3561.1)); + CHECK(cfg.drivetrain.yaw.travel_deg == doctest::Approx(340.0)); +} + +TEST_CASE("a drivetrain current without its sense resistor is refused") { + // Computing registers against a guessed resistor would silently deliver the + // wrong current, which on a rig with a yielding endstop is the whole hazard. + CHECK_THROWS(ConfigLoader::fromMap({ + {"General.tower_name", "t"}, + {"Motor.yaw.run_current_ma", "756"}, + })); +} + +TEST_CASE("an undeclared drivetrain leaves every axis silent") { + auto cfg = ConfigLoader::fromMap({{"General.tower_name", "t"}}); + CHECK_FALSE(cfg.drivetrain.yaw.declared); + CHECK_FALSE(cfg.drivetrain.pitch.declared); + CHECK(cfg.drivetrain.yaw.run_current_ma == 0.0); + CHECK_FALSE(cfg.drivetrain.yaw.reducesForHoming()); +} + +TEST_CASE("a value that only changes spelling gets no history line") { + // The writer formats doubles with %.10g, so a hand-written "0.0" comes back + // as "0". That is the same number, and recording it as a change would spend + // the single ";prev" generation on a value that never moved - displacing the + // real previous value the mechanism exists to preserve. + const std::string before = "[Motor]\nyaw_heading_at_center = 0.0\n"; + const std::string after = + updateIniSectionKeys(before, "Motor", {{"yaw_heading_at_center", "0"}}, "a save"); + CHECK(after.find(";prev") == std::string::npos); + CHECK(after.find("yaw_heading_at_center = 0") != std::string::npos); +} + +TEST_CASE("differently spelled equal numbers are not changes") { + for (auto [old_text, new_text] : std::vector>{ + {"1e3", "1000"}, {"500.0", "500"}, {"0.50", "0.5"}, {" 12 ", "12"}}) { + const std::string out = updateIniSectionKeys("[Motor]\nk = " + old_text + "\n", "Motor", + {{"k", new_text}}, "a save"); + CHECK(out.find(";prev") == std::string::npos); + } +} + +TEST_CASE("a genuine numeric change still records its history") { + const std::string out = updateIniSectionKeys("[Motor]\nyaw_counts_per_deg = 500\n", "Motor", + {{"yaw_counts_per_deg", "3552.867647"}}, + "homed travel"); + CHECK(out.find(";prev yaw_counts_per_deg = 500") != std::string::npos); + CHECK(out.find("homed travel") != std::string::npos); + CHECK(out.find("yaw_counts_per_deg = 3552.867647") != std::string::npos); +} + +TEST_CASE("non-numeric values still compare as text") { + const std::string out = updateIniSectionKeys("[Serial]\ndevice = /dev/ttyACM0\n", "Serial", + {{"device", "/dev/ttyUSB0"}}, "a save"); + CHECK(out.find(";prev device = /dev/ttyACM0") != std::string::npos); +}