PowerDisplay: Improve maximum compatibility mode reliability - #49445
Draft
moooyo wants to merge 29 commits into
Draft
PowerDisplay: Improve maximum compatibility mode reliability#49445moooyo wants to merge 29 commits into
moooyo wants to merge 29 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
Offload direct probe reads, aggregate attempt diagnostics, and prefer live initialization before same-code cache fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea38c04-6f68-4c42-91d9-8a03b49bdd81
moooyo
marked this pull request as draft
July 22, 2026 08:21
Preserve legacy monitor state during migration, require known live values before skipping restore writes, and reject invalid physical monitor handles across probe and initialization paths.
Delay state pruning until an ID is absent from consecutive settings snapshots, and retry the HRESULT form of invalid continuous VCP ranges.
Review of the maximum-compatibility discovery rework surfaced several defects introduced by this branch. Each is fixed at its cause rather than at the symptom. VcpDiscoveryEvidence.Reconcile used "the parsed caps advertise this code" as a proxy for "a live read was already attempted for this code". Those coincide only when the caps string is unusable, which is the only case where the probe runs. On the caps-parsed path no probe runs, so a cached code the caps string omits was applied to the monitor, advertised to the UI, and never read from hardware — and its cache entry could never be refreshed. PreferLiveRead now derives from whether the probe actually touched the code. A probe reply carrying an unusable range no longer hides the feature: the device answered, which proves support (unimplemented codes fail with DDCCI_VCP_NOT_SUPPORTED), so the code stays reachable as it did before the rework. The reply is recorded on the observation rather than inferred. Cache evidence could overwrite VcpCodeInfo entries parsed from the capabilities string, discarding discrete value lists and custom names. Marking support is now add-if-absent through a single helper, so evidence can only widen capabilities. Known-good observations had no invalidation: a monitor that loses DDC/CI support for a code while keeping its DevicePath would keep advertising it indefinitely. LastSuccessfulUtc was persisted but never read; it now bounds how long an observation may stand in for a live read. Successful reads restamp it, so a feature that still works never expires. Monitor-state pruning deleted entries by absence from the rebuilt settings list. A missing or corrupt settings.json makes GetSettingsOrDefault persist and return an empty monitor list that is indistinguishable from a real one, so every disconnected monitor's saved brightness, contrast, volume and known-good cache was deleted. Pruning is now driven by the entries the rebuild observably dropped, so an unreadable snapshot drops nothing. The discrete VCP stage no longer discards a monitor. Handle liveness is already decided by the probe and the continuous stage; repeating the decision after the continuous values were applied meant one bad 0xD6 read removed a working monitor from the flyout, in normal mode as well as maximum compatibility mode. The restore gate compared only the monitor snapshot, so a profile apply issued while a debounced slider commit was pending was skipped and then overwritten by that commit. It now also requires the optimistic UI value to agree. A throwing native read is contained to its own VCP code instead of unwinding into the pipeline-wide catch that drops every monitor sharing the hMonitor. Also fixes a mis-indented XML doc block in MonitorStateRetentionPlanner. Validation: built PowerDisplay.Lib.UnitTests and PowerDisplay for x64 Debug with VS MSBuild; ran PowerDisplay.Lib.UnitTests.dll with vstest.console.exe — 268 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the cache Second-pass review of the previous fix commit found that the 30-day freshness bound on known-good VCP observations was worse than the problem it addressed. The bound assumed every successful read restamps LastSuccessfulUtc, but on the one path where the cache is load-bearing no read happens at all: once the probe has touched a code, PreferLiveRead is false and ContinuousVcpInitializer applies the cached value and returns without reading or upserting. So the clock only advances on a successful probe -- the very event whose absence makes the cache necessary. After thirty days of transient probe failures the entry expired, the reconciled capabilities went null, and DdcCiController dropped the monitor entirely rather than merely hiding one slider. The failure was absorbing: only a successful probe could rebuild the entry, and that was what had been failing. Restamping when the cached value is applied would fix the regression but make the bound unreachable, since an entry can then only expire while its monitor is undiscovered, which settings retention already handles. The bound is removed. That leaves cached observations without invalidation, which was the original objection. Fixing it properly means letting a definitive DDCCI_VCP_NOT_SUPPORTED reply outrank the cache, and that contradicts a deliberate, test-pinned decision in this branch that positive cache evidence survives such a reply in maximum compatibility mode. It is a design question for review, not a unilateral change, and is called out in the PR description instead. Monitor state collection is also narrowed. Removing a whole state entry deleted the user's saved brightness, contrast, volume, color temperature and capabilities for any monitor that aged out of settings -- values that survived indefinitely before this branch introduced the cache. The known-good cache is discovery state this feature owns, so only that sub-object is collected now; saved user values are never removed. RemoveMonitorStates becomes RemoveKnownGoodFeatures. Validation: built PowerDisplay.Lib.UnitTests and PowerDisplay for x64 Debug with VS MSBuild; ran PowerDisplay.Lib.UnitTests.dll with vstest.console.exe -- 264 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The branch introduces three tokens check-spelling does not know. DDCCI appears as an all-caps run in the DDCCI_VCP_NOT_SUPPORTED comments; main only ever had the PascalCase DdcCi, which tokenizes to ddc + ci, and ddc is already expected. AOCB and XMN come from the AOC Q27G3XMN device paths and friendly name used by the new test fixtures. spelling2.yml does not set only_check_changed_files, so the Check Spelling job fails on all three until they are expected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
IsTransient carried eight bare 0xC02625xx literals plus a bare 1460 with no direct test, so neither its membership nor the reasoning behind the gaps in the sequence was reviewable. Move it next to IsPhysicalMonitorUnavailable in DdcErrorClassifier, give every code its winerror.h name, and record why each neighbouring code stays out: VCP_NOT_SUPPORTED is the device's final answer, I2C_NOT_SUPPORTED and I2C_DEVICE_DOES_NOT_EXIST are permanent bus-level facts, MCA_INVALID_CAPABILITIES_STRING belongs to the capabilities path rather than to a VCP read, the handle-class codes are owned by IsPhysicalMonitorUnavailable and must abort rather than retry, and MONITOR_RETURNED_INVALID_TIMING_STATUS_BYTE is raised only by the get-timing-report command, never by GetVCPFeatureAndVCPFeatureReply. CURRENT_CURRENT_VALUE_GREATER_THAN_MAXIMUM_VALUE stays in on purpose and now says why: a device that genuinely reports current > maximum simply exhausts the budget, but the same code also results from a corrupted reply, which a retry does fix. A data-driven test pins both sets and asserts they stay disjoint, since an overlap would keep retrying against a handle already known to be gone. No behaviour change: the membership is identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l write The cache was only ever written from a successful read, so a slider move left it holding the pre-write value. Maximum compatibility mode then republished that stale value on a later discovery whose probe touched the code but could not read it: PreferLiveRead is false once the probe has touched a code, so ContinuousVcpInitializer applies the cached value without re-reading. The flyout does not gate on ReadValues, so the user saw the stale value presented as live, and a one-notch slider move snapped the panel to it. This needed no restart -- UpdateMonitorList rebuilds every MonitorViewModel on any display-change refresh. SetVcpFeatureAsync is the single funnel for all six codes and already holds the device-native value, monitor.Id, the store and the clock, so the refresh goes there. It only refreshes an entry a real read already established -- a successful SetVCPFeature is not evidence that the device implements the code -- and only when the write was scaled against the very maximum that entry holds. That guard is load-bearing: BrightnessVcpMax and its siblings default to the placeholder 100 and Monitor is rebuilt on every discovery, so a monitor whose read failed would otherwise overwrite a read-proven 0-50 range with the placeholder and mis-scale every later write. The internal constructor also gains the delayAsync seam VcpFeatureProbeService already had, and FetchCapabilitiesWithFallbackAsync becomes internal, so the compatibility-mode gate and the probe-observation persist loop are covered for the first time -- InternalsVisibleTo and the injecting constructor existed but no test used them. IntPtr.Zero is a safe handle to drive that with: TryGetCapabilitiesString short-circuits on it without issuing a native call, so the capabilities string is deterministically unusable and every remaining decision comes from the injected reader, clock and store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ocks MarkDirtyAndScheduleSave had no _disposed guard, unlike SaveStateToDiskAsync and Dispose. Dispose snapshots _isDirty, sets _disposed, disposes the debouncer and flushes, so an upsert arriving after that point mutated the state and set _isDirty again with no path left to clear it. The reachable window is the synchronous BuildMonitorFromPhysical block, which performs no cancellation checks and upserts after each successful blocking VCP read. Late observations are now dropped on purpose: the next discovery pass re-derives them. MigrateLegacyKeys stops duplicating the schedule inline and routes through the same helper. The locks this branch added were exercised by nothing. The one concurrency test upserts two different monitor Ids, which GetOrAdd maps to two distinct MonitorState instances and therefore two distinct lock objects, so the suite passed with every lock statement deleted. The contention they actually guard is one monitor Id: the discovery thread upserting several VCP codes while the debounced save enumerates that same KnownGoodVcpFeatures dictionary. The new test drives exactly that from two threads and fails with InvalidOperationException out of the ToDictionary inside GetKnownGoodFeatures once the locks are removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three statements did not match the code they describe. Reconcile claimed the probe "spends its full paced retry budget on every code it touches". ProbeCodeAsync breaks after a single attempt on a non-transient error or a throwing read, which this branch's own ProbeAsync_NonTransientFailureDoes NotRetry asserts. The guarantee that actually holds -- at least one transaction per code the probe touched -- is what justifies not re-reading it. DiscreteVcpInitializer claimed handle liveness is already settled before it runs. A capabilities string that parses but advertises none of 0x10/0x12/0x62 leaves nothing for the continuous stage to read and suppresses the probe, so the discrete reads are the first on that handle. The stage is still safe, for a different reason: failing here only leaves the read flag unset. MonitorRestorePlanner described an unflagged value as merely cached. It is just as often the never-read backing-field default, and naming those defaults -- 0 brightness, 50 contrast, 50 volume, 0x05 color temperature -- explains why a saved value that happens to match one still has to be written. Two decisions are also recorded where they are made, rather than left for the next reviewer to re-litigate. Why a handle-class error discards the monitor outright: Monitor.Handle is captured once per discovery pass and never refreshed, so a monitor kept here would answer every later read and write against a handle already known to be dead. And why positive cache evidence is deliberately permanent while the monitor stays connected: retracting it on a DDCCI_VCP_NOT_SUPPORTED reply trades a stale control that writes into the void for a display that disappears entirely, and that reply is not dependable negative evidence on the hardware this mode exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…known-good store Physical-monitor handles only reach PhysicalMonitorHandleManager through monitors that were successfully built: the map is rebuilt from the returned monitor list, and its cleanup pass only destroys handles that were already in the previous map. A handle dropped inside the discovery loop therefore never gets destroyed. This branch added a new way to drop one -- a handle-class error during continuous VCP initialization now returns PhysicalMonitorUnavailable and discards the monitor, where main logged it, left the read flag unset and kept the monitor so its handle entered the map. A panel that intermittently answers that way leaks one kernel handle per discovery, and a discovery runs on every display-topology change. ReleaseAbandonedPhysical now covers all three abandon paths, including the two that already leaked on main. FetchCapabilitiesWithFallbackAsync also snapshots MaxCompatibilityMode once. The property is settable from the UI thread on every settings reload and the method awaits several times; main read it at a single decision point, this branch grew that to three, so a toggle arriving mid-pass could have one monitor's evidence gathered under one mode and reconciled under the other. NullKnownGoodVcpStore and the two `?? NullKnownGoodVcpStore.Instance` fallbacks are removed. Nothing ever reached them -- MainViewModel always supplies the real MonitorStateManager and every test uses the injecting constructor -- so they were dead code whose only effect would have been to silently disable the discovery cache if a caller ever forgot the argument. The parameter is now required, which turns that into a compile error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ial-value source LoadStateFromDisk guards stateFile.Monitors but not the KnownGoodVcpFeatures list this branch added, even though System.Text.Json writes an explicit JSON null straight over the member initializer and the array can carry null elements. The resulting NullReferenceException escapes to the method-level catch, so every monitor entry after the offending one is dropped -- and the next whole-file rewrite in BuildStateJson makes that loss permanent. The property is now typed `List<KnownGoodVcpFeature>?`, matching how the same file already declares CapabilitiesRaw and reflecting what deserialization can actually produce, and the loop null-checks both the collection and its elements. VcpInitialValue.Source is deleted. It was populated at both construction sites in Reconcile and read by nothing: the record's only consumers -- InitializeFeature and the probe-outcome log line -- read Value, IsLive and PreferLiveRead only. KnownGoodVcpFeature.Source is kept; it is read when a successful write refreshes a cache entry, and it is a useful field diagnostic in monitor_state.json. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three tests added in the previous review round overclaimed. RefreshKnownGoodAfterWrite_EmptyMonitorIdIsIgnored did not pin the guard it is named for: the fake store did its own monitor-Id comparison, so deleting `string.IsNullOrEmpty(monitor.Id)` from the production path left the test green. The fake now answers any Id and counts lookups, and the test asserts the store was never queried -- removing the guard now fails it. HandleClassFailuresAreNeverTransient asserted, verbatim, two rows that IsTransient_RejectsEverythingElse already covers, and its comment claimed to test set disjointness while hardcoding the two handle codes rather than deriving them, so an overlap introduced by widening IsPhysicalMonitorUnavailable would have sailed past it. Deleted; the rationale moves into the DataRow comment that owns those two inputs. The comment on ConcurrentUpsertAndRead_OnSameMonitorDoNotTearTheFeatureMap spent most of its lines critiquing a sibling test instead of describing its own, and would have gone stale the moment that sibling changed. Rewritten to state what this test drives and why two monitor Ids cannot reproduce it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ce a control rests on ReleaseAbandonedPhysical missed one exit. When GetPhysicalMonitorsFromHMONITOR returns more physicals than QueryDisplayConfig has entries for, the loop breaks without releasing physicals[i..] -- the one abandon path in that loop still leaking the handles the helper's own remark describes. The break now releases the remainder before leaving. Two observability gaps in exactly the mechanism most likely to produce a confusing bug report -- a control the cache created rather than the hardware: The probe outcome line reports attempts, status and lastError but not Replied, even though Replied is the single input that flips a feature to Supported when the device answers with a range that cannot scale a percentage. In a shipped build a replied-but-unusable outcome was indistinguishable from three plain failures. It is now in the message. Nothing at all was logged when cached evidence widened a monitor's capabilities past what the capabilities string advertised. The existing probe-outcome block is gated on `live.Count > 0`, and live is only populated when the caps string is unusable -- so on the caps-parsed path, which is where the cache silently adds a code, there was no line to find. Reconcile now reports which codes it supported on cache evidence alone, and discovery logs them with the observation's age, which also gives the persisted LastSuccessfulUtc its first reader. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch introduced IVcpFeatureReader as the seam for native VCP reads, but left DdcCiController.TryGetVcpFeature in place. It deleted six of that helper's seven call sites, so the controller ended up carrying two parallel paths to the same syscall: TryGetVcpFeature and DdcCiNative.ReadVcpFeature both wrap GetVCPFeatureAndVCPFeatureReply and both report Marshal.GetLastWin32Error(). Route the one remaining caller, GetVcpFeatureAsync, through the injected reader and delete the helper. The error value and the log line are unchanged. Alongside removing the duplication this puts GetBrightnessAsync, GetContrastAsync, GetVolumeAsync, GetColorTemperatureAsync, GetInputSourceAsync and GetPowerStateAsync behind the same injectable seam the initializers already use; they had no test coverage before because there was no way to reach them without hardware. VcpFeatureProbeService.TransactionInterval also drops from internal to private. It has no reader outside its own class, in production or in the tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hree claims The commentary this branch added had grown into design essays sitting inside method bodies, and three statements did not survive being checked against the code. Corrections: VcpDiscoveryEvidence said a slipped-through false positive lasts "until its settings entry ages out", which reads as a 30-day bound. RemoveKnownGoodFeatures is driven by MonitorStateRetentionPlanner.BuildDroppedIds, i.e. the Rebuild input minus its output, and MonitorSettingsRebuilder re-emits every currently-discovered monitor with a fresh LastSeenUtc and keeps IsHidden entries unconditionally. Reclamation therefore needs 30 days both undiscovered and unhidden, which a monitor in daily use never reaches. The comment now says so. MonitorRestorePlanner enumerated the never-read values as "0 brightness, 50 contrast, 50 volume, 0x05 color temperature". Those are the Monitor backing-field defaults, but on the DDC path the value it actually sees is MonitorDiscoveryHelper's placeholder of 50 brightness -- the mid-slider value users routinely save -- so the stated cost of a redundant write pointed at the wrong number. MonitorStateManager.MarkDirtyAndScheduleSave claimed Dispose has already flushed by the time a late caller arrives, and that discovery is the only such caller. Dispose sets _disposed before it flushes, and UpdateMonitorParameter reaches the same path; the remark now states what actually holds and why it is still harmless. Trimming: the 24-line block in Reconcile drops to 17 and keeps the traps rather than the argument -- the full asymmetry is argued in the PR description, not at the call site. DiscreteVcpInitializer's remark loses the restatement of when its stage runs but gains what keeping the monitor costs. MonitorViewModel.ShouldRestoreValue and the retention call site in MainViewModel.Settings stop repeating the callee's own remarks verbatim and link to them instead. Also drops the SA1402 suppression from VcpDiscoveryEvidence.cs. StyleCop.json does not override maintainabilityRules.topLevelTypes, so SA1402 uses its default of ["class"], and the file declares one class -- VcpInitialValue is a record struct. Verified by building without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…or values Four tests added by this branch hand their subject an input it cannot distinguish from a test that already exists, and each has a twin asserting a strict superset. ContinuousVcpInitializer never dereferences evidence.Capabilities or CacheSupplementedCodes -- InitializeFeature reads only InitialValues, and feature gating comes from the Monitor -- so Initialize_OmittedCodeFallsBackToCacheWhenLiveReadFails drives the same ApplyCachedFallback branch as its Advertised twin, which additionally asserts BrightnessVcpMax. Its VcpNotSupported reader script remains covered by Initialize_VcpNotSupportedUsesCacheAndContinuesRemainingReads. The sibling Initialize_OmittedCodeCachedValueUsesFreshLiveValueAndPersists is deliberately kept: it is the only test that feeds caps-omitted, cache-supplemented evidence through the initializer and asserts the Monitor is populated, which is the regression e2852c0 fixed. ProbeCodeAsync consults the error code exactly once, through IsTransient, so ProbeAsync_CurrentValueGreaterThanMaximumThenSuccessRetriesWithPacing walks the same path as the InvalidCommand test; its extra Disposition assertion moves to the survivor. 0xC02625D8's membership in the transient set stays pinned by DdcErrorClassifierTests. Reconcile_MaximumCompatibilityMarksAdvertisedCachedCodeAsLiveFallback differs from the union test only in whether the caps string advertises the cached code, and the sole observable effect of that -- CacheSupplementedCodes -- is asserted by Reconcile_ReportsOnlyTheCodesTheCacheAloneProved, which now also pins PreferLiveRead for a caps-advertised cached code. Reconcile_NormalModeReportsNoCacheSupplementedCodes built input byte-identical to Reconcile_NormalModeIgnoresCache; its assertion moves there. Adding DdcErrorClassifierTests.Constants_MatchWinerrorValues. Every other assertion in the suite addresses these codes by name, so the numeric values were pinned only incidentally, by the raw-hex constants the other test files declare as inputs. A typo in a constant would have moved production and tests together and left the suite green. With the values now pinned in one place, VcpFeatureProbeServiceTests' local CurrentValueGreaterThanMaximum constant goes with the test that used it. GetKnownGoodFeatures_UsesExactDevicePathComparer uses the file's existing Feature() helper; its inline initializer differed only in a Source it never asserts on. The round-trip test keeps its explicit initializer, since the fields it spells out are the ones it verifies. Validation: built PowerDisplay.Lib.UnitTests and PowerDisplay for x64 Debug with VS MSBuild -- 0 errors, 0 warnings; ran PowerDisplay.Lib.UnitTests.dll with vstest.console.exe -- 296 passed, 0 failed (299 - 4 + 1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e redundant handle Three cleanups to code this branch introduced, all behaviour-preserving. The continuous VCP set had been triplicated. Base had exactly one list -- DdcCiNative.ProbeableContinuousVcpCodes -- which this branch deleted and replaced with three byte-identical private copies in VcpDiscoveryEvidence, ContinuousVcpInitializer and VcpFeatureProbeService. All three iterate it in declaration order, so a code added to one and not the others is probed but never applied, or applied but never proven. The set moves to NativeConstants.ContinuousVcpCodes, next to the three codes it is built from; hosting it on any of the three consumers would have made the other two depend on a peer for a constant. Both initializers took an IntPtr handle that is provably Monitor.Handle: MonitorDiscoveryHelper.cs:133 is the only write to that property in the module and it stores physicalMonitor.HPhysicalMonitor, which is exactly what BuildMonitorFromPhysical passed alongside the monitor. Beyond the duplication this was a coherence problem -- ContinuousVcpInitializer justifies discarding a monitor by reasoning about Monitor.Handle while actually reading from the parameter, and nothing stopped a caller from passing one that disagreed. Both Initialize signatures lose the parameter and read Monitor.Handle directly. The three near-identical KnownGoodVcpFeature initializers fold into KnownGoodVcpFeature.From, the inverse of the existing ToVcpFeatureValue. RefreshKnownGoodAfterWrite now names the VcpFeatureValue it was already constructing for validation and reuses it. DiscreteVcpInitializerTests' two tests also collapse to one: Initialize never inspects read.ErrorCode -- it is used once, interpolated into a log string, and the file never references DdcErrorClassifier -- so VcpNotSupported and the two handle-class codes are indistinguishable inputs. The DataTestMethod survives because its handle-class rows are what would fail if someone copied ContinuousVcpInitializer's drop-the-monitor branch into this stage; the deleted test's two value assertions move into it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ate the error rationale Reconcile_IndeterminateLiveUsesCachedPositiveEvidence and Reconcile_VcpNotSupportedStillUsesCachedPositiveEvidence differ only in the error code handed to VcpProbeObservation.Indeterminate. Reconcile never reads LastError -- it consults Disposition, IsSuccess and Replied -- and Disposition routes both 0xC0262589 and 0xC0262584 to Indeterminate, since neither is handle-class. Same cached entry, same null parsed capabilities, same includeCache, so the two calls are provably identical inputs. The survivor is the stronger mutation-killer, because a regression where DDCCI_VCP_NOT_SUPPORTED retracts cached evidence fails it and not the other; it inherits the deleted test's IsLive assertion and its retry-budget comment. ProbeAsync_UnansweredCodeIsNotMarkedAsReplied repeated ProbeAsync_NonTransientFailure DoesNotRetry's arrange and act verbatim; its one assertion moves into that test. The <remarks> on DdcErrorClassifier.IsTransient and the DataRow comments on IsTransient_RejectsEverythingElse were written by the same commit and say the same five things. The production remarks win -- that is where someone editing the predicate looks -- and the test keeps only the reason its handle-class rows are load-bearing here, which is specific to the test rather than to the classifier. Validation: built PowerDisplay.Lib.UnitTests and PowerDisplay for x64 Debug with VS MSBuild -- 0 errors, 0 warnings; ran PowerDisplay.Lib.UnitTests.dll with vstest.console.exe -- 293 passed, 0 failed (296 - 2 methods - 1 DataRow case). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eter Third cleanup pass. Where the first two hunted duplicated text, this one asks whether each shape earns its keep. Reconcile took both a `cached` dictionary and an `includeCache` flag, but the sole production caller derives both from the same local: DdcCiController passes `maxCompatibility ? GetKnownGoodFeatures(monitorId) : new Dictionary<...>()` and then restates the decision as `includeCache: maxCompatibility`. Since `!includeCache` always implies `cached` is empty, the `includeCache &&` conjunct could never change the result of the `cached.TryGetValue` beside it -- one decision expressed twice, in two places a maintainer had to keep in agreement by hand. The parameter goes; the gate stays where it has to live anyway, because in normal mode the controller must not even call GetKnownGoodFeatures. That is what DdcCiControllerCompatibilityGateTests.NormalMode_ NeitherProbesNorTouchesTheCache pins, with `store.GetCallCount == 0` -- strictly stronger than the unit test it replaces, which only checked that Reconcile ignored a cache it had already been handed. `using System;` was orphaned in both initializers when round 2 removed their IntPtr parameter, and `using System.Threading.Tasks;` in MonitorStateManagerTests by the test deleted below. Four more tests hand their subject input it cannot distinguish: - Reconcile_NormalModeIgnoresCache existed only to exercise the removed parameter. - Reconcile_NoLiveOrCacheLeavesFeatureUnavailable: Reconcile has no branch keyed on emptiness, so it kills no mutant the other tests miss. - Initialize_AdvertisedCachedCodeUsesFreshLiveValueAndPersists differs from its Omitted twin only in CachedEvidence's flag, which changes evidence.Capabilities -- ContinuousVcpInitializer reads only evidence.InitialValues, and Reconcile emits the identical entry for both. The Omitted twin is kept, being the discriminating input; its one unique assertion moves across. - ConcurrentUpserts_PreserveBothMonitorEntries raced two different monitor Ids, which the PR's own sibling test explains cannot contend: GetOrAdd maps them to two MonitorState instances and therefore two disjoint locks. It exercised ConcurrentDictionary, not this branch. Its non-concurrent content is already asserted by RemoveKnownGoodFeatures_ ClearsCacheButKeepsSavedUserValues, and the real contention path by ConcurrentUpsertAndRead_OnSameMonitorDoNotTearTheFeatureMap. - BuildDroppedIds_KeepsEntriesThatSurvivedTheRebuild: BuildDroppedIds has a single code path and two other tests already assert an empty result. Finally the nine test-local DDC error constants and one inline literal now reference DdcErrorClassifier by name. Round 1 blocked this because those locals were the only value-level check on the constants; Constants_MatchWinerrorValues, added in the same round, is now that oracle, so the duplication can go. Codes DdcErrorClassifier does not declare stay as raw hex. Validation: built PowerDisplay.Lib.UnitTests and PowerDisplay for x64 Debug with VS MSBuild -- 0 errors, 0 warnings; ran PowerDisplay.Lib.UnitTests.dll with vstest.console.exe -- 288 passed, 0 failed (293 - 5). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e-planner tests MonitorStateManagerTests ControllerDerivedCacheKey_MatchesMonitorIdAndSurvivesRetentionRoundTrip arranged a temp directory, two MonitorStateManager instances and a JsonDocument parse for one property. The only mutation it uniquely killed is DeriveMonitorId returning info.DevicePath instead of MonitorIdentity.FromDevicePath(info.DevicePath), which its first two asserts catch on their own. Everything after them either asserts the absence of a key no writer produces -- MonitorIdComparer is OrdinalIgnoreCase, so a raw path with its trailing GUID can never collide with the canonical Id -- or models a RemoveKnownGoodFeatures call production never makes, since the retention path passes Monitor.Id, already canonical. It becomes DeriveMonitorId_ReturnsTheCanonicalIdTheStateFileIsKeyedBy, a pure 14-line test. The save/reload properties it also touched stay pinned by KnownGoodFeatures_RoundTripPreservesObservation and the removal tests. RemoveKnownGoodFeatures_EmptySetKeepsCompleteState is deleted. RemoveKnownGoodFeatures has no branch on the collection being empty -- it is a bare foreach whose body touches only the state it looked up -- so MonitorA's fate there is produced by the same code path as MonitorA's fate in RemoveKnownGoodFeatures_ClearsCacheButKeepsSavedUserValues, where it is likewise never named in the removal list. Its one piece of genuinely unique coverage was accidental and unrelated to its name: it was the only place the Volume field made a disk round trip. That moves to the surviving test, whose comment now also records that it stands for the no-observed-drop case. Both removal tests dropped their JsonDocument assertions. GetMonitorParameters and GetKnownGoodFeatures after a reload already prove what the on-disk shape has to be, and System.Text.Json is no longer needed in the file. MonitorRestorePlannerTests Four of the five DataTestMethods varied readFlag across four rows that cannot discriminate. ShouldWrite is `(ReadValues & readFlag) != readFlag || target != current || target != displayed`, and in those four cases a mis-mapped switch arm yields a Monitor default that still satisfies the clause under test, so every row reaches the same clause with the same verdict. They collapse to one row each, spread across different flags. ShouldWrite_MatchingValueWasRead_ReturnsFalse keeps all four rows: it is the only case where all three clauses are false, so it is the only one whose verdict depends on the switch reading the right field, and a comment now says so. The class also gains `sealed`, matching every other test class in the project. Validation: built PowerDisplay.Lib.UnitTests for x64 Debug with VS MSBuild -- 0 errors, 0 warnings; ran PowerDisplay.Lib.UnitTests.dll with vstest.console.exe -- 275 passed, 0 failed (288 - 1 test method - 12 DataRow cases). No production code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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
On a monitor whose DDC/CI engine answers intermittently, Maximum compatibility mode loses controls it had a moment earlier — or drops the display from the flyout entirely. Three causes: a VCP code is re-read immediately after being probed, the first transient I2C failure is treated as final, and nothing is remembered between discoveries.
This makes the discovery path tolerate an unreliable panel: reuse the probe's own value, retry transient failures, and persist positive observations so a later failing pass can still show the control.
PR Checklist
Two boxes are deliberately left unchecked: the approach has not been agreed with a core contributor (there is an open design question below that needs a maintainer decision), and no dev docs were written.
Detailed Description of the Pull Request / Additional comments
What changed
MonitorReadFlagsstays clear for anything the hardware did not answer this passwinerror.h, and pin in tests both which of them are transient and what each numeric value isDiscovery is restructured around a
VcpDiscoveryEvidencevalue that reconciles three sources — the parsed capabilities string, this pass's live probe, and the persisted known-good cache — so "which VCP codes does this monitor support" is decided in one place.DdcCiController's per-feature initialization moves out intoContinuousVcpInitializerandDiscreteVcpInitializer, and every native VCP read now goes through a single injectableIVcpFeatureReaderseam.Behaviour changes outside Maximum compatibility mode
Both land on the shared discovery/restore path and affect users who never enable the feature.
mainlogged the failure, left the read flag unset and kept the monitor, so its handle reachedPhysicalMonitorHandleManager.ContinuousVcpInitializernow returnsPhysicalMonitorUnavailableandBuildMonitorFromPhysicaldrops the display. This is deliberate:Monitor.Handleis captured once per discovery pass and never refreshed, so keeping the monitor would send every later read and write to a handle already known to be dead, andDisplayChangeWatcherschedules the rediscovery that repairs it. The cost is that a handle dying without a device-arrival/removal or console-display-state notification leaves the monitor out of the flyout until the next such event.TryRestorecompared onlysaved != displayed; it now routes throughMonitorRestorePlanner.ShouldWrite, which also writes when(monitor.ReadValues & flag) != flag. An unread value is a discovery placeholder or a backing-field default rather than an observation, so suppressing the restore on a coincidental match silently dropped it — this is a fix. The visible cost is VCP0x14: on a panel that NAKs0x14reads but accepts writes, every profile apply and every startup restore now re-applies the color preset, which some panels surface on their OSD.Open design question — cache invalidation
This needs a maintainer decision; it is deliberately not resolved here.
Cached observations have no invalidation path: a definitive
DDCCI_VCP_NOT_SUPPORTEDreply does not outrank a cached positive. That is intentional and test-pinned (Reconcile_VcpNotSupportedStillUsesCachedPositiveEvidence), with the reasoning recorded atVcpDiscoveryEvidence.Reconcile.A 30-day freshness bound was implemented and reverted in ebe397f. It assumed every successful read restamps
LastSuccessfulUtc, but on the one path where the cache is load-bearing no read happens at all: once the probe has touched a code,PreferLiveReadis false andContinuousVcpInitializerapplies the cached value without reading or upserting. Thirty days of transient probe failures expired the entry, the reconciled capabilities went null, and the monitor was dropped entirely rather than losing one slider — an absorbing failure, since only a successful probe could rebuild what had been failing.That reasoning is no longer complete.
RefreshKnownGoodAfterWrite, added later in 61d75fc, restampsLastSuccessfulUtcon a successfulSetVCPFeature— including on exactly that path, becauseContinuousVcpInitializersetsBrightnessVcpMaxfrom the cached entry, so themaximum == existing.Maximumguard passes. Moving a slider therefore advances the clock with no read having succeeded. The bound is still not worth reinstating — a monitor nobody touches for 30 days still expires — but a successful write is a discriminator that was not available when ebe397f was written: a genuine phantom'sSetVCPFeatureis rejected by the panel and never restamps.The remaining option is to let a definitive
DDCCI_VCP_NOT_SUPPORTEDoutrank the cache. The trade-off is asymmetric:current=0 / max=0garbage reply failsVcpFeatureValue.IsValidand is never cached.NOT_SUPPORTEDNOT_SUPPORTEDis deliberately outside the transient set, so it gets a single attempt — while the capabilities string gets three.If eviction is wanted, it has to run after
Reconcile, not before: dropping the entry inline leavesReconcilewith null capabilities, andBuildMonitorFromPhysicalthen discards the display outright, taking its rotation and discrete VCPs with it.Known limitations
MonitorSettingsRebuilderre-emits every currently-discovered monitor with a freshLastSeenUtcand keepsIsHiddenentries unconditionally, so for a monitor in daily use the phantom is permanent in practice. There is no user-facing reset; a dedicated "reset detection cache" action was considered and deferred as out of scope.ReleaseAbandonedPhysicalships without test coverage. Reaching its call sites means faking the whole native enumeration surface (EnumDisplayMonitors,GetMonitorInfo,GetPhysicalMonitorsFromHMONITOR), a larger seam than this PR should introduce; the abandon paths were verified by reading instead.GetPhysicalMonitorsWithRetryAsyncdiscards a whole array of live handles, and cancellation unwinds beforeUpdateHandleMapruns. Both predate this branch and are better addressed separately.Validation Steps Performed
PowerDisplay.Lib.UnitTestsandPowerDisplayfor x64 Debug with VS MSBuild — 0 errors, 0 warningsPowerDisplay.Lib.UnitTests.dllwithvstest.console.exe: 275 passed, 0 failedMonitorStateManagerwas verified by deleting thelock (state)statements and confirmingConcurrentUpsertAndRead_OnSameMonitorDoNotTearTheFeatureMapthen fails withInvalidOperationException: Collection was modifiedRefreshKnownGoodAfterWritewas verified the same way — removing it failsRefreshKnownGoodAfterWrite_EmptyMonitorIdIsIgnoredDdcErrorClassifierTestspins both the membership of the two error sets and the numeric value of every constant againstwinerror.h, so a typo cannot move production and tests togetherAffected-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.