diff --git a/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/ContinuousVcpInitializerTests.cs b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/ContinuousVcpInitializerTests.cs new file mode 100644 index 0000000000..9452ced6a9 --- /dev/null +++ b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/ContinuousVcpInitializerTests.cs @@ -0,0 +1,209 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using PowerDisplay.Common.Drivers; +using PowerDisplay.Common.Drivers.DDC; +using PowerDisplay.Common.Models; +using static PowerDisplay.UnitTests.DdcFakes; + +namespace PowerDisplay.UnitTests; + +[TestClass] +public sealed class ContinuousVcpInitializerTests +{ + [TestMethod] + public void Initialize_ProbedValueIsAppliedWithoutReadingAgain() + { + // The reader is primed with a failure it must never reach: if the initializer re-reads a + // code the probe already answered, this test fails on the CallCount assertion rather than + // on a fabricated value. The probed range is deliberately not 0-100, so both the raw + // maximum and the percent scaling have to survive the seam for the assertions to hold. + var reader = new RecordingVcpReader(VcpReadAttempt.Failure(1)); + var initializer = new ContinuousVcpInitializer(reader); + var monitor = BrightnessMonitor(); + + var result = initializer.Initialize(monitor, Evidence((0x10, new VcpFeatureValue(15, 0, 50)))); + + Assert.IsTrue(result); + Assert.AreEqual(0, reader.CallCount); + Assert.AreEqual(30, monitor.CurrentBrightness); + Assert.AreEqual(50, monitor.BrightnessVcpMax); + Assert.IsTrue(monitor.ReadValues.HasFlag(MonitorReadFlags.Brightness)); + } + + [TestMethod] + public void Initialize_CodeWithoutAProbedValueIsReadOnce() + { + var reader = new RecordingVcpReader(VcpReadAttempt.Success(55, 100)); + var initializer = new ContinuousVcpInitializer(reader); + var monitor = BrightnessMonitor(); + + var result = initializer.Initialize(monitor, Evidence()); + + Assert.IsTrue(result); + Assert.AreEqual(1, reader.CallCount); + Assert.AreEqual(55, monitor.CurrentBrightness); + Assert.IsTrue(monitor.ReadValues.HasFlag(MonitorReadFlags.Brightness)); + } + + [TestMethod] + public void Initialize_InvalidReadRangeIsNotApplied() + { + var reader = new RecordingVcpReader(VcpReadAttempt.Success(55, 0)); + var initializer = new ContinuousVcpInitializer(reader); + var monitor = BrightnessMonitor(); + + var result = initializer.Initialize(monitor, Evidence()); + + Assert.IsTrue(result); + Assert.AreEqual(1, reader.CallCount); + Assert.AreEqual(0, monitor.CurrentBrightness); + Assert.IsFalse(monitor.ReadValues.HasFlag(MonitorReadFlags.Brightness)); + } + + [DataTestMethod] + [DataRow(DdcErrorClassifier.ErrorGraphicsInvalidPhysicalMonitorHandle)] + [DataRow(DdcErrorClassifier.ErrorGraphicsMonitorNoLongerExists)] + public void Initialize_HandleClassFailureStopsAndReportsToTheCaller(int errorCode) + { + var reader = new RecordingVcpReader(VcpReadAttempt.Failure(errorCode)); + var initializer = new ContinuousVcpInitializer(reader); + var monitor = BrightnessAndContrastMonitor(); + + var result = initializer.Initialize(monitor, Evidence()); + + Assert.IsFalse(result, "A handle-class read failure must tell the caller to drop the monitor."); + CollectionAssert.AreEqual(new byte[] { 0x10 }, reader.Codes, "The dead handle must not be used again."); + Assert.AreEqual(MonitorReadFlags.None, monitor.ReadValues); + } + + [TestMethod] + public void Initialize_FeatureLevelFailureSkipsOnlyThatCode() + { + // DDCCI_VCP_NOT_SUPPORTED is the device's answer about one opcode, not about the handle, + // so the remaining codes still get their read. + var reader = new RecordingVcpReader( + VcpReadAttempt.Failure(DdcErrorClassifier.ErrorGraphicsDdcCiVcpNotSupported), + VcpReadAttempt.Success(60, 100)); + var initializer = new ContinuousVcpInitializer(reader); + var monitor = BrightnessAndContrastMonitor(); + + var result = initializer.Initialize(monitor, Evidence()); + + Assert.IsTrue(result); + CollectionAssert.AreEqual(new byte[] { 0x10, 0x12 }, reader.Codes); + Assert.IsFalse(monitor.ReadValues.HasFlag(MonitorReadFlags.Brightness)); + Assert.AreEqual(60, monitor.CurrentContrast); + Assert.IsTrue(monitor.ReadValues.HasFlag(MonitorReadFlags.Contrast)); + } + + [TestMethod] + public void Initialize_EveryContinuousCodeIsReadAndApplied() + { + // Pins the invariant ContinuousVcpInitializer's own remarks declare but nothing else + // enforced: every entry in ContinuousVcpCodes needs an arm in both IsSupported and + // ApplyValue. Without an IsSupported arm the code is never read, which the Codes assertion + // catches; without an ApplyValue arm it is read and then discarded, which the per-feature + // assertions catch. Ranges and percentages are all distinct so a cross-wired arm cannot + // pass by coincidence. + Assert.AreEqual( + 3, + NativeConstants.ContinuousVcpCodes.Length, + "ContinuousVcpCodes grew — give the new code an IsSupported and an ApplyValue arm, then extend this test."); + + var reader = new RecordingVcpReader( + VcpReadAttempt.Success(15, 50), + VcpReadAttempt.Success(20, 40), + VcpReadAttempt.Success(7, 10)); + var initializer = new ContinuousVcpInitializer(reader); + var monitor = AllContinuousMonitor(); + + var result = initializer.Initialize(monitor, Evidence()); + + Assert.IsTrue(result); + CollectionAssert.AreEqual(NativeConstants.ContinuousVcpCodes, reader.Codes); + + Assert.AreEqual(30, monitor.CurrentBrightness); + Assert.AreEqual(50, monitor.BrightnessVcpMax); + Assert.AreEqual(50, monitor.CurrentContrast); + Assert.AreEqual(40, monitor.ContrastVcpMax); + Assert.AreEqual(70, monitor.CurrentVolume); + Assert.AreEqual(10, monitor.VolumeVcpMax); + Assert.AreEqual( + MonitorReadFlags.Brightness | MonitorReadFlags.Contrast | MonitorReadFlags.Volume, + monitor.ReadValues); + } + + [TestMethod] + public void Initialize_ProbedVolumeIsAppliedWithoutReadingAgain() + { + // Volume is the one continuous code the probe path is not otherwise exercised against, and + // it is the one whose ApplyValue arm has no neighbour to shadow a mistake. + var reader = new RecordingVcpReader(VcpReadAttempt.Failure(1)); + var initializer = new ContinuousVcpInitializer(reader); + var monitor = VolumeMonitor(); + + var result = initializer.Initialize(monitor, Evidence((0x62, new VcpFeatureValue(7, 0, 10)))); + + Assert.IsTrue(result); + Assert.AreEqual(0, reader.CallCount); + Assert.AreEqual(70, monitor.CurrentVolume); + Assert.AreEqual(10, monitor.VolumeVcpMax); + Assert.IsTrue(monitor.ReadValues.HasFlag(MonitorReadFlags.Volume)); + } + + private static Monitor BrightnessMonitor() => new() + { + Id = MonitorId, + Handle = new IntPtr(1), + Capabilities = MonitorCapabilities.DdcCi | MonitorCapabilities.Brightness, + }; + + private static Monitor BrightnessAndContrastMonitor() => new() + { + Id = MonitorId, + Handle = new IntPtr(1), + Capabilities = MonitorCapabilities.DdcCi | + MonitorCapabilities.Brightness | + MonitorCapabilities.Contrast, + }; + + private static Monitor VolumeMonitor() => new() + { + Id = MonitorId, + Handle = new IntPtr(1), + Capabilities = MonitorCapabilities.DdcCi | MonitorCapabilities.Volume, + }; + + private static Monitor AllContinuousMonitor() => new() + { + Id = MonitorId, + Handle = new IntPtr(1), + Capabilities = MonitorCapabilities.DdcCi | + MonitorCapabilities.Brightness | + MonitorCapabilities.Contrast | + MonitorCapabilities.Volume, + }; + + /// + /// Builds evidence carrying only the probed values: + /// reads and nothing else, so a capabilities + /// object here would be scaffolding no assertion depends on. Support is expressed by the + /// fixture's flags instead, which is what the initializer + /// actually gates on. + /// + private static VcpDiscoveryEvidence Evidence(params (byte Code, VcpFeatureValue Value)[] probed) + { + var values = new Dictionary(); + foreach (var (code, value) in probed) + { + values[code] = value; + } + + return new VcpDiscoveryEvidence(string.Empty, new VcpCapabilities(), values); + } +} diff --git a/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/DdcErrorClassifierTests.cs b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/DdcErrorClassifierTests.cs index c1f075b264..693879f0a5 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/DdcErrorClassifierTests.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/DdcErrorClassifierTests.cs @@ -82,4 +82,16 @@ public sealed class DdcErrorClassifierTests [DataRow(0)] public void IsPhysicalMonitorUnavailable_RejectsFeatureLevelFailures(int errorCode) => Assert.IsFalse(DdcErrorClassifier.IsPhysicalMonitorUnavailable(errorCode)); + + [TestMethod] + public void Format_RendersTheUnsignedHexTheSdkDocuments() + { + // Marshal.GetLastWin32Error hands 0xC026258C back as a negative int, which greps against + // nothing. The log has to carry the spelling winerror.h uses. + Assert.AreEqual( + "0xC026258C", + DdcErrorClassifier.Format(DdcErrorClassifier.ErrorGraphicsInvalidPhysicalMonitorHandle)); + Assert.AreEqual("0x000005B4", DdcErrorClassifier.Format(DdcErrorClassifier.ErrorTimeout)); + Assert.AreEqual("none", DdcErrorClassifier.Format(null)); + } } diff --git a/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/DdcFakes.cs b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/DdcFakes.cs new file mode 100644 index 0000000000..bf72fe9bf0 --- /dev/null +++ b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/DdcFakes.cs @@ -0,0 +1,48 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using PowerDisplay.Common.Drivers.DDC; + +namespace PowerDisplay.UnitTests; + +/// +/// Test doubles and fixtures shared by the DDC discovery tests. They live nested inside one +/// container so the file keeps a single top-level type; call sites pull them into scope with +/// using static PowerDisplay.UnitTests.DdcFakes; and use them unqualified. +/// +internal static class DdcFakes +{ + /// + /// A canonical DevicePath-shaped monitor Id, so a change to what a canonical Id looks like + /// shows up in one place. + /// + internal const string MonitorId = @"\\?\DISPLAY#AOCB326#5&ABC&0&UID1"; + + /// + /// Serves a scripted sequence of read results and records what it was asked for, so a test can + /// pin both how many native reads happened and against which codes. + /// + /// + /// Dequeuing past the end throws rather than yielding a default-valued result, so a fabricated + /// reply never reaches the assertions. The throw is not itself the failure message: an extra + /// read is named by the and assertions instead. + /// + internal sealed class RecordingVcpReader(params VcpReadAttempt[] results) : IVcpFeatureReader + { + private readonly Queue _results = new(results); + + public int CallCount { get; private set; } + + public List Codes { get; } = new(); + + public VcpReadAttempt Read(IntPtr handle, byte code) + { + CallCount++; + Codes.Add(code); + return _results.Dequeue(); + } + } +} diff --git a/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/VcpDiscoveryEvidenceTests.cs b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/VcpDiscoveryEvidenceTests.cs new file mode 100644 index 0000000000..ed59849c56 --- /dev/null +++ b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/VcpDiscoveryEvidenceTests.cs @@ -0,0 +1,130 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using PowerDisplay.Common.Drivers.DDC; +using PowerDisplay.Common.Models; + +namespace PowerDisplay.UnitTests; + +/// +/// Covers what discovery concludes from a set of probe observations: which VCP codes count as +/// supported, and which of them carry a value the build stage may use instead of re-reading. +/// +[TestClass] +public sealed class VcpDiscoveryEvidenceTests +{ + [TestMethod] + public void Reconcile_SuccessfulProbeAdvertisesSupportAndCarriesTheValue() + { + // The whole point of the evidence: a code the probe read must not be read again. + var result = VcpDiscoveryEvidence.Reconcile( + capabilitiesRaw: string.Empty, + parsedCapabilities: null, + live: Observations((0x10, VcpProbeObservation.Success(0x10, new VcpFeatureValue(30, 0, 100))))); + + Assert.IsTrue(result.Capabilities!.SupportsVcpCode(0x10)); + Assert.AreEqual(30, result.InitialValues[0x10].Current); + Assert.AreEqual(100, result.InitialValues[0x10].Maximum); + } + + [TestMethod] + public void Reconcile_RepliedProbeWithUnusableRangeAdvertisesSupportWithoutAValue() + { + // The device answered, which proves the opcode exists, but max=0 cannot scale a percentage. + // Support is kept so the control stays reachable; the absent value sends the initializer + // back to the hardware for one more read. + var result = VcpDiscoveryEvidence.Reconcile( + capabilitiesRaw: string.Empty, + parsedCapabilities: null, + live: Observations((0x62, VcpProbeObservation.Indeterminate(0x62, lastError: null, attempts: 1, replied: true)))); + + Assert.IsTrue(result.Capabilities!.SupportsVcpCode(0x62)); + Assert.IsFalse(result.InitialValues.ContainsKey(0x62)); + } + + [TestMethod] + public void Reconcile_UnansweredProbeDoesNotAdvertiseSupport() + { + var result = VcpDiscoveryEvidence.Reconcile( + capabilitiesRaw: string.Empty, + parsedCapabilities: null, + live: Observations((0x62, VcpProbeObservation.Indeterminate( + 0x62, + DdcErrorClassifier.ErrorGraphicsDdcCiVcpNotSupported)))); + + Assert.IsNull(result.Capabilities); + Assert.AreEqual(0, result.InitialValues.Count); + } + + [DataTestMethod] + [DataRow(DdcErrorClassifier.ErrorGraphicsInvalidPhysicalMonitorHandle)] + [DataRow(DdcErrorClassifier.ErrorGraphicsMonitorNoLongerExists)] + public void Reconcile_PhysicalMonitorUnavailableDiscardsEverything(int errorCode) + { + // A dead handle invalidates the whole pass, including the codes that answered before it + // died — nothing gathered through it can be trusted. + var parsed = new VcpCapabilities(); + parsed.SupportedVcpCodes[0x10] = new VcpCodeInfo(0x10, "Brightness"); + + var result = VcpDiscoveryEvidence.Reconcile( + capabilitiesRaw: "caps", + parsedCapabilities: parsed, + live: Observations( + (0x10, VcpProbeObservation.Success(0x10, new VcpFeatureValue(30, 0, 100))), + (0x12, VcpProbeObservation.Indeterminate(0x12, errorCode)))); + + Assert.IsTrue(result.IsPhysicalMonitorUnavailable); + Assert.IsNull(result.Capabilities); + Assert.AreEqual(0, result.InitialValues.Count); + } + + [TestMethod] + public void Reconcile_ProbedCodeOutsideTheDefaultSweepIsStillHonoured() + { + // Reconcile is driven by what the probe reported, not by NativeConstants.ContinuousVcpCodes, + // so widening VcpFeatureProbeService's constructor-injected sweep list does not silently drop + // a code that answered. Honouring it here is necessary but not sufficient: the carried value + // is only consumed for codes ContinuousVcpInitializer walks, so a widened sweep still needs a + // matching edit there. + var result = VcpDiscoveryEvidence.Reconcile( + capabilitiesRaw: string.Empty, + parsedCapabilities: null, + live: Observations((0x60, VcpProbeObservation.Success(0x60, new VcpFeatureValue(0x11, 0, 0x12))))); + + Assert.IsTrue(result.Capabilities!.SupportsVcpCode(0x60)); + Assert.AreEqual(0x11, result.InitialValues[0x60].Current); + } + + [TestMethod] + public void Reconcile_ParsedCapabilitiesSurviveWhenNoProbeRan() + { + // The probe only runs when the caps string is unusable, so the parsed path must be a + // pass-through: no codes added, no values invented. + var parsed = new VcpCapabilities(); + parsed.SupportedVcpCodes[0x10] = new VcpCodeInfo(0x10, "Brightness"); + + var result = VcpDiscoveryEvidence.Reconcile( + capabilitiesRaw: "caps", + parsedCapabilities: parsed, + live: new Dictionary()); + + Assert.AreSame(parsed, result.Capabilities); + Assert.AreEqual(0, result.InitialValues.Count); + Assert.AreEqual("caps", result.CapabilitiesRaw); + } + + private static Dictionary Observations( + params (byte Code, VcpProbeObservation Observation)[] entries) + { + var observations = new Dictionary(); + foreach (var (code, observation) in entries) + { + observations[code] = observation; + } + + return observations; + } +} diff --git a/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/VcpFeatureProbeServiceTests.cs b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/VcpFeatureProbeServiceTests.cs index b1577c1a81..88159b0c61 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/VcpFeatureProbeServiceTests.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/VcpFeatureProbeServiceTests.cs @@ -8,6 +8,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.VisualStudio.TestTools.UnitTesting; using PowerDisplay.Common.Drivers.DDC; +using static PowerDisplay.UnitTests.DdcFakes; namespace PowerDisplay.UnitTests; @@ -304,31 +305,4 @@ public sealed class VcpFeatureProbeServiceTests ? throw new InvalidOperationException("simulated native failure") : otherwise; } - - /// - /// Serves a scripted sequence of read results and records what it was asked for, so a test can - /// pin both how many native reads happened and against which codes. - /// - /// - /// Dequeuing past the end throws rather than yielding a default-valued result, so a fabricated - /// reply never reaches the assertions. The throw is not itself the failure message: it is - /// raised inside the reader, and 's catch-all turns it into - /// an indeterminate observation. An extra read is named by the and - /// assertions instead. - /// - private sealed class RecordingVcpReader(params VcpReadAttempt[] results) : IVcpFeatureReader - { - private readonly Queue _results = new(results); - - public int CallCount { get; private set; } - - public List Codes { get; } = new(); - - public VcpReadAttempt Read(IntPtr handle, byte code) - { - CallCount++; - Codes.Add(code); - return _results.Dequeue(); - } - } } diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/ContinuousVcpInitializer.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/ContinuousVcpInitializer.cs new file mode 100644 index 0000000000..038d0e1b64 --- /dev/null +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/ContinuousVcpInitializer.cs @@ -0,0 +1,138 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using ManagedCommon; +using PowerDisplay.Common.Models; +using PowerDisplay.Common.Utils; +using static PowerDisplay.Common.Drivers.NativeConstants; + +namespace PowerDisplay.Common.Drivers.DDC +{ + /// + /// Applies the percent-scaled VCP features — brightness, contrast, volume — to a freshly built + /// . + /// + internal sealed class ContinuousVcpInitializer + { + private readonly IVcpFeatureReader _reader; + + public ContinuousVcpInitializer(IVcpFeatureReader reader) + { + _reader = reader; + } + + /// + /// Applies the continuous VCP features to , reading from + /// only for codes the evidence does not already carry a value + /// for. + /// + /// + /// False when a read failed with a handle-class error, which invalidates + /// itself rather than the one feature — the caller must then + /// discard the monitor instead of publishing it. + /// + public bool Initialize(Monitor monitor, VcpDiscoveryEvidence evidence) + { + foreach (var code in ContinuousVcpCodes) + { + if (!InitializeFeature(monitor, evidence, code)) + { + return false; + } + } + + return true; + } + + private bool InitializeFeature(Monitor monitor, VcpDiscoveryEvidence evidence, byte code) + { + if (!IsSupported(monitor, code)) + { + return true; + } + + // The probe already spent at least one transaction on every code it answered for, so + // reading one of those again in the same pass is pure I2C noise on a bus that is slow + // and, on the hardware this path exists for, unreliable. + if (evidence.InitialValues.TryGetValue(code, out var probed)) + { + ApplyValue(monitor, code, probed); + return true; + } + + var read = _reader.Read(monitor.Handle, code); + if (!read.IsSuccess) + { + Logger.LogError( + $"DDC: [{monitor.Id}] Failed to read VCP 0x{code:X2}, " + + $"error={DdcErrorClassifier.Format(read.ErrorCode)}"); + if (DdcErrorClassifier.IsPhysicalMonitorUnavailable(read.ErrorCode)) + { + // Dropping the monitor is deliberate: Monitor.Handle is captured once per + // discovery pass and never refreshed, so a monitor kept here would send every + // later read and write to a handle already known to be dead. A rediscovery is + // what repairs it — DisplayChangeWatcher schedules one for the topology changes + // that invalidate a handle, and the flyout's Refresh button forces one on + // demand. + return false; + } + + return true; + } + + var value = new VcpFeatureValue((int)read.Current, 0, (int)read.Maximum); + if (!value.IsValid) + { + Logger.LogWarning( + $"DDC: [{monitor.Id}] Ignoring invalid {VcpNames.GetCodeName(code).ToLowerInvariant()} " + + $"range current={read.Current}, max={read.Maximum}"); + return true; + } + + ApplyValue(monitor, code, value); + return true; + } + + /// + /// Whether advertises . + /// + /// + /// This switch and 's must between them cover every entry in + /// : a code added to that array without an + /// arm here is skipped, and one without an arm there is read and then discarded. Both are + /// silent, so the array and the two switches are edited together. + /// + private static bool IsSupported(Monitor monitor, byte code) => code switch + { + VcpCodeBrightness => monitor.SupportsBrightness, + VcpCodeContrast => monitor.SupportsContrast, + VcpCodeVolume => monitor.SupportsVolume, + _ => false, + }; + + private static void ApplyValue(Monitor monitor, byte code, VcpFeatureValue value) + { + switch (code) + { + case VcpCodeBrightness: + monitor.BrightnessVcpMax = value.Maximum; + monitor.CurrentBrightness = value.ToPercentage(); + monitor.ReadValues |= MonitorReadFlags.Brightness; + break; + + case VcpCodeContrast: + monitor.ContrastVcpMax = value.Maximum; + monitor.CurrentContrast = value.ToPercentage(); + monitor.ReadValues |= MonitorReadFlags.Contrast; + break; + + case VcpCodeVolume: + monitor.VolumeVcpMax = value.Maximum; + monitor.CurrentVolume = value.ToPercentage(); + monitor.ReadValues |= MonitorReadFlags.Volume; + break; + } + } + } +} diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs index 27623096ba..3d1da399c0 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs @@ -35,6 +35,7 @@ namespace PowerDisplay.Common.Drivers.DDC private readonly PhysicalMonitorHandleManager _handleManager = new(); private readonly MonitorDiscoveryHelper _discoveryHelper; private readonly VcpFeatureProbeService _probeService; + private readonly ContinuousVcpInitializer _continuousInitializer; private bool _disposed; @@ -48,7 +49,9 @@ namespace PowerDisplay.Common.Drivers.DDC public DdcCiController() { _discoveryHelper = new MonitorDiscoveryHelper(); - _probeService = new VcpFeatureProbeService(new NativeVcpFeatureReader()); + var vcpReader = new NativeVcpFeatureReader(); + _probeService = new VcpFeatureProbeService(vcpReader); + _continuousInitializer = new ContinuousVcpInitializer(vcpReader); } public string Name => "DDC/CI Monitor Controller"; @@ -285,10 +288,11 @@ namespace PowerDisplay.Common.Drivers.DDC private Monitor? BuildMonitorFromPhysical( PHYSICAL_MONITOR physical, MonitorDisplayInfo info, - string capsString, - VcpCapabilities? caps) + VcpDiscoveryEvidence evidence) { - if (caps == null) + // The caller already skipped the unusable-capabilities and dead-handle cases, each with + // its own log line; this null check only carries that contract into the nullable flow. + if (evidence.Capabilities == null) { return null; } @@ -301,31 +305,26 @@ namespace PowerDisplay.Common.Drivers.DDC return null; } - if (!string.IsNullOrEmpty(capsString)) + if (!string.IsNullOrEmpty(evidence.CapabilitiesRaw)) { - monitor.CapabilitiesRaw = capsString; + monitor.CapabilitiesRaw = evidence.CapabilitiesRaw; } - monitor.VcpCapabilitiesInfo = caps; - UpdateMonitorCapabilitiesFromVcp(monitor, caps); + monitor.VcpCapabilitiesInfo = evidence.Capabilities; + UpdateMonitorCapabilitiesFromVcp(monitor, evidence.Capabilities); - // Initialize current values for every VCP feature the device reports - // support for, ordered continuous-range first (percent-scaled), - // then discrete-enum VCPs. Each guard is independent — a controller - // can support any subset. - if (monitor.SupportsBrightness) + // Continuous (percent-scaled) VCPs first, then the discrete-enum ones. Only the + // continuous stage can discard the monitor, and that is a policy choice rather + // than a property of the stage: losing a whole display because 0xD6 answered + // badly is worse than showing it without a power control. Note the check is not + // on every path — a caps string that parses but advertises none of 0x10/0x12/0x62 + // leaves the continuous stage nothing to read and suppresses the probe, so such a + // monitor is published with a handle no VCP read has exercised. + if (!_continuousInitializer.Initialize(monitor, evidence)) { - InitializeBrightness(monitor, physical.HPhysicalMonitor); - } - - if (monitor.SupportsContrast) - { - InitializeContrast(monitor, physical.HPhysicalMonitor); - } - - if (monitor.SupportsVolume) - { - InitializeVolume(monitor, physical.HPhysicalMonitor); + Logger.LogWarning( + $"DDC: [DevicePath={info.DevicePath}] monitor ignored — physical monitor handle became unavailable during continuous VCP initialization"); + return null; } if (monitor.SupportsColorTemperature) @@ -389,7 +388,7 @@ namespace PowerDisplay.Common.Drivers.DDC /// (cooperative — checked between attempts) and during the 1 s delay between /// retries. /// - private async Task<(string CapsString, VcpCapabilities? Caps)> FetchCapabilitiesWithFallbackAsync( + private async Task FetchCapabilitiesWithFallbackAsync( IntPtr hPhysicalMonitor, CancellationToken cancellationToken) { @@ -442,56 +441,37 @@ namespace PowerDisplay.Common.Drivers.DDC $"DDC: cap string still empty after {maxAttempts} attempts (handle=0x{hPhysicalMonitor:X})"); } + IReadOnlyDictionary live = + new Dictionary(); + if (caps == null && MaxCompatibilityMode) { Logger.LogInfo( $"DDC: [max-compat] caps unusable for handle=0x{hPhysicalMonitor:X}; probing VCP features directly"); - var observations = await _probeService.ProbeAsync(hPhysicalMonitor, cancellationToken); - caps = BuildCapabilitiesFromProbe(observations); + live = await _probeService.ProbeAsync(hPhysicalMonitor, cancellationToken); + } - if (caps != null) + var evidence = VcpDiscoveryEvidence.Reconcile(capsString ?? string.Empty, caps, live); + + if (live.Count > 0) + { + var message = + $"DDC: [max-compat] probe outcome for handle=0x{hPhysicalMonitor:X}: " + + $"{evidence.InitialValues.Count}/{live.Count} feature(s) read, " + + $"{evidence.Capabilities?.SupportedVcpCodes.Count ?? 0} advertised"; + + if (evidence.Capabilities != null) { - Logger.LogInfo( - $"DDC: [max-compat] recovered monitor (handle=0x{hPhysicalMonitor:X}) " + - $"with {caps.SupportedVcpCodes.Count} probed feature(s)"); + Logger.LogInfo(message); } else { - Logger.LogWarning( - $"DDC: [max-compat] probe returned no supported features for handle=0x{hPhysicalMonitor:X}"); + Logger.LogWarning(message); } } - return (capsString ?? string.Empty, caps); - } - - /// - /// Synthesizes capabilities from the VCP codes the device answered for, or null when none - /// did — which the caller treats the same way as an unusable capabilities string. - /// - /// - /// Membership is decided by , not by whether the - /// value was usable. A reply proves the device implements the opcode even when the reported - /// range cannot scale a percentage; an unimplemented code fails with - /// DDCCI_VCP_NOT_SUPPORTED instead and never sets the flag. Friendly names come from - /// to keep a single source of truth. - /// - private static VcpCapabilities? BuildCapabilitiesFromProbe( - IReadOnlyDictionary observations) - { - var caps = new VcpCapabilities(); - - foreach (var observation in observations.Values) - { - if (observation.Replied) - { - caps.SupportedVcpCodes[observation.Code] = - new VcpCodeInfo(observation.Code, VcpNames.GetCodeName(observation.Code)); - } - } - - return caps.SupportedVcpCodes.Count > 0 ? caps : null; + return evidence; } /// @@ -530,72 +510,6 @@ namespace PowerDisplay.Common.Drivers.DDC } } - /// - /// Initialize brightness value for a monitor using VCP 0x10. - /// Persists the device-reported raw maximum so subsequent writes can scale percent → raw. - /// - private static void InitializeBrightness(Monitor monitor, IntPtr handle) - { - if (TryGetVcpFeature(handle, VcpCodeBrightness, monitor.Id, out uint current, out uint max)) - { - var brightnessInfo = new VcpFeatureValue((int)current, 0, (int)max); - if (!brightnessInfo.IsValid) - { - Logger.LogWarning( - $"DDC: [{monitor.Id}] Ignoring invalid brightness range current={current}, max={max}"); - return; - } - - monitor.BrightnessVcpMax = (int)max; - monitor.CurrentBrightness = brightnessInfo.ToPercentage(); - monitor.ReadValues |= MonitorReadFlags.Brightness; - } - } - - /// - /// Initialize contrast value for a monitor using VCP 0x12. - /// Persists the device-reported raw maximum so subsequent writes can scale percent → raw. - /// - private static void InitializeContrast(Monitor monitor, IntPtr handle) - { - if (TryGetVcpFeature(handle, VcpCodeContrast, monitor.Id, out uint current, out uint max)) - { - var contrastInfo = new VcpFeatureValue((int)current, 0, (int)max); - if (!contrastInfo.IsValid) - { - Logger.LogWarning( - $"DDC: [{monitor.Id}] Ignoring invalid contrast range current={current}, max={max}"); - return; - } - - monitor.ContrastVcpMax = (int)max; - monitor.CurrentContrast = contrastInfo.ToPercentage(); - monitor.ReadValues |= MonitorReadFlags.Contrast; - } - } - - /// - /// Initialize volume value for a monitor using VCP 0x62. - /// Persists the device-reported raw maximum so subsequent writes can scale percent → raw. - /// - private static void InitializeVolume(Monitor monitor, IntPtr handle) - { - if (TryGetVcpFeature(handle, VcpCodeVolume, monitor.Id, out uint current, out uint max)) - { - var volumeInfo = new VcpFeatureValue((int)current, 0, (int)max); - if (!volumeInfo.IsValid) - { - Logger.LogWarning( - $"DDC: [{monitor.Id}] Ignoring invalid volume range current={current}, max={max}"); - return; - } - - monitor.VolumeVcpMax = (int)max; - monitor.CurrentVolume = volumeInfo.ToPercentage(); - monitor.ReadValues |= MonitorReadFlags.Volume; - } - } - /// /// Wrapper for GetVCPFeatureAndVCPFeatureReply that logs errors on failure. /// @@ -800,10 +714,18 @@ namespace PowerDisplay.Common.Drivers.DDC // Async caps fetch (retry + max-compat probe). Awaits Task.Delay between // retries instead of blocking the threadpool. - var (capsString, caps) = await FetchCapabilitiesWithFallbackAsync( + var evidence = await FetchCapabilitiesWithFallbackAsync( physical.HPhysicalMonitor, cancellationToken); - if (caps == null) + if (evidence.IsPhysicalMonitorUnavailable) + { + Logger.LogWarning( + $"DDC: [DevicePath={info.DevicePath}] monitor ignored — physical monitor handle is no longer valid"); + ReleaseAbandonedPhysical(physical); + continue; + } + + if (evidence.Capabilities == null) { Logger.LogWarning( $"DDC: [DevicePath={info.DevicePath}] monitor ignored — capabilities unavailable"); @@ -811,11 +733,11 @@ namespace PowerDisplay.Common.Drivers.DDC continue; } - // Heavy sync block (~6 × ~100 ms VCP reads on this one I2C bus). - // Dispatch to the threadpool; await before the next physical because - // they share the same hMonitor's I2C arbitration. + // Heavy sync block (VCP reads on this one I2C bus, minus whatever the probe + // already answered). Dispatch to the threadpool; await before the next physical + // because they share the same hMonitor's I2C arbitration. var monitor = await Task.Run( - () => BuildMonitorFromPhysical(physical, info, capsString, caps), + () => BuildMonitorFromPhysical(physical, info, evidence), cancellationToken); if (monitor != null) diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcErrorClassifier.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcErrorClassifier.cs index d200264da8..008281b1bb 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcErrorClassifier.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcErrorClassifier.cs @@ -10,8 +10,12 @@ namespace PowerDisplay.Common.Drivers.DDC /// names mirror winerror.h exactly. /// /// - /// is the only consumer today. The discovery-time value - /// reads in DdcCiController.TryGetVcpFeature are still single-shot and unclassified. + /// Consumed by , which retries only what + /// admits, and by , which uses + /// to tell a dead handle from one feature the device + /// refused. The discrete-VCP reads in DdcCiController.Initialize* and the runtime value + /// reads behind DdcCiController.GetVcpFeatureAsync are still single-shot and + /// unclassified. /// internal static class DdcErrorClassifier { @@ -83,5 +87,13 @@ namespace PowerDisplay.Common.Drivers.DDC ErrorGraphicsDdcCiInvalidMessageChecksum or ErrorGraphicsDdcCiCurrentCurrentValueGreaterThanMaximumValue or ErrorTimeout; + + /// + /// Renders an error for a log line as the unsigned hex the SDK documents it under, so a + /// reader can grep it against winerror.h. Marshal.GetLastWin32Error hands these back + /// as a negative int, which matches nothing. + /// + public static string Format(int? errorCode) => + errorCode.HasValue ? $"0x{unchecked((uint)errorCode.Value):X8}" : "none"; } } diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/VcpDiscoveryEvidence.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/VcpDiscoveryEvidence.cs new file mode 100644 index 0000000000..c2bd5bc0b1 --- /dev/null +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/VcpDiscoveryEvidence.cs @@ -0,0 +1,115 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; +using PowerDisplay.Common.Models; +using PowerDisplay.Common.Utils; + +namespace PowerDisplay.Common.Drivers.DDC +{ + /// + /// What discovery learned about one physical monitor before it is turned into a + /// : the capabilities it advertises and the values already read off it. + /// + /// + /// This is the seam between the async fetch stage, which owns the I2C traffic, and the + /// synchronous build stage, which owns the object. Carrying the probe's + /// values across it is the point: without them the build stage re-reads every code the probe + /// just answered. + /// + internal sealed class VcpDiscoveryEvidence + { + public VcpDiscoveryEvidence( + string capabilitiesRaw, + VcpCapabilities? capabilities, + IReadOnlyDictionary initialValues, + bool isPhysicalMonitorUnavailable = false) + { + CapabilitiesRaw = capabilitiesRaw; + Capabilities = capabilities; + InitialValues = initialValues; + IsPhysicalMonitorUnavailable = isPhysicalMonitorUnavailable; + } + + public string CapabilitiesRaw { get; } + + public VcpCapabilities? Capabilities { get; } + + /// + /// Gets the values this pass already read off the hardware, keyed by VCP code. A code that + /// is absent still owes the hardware a read; a code that is present must not be re-read. + /// + public IReadOnlyDictionary InitialValues { get; } + + public bool IsPhysicalMonitorUnavailable { get; } + + /// + /// Folds this pass's probe observations into the parsed capabilities. + /// + /// + /// The probe only runs when the capabilities string is unusable, so on the parsed path + /// is empty and this is a pass-through. + /// + public static VcpDiscoveryEvidence Reconcile( + string capabilitiesRaw, + VcpCapabilities? parsedCapabilities, + IReadOnlyDictionary live) + { + foreach (var observation in live.Values) + { + if (observation.IsPhysicalMonitorUnavailable) + { + // The handle itself is gone, so nothing gathered against it can be trusted and + // nothing more may be issued through it. + return new VcpDiscoveryEvidence( + capabilitiesRaw, + capabilities: null, + new Dictionary(), + isPhysicalMonitorUnavailable: true); + } + } + + var capabilities = parsedCapabilities; + var values = new Dictionary(); + + // Driven by what the probe reported rather than by NativeConstants.ContinuousVcpCodes, + // which VcpFeatureProbeService only takes as the default for its constructor-injected + // sweep list. Widening that sweep still needs a matching edit in + // ContinuousVcpInitializer for the carried value to be used — this loop only keeps the + // code from being dropped on the way there. + foreach (var (code, observation) in live) + { + if (!observation.Replied) + { + continue; + } + + // A reply proves the device implements the opcode. Membership does not depend on + // the value being usable — an unimplemented code fails with DDCCI_VCP_NOT_SUPPORTED + // instead and never sets Replied. + capabilities = MarkSupported(capabilities, code); + + if (observation.IsSuccess) + { + values[code] = observation.Value; + } + } + + return new VcpDiscoveryEvidence(capabilitiesRaw, capabilities, values); + } + + /// + /// Records that is supported, creating the container when discovery + /// produced no parsed capabilities. Adds only: an entry parsed from the capabilities string + /// carries discrete-value and custom-name metadata a synthesized + /// does not. + /// + private static VcpCapabilities MarkSupported(VcpCapabilities? capabilities, byte code) + { + capabilities ??= new VcpCapabilities(); + capabilities.SupportedVcpCodes.TryAdd(code, new VcpCodeInfo(code, VcpNames.GetCodeName(code))); + return capabilities; + } + } +} diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/VcpFeatureProbeService.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/VcpFeatureProbeService.cs index db69f975b0..a7b7786f7a 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/VcpFeatureProbeService.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/VcpFeatureProbeService.cs @@ -115,7 +115,7 @@ namespace PowerDisplay.Common.Drivers.DDC Logger.LogDebug( $"DDC: [max-compat] VCP probe attempt " + $"(handle=0x{handle:X}, code=0x{code:X2}, attempt={attempt}/{MaxAttempts}, " + - $"status=failed, error={FormatError(lastError)})"); + $"status=failed, error={DdcErrorClassifier.Format(lastError)})"); if (!DdcErrorClassifier.IsTransient(result.ErrorCode)) { break; @@ -141,7 +141,7 @@ namespace PowerDisplay.Common.Drivers.DDC var message = $"DDC: [max-compat] VCP probe outcome " + $"(handle=0x{handle:X}, code=0x{observation.Code:X2}, attempts={observation.Attempts}, " + - $"status={status}, replied={observation.Replied}, lastError={FormatError(observation.LastError)})"; + $"status={status}, replied={observation.Replied}, lastError={DdcErrorClassifier.Format(observation.LastError)})"; if (observation.IsSuccess) { @@ -154,9 +154,6 @@ namespace PowerDisplay.Common.Drivers.DDC return observation; } - - private static string FormatError(int? errorCode) => - errorCode.HasValue ? $"0x{unchecked((uint)errorCode.Value):X8}" : "none"; } internal readonly record struct VcpReadAttempt(bool IsSuccess, uint Current, uint Maximum, int ErrorCode)