Skip to content

fix(openrfid): fix review issues from PR #306 (PIDFILE, stray echo, help_url, error handling)#321

Closed
kbaker827 wants to merge 3 commits intopaxx12:developfrom
kbaker827:fix/openrfid-review
Closed

fix(openrfid): fix review issues from PR #306 (PIDFILE, stray echo, help_url, error handling)#321
kbaker827 wants to merge 3 commits intopaxx12:developfrom
kbaker827:fix/openrfid-review

Conversation

@kbaker827
Copy link
Copy Markdown

@kbaker827 kbaker827 commented Mar 4, 2026

Summary

Bug fixes for the OpenRFID overlay introduced in #306, addressing issues found during review.

  • Wrong PIDFILE path in S99openrfid — The init script used /var/run/filament-detect.pid, a leftover from the previous service name. This has been corrected to /var/run/openrfid.pid. As written, if any remnant PID file from the old filament-detect service existed, the already-running guard would fire incorrectly and OpenRFID would silently refuse to start.

  • Stray echo in 15_settings_openrfid.yaml — The openrfid-generic activation command ended with echo Please read the relevant documentation.... This was redundant (the confirm dialog already shows this message before the user proceeds) and was explicitly flagged for removal in the review thread.

  • Broken help_url anchor13_settings_rfid_reader.yaml pointed to #alternative-detection-system (singular) but the corresponding docs heading is ## Alternative Detection Systems, which generates the anchor #alternative-detection-systems. The ? button in the firmware-config UI would silently 404 on this anchor.

  • No error handling in openrfid.py_snapmaker_key() would crash with an unhelpful AttributeError: 'NoneType' object has no attribute 'group' if the Klipper RFID driver file was missing or had been updated to rename the HKDF key constant. It now prints a descriptive message to stderr and exits with code 1 in both failure cases.

Test plan

  • With no prior filament-detect service installed: enable OpenRFID and confirm the PID file is created at /var/run/openrfid.pid
  • Confirm S99openrfid stop cleans up /var/run/openrfid.pid
  • Enable OpenRFID (force generic vendor): confirm only the confirm dialog message appears; no extra text in the action output
  • Click the ? help icon on the Filament Detection setting: confirm it navigates to the correct docs anchor
  • Remove fm175xx_reader.py temporarily and run openrfid.py: confirm a clear error message is printed instead of an AttributeError

🤖 Generated with Claude Code

paxx12 and others added 3 commits March 4, 2026 10:04
Introduce a Sims filament-detect overlay that installs a startup service and detector script.

Add firmware-config settings wiring plus base, generic, and vendor profile configs for broader spool support.

Update RFID/settings docs and index references to document the new behavior and configuration.
- Fix wrong PIDFILE name in S99openrfid: was /var/run/filament-detect.pid
  (a leftover from the old filament-detect service name), now correctly
  /var/run/openrfid.pid.

- Remove stray echo at the end of the openrfid-generic activation cmd;
  the confirm dialog already shows the same message before the user
  commits to the change.

- Fix broken help_url anchor in 13_settings_rfid_reader.yaml: the URL
  pointed to #alternative-detection-system (singular) but the docs
  heading is "## Alternative Detection Systems" which generates the
  anchor #alternative-detection-systems.

- Add proper error handling to openrfid.py _snapmaker_key(): instead of
  crashing with an AttributeError if the HKDF key regex doesn't match
  (e.g. after a firmware update renames the constant), the script now
  prints a descriptive message to stderr and exits with code 1.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 4, 2026

✅ Build Artifacts

Version: 1.1.1-paxx12-test-pr-321
Build: 87283c2 (merge of 91b696c into develop)
Duration: 5m 17s

Artifact Size
extended-devel-build 240.69 MB
basic-devel-build 205.72 MB
extended-build 240.28 MB
basic-build 205.31 MB

View workflow run

@kbaker827
Copy link
Copy Markdown
Author

Test plan results

Going through each item — static analysis for the items that don't need printer hardware, live Python execution for #5.


✅ Test 1 & 2 — PIDFILE path and stop cleanup

All references to PIDFILE in S99openrfid now point to /var/run/openrfid.pid. No remaining references to filament-detect.pid:

line 3:  PIDFILE="/var/run/openrfid.pid"
line 24: if [ -f "$PIDFILE" ] && kill -0 "$(cat "$PIDFILE")" 2>/dev/null;  # already-running guard
line 36: start-stop-daemon -S -b -m -p "$PIDFILE" ...                       # start writes PID here
line 47: if [ -f "$PIDFILE" ];                                               # stop checks
line 48: start-stop-daemon -K -p "$PIDFILE" -s TERM                         # stop kills
line 49: rm -f "$PIDFILE"                                                    # stop cleans up

The stop() function correctly removes the file after sending SIGTERM. ✅


✅ Test 3 — No stray echo in 15_settings_openrfid.yaml

grep for echo and Please read finds only the two confirm: keys (which are correct and intentional) — nothing inside any cmd: block:

line 8:  confirm: Please read the relevant documentation...   ← openrfid confirm
line 20: confirm: Please read the relevant documentation...   ← openrfid-generic confirm

No echo in any bash cmd. ✅


✅ Test 4 — help_url anchor matches docs heading

Value
help_url in YAML …/rfid_support#alternative-detection-systems
Heading in docs/rfid_support.md (line 123) ## Alternative Detection Systems
Generated anchor #alternative-detection-systems

Exact match. ✅


✅ Test 5 — openrfid.py error handling

Ran all three paths of _snapmaker_key() locally:

5a — driver file missing:

exit code: 1
stderr:    Error: cannot read Klipper RFID driver: [Errno 2] No such file or directory: '/nonexistent/path.py'

PASS ✅ (was previously AttributeError or FileNotFoundError with no context)

5b — file present but HKDF key constant not found:

exit code: 1
stderr:    Error: Snapmaker HKDF key not found. The firmware driver may have changed.

PASS ✅ (was previously AttributeError: 'NoneType' object has no attribute 'group')

5c — key found, correct hex returned:

exit code: 0
stdout:    736e61706d616b65725f7365637265745f6b65795f68657265   ← correct hex encoding

PASS ✅


All 5 test plan items verified. 🟢

@paxx12 paxx12 closed this Mar 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants