Repository navigation
Make the toolchange retract relative, whatever mode the slicer is in - #122
Merged
Merged
Conversation
PHROZEN_TOOLCHANGE opens with `G1 E-1 F3600`, and nothing above it sets the extrusion mode. In M83 that is the 1 mm retract it reads as. In M82 it means "move E to absolute position -1", which mid-print is a retract of everything extruded since the last G92. Klipper does not stop that. max_extrude_only_distance is 3000 on this printer -- Phrozen raised it from the 50 mm default -- so the move is well inside the limit and is carried out. No error, no message: the filament is pulled clear of the extruder and the rest of the job prints air. WHO IS EXPOSED. The Orca profile we ship sets use_relative_e_distances 1, so nobody following the manual meets this. It is reachable with a slicer or a profile that emits absolute E -- which is Cura's default -- and it fires at every colour change once it does. The three other bare E moves in this file, in LOAD_FILAMENT and UNLOAD_FILAMENT, all set M83 first; this was the only one that did not. The mode is read from printer.gcode_move.absolute_extrude, checked present on a running printer rather than assumed. Jinja renders before the first command runs, so the value is the mode on entry -- correct here because nothing above the read changes it, unlike a QUERY_ENDSTOPS read in the same macro. Rendered both ways against Klipper's own jinja2: absolute gives M83 / retract / M82, relative gives M83 / retract / a comment-only line. DELIVERY. The macro sits inside #@feat auto_mode, and the merge adds and removes whole blocks -- it never rewrites one a printer already has, so an edit in place would reach fresh flashes and nowhere else. #@drop is no help either: it refuses a section a block already owns. So the corrected block ships as auto_mode2 and auto_mode is retired in the same run, the way g30 -> g30_meshdone shipped. The pairing is enforced by addon_merge, not trusted: if the block cannot be added the retirement does not run, so the two print macros this block carries cannot go missing. Verified against the real AddOn.cfg of a running printer, processed by that printer's own installed merge: retired 1, merged 1, PHROZEN_TOOLCHANGE and PHROZEN_AMS_START each present exactly once afterwards, the retract carrying its M83, and a second run reporting nothing to do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solutionphil
added a commit
that referenced
this pull request
Sep 4, 2026
A design panel was run over the #@reViSe proposal on six lenses. Its own summary is worthless -- 16 of 38 agents died on a session limit, including every refuter for three lenses and the synthesis, so "no surviving concerns" means NOT CHECKED, not nothing found. The raised concerns were read and judged by hand instead. Most described the proposal as worded rather than what was built: the collision check does not copy the retire pop-loop, the writer does not splice in sequence, the off-state test is addon_features' own first-non-blank-line rule. Four were real. 1. A revision's newly gained sections were checked against `declared` but never written back into it, so an add later in the SAME run tested against a map that predated the revision. Control: without the fix a block adding the same section is merged and the file ends with two of it -- a config klippy refuses. With it the add is skipped and reported. 2. The retirement pairing rule looked only at `add`. A retirement whose sections a REVISED block takes over is a replacement just as much as one handed back by an addition, so it was held back and both copies stayed. Same control, same two-sections-in-the-file result without the fix. 3. auto_mode2 exists on alpha and nowhere else, but a printer that ever ran that channel has it. Restoring #@feat auto_mode without withdrawing auto_mode2 would leave such a printer carrying both -- two PHROZEN_TOOLCHANGE sections. The template now retires it, so those printers take the add path for auto_mode and lose auto_mode2 in the same run. Rehearsed end to end: alpha template first, then this one, ending with one of each section and the fix in place. 4. The guard and the merge disagreed on what "unchanged" means: body_key rstrips every line, the guard's awk did not. An AddOn.cfg edited on Windows carries a CR on every line, which would have made the guard call the block stale on every single boot and start python to be told there was nothing to do. The awk now strips CR and trailing whitespace the same way. Verified with a block given trailing spaces and CRs: the guard stays quiet. Also corrected a docstring that called its own trailing-blank trim redundant. It is not: blocks() returns an unterminated final block all the way to EOF. CI now refuses to ship a template like that, but the function should not assume it. While testing #122 earlier, a merge run against a scratch copy used the real HOME and appended auto_mode2 to the dev printer's addon-seeded-features -- a feature that printer never had. Removed; every run since uses an isolated HOME. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solutionphil
added a commit
that referenced
this pull request
Sep 4, 2026
* Let the template update a block a printer already has The toolchange retract fix had to ship as a renamed block -- #@retire auto_mode plus #@feat auto_mode2 -- because the merge moves whole blocks and has no way to change one that is already on a printer. That works, and it is wrong: it renames a feature the owner can see, for a reason that is none of their business, and it takes the block carrying both print macros off the printer for the length of a merge to put a corrected copy back. So the template can now carry #@reViSe <feature-id> | why and the printer's copy of that block is replaced, in place and at the same position in the file, by the template's current version. auto_mode keeps its name and gets the fix. This is the only operation that reaches inside a block, so it is narrow by construction: * It acts only on a block that is ALREADY THERE. It never adds and never resurrects -- a block the owner deleted by hand stays deleted, which is still the seeded record's decision, not this marker's. * The owner's toggle is carried over. A body that was switched off comes back switched off, line for line the way addon_features.set_block writes it. Verified through addon_features itself: --list reports OFF before and after, and switching the feature back on afterwards yields the NEW body with no sentinel left behind. * A block that already matches is not touched and not reported, so the marker can stay in the template instead of being cleaned up a release later. * The #@ENDFEAT guard applies as everywhere else. With the terminator deleted by hand the run refuses and the file keeps all 68 of its sections. * Only the sections a newer version GAINS are checked for collision; the ones the block already owns would otherwise read as a clash with itself. A revision that would add a section living elsewhere is skipped, not applied. THE WRITER IS NOW ONE WALK. Removals used to be a set of line numbers filtered out and additions a tail append. A replacement is neither, and doing the three in sequence would shift every span measured after the first edit. So drop, loose and revise go into one edit map keyed by original line number and the file is walked once. An overlap check sits on it that should never fire -- blocks do not nest and #@drop only names sections outside them -- and refuses the whole run rather than writing a half-applied file. THE GUARD HAD TO LEARN IT TOO. apply-addon-merge.sh starts python only when a feature id is MISSING, and a revision adds no id -- so without this the mechanism would never have run in the field at all. It now compares the block itself, sentinel stripped, and starts the merge when it differs. Measured: an out-of-date block starts it, and once revised the guard produces no output at all, so python is not started on every klipper start. The control case -- a block edited to look stale again -- starts it once more, so that silence means current rather than blind. WHAT IT COSTS, documented in the header rather than discovered later: a revised block belongs to the kit, not to the owner. Hand edits inside it are overwritten the next time the template moves -- loudly, and with the previous whole file kept as .pre-merge.bak, but overwritten. Checked: the owner's line is gone from AddOn.cfg and present in the backup. Every case above was run against the real AddOn.cfg of a running printer, not a fixture. Fresh installs stay silent: install-addon-cfg.sh copies the template verbatim, so the marker lands in the owner's file, where it is inert -- markers are read from the template only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Refuse a marker that resolves to nothing, in CI rather than on a printer addon_merge guards itself well at run time: an unpaired #@drop is refused, a #@retire for a feature the template still ships is ignored, a #@reViSe naming a block that is not there is skipped. Each is the right thing to do on a printer, and each is silent to whoever wrote the marker -- the message is printed in a journal in somebody's workshop, not in the pull request that got it wrong. The P114 gate ran two versions out of date for months for that kind of reason: the delivery looked like it was working. The same rules, checked where a typo is still cheap. Seven of them: a block that is never closed or that opens while another is still open, a feature id declared twice, a #@retire for a feature still shipped, a #@reViSe with no matching #@feat, #@reViSe and #@retire naming the same id, an unpaired #@drop, and a section declared twice in one template. Each rule has a control case that must fail, run against a copy of the real template with exactly that damage introduced: all seven are caught, and the undamaged template still passes. A gate that can only say yes is worth nothing, which is why the controls exist rather than the passing run alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Close four defects an adversarial review found in the revise path A design panel was run over the #@reViSe proposal on six lenses. Its own summary is worthless -- 16 of 38 agents died on a session limit, including every refuter for three lenses and the synthesis, so "no surviving concerns" means NOT CHECKED, not nothing found. The raised concerns were read and judged by hand instead. Most described the proposal as worded rather than what was built: the collision check does not copy the retire pop-loop, the writer does not splice in sequence, the off-state test is addon_features' own first-non-blank-line rule. Four were real. 1. A revision's newly gained sections were checked against `declared` but never written back into it, so an add later in the SAME run tested against a map that predated the revision. Control: without the fix a block adding the same section is merged and the file ends with two of it -- a config klippy refuses. With it the add is skipped and reported. 2. The retirement pairing rule looked only at `add`. A retirement whose sections a REVISED block takes over is a replacement just as much as one handed back by an addition, so it was held back and both copies stayed. Same control, same two-sections-in-the-file result without the fix. 3. auto_mode2 exists on alpha and nowhere else, but a printer that ever ran that channel has it. Restoring #@feat auto_mode without withdrawing auto_mode2 would leave such a printer carrying both -- two PHROZEN_TOOLCHANGE sections. The template now retires it, so those printers take the add path for auto_mode and lose auto_mode2 in the same run. Rehearsed end to end: alpha template first, then this one, ending with one of each section and the fix in place. 4. The guard and the merge disagreed on what "unchanged" means: body_key rstrips every line, the guard's awk did not. An AddOn.cfg edited on Windows carries a CR on every line, which would have made the guard call the block stale on every single boot and start python to be told there was nothing to do. The awk now strips CR and trailing whitespace the same way. Verified with a block given trailing spaces and CRs: the guard stays quiet. Also corrected a docstring that called its own trailing-blank trim redundant. It is not: blocks() returns an unterminated final block all the way to EOF. CI now refuses to ship a template like that, but the function should not assume it. While testing #122 earlier, a merge run against a scratch copy used the real HOME and appended auto_mode2 to the dev printer's addon-seeded-features -- a feature that printer never had. Removed; every run since uses an isolated HOME. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PHROZEN_TOOLCHANGEopens withG1 E-1 F3600, and nothing above it sets the extrusion mode. InM83that is the 1 mm retract it reads as. InM82it means move E to absolute position −1 — mid-print, a retract of everything extruded since the lastG92.Klipper does not stop it.
max_extrude_only_distanceis 3000 on this printer (Phrozen raised it from the 50 mm default), so the move is well inside the limit and is carried out. No error, no message: the filament is pulled clear of the extruder and the rest of the job prints air.Who is exposed
The Orca profile we ship sets
use_relative_e_distances: 1, so nobody following the manual meets this. It is reachable with a slicer or profile that emits absolute E — Cura's default — and then it fires at every colour change.The three other bare E moves in this file, in
LOAD_FILAMENTandUNLOAD_FILAMENT, all setM83first. This was the only one that did not.The fix
absolute_extrudewas checked present on a running printer, not assumed. Jinja renders before the first command runs, so the value is the mode on entry — correct here because nothing above the read changes it (unlike aQUERY_ENDSTOPSread in the same macro). Rendered both ways against Klipper's own jinja2:M82absoluteM83→ retract →M82M83relativeM83→ retract → comment-only lineDelivery — why the block changes id
The macro sits inside
#@FEAT auto_mode, and the merge adds and removes whole blocks; it never rewrites one a printer already has, so an edit in place would reach fresh flashes and nowhere else.#@DROPis no help either — it refuses a section a block already owns ("the block owns it").So the corrected block ships as
auto_mode2andauto_modeis retired in the same run, exactly asg30→g30_meshdoneshipped. The pairing is enforced byaddon_merge, not trusted: if the block cannot be added, the retirement does not run — the two print macros this block carries cannot go missing.Verified against the real
AddOn.cfgof a running printer, processed by that printer's own installed merge:PHROZEN_TOOLCHANGEafterwardsPHROZEN_AMS_STARTafterwardsM83