diff --git a/Directory.Packages.props b/Directory.Packages.props index 950e4e9b93..9cce2118b2 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -89,6 +89,7 @@ + diff --git a/NOTICE.md b/NOTICE.md index a2073e7b20..08a99bafcf 100644 --- a/NOTICE.md +++ b/NOTICE.md @@ -1587,6 +1587,7 @@ SOFTWARE. - NLog.Extensions.Logging - NLog.Schema - OpenAI +- Polly.Core - ReverseMarkdown - ScipBe.Common.Office.OneNote - SharpCompress diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs index 0c9fc598f3..d1ac685a27 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs @@ -8,6 +8,8 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; using ManagedCommon; +using Polly; +using Polly.Retry; using PowerDisplay.Common.Interfaces; using PowerDisplay.Common.Models; using PowerDisplay.Common.Utils; @@ -45,6 +47,42 @@ namespace PowerDisplay.Common.Drivers.DDC /// private const int RetryDelayMs = 100; + /// + /// Retry pipeline for getting capabilities string length (3 retries). + /// + private static readonly ResiliencePipeline CapabilitiesLengthRetryPipeline = + new ResiliencePipelineBuilder() + .AddRetry(new RetryStrategyOptions + { + MaxRetryAttempts = 2, // 2 retries = 3 total attempts + Delay = TimeSpan.FromMilliseconds(RetryDelayMs), + ShouldHandle = new PredicateBuilder().HandleResult(len => len == 0), + OnRetry = static args => + { + Logger.LogWarning($"[Retry] GetCapabilitiesStringLength returned invalid result on attempt {args.AttemptNumber + 1}, retrying..."); + return default; + }, + }) + .Build(); + + /// + /// Retry pipeline for getting capabilities string (5 retries). + /// + private static readonly ResiliencePipeline CapabilitiesStringRetryPipeline = + new ResiliencePipelineBuilder() + .AddRetry(new RetryStrategyOptions + { + MaxRetryAttempts = 4, // 4 retries = 5 total attempts + Delay = TimeSpan.FromMilliseconds(RetryDelayMs), + ShouldHandle = new PredicateBuilder().HandleResult(static str => string.IsNullOrEmpty(str)), + OnRetry = static args => + { + Logger.LogWarning($"[Retry] GetCapabilitiesString returned invalid result on attempt {args.AttemptNumber + 1}, retrying..."); + return default; + }, + }) + .Build(); + private readonly PhysicalMonitorHandleManager _handleManager = new(); private readonly MonitorDiscoveryHelper _discoveryHelper; @@ -142,39 +180,33 @@ namespace PowerDisplay.Common.Drivers.DDC try { // Step 1: Get capabilities string length with retry - var length = RetryHelper.ExecuteWithRetry( - () => + var length = CapabilitiesLengthRetryPipeline.Execute(() => + { + if (GetCapabilitiesStringLength(monitor.Handle, out uint len) && len > 0) { - if (GetCapabilitiesStringLength(monitor.Handle, out uint len) && len > 0) - { - return len; - } + return len; + } - return 0u; - }, - len => len > 0, - maxRetries: 3, - delayMs: RetryDelayMs, - operationName: "GetCapabilitiesStringLength"); + return 0u; + }); if (length == 0) { + Logger.LogWarning("[Retry] GetCapabilitiesStringLength failed after 3 attempts"); return string.Empty; } // Step 2: Get actual capabilities string with retry - var capsString = RetryHelper.ExecuteWithRetry( - () => TryGetCapabilitiesString(monitor.Handle, length), - str => !string.IsNullOrEmpty(str), - maxRetries: 5, - delayMs: RetryDelayMs, - operationName: "GetCapabilitiesString"); + var capsString = CapabilitiesStringRetryPipeline.Execute( + () => TryGetCapabilitiesString(monitor.Handle, length)); if (!string.IsNullOrEmpty(capsString)) { Logger.LogDebug($"Got capabilities string (length: {capsString.Length})"); return capsString; } + + Logger.LogWarning("[Retry] GetCapabilitiesString failed after 5 attempts"); } catch (Exception ex) { diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/PhysicalMonitorHandleManager.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/PhysicalMonitorHandleManager.cs index 20c74eb041..6c430accc9 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/PhysicalMonitorHandleManager.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/PhysicalMonitorHandleManager.cs @@ -3,9 +3,10 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Concurrent; using System.Collections.Generic; +using System.Linq; using ManagedCommon; -using PowerDisplay.Common.Utils; using static PowerDisplay.Common.Drivers.PInvoke; namespace PowerDisplay.Common.Drivers.DDC @@ -16,7 +17,8 @@ namespace PowerDisplay.Common.Drivers.DDC public partial class PhysicalMonitorHandleManager : IDisposable { // Mapping: monitorId -> physical handle (thread-safe) - private readonly LockedDictionary _monitorIdToHandleMap = new(); + private readonly ConcurrentDictionary _monitorIdToHandleMap = new(); + private readonly object _handleLock = new(); private bool _disposed; /// @@ -24,28 +26,28 @@ namespace PowerDisplay.Common.Drivers.DDC /// public void UpdateHandleMap(Dictionary newHandleMap) { - _monitorIdToHandleMap.ExecuteWithLock(dict => + // Lock to ensure atomic update (cleanup + replace) + lock (_handleLock) { // Clean up unused handles before updating - CleanupUnusedHandles(dict, newHandleMap); + CleanupUnusedHandles(newHandleMap); // Update the device key map - dict.Clear(); + _monitorIdToHandleMap.Clear(); foreach (var kvp in newHandleMap) { - dict[kvp.Key] = kvp.Value; + _monitorIdToHandleMap[kvp.Key] = kvp.Value; } - }); + } } /// /// Clean up handles that are no longer in use. - /// Called within ExecuteWithLock context with the internal dictionary. - /// Optimized to O(n) using HashSet lookup instead of O(n*m) nested loops. + /// Called within lock context. Optimized to O(n) using HashSet lookup. /// - private void CleanupUnusedHandles(Dictionary currentHandles, Dictionary newHandles) + private void CleanupUnusedHandles(Dictionary newHandles) { - if (currentHandles.Count == 0) + if (_monitorIdToHandleMap.IsEmpty) { return; } @@ -54,14 +56,9 @@ namespace PowerDisplay.Common.Drivers.DDC var reusedHandles = new HashSet(newHandles.Values); // Find handles to destroy: in old map but not reused (O(n) with O(1) lookup) - var handlesToDestroy = new List(); - foreach (var oldHandle in currentHandles.Values) - { - if (oldHandle != IntPtr.Zero && !reusedHandles.Contains(oldHandle)) - { - handlesToDestroy.Add(oldHandle); - } - } + var handlesToDestroy = _monitorIdToHandleMap.Values + .Where(h => h != IntPtr.Zero && !reusedHandles.Contains(h)) + .ToList(); // Destroy unused handles foreach (var handle in handlesToDestroy) @@ -86,7 +83,7 @@ namespace PowerDisplay.Common.Drivers.DDC } // Release all physical monitor handles - get snapshot to avoid holding lock during cleanup - var handles = _monitorIdToHandleMap.GetValuesSnapshot(); + var handles = _monitorIdToHandleMap.Values.ToList(); foreach (var handle in handles) { if (handle != IntPtr.Zero) diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/PowerDisplay.Lib.csproj b/src/modules/powerdisplay/PowerDisplay.Lib/PowerDisplay.Lib.csproj index 5e488d8deb..d2bb5486b5 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib/PowerDisplay.Lib.csproj +++ b/src/modules/powerdisplay/PowerDisplay.Lib/PowerDisplay.Lib.csproj @@ -23,6 +23,7 @@ false + diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Services/MonitorStateManager.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Services/MonitorStateManager.cs index 9cc7761836..b90d6df4bb 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Services/MonitorStateManager.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Services/MonitorStateManager.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.IO; using System.Text.Json; @@ -23,7 +24,8 @@ namespace PowerDisplay.Common.Services public partial class MonitorStateManager : IDisposable { private readonly string _stateFilePath; - private readonly LockedDictionary _states = new(); + private readonly ConcurrentDictionary _states = new(); + private readonly object _statesLock = new(); private readonly SimpleDebouncer _saveDebouncer; private bool _disposed; @@ -83,39 +85,35 @@ namespace PowerDisplay.Common.Services return; } - bool shouldSave = _states.ExecuteWithLock(dict => + var state = _states.GetOrAdd(hardwareId, _ => new MonitorState()); + + // Update the specific property + bool shouldSave = true; + switch (property) { - // Get or create state entry using HardwareId - if (!dict.TryGetValue(hardwareId, out var state)) - { - state = new MonitorState(); - dict[hardwareId] = state; - } - - // Update the specific property - switch (property) - { - case "Brightness": - state.Brightness = value; - break; - case "ColorTemperature": - state.ColorTemperatureVcp = value; - break; - case "Contrast": - state.Contrast = value; - break; - case "Volume": - state.Volume = value; - break; - default: - Logger.LogWarning($"Unknown property: {property}"); - return false; - } + case "Brightness": + state.Brightness = value; + break; + case "ColorTemperature": + state.ColorTemperatureVcp = value; + break; + case "Contrast": + state.Contrast = value; + break; + case "Volume": + state.Volume = value; + break; + default: + Logger.LogWarning($"Unknown property: {property}"); + shouldSave = false; + break; + } + if (shouldSave) + { // Mark dirty for flush on dispose _isDirty = true; - return true; - }); + } // Schedule debounced save (SimpleDebouncer handles cancellation of previous calls) if (shouldSave) @@ -141,7 +139,7 @@ namespace PowerDisplay.Common.Services return null; } - if (_states.TryGetValue(hardwareId, out var state) && state != null) + if (_states.TryGetValue(hardwareId, out var state)) { return (state.Brightness, state.ColorTemperatureVcp, state.Contrast, state.Volume); } @@ -167,23 +165,20 @@ namespace PowerDisplay.Common.Services if (stateFile?.Monitors != null) { - _states.ExecuteWithLock(dict => + foreach (var kvp in stateFile.Monitors) { - foreach (var kvp in stateFile.Monitors) - { - var monitorKey = kvp.Key; // Should be HardwareId (e.g., "GSM5C6D") - var entry = kvp.Value; + var monitorKey = kvp.Key; // Should be HardwareId (e.g., "GSM5C6D") + var entry = kvp.Value; - dict[monitorKey] = new MonitorState - { - Brightness = entry.Brightness, - ColorTemperatureVcp = entry.ColorTemperatureVcp, - Contrast = entry.Contrast, - Volume = entry.Volume, - CapabilitiesRaw = entry.CapabilitiesRaw, - }; - } - }); + _states[monitorKey] = new MonitorState + { + Brightness = entry.Brightness, + ColorTemperatureVcp = entry.ColorTemperatureVcp, + Contrast = entry.Contrast, + Volume = entry.Volume, + CapabilitiesRaw = entry.CapabilitiesRaw, + }; + } Logger.LogInfo($"[State] Loaded state for {stateFile.Monitors.Count} monitors from {_stateFilePath}"); Logger.LogInfo($"[State] Monitor keys in state file: {string.Join(", ", stateFile.Monitors.Keys)}"); @@ -258,24 +253,21 @@ namespace PowerDisplay.Common.Services LastUpdated = now, }; - _states.ExecuteWithLock(dict => + foreach (var kvp in _states) { - foreach (var kvp in dict) - { - var monitorId = kvp.Key; - var state = kvp.Value; + var monitorId = kvp.Key; + var state = kvp.Value; - stateFile.Monitors[monitorId] = new MonitorStateEntry - { - Brightness = state.Brightness, - ColorTemperatureVcp = state.ColorTemperatureVcp, - Contrast = state.Contrast, - Volume = state.Volume, - CapabilitiesRaw = state.CapabilitiesRaw, - LastUpdated = now, - }; - } - }); + stateFile.Monitors[monitorId] = new MonitorStateEntry + { + Brightness = state.Brightness, + ColorTemperatureVcp = state.ColorTemperatureVcp, + Contrast = state.Contrast, + Volume = state.Volume, + CapabilitiesRaw = state.CapabilitiesRaw, + LastUpdated = now, + }; + } var json = JsonSerializer.Serialize(stateFile, ProfileSerializationContext.Default.MonitorStateFile); return (json, stateFile.Monitors.Count); diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Utils/LockedDictionary.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Utils/LockedDictionary.cs deleted file mode 100644 index 93000dbcdb..0000000000 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Utils/LockedDictionary.cs +++ /dev/null @@ -1,93 +0,0 @@ -// 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; - -#pragma warning disable SA1649 // File name should match first type name (generic class name is valid) - -namespace PowerDisplay.Common.Utils -{ - /// - /// A thread-safe dictionary wrapper that provides atomic operations with minimal locking overhead. - /// Designed for scenarios where simple get/set operations are common but complex transactions are rare. - /// For complex multi-step transactions, use to ensure atomicity. - /// - /// The type of keys in the dictionary. - /// The type of values in the dictionary. - public class LockedDictionary - where TKey : notnull - { - private readonly Dictionary _dictionary = new(); - private readonly object _lock = new(); - - /// - /// Tries to get the value associated with the specified key. - /// - /// The key to locate. - /// When this method returns, contains the value if found; otherwise, the default value. - /// True if the key was found; otherwise, false. - public bool TryGetValue(TKey key, out TValue? value) - { - lock (_lock) - { - return _dictionary.TryGetValue(key, out value); - } - } - - /// - /// Removes all key/value pairs from the dictionary. - /// - public void Clear() - { - lock (_lock) - { - _dictionary.Clear(); - } - } - - /// - /// Gets a snapshot of all values in the dictionary. - /// Returns a copy to ensure thread safety. - /// - /// A list containing copies of all values. - public List GetValuesSnapshot() - { - lock (_lock) - { - return new List(_dictionary.Values); - } - } - - /// - /// Executes an action within the lock, providing the internal dictionary for complex operations. - /// Use this for multi-step transactions that need to be atomic. - /// WARNING: Do not store or return references to the dictionary outside the action. - /// - /// The action to execute with the dictionary. - public void ExecuteWithLock(Action> action) - { - lock (_lock) - { - action(_dictionary); - } - } - - /// - /// Executes a function within the lock, providing the internal dictionary for complex operations. - /// Use this for multi-step transactions that need to be atomic and return a result. - /// WARNING: Do not store or return references to the dictionary outside the function. - /// - /// The type of the result. - /// The function to execute with the dictionary. - /// The result of the function. - public TResult ExecuteWithLock(Func, TResult> func) - { - lock (_lock) - { - return func(_dictionary); - } - } - } -} diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Utils/MonitorFeatureHelper.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Utils/MonitorFeatureHelper.cs new file mode 100644 index 0000000000..a470a47d43 --- /dev/null +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Utils/MonitorFeatureHelper.cs @@ -0,0 +1,112 @@ +// 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 System.Globalization; +using PowerDisplay.Common.Drivers; + +namespace PowerDisplay.Common.Utils +{ + /// + /// Unified helper class for parsing monitor feature support from VCP capabilities. + /// This eliminates duplicate VCP parsing logic across PowerDisplay.exe and Settings.UI. + /// + public static class MonitorFeatureHelper + { + /// + /// Result of parsing monitor feature support from VCP capabilities + /// + public readonly struct FeatureSupportResult + { + public bool SupportsBrightness { get; init; } + + public bool SupportsContrast { get; init; } + + public bool SupportsColorTemperature { get; init; } + + public bool SupportsVolume { get; init; } + + public bool SupportsInputSource { get; init; } + + public string CapabilitiesStatus { get; init; } + + public static FeatureSupportResult Unavailable => new() + { + SupportsBrightness = false, + SupportsContrast = false, + SupportsColorTemperature = false, + SupportsVolume = false, + SupportsInputSource = false, + CapabilitiesStatus = "unavailable", + }; + } + + /// + /// Parse feature support from a list of VCP code strings. + /// This is the single source of truth for determining monitor capabilities. + /// + /// List of VCP codes as strings (e.g., "0x10", "10", "0x12") + /// Raw capabilities string, used to determine availability status + /// Feature support result + public static FeatureSupportResult ParseFeatureSupport(IReadOnlyList? vcpCodes, string? capabilitiesRaw) + { + // Check capabilities availability + if (string.IsNullOrEmpty(capabilitiesRaw)) + { + return FeatureSupportResult.Unavailable; + } + + // Convert all VCP codes to integers for comparison + var vcpCodeInts = ParseVcpCodesToIntegers(vcpCodes); + + // Determine feature support based on VCP codes + return new FeatureSupportResult + { + SupportsBrightness = vcpCodeInts.Contains(NativeConstants.VcpCodeBrightness), + SupportsContrast = vcpCodeInts.Contains(NativeConstants.VcpCodeContrast), + SupportsColorTemperature = vcpCodeInts.Contains(NativeConstants.VcpCodeSelectColorPreset), + SupportsVolume = vcpCodeInts.Contains(NativeConstants.VcpCodeVolume), + SupportsInputSource = vcpCodeInts.Contains(NativeConstants.VcpCodeInputSource), + CapabilitiesStatus = "available", + }; + } + + /// + /// Parse VCP codes from string list to integer set + /// Handles both hex formats: "0x10" and "10" + /// + private static HashSet ParseVcpCodesToIntegers(IReadOnlyList? vcpCodes) + { + var result = new HashSet(); + + if (vcpCodes == null) + { + return result; + } + + foreach (var code in vcpCodes) + { + if (string.IsNullOrWhiteSpace(code)) + { + continue; + } + + // Remove "0x" prefix if present and parse as hex + var cleanCode = code.Trim(); + if (cleanCode.StartsWith("0x", StringComparison.OrdinalIgnoreCase)) + { + cleanCode = cleanCode.Substring(2); + } + + if (int.TryParse(cleanCode, NumberStyles.HexNumber, CultureInfo.InvariantCulture, out int codeInt)) + { + result.Add(codeInt); + } + } + + return result; + } + } +} diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Utils/RetryHelper.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Utils/RetryHelper.cs deleted file mode 100644 index 193ce87e5a..0000000000 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Utils/RetryHelper.cs +++ /dev/null @@ -1,99 +0,0 @@ -// 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.Threading; -using ManagedCommon; - -namespace PowerDisplay.Common.Utils -{ - /// - /// Helper class for executing operations with retry logic. - /// Provides a unified retry pattern for DDC/CI operations that may fail intermittently. - /// - public static class RetryHelper - { - /// - /// Default delay between retry attempts in milliseconds. - /// - public const int DefaultRetryDelayMs = 100; - - /// - /// Default maximum number of retry attempts. - /// - public const int DefaultMaxRetries = 3; - - /// - /// Executes a synchronous operation with retry logic. - /// - /// The return type of the operation. - /// The operation to execute. - /// Predicate to determine if the result is valid. - /// Maximum number of retry attempts (default: 3). - /// Delay between retries in milliseconds (default: 100). - /// Optional name for logging purposes. - /// The result of the operation, or default if all retries failed. - public static T? ExecuteWithRetry( - Func operation, - Func isValid, - int maxRetries = DefaultMaxRetries, - int delayMs = DefaultRetryDelayMs, - string? operationName = null) - { - for (int attempt = 0; attempt < maxRetries; attempt++) - { - try - { - var result = operation(); - - if (isValid(result)) - { - if (attempt > 0 && !string.IsNullOrEmpty(operationName)) - { - Logger.LogDebug($"[Retry] {operationName} succeeded on attempt {attempt + 1}"); - } - - return result; - } - - if (attempt < maxRetries - 1) - { - if (!string.IsNullOrEmpty(operationName)) - { - Logger.LogWarning($"[Retry] {operationName} returned invalid result on attempt {attempt + 1}, retrying..."); - } - - Thread.Sleep(delayMs); - } - } - catch (Exception ex) - { - if (attempt < maxRetries - 1) - { - if (!string.IsNullOrEmpty(operationName)) - { - Logger.LogWarning($"[Retry] {operationName} failed on attempt {attempt + 1}: {ex.Message}, retrying..."); - } - - Thread.Sleep(delayMs); - } - else - { - if (!string.IsNullOrEmpty(operationName)) - { - Logger.LogWarning($"[Retry] {operationName} failed after {maxRetries} attempts: {ex.Message}"); - } - } - } - } - - if (!string.IsNullOrEmpty(operationName)) - { - Logger.LogWarning($"[Retry] {operationName} failed after {maxRetries} attempts"); - } - - return default; - } - } -} diff --git a/src/settings-ui/Settings.UI/ViewModels/PowerDisplayViewModel.cs b/src/settings-ui/Settings.UI/ViewModels/PowerDisplayViewModel.cs index e9ddd7d1bb..35117f9dcc 100644 --- a/src/settings-ui/Settings.UI/ViewModels/PowerDisplayViewModel.cs +++ b/src/settings-ui/Settings.UI/ViewModels/PowerDisplayViewModel.cs @@ -565,7 +565,7 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels private ObservableCollection _profiles = new ObservableCollection(); /// - /// Collection of available profiles (for button display) + /// Gets or sets collection of available profiles (for button display) /// public ObservableCollection Profiles {