PowerDisplay: Always write the saved value when restoring monitor settings - #49577
Merged
Conversation
moooyo
force-pushed
the
yuleng/powerdisplay/restore-gate/1
branch
from
July 30, 2026 05:56
59bea3b to
9bed3fa
Compare
moooyo
changed the title
moooyo
marked this pull request as ready for review
July 30, 2026 05:58
leileizhang (lei9444)
approved these changes
Jul 30, 2026
Shawn Yuan (shuaiyuanxx)
approved these changes
Jul 30, 2026
moooyo
pushed a commit
that referenced
this pull request
Jul 30, 2026
…ts own PR MonitorRestorePlanner, its tests and MonitorViewModel.ShouldRestoreValue are removed, and TryRestore is aligned with the shape #49577 lands on main: an unconditional write, no comparison at all. #49577 fixes the same bug — a saved value silently dropped because it happened to match a discovery placeholder the hardware never answered for — by deleting the skip-if-equal check outright rather than by teaching it which values are observations. Keeping this branch's version would re-introduce the more elaborate one on top of it. Only the known-good retention block remains in MainViewModel.Settings.cs.
7 tasks
moooyo
added a commit
that referenced
this pull request
Aug 3, 2026
…ails (#49445) ## Summary of the Pull Request On a monitor whose DDC/CI engine answers intermittently, every discovery pass starts from nothing. A panel that reported its brightness a minute ago can lose that control — or drop out of the flyout entirely — because one pass happened to fail. This persists every range-valid VCP value read off a monitor, keyed by its canonical DevicePath. In Maximum compatibility mode a later discovery falls back to that value when the hardware will not answer. Scope is intermittent failure, not permanent failure: the cache can only replay a value the hardware answered at least once, so a panel that never reads a code successfully sees no change. This partially addresses #49342. ## PR Checklist - [x] Closes: #49342 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [x] **Tests:** Added/updated and all pass - [x] **Localization:** All end-user-facing strings can be localized — this PR adds none - [ ] **Dev docs:** Added/updated - [x] **New binaries:** Added on the required places — none added, so no signing JSON, installer WXS or CI YML change is required - [ ] **Documentation updated** ## Detailed Description of the Pull Request / Additional comments ### What is stored `MonitorStateManager` implements `IKnownGoodVcpStore`, so the cache rides in the existing `monitor_state.json` next to the user's saved brightness rather than in a new file. Each entry is a `KnownGoodVcpFeature`: code, current, maximum, and when it was last read. Only range-valid observations are stored, so the common `current=0 / max=0` garbage reply never enters — it fails `VcpFeatureValue.IsValid`. Writes are not gated on Maximum compatibility mode, only reads are. A monitor that reads cleanly today can start failing after a cable or dock change, and a lazily populated cache would be empty on exactly the first pass that needs it. ### How a cached value is used `VcpDiscoveryEvidence.Reconcile` gains the cache as a third source alongside the parsed capabilities string and this pass's probe: | this pass | cache | result | | --- | --- | --- | | read succeeded | — | live value wins, cache refreshed | | replied, range unusable | hit | cached value applied, `MonitorReadFlags` left clear | | no reply | hit | cached value applied, `MonitorReadFlags` left clear | | code never probed (caps parsed) | hit | value applied only after one live read is attempted | The last row matters: on the caps-parsed path nothing has confirmed the cached value this pass, so the hardware is asked first. On the probe path it has already been asked, and re-reading would be pure I2C noise. `MonitorReadFlags` stays clear for anything the hardware did not answer, so a cached value never masquerades as an observation — which #49577 depends on, since it made the restore path write whenever the flag is unset. One consequence is worth naming: the flyout draws a slider at the cached position while `powerdisplay get` reports that setting as unknown, because `MonitorDtoProjector` gates on `supported && read`. ### Keeping the cache current `RefreshKnownGoodAfterWrite` restamps an entry after a successful `SetVCPFeature`, so a slider move cannot leave the cache holding the pre-write value. It refreshes only an entry a real read established, and only when the value was scaled against the maximum that entry holds — a monitor whose discovery read failed still carries a placeholder max, and writing that back would mis-scale every later write. `RemoveKnownGoodFeatures` clears the cache for monitors a settings reconciliation observably dropped, leaving the user's saved values alone. Cleanup is driven by an observed drop, never by absence from the rebuilt list: a missing or corrupt `settings.json` yields a defaults object indistinguishable from a real one, and pruning by absence would wipe every monitor not connected at that instant. A re-observation that changes nothing refreshes the in-memory timestamp but does not mark the file dirty, so a discovery pass no longer rewrites `monitor_state.json` for a moved timestamp alone. ## Validation Steps Performed - `PowerDisplay.Lib`, `PowerDisplay.Lib.UnitTests` and `PowerDisplay` built for x64 Debug with VS MSBuild — 0 errors, 0 warnings; `PowerDisplay.Lib.UnitTests.dll` under `vstest.console.exe`: **301 passed, 0 failed** - **Affected-hardware validation on the AOC Q27G3XMN is still pending.** That monitor, or an equivalent controllable DDC/CI setup, was not available locally. The paths this PR changes are reachable only on hardware whose capabilities string is unusable or whose VCP reads fail intermittently, so this is the main outstanding risk. --------- Co-authored-by: Yu Leng <yuleng@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
TryRestoreskipped writing a saved monitor value when it already equalled the valueMonitorViewModelwas showing. That displayed value is only an observation when the discovery-time VCP read succeeded. When the read failed it is a placeholder:50MonitorDiscoveryHelperstamps it — "Initial placeholder; overwritten if the VCP read succeeds"50Monitorbacking-field default50Monitorbacking-field default0x05(6500K)Monitorbacking-field defaultA saved value that happened to equal one of those silently suppressed the restore, and the monitor kept whatever it powered on with.
50is the mid-slider value and0x05is the most common preset, so the coincidence is not rare.This drops the comparison: a restore now always writes.
PR Checklist
TryRestoreis a private helper in thePowerDisplayapp project, which has no test projectDetailed Description of the Pull Request / Additional comments
Why remove the check rather than refine it
The skip-if-equal check dates from PowerDisplay's first commit (#42642, where it read
// Restore brightness if different from current); #47051 only refactored it into the sharedTryRestorehelper. It is day-one "obviously we shouldn't write twice" code, not a response to a reported problem.Removing it is correct by construction: with no skip branch there is no state in which a restore silently does nothing. Any narrower fix has to decide when the displayed value can be trusted, and gets that decision wrong in exactly the cases that are hardest to reproduce.
Cost
Two, both bounded:
RestoreSettingsOnStartupis enabled, and a profile apply happens because the user invoked that profile.Task.WhenAll.The
isVisibleguard is untouched, so a monitor still never receives a write for a feature it does not expose — an unsupported VCP0x14is not written just because a profile carries a color temperature. Input source and power state are not restored here at all.If the redundant write turns out to matter
The narrower fix is to keep the comparison and add one clause: also write when
(monitor.ReadValues & flag) != flag, i.e. when the compared value was never read off the hardware.MonitorReadFlagsalready carries exactly that information, andMonitor.ReadValuesis already maintained by the discovery-timeInitialize*methods, so it is a small change on top of this one. I went with the simpler version first — happy to switch if a maintainer would rather keep the optimisation.Validation Steps Performed
PowerDisplayandPowerDisplay.Lib.UnitTestsfor x64 Debug with VS MSBuild — 0 errors, 0 warningsPowerDisplay.Lib.UnitTests.dllwithvstest.console.exe: 186 passed, 0 failed — unchanged frommain; this PR touches only the app project and adds no tests