fix: honor FanResponseDelay in PWM readback verification; restore fan state after failed or interrupted CLI init - #512
Conversation
| } | ||
|
|
||
| // RestoreControlMode restores the fan state that was captured when the controller started. | ||
| func (f *DefaultFanController) RestoreControlMode() { |
There was a problem hiding this comment.
if we simply expose this method like that we also need to make sure the caller stops the fan controller. Restoring the ControlMode and keeping the controller active will lead to a lot of error message.
TBH I am not sure we should expose this at all. The FanController should handle all of this correctly by itself. If the problem is caused by the RunInitialization method being used not only internally but also directly by the CLI command, maybe we should separate the internal "runInitialization" method from a public one, and do the error handling and fan restoration within the public one.
The CLI command code shouldn't need to know about restoring the fan control mode which is an internal property of the fan controller.
There was a problem hiding this comment.
Agreed, the exported method is gone. runInitialization() is now the private worker, and the public RunInitialization(ctx) captures the fan state first and owns the restore on any failure. To keep the CLI out of fan state handling entirely, the initialization sequence is now context-aware: fan init just wraps its context with signal.NotifyContext, and cancellation aborts the sweep's waits and goes through the same restore path. This also fixes a small inconsistency in the old code, where the AttachFanRpmCurveData error path returned without restoring.
Verified on the affected hardware: Ctrl-C and SIGTERM during a sweep now hand the channels back to EC control on their own, and a full init still completes with the same boundaries as before.
| // Note: this is the raw value read from the fan, no pwmMap is applied to it | ||
| originalPwmValue int | ||
| // whether storeCurrentFanState() has captured the two fields above | ||
| originalStateCaptured bool |
There was a problem hiding this comment.
Since we need this to distinguish presence and absence of data, we should probably wrap the "originalPwmValue" and "originalControlMode" properties into a SavedFanState/OriginalFanState/FanStateSnapshot struct, which can either be null or have a value. That way we don't need an additional boolean variable that can (in theory) get out of sync with the actual data.
There was a problem hiding this comment.
Done — replaced the two fields with a nullable FanStateSnapshot and dropped the boolean. restoreControlMode() now skips the restore when the snapshot was never captured.
| for attempt := 0; attempt <= pwmMismatchRetries; attempt++ { | ||
| if attempt > 0 { | ||
| // The reported PWM may lag behind the requested value on some hardware, so wait FanResponseDelay before retrying. | ||
| time.Sleep(time.Duration(configuration.CurrentConfig.FanResponseDelay) * time.Second) |
There was a problem hiding this comment.
👍 probably got lost in the rewrite
|
Please rebase your branch to fix the "codespell" CI error. |
…on retries On hardware where the reported PWM value converges towards the requested one gradually (e.g. the NCT6687 EC ramps DC-controlled fans, and the nct6687d driver serves cached sysfs reads), the readback verification in measureAtPwm() could never succeed: it re-checked after only PwmSetDelay (5ms by default), so every sample was skipped as a mismatch (-1) and boundary discovery failed in under a second with "unable to detect start boundary, no spinning PWM found", while the fans were audibly spinning up towards the probed values. Wait FanResponseDelay before each mismatch retry so the hardware gets the same response time the rest of the measurement already grants it.
4830bec to
87733dd
Compare
…ed via CLI The `fan init` CLI command calls RunInitialization() directly, without going through Run(), so storeCurrentFanState() was never called. When the initialization failed, restoreControlMode() then applied the zero values of the original-state fields: ControlMode 0 (disabled) was written to pwm_enable, which some drivers reject (e.g. nct6687d returns EINVAL), and the SetControlMode error path then set the fan to PWM 255 in manual mode, leaving it at full speed with no controller attached. Wrap the original state into a nullable FanStateSnapshot so its absence is representable, capture it at the start of RunInitialization(), and skip the restore entirely when no state was ever captured, since writing the zero value of ControlMode would disable fan control.
The measurement leaves the fan in manual mode at whatever PWM value was probed last. Interrupting the CLI initialization with SIGINT/SIGTERM killed the process without any cleanup, leaving the fan stuck there. Split RunInitialization() into a public wrapper that captures the fan state and owns the restore on any failure, and a private worker holding the sequence itself. The initialization sequence is context-aware, so the CLI cancels the context via signal.NotifyContext on SIGINT/SIGTERM and the controller restores the fan state on its own, without the CLI knowing anything about fan state internals.
87733dd to
00e9a4a
Compare
|
Rebased on master — codespell is green now. I also reworked the branch to address both review comments, see the inline replies. |
|
Thx I will check it out later ❤️ |
| ControlMode fans.ControlMode | ||
| // the raw pwm value read from the fan before the controller started | ||
| // Note: this is the raw value, no pwmMap is applied to it | ||
| PwmValue int |
|
|
||
| func (f *DefaultFanController) storeCurrentFanState() error { | ||
| if f.originalFanState != nil { | ||
| return nil |
There was a problem hiding this comment.
Hmm, is this a good idea? The method name doesn't suggest that a once set snapshot will never be reassigned. It might be confusing for a dev to only find out about that later. If we need a way to store the fan state snapshot only once regardless of how often we call the method, I would suggest to create separate methods with that explicit purpose, something like "storeInitialFanState()", with an accompanying "clearInitialFanState()" (if that is something we need), which use the storeCurrentFanState under the hood.
There was a problem hiding this comment.
Updated! I have separated storeCurrentFanState() (which now unconditionally captures and overwrites the snapshot) from storeInitialFanState() (which captures only if not set yet) and added clearInitialFanState(), along with unit tests for each.
…anState Update storeCurrentFanState to unconditionally capture and overwrite the current fan state snapshot. Introduce storeInitialFanState to preserve the initial snapshot on subsequent calls, and clearInitialFanState to reset it. Add unit tests for storeInitialFanState, storeCurrentFanState, and clearInitialFanState.
|
Pushed a new commit to address the review comment: separated storeCurrentFanState() and storeInitialFanState(), added clearInitialFanState(), and added unit tests for all three. |
|
Looks good, will merge. Thx! |
2 participants
Fixes #511.
Since 0.14.0 the two-phase adaptive sweep verifies the PWM readback before sampling RPM, but re-checks after only
PwmSetDelay(5 ms by default). On hardware where the reported PWM converges slowly towards the requested value (the NCT6687 EC ramps DC outputs gradually; the nct6687d driver serves cached sysfs reads) the check can never pass, so every sample is skipped as a mismatch and boundary discovery fails in under a second with "unable to detect start boundary, no spinning PWM found" while the fans are audibly spinning. Full logs and analysis in the issue.Three small fixes, one commit each:
measureAtPwm()now waitsFanResponseDelaybefore each mismatch retry.RunInitialization()captures the original fan state, so a failed init started from the CLI (which does not go throughRun()) restores the previous control mode instead of applying the zero value ofControlMode. That zero value (disabled) was rejected by the driver, and theSetControlModeerror fallback then left the fan in manual mode at PWM 255.restoreControlMode()also skips the restore when no state was ever captured.fan inithandles SIGINT/SIGTERM and restores the fan state when interrupted.Verified on the affected hardware (MSI MPG B650 Carbon, NCT6687, three DC-controlled case fans):
fan initcompletes in ~4 min per fan with real settle pauses and a full RPM curve, manualminPwm/startPwm/maxPwmare honored, and both Ctrl-C during init and daemon shutdown hand the channels back to the EC (pwm_enablereturns to 2). Regression tests included for all three fixes.