From bd3a568508d17c8363be2864f28b4c79287801d1 Mon Sep 17 00:00:00 2001 From: pgdalmeida Date: Wed, 23 Sep 2026 07:36:23 +0200 Subject: [PATCH] Send the whole drivetrain on connect, so a restart is enough to retune --- docs/configuration.md | 30 ++++--- docs/hardware-porting-plan.md | 8 +- src/core/Application.cpp | 142 +++++++++++++++++++++++----------- 3 files changed, 124 insertions(+), 56 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 76e8ddc..04f56cc 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -731,10 +731,10 @@ full torque — the fire it is there to prevent. ### The config handshake — the firmware asks, `hardware.ini` answers -Today the firmware carries a full parameter table and **guesses** when the host -says nothing, so every drivetrain value exists in two places and the two can -disagree. Section 6 of the config plan replaces that: the firmware declares what -it needs and refuses to move until it has it. +The firmware used to carry a full parameter table and **guess** when the host +said nothing, so every drivetrain value existed in two places and the two could +disagree. It no longer guesses: it declares what it needs and refuses to move +until it has it. ``` firmware -> BOOT build=abc123 @@ -767,11 +767,23 @@ Three rules, each protecting against a different silent failure: > `ST` line — deliberately not red, and deliberately not "no telemetry": the link > is fine and the axis is talking, it just has not been told what it is driving. > -> **Status: the host half only.** The host answers `NEED` and the responder is -> verified end to end, but the firmware that *asks* is not written yet. Set -> `[Features] mock_needcfg = 1` to make the simulated controller run the -> handshake — emit `NEED` at start, reject `HOME` / `MOVE` / `ENABLE` until every -> requested key is answered — which is how this is exercised on a dev box. +> **Retuning needs only a restart.** The firmware asks exactly once, at boot, +> and only while it is in `NEEDCFG` — so the handshake alone would mean that +> editing `hardware.ini` and restarting the *host* changed nothing, because the +> board is already configured and says nothing to reply to. The host therefore +> re-sends the whole declared block on every connect, whether or not it was +> asked. Edit, restart, done; no re-flash. +> +> It also makes the handshake robust to a `NEED` line lost on the wire: an axis +> still sitting in `NEEDCFG` is completed by that re-send rather than refusing to +> move until someone notices. +> +> **Both halves are live**, and validated on tower 121: the firmware emits `NEED` +> at boot and refuses `HOME` / `MOVE` / `ENABLE` until every required key is +> answered, and the host answers it. On a dev box with no board attached, set +> `[Features] mock_needcfg = 1` to make the simulated controller run the same +> handshake, or `0` to have it stand in for a board that is already configured — +> which is what exercises the unprompted re-send. ### Why speed is declared in degrees per second diff --git a/docs/hardware-porting-plan.md b/docs/hardware-porting-plan.md index 040d630..5d6d5b6 100644 --- a/docs/hardware-porting-plan.md +++ b/docs/hardware-porting-plan.md @@ -361,8 +361,12 @@ The fix is to make the search weak enough that the screw wins: Declared per tower in `hardware.ini` as `[Motor.yaw] homing_current_ma`, converted to registers by the host and sent as `CURRENT` before every home — so retuning it -is a restart, not a reflash. See [configuration.md](configuration.md) for the keys -and `firmware/docs/hardware-notes.md` for the mechanism. +is a restart, not a reflash. **That is now true of the whole drivetrain block**: +the host re-sends every declared value on connect, not only the ones with their +own command. It was briefly true of the current alone, which made half a +`hardware.ini` edit take effect on a restart and half silently not. See +[configuration.md](configuration.md) for the keys and +`firmware/docs/hardware-notes.md` for the mechanism. Three things this exposed, all since fixed: diff --git a/src/core/Application.cpp b/src/core/Application.cpp index 165525c..0dbc71c 100644 --- a/src/core/Application.cpp +++ b/src/core/Application.cpp @@ -1243,54 +1243,100 @@ struct Application::Impl { LOG_WARN << "unparseable NEED line from the firmware: " << line; continue; } - const bool is_yaw = need->axis == 'Y'; - - // An axis the tower says it does not have. That is not a missing - // declaration - it is a firmware built for more axes than this rig, - // and answering would configure a driver that is not there. Same - // class of fault as presence reconciliation catches, so it is named - // that way rather than as a hole in hardware.ini. - const char* axis_name = is_yaw ? "yaw" : "pitch"; - if (cfg.hardware.declared && !cfg.hardware.hasAxis(axis_name)) { - LOG_ERROR << "firmware asked for axis " << need->axis << " (" << axis_name - << "), which this tower does not declare (axes " - << cfg.hardware.axesLabel() - << ") -- the firmware was built for more axes than the rig has; " - "rebuild it for this board rather than declaring an axis that is " - "not fitted"; - continue; - } - const auto src = is_yaw - ? axisConfigSource("yaw", cfg.geometry.yaw, cfg.drivetrain.yaw, - yaw_current_, cfg.yaw_soft_margin_counts) - : axisConfigSource("pitch", cfg.geometry.pitch, - cfg.drivetrain.pitch, pitch_current_, - cfg.pitch_soft_margin_counts); - - const auto plan = planFirmwareConfig(*need, src); - for (const auto& cmd : plan.sets) motor->sendCommand(cmd); - - if (!plan.unanswered.empty()) { - std::string keys; - for (const auto& k : plan.unanswered) keys += (keys.empty() ? "" : ", ") + k; - LOG_ERROR << "firmware asked axis " << need->axis << " for " << keys - << " and hardware.ini does not declare " - << (plan.unanswered.size() == 1 ? "it" : "them") - << "; the axis stays in NEEDCFG and will not move"; - } - if (!plan.unknown.empty()) { - std::string keys; - for (const auto& k : plan.unknown) keys += (keys.empty() ? "" : ", ") + k; - LOG_ERROR << "firmware asked axis " << need->axis << " for " << keys - << ", which this host does not know -- the firmware is newer than the " - "host; upgrade the host rather than editing hardware.ini"; - } - if (plan.complete()) - LOG_INFO << "firmware config: answered " << plan.sets.size() << " keys for axis " - << need->axis; + answerNeed(*need, /*asked=*/true); } } + // Every key the host can answer, by its canonical name. Used to re-send the + // whole declared set on connect - see resendDeclaredConfig(). + static NeedRequest fullRequest(char axis) { + NeedRequest r; + r.axis = axis; + r.keys = {"steps_per_rev", "global_scaler", "run_irun", "homing_irun", + "center_count", "soft_margin", "enc_deviation", "normal_speed", + "homing_speed", "center_speed", "sw_mode", "has_encoder"}; + return r; + } + + // Push the declared drivetrain at the firmware whether or not it asked. + // + // The firmware asks exactly once, at boot, and only while it is in NEEDCFG. + // Without this, editing hardware.ini and restarting the HOST changes nothing + // - the board is already configured, so it says nothing, and the host has + // nothing to reply to. The tower keeps running the old values silently. + // Re-tuning would need a re-flash, which is seven seconds on a bench and a + // trip up a mast in the field. + // + // CENTER and MARGIN are sent again as SET here, having just gone out as + // their own commands. That is idempotent and costs two lines; keeping the + // set uniform is worth more than saving them. + // + // It also makes the handshake robust to a NEED line lost on the wire: an + // axis still in NEEDCFG is completed by this re-send rather than sitting + // there refusing to move. + void resendDeclaredConfig() { + for (const char* name : {"yaw", "pitch"}) { + if (!cfg.hardware.hasAxis(name)) continue; + answerNeed(fullRequest(name[0] == 'y' ? 'Y' : 'P'), /*asked=*/false); + } + } + + // `asked` distinguishes the two callers. When the firmware ASKED, a key the + // host cannot supply is a startup failure - the axis stays in NEEDCFG and + // will not move, so it is an error. On a proactive re-send it means only + // "hardware.ini does not declare this one", and the firmware simply keeps + // what it already had; saying ERROR there would cry wolf on every start. + void answerNeed(const NeedRequest& need, bool asked) { + const bool is_yaw = need.axis == 'Y'; + + // An axis the tower says it does not have. That is not a missing + // declaration - it is a firmware built for more axes than this rig, + // and answering would configure a driver that is not there. Same + // class of fault as presence reconciliation catches, so it is named + // that way rather than as a hole in hardware.ini. + const char* axis_name = is_yaw ? "yaw" : "pitch"; + if (cfg.hardware.declared && !cfg.hardware.hasAxis(axis_name)) { + LOG_ERROR << "firmware asked for axis " << need.axis << " (" << axis_name + << "), which this tower does not declare (axes " + << cfg.hardware.axesLabel() + << ") -- the firmware was built for more axes than the rig has; " + "rebuild it for this board rather than declaring an axis that is " + "not fitted"; + return; + } + const auto src = is_yaw + ? axisConfigSource("yaw", cfg.geometry.yaw, cfg.drivetrain.yaw, + yaw_current_, cfg.yaw_soft_margin_counts) + : axisConfigSource("pitch", cfg.geometry.pitch, + cfg.drivetrain.pitch, pitch_current_, + cfg.pitch_soft_margin_counts); + + const auto plan = planFirmwareConfig(need, src); + for (const auto& cmd : plan.sets) motor->sendCommand(cmd); + + if (!plan.unanswered.empty() && asked) { + std::string keys; + for (const auto& k : plan.unanswered) keys += (keys.empty() ? "" : ", ") + k; + LOG_ERROR << "firmware asked axis " << need.axis << " for " << keys + << " and hardware.ini does not declare " + << (plan.unanswered.size() == 1 ? "it" : "them") + << "; the axis stays in NEEDCFG and will not move"; + } + if (!plan.unknown.empty() && asked) { + std::string keys; + for (const auto& k : plan.unknown) keys += (keys.empty() ? "" : ", ") + k; + LOG_ERROR << "firmware asked axis " << need.axis << " for " << keys + << ", which this host does not know -- the firmware is newer than the " + "host; upgrade the host rather than editing hardware.ini"; + } + if (asked && plan.complete()) + LOG_INFO << "firmware config: answered " << plan.sets.size() << " keys for axis " + << need.axis; + if (!asked && !plan.sets.empty()) + LOG_INFO << "drivetrain re-sent to axis " << need.axis << ": " << plan.sets.size() + << " values from hardware.ini"; + } + // Convert this tower's declared milliamps into TMC register fields, once. // The host owns this because hardware.ini is where the sense resistor lives; // the firmware is told registers and keeps working compile-time defaults. @@ -1389,6 +1435,12 @@ struct Application::Impl { }; sendCurrent('Y', yaw_current_); sendCurrent('P', pitch_current_); + + // And the rest of the declared drivetrain. CENTER/MARGIN/CURRENT above + // have always been re-sent on every start; the keys the config handshake + // added were not, so half the tuning took effect on a restart and half + // silently did not. This makes all of it behave the same way. + resendDeclaredConfig(); } // Derive each axis's reachable degree window from the freshly homed soft