Repository navigation
Fix probe calibration saves and Fluidd file metadata permissions - #106
iKnowMYABGZ wants to merge 2 commits 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 an existing installation that sets [probe] z_offset in mod_data/user.cfg and has no main-file [probe] section, fix_config has already added the deferred user include (.cfg/tuning.off.cfg:1). This branch appends the base offset after that include, so Klipper's linear include precedence silently changes the effective calibration: for example, -0.72 from user.cfg becomes the base -0.25. Insert the new section before deferred includes, or otherwise preserve their precedence, so an upgrade does not change the nozzle-to-bed calibration.
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.
If an existing main file has an ignored SAVE_CONFIG block—for example, a normal # comment was appended below its saved probe entry—Klipper's _find_autosave_data() rejects that block and still uses the live base offset. This helper instead skips the invalid lines and reports a saved offset, causing migrate() to skip the destination write and delete the only active z_offset. The next Klipper start then fails its required probe-offset read. Validate the complete block using the config reader's rules before treating a saved entry as a replacement.
Useful? React with 👍 / 👎.
| lines = base.splitlines(keepends=True) | ||
| del lines[index] | ||
| atomic_write(base_path, ''.join(lines)) |
There was a problem hiding this comment.
When a valid Klipper offset is split across lines (z_offset: followed by an indented numeric value), offset_lines() yields only the assignment line. The migration consequently writes an empty z_offset to the main file, and this deletion leaves the numeric continuation behind under the preceding base-file option. A configuration that previously loaded -0.25 then fails getfloat('z_offset') on restart. Move the complete option span, or decline the migration without modifying either file when a continuation is present.
Useful? React with 👍 / 👎.
iKnowMYABGZ
changed the title
|
The proposed changes are unnecessary and potentially dangerous, especially the config migration, which could break existing configurations or mess up users' Z-offsets. I don't think it's worth the risk. As for Fluidd, this issue has already been fixed upstream (#1996). We just need to wait for the next release and update Fluidd. I don't see a reason to patch Moonraker for something we'd have to remove later anyway. |
Two fixes encountered on an AD5M, submitted from my maintained
1.4.2branch. Replaces #105.Probe calibration persistence
After
PROBE_CALIBRATEand the paper test,SAVE_CONFIGfails because[probe] z_offsetis defined in includedprinter.base.cfg.Move the AD5M/AD5M Pro default offset into main
printer.cfg. Migrate existing installations during initialization/reload and after stock config migration. Preserve existing main-file or autosaved values, otherwise move the base-file value unchanged. Other probe options, mesh data, and extrusion calibration remain intact. Atomic destination-first writes make retries safe after interruption.Fluidd Jobs menu after metadata updates
server.files.metadataandserver.files.metascanomitpermissions. Fluidd v1.37.7 replaces a file row with the metadata response, then its context menu callsfile.permissions.includes('r'). A newly uploaded/refreshed file can therefore lose its menu while directory-loaded files work. Opening the same file in a fresh browser session worked, consistent with a directory refresh restoring the access field.Both metadata responses now include permissions from the existing
get_path_infologic, preserving read-only, protected-path and symlink restrictions. Build a response copy rather than mutating stored metadata.Validation
Probe/config changes: 62 tests passed across regression, initialization/migration, config backup, and stock config suites; one optional stock-firmware binary test skipped. Actual Klipper config-reader regression reproduces the original conflict and verifies two successive saves/reloads, both model templates, existing overrides/autosaved values, partial sections, idempotence and interrupted writes.
Metadata changes: 5 new endpoint regression tests and 19 related upload tests passed. The regression fails on the original endpoint's missing permissions. The 8 probe migration tests were rerun and passed. Diff whitespace and shell syntax checks passed.
The equivalent probe config relocation was verified on my AD5M with successful SAVE_CONFIG and Ready state. The new migration and metadata code have host-test coverage; they have not been deployed to the live printer. The reported Orca Device menu still needs post-update verification.