Repository navigation
Fix probe Z-offset SAVE_CONFIG conflicts on installs and upgrades - #105
iKnowMYABGZ wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a46bbcf08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| marker = next((i for i, line in enumerate(lines) | ||
| if line.startswith('#*# <') and 'SAVE_CONFIG' in line), len(lines)) | ||
| before = ''.join(lines[:marker]).rstrip('\r\n') | ||
| updated = before + '\n\n[probe]\n' + value.rstrip('\r\n') + '\n\n' + ''.join(lines[marker:]) |
There was a problem hiding this comment.
On upgrades where the probe offset is customized in mod_data/user.cfg and printer.cfg has no [probe] section, this places the stock value after the deferred user/tuning include (.cfg/tuning.off.cfg:1, or tuning.cfg:31). Running fix_config() with the actual Klipper reader changes the effective offset from, for example, -0.72 to -0.25: the previously working user override is silently shadowed. Insert the relocated default before deferred overrides, or otherwise preserve the effective configured value, so upgrading does not change the printer's calibrated probe offset.
Useful? React with 👍 / 👎.
| if len(offsets) != 1: | ||
| raise ValueError('Expected one [probe] z_offset in printer.base.cfg') |
There was a problem hiding this comment.
If an existing base file repeats z_offset, including in a second [probe] section, Klipper's parser explicitly uses strict=False (.py/klipper/patches/configfile.py:294-295) and loads the last value. This instead raises ValueError for that otherwise working configuration; the new mandatory call from fix_config() propagates the failure and stops normal initialization. Preserve the effective last value and remove all migrated definitions, or leave the configuration unchanged with a warning rather than preventing Forge-X from booting.
Useful? React with 👍 / 👎.
| if (HEADER.match(line.rstrip('\r\n')) and | ||
| HEADER.match(line.rstrip('\r\n')).group(1).strip() == 'probe')), None) | ||
| if insert is not None: | ||
| lines.insert(insert, value.rstrip('\r\n') + '\n') |
There was a problem hiding this comment.
When an already initialized printer.cfg ends with an empty [probe] header without a trailing newline, the configuration is valid and inherits its probe options from the base include. However, inserting this line produces [probe]z_offset: -0.25 on one line, and migration then deletes the real offset from the base. The resulting configuration has no probe.z_offset, so the required getfloat('z_offset') in extras/probe.py fails and Klipper cannot start. Ensure the preceding line is terminated before inserting the option.
Useful? React with 👍 / 👎.
| elif in_saved and line.startswith('#*#'): | ||
| saved.append(line[3:].lstrip(' ')) | ||
| return bool(list(offset_lines(''.join(saved)))) |
There was a problem hiding this comment.
When a commented probe offset exists in a SAVE_CONFIG block that Klipper rejects—for example, because a normal # comment was appended after the block—Klipper's _find_autosave_data() ignores the saved data and continues using the base offset. This detector nevertheless returns true because it ignores non-#*# lines, so migration skips writing a replacement and removes the only active offset. The previously runnable configuration then fails Klipper startup because probe.z_offset is missing. Apply the runtime's autosave validation rules before treating a commented offset as a usable replacement.
Useful? React with 👍 / 👎.
Completing
PROBE_CALIBRATEand the paper test can produce a valid offset, but saving it fails with:The AD5M and AD5M Pro defaults define the offset in included
printer.base.cfg, while Klipper writes calibration results into the mainprinter.cfg.This change moves the default offset into the main config and migrates existing installations during initialization/reload. Existing main-file or autosaved offsets take precedence; otherwise the base-file value is moved unchanged. Other probe options, mesh data, and extrusion calibration are preserved. Migration writes the destination first, uses atomic replacements, and is idempotent/recoverable if interrupted. It runs again after stock config migration, which may reintroduce the option.
Validation: 62 tests passed across probe-offset regression, initialization/migration, config backup, and stock config suites; one optional real stock-firmware binary test skipped. Shell syntax and diff whitespace checks passed. The new regression reproduces the original conflict using the actual Klipper config reader, then verifies two successive SAVE_CONFIG/reload cycles. Additional cases cover both model templates, existing overrides/autosaved values, partial probe sections, idempotence, and write failures.
The equivalent config relocation was also verified on my AD5M: SAVE_CONFIG succeeded, Klipper returned Ready, and the calibrated probe offset was loaded. The new boot migration itself has host-test coverage; I have not deployed that code to the printer yet.