From 02a3410dc108c6e6d6edd91b4466301ec10ff409 Mon Sep 17 00:00:00 2001 From: solutionphil Date: Fri, 4 Sep 2026 01:17:25 +0200 Subject: [PATCH 1/3] 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 | 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 --- .github/CONTRIBUTING.md | 6 +- README.html | 2 +- README.md | 2 +- config-templates/AddOn.cfg.template | 15 +-- scripts/addon_merge.py | 196 +++++++++++++++++++++++++--- scripts/apply-addon-merge.sh | 26 +++- 6 files changed, 217 insertions(+), 30 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index ee5ec58..472cf6c 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -120,8 +120,10 @@ git checkout -B _sync2 origin/alpha && git merge --no-ff origin/beta && git push same change. Its config section arrives with the same update, and a section whose module is missing is not a missing feature — klippy refuses the whole config. - Changing `config-templates/AddOn.cfg.template`? Read the header of `scripts/addon_merge.py` first. - That file is never regenerated on a printer that already has one, so only whole `#@FEAT` blocks - reach existing machines. A change *inside* an existing block reaches nobody. + That file is never regenerated on a printer that already has one, so a new `#@FEAT` block is what + normally reaches existing machines. A change *inside* a block reaches nobody unless the template + also names that block in a `#@REVISE`, which replaces the printer's copy of it in place — read what + that costs before reaching for it. ## Version numbers diff --git a/README.html b/README.html index 08352e6..9136695 100644 --- a/README.html +++ b/README.html @@ -985,7 +985,7 @@

OrcaSlicer — multicolor cell. G30 then loads your saved phrozen mesh instantly — calibrating and saving it the first time, if none exists yet. Homing is automatic. It needs the -auto_mode2 and ams features in +auto_mode and ams features in AddOn.cfg, both on by default, and the gcode_shell_command extension.

