Fix PWM10+ discovery and sort order in Common.php - #10
mrjacobarussell wants to merge 2 commits into
Conversation
Two bugs, both hit by any hwmon device with 10+ PWM channels
(e.g. ARCTIC Fan Controller, 10 channels):
1. glob("pwm[0-9]") and find -iname 'pwm[0-9]' match exactly one
digit, so pwm10 (and above) are silently dropped from both
build_pwm_map() and list_pwm(). Fixed by globbing broadly
(pwm*) and filtering with a strict ^pwm\d+$ regex, which also
avoids matching auxiliary attributes like pwm1_enable,
pwm1_auto_point1_pwm, etc.
2. list_pwm()'s usort() used strcmp(), which sorts alphabetically
("pwm10" < "pwm2" as strings) rather than numerically. Fixed
by switching to strnatcmp().
Verified against a live 10-channel ARCTIC Fan Controller on Unraid:
before the fix, channel 10 was missing from the plugin UI entirely
and pwm10 sorted between pwm1 and pwm2; after, all 10 channels
appear in correct numeric order.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesPWM channel enumeration
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
-regextype is incompatible with some supported BusyBox find environments and may hide all PWM channels; regression coverage is also missing.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes PWM discovery for 10+ channels and ensures numeric channel ordering.
Changes:
- Discovers multi-digit PWM files while excluding auxiliary attributes.
- Uses natural sorting for PWM channels.
| File | Summary |
|---|---|
src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php |
Updates PWM discovery and sorting; portability and regression-test improvements are needed. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
andrebrait
left a comment
andrebrait
left a comment
There was a problem hiding this comment.
The change overall is good but I think it could've been simpler.
And I need to improve the testing here
…ute match
- Replaced the find(1) + GNU-only -regextype chip discovery in list_pwm()
with the same glob('/sys/class/hwmon/hwmon*') approach build_pwm_map()
already uses. No more shell exec() for this, and no portability concern
about -regextype being a GNU find extension.
- Kept the strict ^pwm\d+$ regex filter rather than switching to a bare
"pwm[0-9]*" glob as suggested in review. Verified directly: a glob char
class only consumes one character, so "pwm[0-9]*" still matches
"pwm1_enable", "pwm10_enable", "pwm1_auto_point1_pwm", etc. -- the same
auxiliary-attribute flooding the regex filter was added to prevent.
fnmatch() confirms this:
pwm1_enable vs pwm[0-9]*: MATCHES
pwm10_enable vs pwm[0-9]*: MATCHES
pwm1_auto_point1_pwm vs pwm[0-9]*: MATCHES
Happy to go with a different structure if preferred, but "pwm[0-9]*"
specifically doesn't fix the bug this PR is for.
|
Pushed an update addressing both review threads: On $patterns = ["pwm1", "pwm1_enable", "pwm1_auto_point1_pwm", "pwm10", "pwm10_enable"];
foreach ($patterns as $p) {
var_dump(fnmatch("pwm[0-9]*", $p));
}
// pwm1_enable -> true (matches)
// pwm10_enable -> true (matches)
// pwm1_auto_point1_pwm -> true (matches)A glob character class only ever consumes one character, so On the Let me know if you'd rather structure it differently -- happy to adjust, just wanted to flag that |
What's changed
Two bugs, both hit by any hwmon device with 10+ PWM channels (found via a real ARCTIC Fan Controller, which has 10 channels):
glob("pwm[0-9]")andfind -iname 'pwm[0-9]'only match a single digit, sopwm10and above are silently dropped from bothbuild_pwm_map()andlist_pwm(). Fixed by globbing broadly (pwm*) and filtering with a strict^pwm\d+$regex — this also avoids accidentally matching auxiliary attributes likepwm1_enableorpwm1_auto_point1_pwmthat a looserpwm[0-9]*pattern would pull in.list_pwm()'susort()usedstrcmp(), which sorts alphabetically ("pwm10" < "pwm2"as strings) instead of numerically. Switched tostrnatcmp().Verification
Tested against a live 10-channel ARCTIC Fan Controller on Unraid. Before the fix, channel 10 was missing from the plugin UI entirely, and once naively patched to just match
pwm10, it sorted betweenpwm1andpwm2(and a looser glob pattern pulled in every per-channel attribute, not just the base PWM control file). After this fix, all 10 channels appear, in correct numeric order, with nothing extraneous.Upgrade compatibility
No config/migration impact — this only affects which hwmon files get discovered and how they're ordered for display.
Summary by CodeRabbit