diff --git a/README.md b/README.md index 2e4732b..01d6eaa 100644 --- a/README.md +++ b/README.md @@ -498,7 +498,7 @@ colour** — otherwise a long print changes shade partway through, and nothing w The start G-code heats the **bed straight to print temperature** and holds the **nozzle at a probe-safe 140 °C** through home and probe, so it cannot ooze onto the load cell. **`G30`** then loads your saved `phrozen` mesh instantly — calibrating and saving it the first time, if none exists yet. Homing is -automatic. It needs the `auto_mode2` and `ams` features in `AddOn.cfg`, both on by default, and the +automatic. It needs the `auto_mode` and `ams` features in `AddOn.cfg`, both on by default, and the `gcode_shell_command` extension. diff --git a/config-templates/AddOn.cfg.template b/config-templates/AddOn.cfg.template index 5a58c91..ab47661 100644 --- a/config-templates/AddOn.cfg.template +++ b/config-templates/AddOn.cfg.template @@ -773,14 +773,13 @@ gcode: #@RETIRE ams | AMS on/off switch - the AMS is detected now, so there is nothing to declare # -# PHROZEN_TOOLCHANGE lives inside this block, and a change *inside* a block reaches fresh flashes -# and nowhere else -- the merge adds and removes whole blocks, it never rewrites one a printer -# already has. So the corrected macro ships under a new id and the old block is retired in the same -# run, under the same backup, exactly as the g30 -> g30_meshdone replacement did. The pairing is -# enforced by addon_merge, not trusted: if this block cannot be added, the retirement does not run, -# so the two print macros this block carries can never go missing. -#@RETIRE auto_mode | replaced by auto_mode2, which fixes a retract that assumed relative E -#@FEAT auto_mode2 | Orca auto AMS-mode: reads the 'ams' flag -> P0 M1/M2/M3 at print start +# PHROZEN_TOOLCHANGE lives inside this block. Blocks are the unit the merge moves, so an edit made +# inside one used to reach fresh flashes and nowhere else -- which is why this marker exists. It says: +# this block is the kit's, bring an existing printer's copy up to the version below. The owner's +# on/off toggle is carried over, a block that already matches is left alone and not even mentioned, +# and a block the owner deleted by hand is NOT put back -- that stays the seeded record's decision. +#@REVISE auto_mode | the toolchange retract assumed relative E; see the M83 note in the macro +#@FEAT auto_mode | Orca auto AMS-mode: reads the 'ams' flag -> P0 M1/M2/M3 at print start # ───── Flag-driven AMS mode for OrcaSlicer (Arco Unleashed) ───── # The 'ams' flag follows the AMS device node, kept in step by [arco_tool_gate] -- nobody sets # it, and there is nothing to remember to set. This macro reads it at print start and applies diff --git a/scripts/addon_merge.py b/scripts/addon_merge.py index 196faf1..5be3f57 100755 --- a/scripts/addon_merge.py +++ b/scripts/addon_merge.py @@ -10,10 +10,12 @@ # template reached fresh flashes only. The startup banner and the one-time welcome dialog shipped, and # every existing printer silently did not get them, including the two testers' and the dev machine's. # -# WHAT IT WILL NOT DO. It never edits a block that is present, never reorders, and never touches a -# toggle. Only whole blocks that are absent get appended. That is why the banner was split out of the -# `beeper` feature into one of its own: a change INSIDE an existing block cannot be delivered this -# way, and pretending otherwise would mean rewriting the owner's file. +# WHAT IT WILL NOT DO. It never reorders, and it never changes a toggle. Blocks that are absent get +# appended; a block that is present is left exactly as it stands UNLESS the template names it in a +# #@REVISE, which is the one deliberate exception and is described below. That exception is recent: +# for a long time a change INSIDE an existing block could not be delivered at all, which is why the +# banner was split out of the `beeper` feature into a block of its own -- splitting was the only +# honest way round it. # # RETIRING A FEATURE. Adding was enough until a feature turned out to be wrong rather than merely # improvable -- the AMS on/off switch, which asked the owner a question the printer answers better by @@ -39,6 +41,35 @@ # being added this run. Either check failing means nothing is removed. An unpaired #@DROP is a # delete instruction sitting in a template, one typo from taking a working macro off every printer. # +# UPDATING A BLOCK IN PLACE. Splitting works when the change can be expressed as a NEW block. It does +# not when the thing to fix is a line in the middle of a macro that already ships. PHROZEN_TOOLCHANGE +# opened its retract without setting the extrusion mode, and the block that carries it carries the +# printer's two print macros as well -- so retiring it to ship a corrected copy under a new id would +# have meant taking those two off every printer for the length of one merge, and would have renamed a +# feature the owner can see for a reason that is none of their business. So the template can carry +# +# #@REVISE | 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. This is the only operation that reaches INSIDE a block, so it is +# deliberately narrow: +# +# * It acts only on a block that is ALREADY THERE. A #@REVISE never adds and never resurrects; a +# block the owner deleted by hand stays deleted, which is still the seeded record's decision. +# * 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 -- otherwise an update would quietly switch +# a feature back on, which is the one thing a toggle exists to prevent. +# * A block that already matches is not touched and not reported, so the marker can stay in the +# template for good instead of needing to be cleaned up a release later. +# * The #@ENDFEAT guard applies as everywhere else: an unterminated block is left alone. +# * Only the sections the newer version GAINS are checked for collision. The ones the block already +# owns are its own, and would otherwise read as a clash with itself. +# +# And it has a real cost, stated here rather than discovered: 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. Anyone wanting their own +# version should switch the feature off and put theirs outside the block. +# # A section that already sits inside a #@FEAT block is never touched by #@DROP -- the block # machinery owns it -- which is also what makes the operation idempotent once it has run. Removal is deliberately blunt and deliberately loud: whole block, reported by name, with # the owner's previous file kept. Two guards sit on it -- a block whose #@ENDFEAT is missing is left @@ -64,6 +95,7 @@ FEAT = re.compile(r'^#@FEAT\s+(\S+)\s*\|\s*(.*)$') RETIRE = re.compile(r'^#@RETIRE\s+(\S+)\s*\|\s*(.*)$') +REVISE = re.compile(r'^#@REVISE\s+(\S+)\s*\|\s*(.*)$') DROP = re.compile(r'^#@DROP\s+(.+?)\s*\|\s*(.*)$') END = "#@ENDFEAT" OFF = "#:off:" @@ -105,6 +137,58 @@ def retirements(path): return out + + +def revisions(path): + """{feature_id: reason} the template wants brought up to date IN PLACE.""" + try: + lines = open(path, encoding="utf-8").read().split("\n") + except OSError: + return {} + out = {} + for l in lines: + m = REVISE.match(l) + if m: + out[m.group(1)] = m.group(2).strip() + return out + + +def block_is_off(body): + """The owner's toggle, read the way addon_features.py writes it: the FIRST non-blank body line + between the markers decides. Nothing else in the block is consulted, so a block that is half + prefixed -- a hand edit, or an older toggle -- reads as whatever its first real line says, which + is exactly what the features tool would report and what SET_FEATURE would act on.""" + for l in body[1:]: + if l.startswith(END): + break + if l.strip(): + return l.startswith(OFF) + return False + + +def with_off(body): + """Put a block into the switched-off shape: the sentinel on every non-blank BODY line, the #@FEAT + and #@ENDFEAT lines left bare. Mirrors set_block() in addon_features.py line for line -- if the + two ever disagree, the features tool would report a state the file does not have.""" + out = [body[0]] + for l in body[1:]: + if l.startswith(END) or not l.strip() or l.startswith(OFF): + out.append(l) + else: + out.append(OFF + l) + return out + + +def body_key(body): + """A block reduced to what a revision actually cares about: the toggle is not content, and + trailing blank lines are not either. Two blocks with the same key need no revision, which is what + makes a #@REVISE that has already run a silent no-op rather than a rewrite on every boot.""" + out = [l[len(OFF):] if l.startswith(OFF) else l for l in body] + out = [l.rstrip() for l in out] + while out and not out[-1]: + out.pop() + return out + def block_span(lines, fid): """(start, end_inclusive) of a complete #@FEAT block, or None if absent or unterminated.""" for i, l in enumerate(lines): @@ -225,6 +309,7 @@ def main(): want_ids = {f for f, _, _ in want} retire = retirements(tpl) + revise = revisions(tpl) drop = [] for fid in sorted(retire): if fid in want_ids: @@ -260,8 +345,40 @@ def main(): continue # absent, or already inside a block: either way not ours to touch loose.append((name, why, span)) + + # A block that is PRESENT but out of date. Adding and retiring move whole blocks around; this is + # the only operation that reaches INSIDE a printer's existing file, so it is deliberately narrow. + # It acts on a block that is already there and never on one that is not: resurrecting a block the + # owner deleted by hand is what the seeded record exists to prevent, and a #@REVISE must not walk + # around it. A block that already matches the template is left alone and not reported, so the + # marker can stay in the template for good instead of being cleaned up one release later. + skip_rev = [] + rev = [] + for fid in sorted(revise): + if fid not in have: + continue # absent: the add path and the seeded record decide, not this + if fid in {f for f, _, _ in drop}: + skip_rev.append((fid, "the template both retires and revises it — doing neither")) + continue + span = block_span(cfg_lines, fid) + if span is None: + skip_rev.append((fid, "its #@ENDFEAT is missing — left alone rather than rewriting past it")) + continue + old = cfg_lines[span[0]:span[1] + 1] + new = None + for f, _, body in want: + if f == fid: + new = body + break + if new is None: + skip_rev.append((fid, "the template revises a block it no longer ships — ignoring it")) + continue + if body_key(old) == body_key(new): + continue # already current: silent, so a run changes nothing twice + rev.append((fid, revise[fid], span, with_off(new) if block_is_off(old) else new)) + missing = [(f, d, b) for f, d, b in want if f not in have] - if not missing and not drop and not loose: + if not missing and not drop and not loose and not rev and not skip_rev: print(" AddOn.cfg already carries all %d features — nothing to do." % len(want)) return 0 @@ -276,6 +393,23 @@ def main(): for _, _, (a, b) in drop: for name in sections_in(cfg_lines[a:b + 1]): declared.pop(name, None) + + # A revision normally hands back the same sections it takes out, so it cannot collide with itself. + # It CAN collide if the newer version of the block declares a section that meanwhile exists + # somewhere else in the config directory -- then applying it would hand klippy two of the same + # section and the printer would not come back. Only the genuinely NEW names are checked; the ones + # the block already owns are its own and are not a clash. + kept_rev = [] + for fid, why, span, body in rev: + gained = sections_in(body) - sections_in(cfg_lines[span[0]:span[1] + 1]) + clash = sorted(s for s in gained if s in declared) + if clash: + skip_rev.append((fid, "its newer version would add %s (already in %s) — not revising it" + % (", ".join("[%s]" % c for c in clash), declared[clash[0]]))) + continue + kept_rev.append((fid, why, span, body)) + rev = kept_rev + add, skip = [], [] for fid, desc, body in missing: if fid in seeded: @@ -314,6 +448,7 @@ def replaced_elsewhere(span): "not retiring it")) drop = [d for d in drop if d[0] not in held] + skip.extend(skip_rev) for fid, why in skip: print(" SKIP %-18s %s" % (fid, why)) for fid, why, _ in drop: @@ -321,24 +456,53 @@ def replaced_elsewhere(span): for name, why, _ in loose: print(" %s %-18s %s" % ("would replace" if verb == "check" else "replacing ", name.split()[-1], why)) + for fid, why, _, _ in rev: + print(" %s %-18s %s" % ("would revise" if verb == "check" else "revising ", fid, why)) for fid, desc, _ in add: print(" %s %-18s %s" % ("would add " if verb == "check" else "adding ", fid, desc)) if verb == "check": - print(" (check only — nothing written. Run with 'apply' to merge %d, retire %d, replace %d.)" - % (len(add), len(drop), len(loose))) + print(" (check only — nothing written. Run with 'apply' to merge %d, retire %d, replace %d, " + "revise %d.)" % (len(add), len(drop), len(loose), len(rev))) return 0 - if not add and not drop and not loose: + if not add and not drop and not loose and not rev: print(" nothing to add.") return 0 shutil.copy2(cfg, cfg + ".pre-merge.bak") - kept = cfg_lines - if drop or loose: - gone = set() - for _, _, (a, b) in list(drop) + [(n, w, s) for n, w, s in loose]: - gone.update(range(a, b + 1)) - kept = [l for i, l in enumerate(cfg_lines) if i not in gone] + # ONE edit map, over the ORIGINAL line numbers. Every span in drop / loose / rev was measured + # against cfg_lines, so they are only valid together: applying them one after another would shift + # every span that comes after the first edit. Walking the file once and consulting the map keeps + # them all correct, and it is the same walk whether a span is being emptied (a removal) or + # refilled (a revision) -- which is why those two do not need separate code paths. + edit = {} + for _, _, span in drop: + edit[span[0]] = (span[1], []) + for _, _, span in loose: + edit[span[0]] = (span[1], []) + for _, _, span, body in rev: + edit[span[0]] = (span[1], body) + # Overlap would mean one edit eating another's lines, and a file nobody intended. It cannot + # happen as things stand -- blocks do not nest, and a #@DROP only ever names a section that sits + # outside every block -- so this is a check that should never fire rather than a fix for a known + # case. If it ever does, refusing is right: nothing has been written yet at this point, so the + # printer keeps the file it had. + covered = set() + for a, (b, _) in sorted(edit.items()): + span_lines = set(range(a, b + 1)) + if span_lines & covered: + print(" two edits overlap in this file — refusing to write anything.") + return 1 + covered |= span_lines + kept, i = [], 0 + while i < len(cfg_lines): + if i in edit: + end, body = edit[i] + kept.extend(body) + i = end + 1 + else: + kept.append(cfg_lines[i]) + i += 1 with open(cfg, "w", encoding="utf-8") as fh: fh.write("\n".join(kept).rstrip("\n") + "\n") for _, _, body in add: @@ -349,8 +513,8 @@ def replaced_elsewhere(span): fh.write(fid + "\n") # os.fsync on the directory as well: this runs immediately before selfupdate.sh asks for a # power-cycle, and the rootfs is mounted commit=120. See the sync in selfupdate.sh. - print(" merged %d, retired %d, replaced %d; previous file kept as %s" - % (len(add), len(drop), len(loose), os.path.basename(cfg) + ".pre-merge.bak")) + print(" merged %d, retired %d, replaced %d, revised %d; previous file kept as %s" + % (len(add), len(drop), len(loose), len(rev), os.path.basename(cfg) + ".pre-merge.bak")) print(" They take effect when the klipper SERVICE restarts.") return 0 diff --git a/scripts/apply-addon-merge.sh b/scripts/apply-addon-merge.sh index 9075728..b9e258d 100755 --- a/scripts/apply-addon-merge.sh +++ b/scripts/apply-addon-merge.sh @@ -49,9 +49,31 @@ want="$(feats "$TPL")" # forever and start python on every single boot to be told no. known="$( { feats "$CFG"; sort -u "$STATE" 2>/dev/null; } | sort -u )" missing="$(comm -23 <(printf '%s\n' "$want") <(printf '%s\n' "$known") 2>/dev/null)" -[ -n "$missing" ] || exit 0 -echo " AddOn.cfg: $(printf '%s\n' "$missing" | grep -c .) new feature(s) in the template — merging" +# A #@REVISE names a block the printer ALREADY has, so it adds no feature id and the comparison above +# cannot see it -- without this the merge would never be started for one. Compare the block itself, +# with the off-sentinel stripped so a feature the owner switched off is not mistaken for a stale one. +# POSIX character classes are avoided inside awk on purpose: mawk 1.3.3 does not have them, and this +# script is expected to keep working on a stock printer as well as an Unleashed one. +blk(){ awk -v id="$2" 'BEGIN{ re = "^#@FEAT[ \t]+" id "[ \t]*[|]" } + $0 ~ re { f=1 } + f { sub(/^#:off:/, ""); print } + f && /^#@ENDFEAT/ { exit }' "$1" 2>/dev/null; } +stale="" +for id in $(grep -oE '^#@REVISE[[:space:]]+[^[:space:]]+' "$TPL" 2>/dev/null | awk '{print $2}' | sort -u); do + here="$(blk "$CFG" "$id")" + [ -n "$here" ] || continue # not in this file: the add path and the seeded record decide + [ "$here" = "$(blk "$TPL" "$id")" ] || { stale="$id"; break; } +done + + +[ -n "$missing" ] || [ -n "$stale" ] || exit 0 + +if [ -n "$missing" ]; then + echo " AddOn.cfg: $(printf '%s\n' "$missing" | grep -c .) new feature(s) in the template — merging" +else + echo " AddOn.cfg: the template carries a newer '$stale' — merging" +fi python3 "$DIR/addon_merge.py" apply "$CFG" "$TPL" 2>&1 | sed 's/^/ /' # Never fail the unit. The '-' prefix on the ExecStartPre already covers this, but a guard that returns From f55750fb5269d3b5cbb9c56ea8295a9a30cc5a3f Mon Sep 17 00:00:00 2001 From: solutionphil Date: Fri, 4 Sep 2026 01:19:50 +0200 Subject: [PATCH 2/3] 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 --- .github/ci/check-addon-markers.py | 123 ++++++++++++++++++++++++++++++ .github/workflows/ci.yml | 6 ++ 2 files changed, 129 insertions(+) create mode 100755 .github/ci/check-addon-markers.py diff --git a/.github/ci/check-addon-markers.py b/.github/ci/check-addon-markers.py new file mode 100755 index 0000000..6e8c4b6 --- /dev/null +++ b/.github/ci/check-addon-markers.py @@ -0,0 +1,123 @@ +#!/usr/bin/env python3 +"""Every marker in AddOn.cfg.template does something, or the build stops. + +addon_merge.py 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 of those is the right thing to do on a printer -- and each is silent to whoever wrote the +marker, because the message is printed on a machine in somebody's workshop rather than in the pull +request that got it wrong. The P114 gate ran two versions out of date for months for exactly this +kind of reason: the delivery looked like it was working. + +So the same rules are checked here, where a typo is still cheap. +""" +import re +import sys +import os + +TPL = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", + "config-templates", "AddOn.cfg.template") + +FEAT = re.compile(r'^#@FEAT\s+(\S+)\s*\|\s*(.*)$') +RETIRE = re.compile(r'^#@RETIRE\s+(\S+)\s*\|\s*(.*)$') +REVISE = re.compile(r'^#@REVISE\s+(\S+)\s*\|\s*(.*)$') +DROP = re.compile(r'^#@DROP\s+(.+?)\s*\|\s*(.*)$') +END = "#@ENDFEAT" +OFF = "#:off:" +SECTION = re.compile(r'^(?:' + re.escape(OFF) + r')?\[([^\]]+)\]\s*$') + + +def main(): + path = sys.argv[1] if len(sys.argv) > 1 else TPL + lines = open(path, encoding="utf-8").read().split("\n") + bad = [] + + # 1. Every block is closed, and closed before the next one opens. addon_merge refuses to touch an + # unterminated block precisely because removing it would swallow the neighbour -- which in this + # file means the two print macros. + feats, open_at, ids = {}, None, [] + for n, l in enumerate(lines, 1): + m = FEAT.match(l) + if m: + if open_at is not None: + bad.append("line %d: #@FEAT %s opens while %s is still open (a missing #@ENDFEAT)" + % (n, m.group(1), open_at[1])) + open_at = (n, m.group(1)) + ids.append(m.group(1)) + feats[m.group(1)] = [n, None] + elif l.startswith(END): + if open_at is None: + bad.append("line %d: #@ENDFEAT with no #@FEAT above it" % n) + else: + feats[open_at[1]][1] = n + open_at = None + if open_at is not None: + bad.append("line %d: #@FEAT %s is never closed" % (open_at[0], open_at[1])) + + dupes = sorted({f for f in ids if ids.count(f) > 1}) + for d in dupes: + bad.append("#@FEAT %s is declared more than once — the merge would only ever see the first" % d) + + shipped = set(feats) + + # 2. A #@RETIRE for a feature the template still ships is ignored at run time (addon_merge line + # ~231). Silently: the block stays on every printer and the author believes it went. + for n, l in enumerate(lines, 1): + m = RETIRE.match(l) + if m and m.group(1) in shipped: + bad.append("line %d: #@RETIRE %s, but the template still ships that #@FEAT — " + "the retirement would be ignored" % (n, m.group(1))) + + # 3. A #@REVISE is the mirror image: it can only act on a block the template also ships, because + # the replacement body comes from that block. + revised = set() + for n, l in enumerate(lines, 1): + m = REVISE.match(l) + if not m: + continue + revised.add(m.group(1)) + if m.group(1) not in shipped: + bad.append("line %d: #@REVISE %s, but no #@FEAT %s in this template — " + "there is nothing to revise it to" % (n, m.group(1), m.group(1))) + for f in sorted(revised & {m.group(1) for m in (RETIRE.match(l) for l in lines) if m}): + bad.append("#@REVISE and #@RETIRE both name %s — the merge would do neither" % f) + + # 4. A #@DROP must be paired with a block that hands the section back, or it is a delete + # instruction with nothing behind it. + in_block = set() + for fid, (a, b) in feats.items(): + if b is None: + continue + for l in lines[a - 1:b]: + m = SECTION.match(l) + if m: + in_block.add(m.group(1).strip()) + for n, l in enumerate(lines, 1): + m = DROP.match(l) + if m and m.group(1) not in in_block: + bad.append("line %d: #@DROP %s, but no #@FEAT block in this template declares that " + "section — the merge would refuse to remove it" % (n, m.group(1))) + + # 5. Two sections of the same name in one template is a config klippy refuses, and it would be + # delivered to every fresh install. + seen = {} + for n, l in enumerate(lines, 1): + m = SECTION.match(l) + if not m: + continue + name = m.group(1).strip() + if name in seen: + bad.append("line %d: [%s] is declared twice (first at line %d)" % (n, name, seen[name])) + seen[name] = n + + if bad: + print("AddOn.cfg.template markers:") + for b in bad: + print(" ERROR %s" % b) + return 1 + print("AddOn.cfg.template: %d features, %d revisions, every marker resolves." + % (len(shipped), len(revised))) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index eefb652..bbe9cce 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -196,6 +196,12 @@ jobs: - name: Anchors and step numbers run: python3 .github/ci/check-docs.py + # A marker that resolves to nothing is refused on the printer and silent to whoever wrote it: + # the message goes to a journal in somebody's workshop, not to the pull request. The P114 gate + # ran two versions out of date for months for that kind of reason. Same rules, checked here. + - name: AddOn.cfg markers + run: python3 .github/ci/check-addon-markers.py + # The documents overlap on purpose and drift for free. This fails on text that has quietly # acquired a second owner, and -- when there is a base commit to compare against -- points out # sentences elsewhere that closely matched the wording just changed here. The second half is a From 630036874b8ba47860624846f65ac35a6e1025f9 Mon Sep 17 00:00:00 2001 From: solutionphil Date: Fri, 4 Sep 2026 11:07:03 +0200 Subject: [PATCH 3/3] 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 --- config-templates/AddOn.cfg.template | 7 +++++++ scripts/addon_merge.py | 21 +++++++++++++++++---- scripts/apply-addon-merge.sh | 2 +- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/config-templates/AddOn.cfg.template b/config-templates/AddOn.cfg.template index ab47661..7e11b21 100644 --- a/config-templates/AddOn.cfg.template +++ b/config-templates/AddOn.cfg.template @@ -778,6 +778,13 @@ gcode: # this block is the kit's, bring an existing printer's copy up to the version below. The owner's # on/off toggle is carried over, a block that already matches is left alone and not even mentioned, # and a block the owner deleted by hand is NOT put back -- that stays the seeded record's decision. +# One printer generation carried this block as `auto_mode2`, from the short-lived attempt to deliver +# the retract fix by renaming the block instead of revising it. #@REVISE cannot reach those: they have +# no `auto_mode` to revise. They get `auto_mode` through the normal add path, and this line takes the +# old copy out in the same run -- without it such a printer would end up with both, which is two +# [gcode_macro PHROZEN_TOOLCHANGE] sections and a config klippy refuses. +#@RETIRE auto_mode2 | folded back into auto_mode, which is the name it should never have lost + #@REVISE auto_mode | the toolchange retract assumed relative E; see the M83 note in the macro #@FEAT auto_mode | Orca auto AMS-mode: reads the 'ams' flag -> P0 M1/M2/M3 at print start # ───── Flag-driven AMS mode for OrcaSlicer (Arco Unleashed) ───── diff --git a/scripts/addon_merge.py b/scripts/addon_merge.py index 5be3f57..007626d 100755 --- a/scripts/addon_merge.py +++ b/scripts/addon_merge.py @@ -180,9 +180,13 @@ def with_off(body): def body_key(body): - """A block reduced to what a revision actually cares about: the toggle is not content, and - trailing blank lines are not either. Two blocks with the same key need no revision, which is what - makes a #@REVISE that has already run a silent no-op rather than a rewrite on every boot.""" + """A block reduced to what a revision actually cares about. The toggle is not content, and neither + is trailing whitespace -- an owner who opened AddOn.cfg in a Windows editor gets a CR on every + line, and without rstrip every one of those blocks would read as stale for good. The trailing + blank trim is for the one body that does not end at #@ENDFEAT: blocks() returns a final + unterminated block all the way to EOF. CI refuses to ship a template like that, but this function + is not the place to assume it. Equal keys mean no revision, which is what makes a #@REVISE that + has already run a silent no-op rather than a rewrite on every boot.""" out = [l[len(OFF):] if l.startswith(OFF) else l for l in body] out = [l.rstrip() for l in out] while out and not out[-1]: @@ -408,6 +412,11 @@ def main(): % (", ".join("[%s]" % c for c in clash), declared[clash[0]]))) continue kept_rev.append((fid, why, span, body)) + # The names this revision brings in are declared from now on. Without this the add loop that + # runs next would test its blocks against a map that predates the revision and could hand + # klippy the same section twice -- the one failure mode this whole check exists to stop. + for s in gained: + declared[s] = os.path.basename(cfg) rev = kept_rev add, skip = [], [] @@ -439,10 +448,14 @@ def replaced_elsewhere(span): gone = sections_in(cfg_lines[span[0]:span[1] + 1]) return gone and any(gone & sections_in(b) for _, _, b in want) + # A revision hands sections back exactly like an addition does, so it counts here too: retiring a + # block whose sections a REVISED block takes over is a replacement, and looking only at `add` + # would hold the retirement back and leave both copies in the file. + handed_back = [b for _, _, b in add] + [b for _, _, _, b in rev] held = [f for f, _, s in drop if replaced_elsewhere(s) and not any(sections_in(cfg_lines[s[0]:s[1] + 1]) & sections_in(b) - for _, _, b in add)] + for b in handed_back)] for f in held: skip.append((f, "the block that carries its sections is not being added this run — " "not retiring it")) diff --git a/scripts/apply-addon-merge.sh b/scripts/apply-addon-merge.sh index b9e258d..eaf0515 100755 --- a/scripts/apply-addon-merge.sh +++ b/scripts/apply-addon-merge.sh @@ -57,7 +57,7 @@ missing="$(comm -23 <(printf '%s\n' "$want") <(printf '%s\n' "$known") 2>/dev/nu # script is expected to keep working on a stock printer as well as an Unleashed one. blk(){ awk -v id="$2" 'BEGIN{ re = "^#@FEAT[ \t]+" id "[ \t]*[|]" } $0 ~ re { f=1 } - f { sub(/^#:off:/, ""); print } + f { sub(/^#:off:/, ""); gsub(/\r/, ""); sub(/[ \t]+$/, ""); print } f && /^#@ENDFEAT/ { exit }' "$1" 2>/dev/null; } stale="" for id in $(grep -oE '^#@REVISE[[:space:]]+[^[:space:]]+' "$TPL" 2>/dev/null | awk '{print $2}' | sort -u); do