diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs index 17af0ee0c9..285d0811d4 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs @@ -181,7 +181,7 @@ namespace PowerDisplay.Common.Drivers.DDC if (DdcCiNative.TryGetVCPFeature(monitor.Handle, VcpCodeSelectColorPreset, out uint current, out uint max)) { var presetName = VcpValueNames.GetFormattedName(0x14, (int)current); - Logger.LogInfo($"[{monitor.Id}] Color temperature via 0x14: {presetName}"); + Logger.LogDebug($"[{monitor.Id}] Color temperature via 0x14: {presetName}"); return new BrightnessInfo((int)current, 0, (int)max); } @@ -266,7 +266,7 @@ namespace PowerDisplay.Common.Drivers.DDC if (DdcCiNative.TryGetVCPFeature(monitor.Handle, VcpCodeInputSource, out uint current, out uint max)) { var sourceName = VcpValueNames.GetFormattedName(0x60, (int)current); - Logger.LogInfo($"[{monitor.Id}] Input source via 0x60: {sourceName}"); + Logger.LogDebug($"[{monitor.Id}] Input source via 0x60: {sourceName}"); return new BrightnessInfo((int)current, 0, (int)max); } @@ -322,7 +322,7 @@ namespace PowerDisplay.Common.Drivers.DDC var verifyName = VcpValueNames.GetFormattedName(0x60, (int)verifyValue); if (verifyValue == (uint)inputSource) { - Logger.LogInfo($"[{monitor.Id}] Input source verified: {verifyName} (0x{verifyValue:X2})"); + Logger.LogDebug($"[{monitor.Id}] Input source verified: {verifyName} (0x{verifyValue:X2})"); } else { @@ -409,7 +409,7 @@ namespace PowerDisplay.Common.Drivers.DDC if (!string.IsNullOrEmpty(capsString)) { - Logger.LogInfo($"Got capabilities string (length: {capsString.Length})"); + Logger.LogDebug($"Got capabilities string (length: {capsString.Length})"); return capsString; } } diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Services/ProfileService.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Services/ProfileService.cs index 8d5f3d57a8..53d8f7163a 100644 --- a/src/modules/powerdisplay/PowerDisplay.Lib/Services/ProfileService.cs +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Services/ProfileService.cs @@ -16,8 +16,21 @@ namespace PowerDisplay.Common.Services /// Centralized service for managing PowerDisplay profiles storage and retrieval. /// Provides unified access to profile data for PowerDisplay, Settings UI, and LightSwitch modules. /// Thread-safe and AOT-compatible. - /// Implements for dependency injection support. /// + /// + /// Design Note: This class provides both static and instance-based access patterns: + /// + /// Static methods: Direct access for backward compatibility with existing code. Thread-safe with internal locking. + /// Instance methods (via ): For dependency injection consumers. Delegates to static methods internally. + /// + /// Usage Guidelines: + /// + /// New code should prefer DI: serviceProvider.GetService<IProfileService>() + /// Or use the singleton: ProfileService.Instance.LoadProfiles() + /// Static methods remain available for backward compatibility + /// + /// All access patterns are thread-safe and share the same underlying lock. + /// public class ProfileService : IProfileService { private const string LogPrefix = "[ProfileService]"; diff --git a/src/modules/powerdisplay/PowerDisplay.Lib/Utils/EventHelper.cs b/src/modules/powerdisplay/PowerDisplay.Lib/Utils/EventHelper.cs new file mode 100644 index 0000000000..3d8a8330da --- /dev/null +++ b/src/modules/powerdisplay/PowerDisplay.Lib/Utils/EventHelper.cs @@ -0,0 +1,47 @@ +// 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 Windows named event operations. + /// Provides unified event signaling with consistent error handling and logging. + /// + public static class EventHelper + { + /// + /// Signals a named event. Creates the event if it doesn't exist. + /// + /// The name of the event to signal. + /// True if the event was signaled successfully, false otherwise. + public static bool SignalEvent(string eventName) + { + if (string.IsNullOrEmpty(eventName)) + { + Logger.LogWarning("[EventHelper] SignalEvent called with null or empty event name"); + return false; + } + + try + { + using var eventHandle = new EventWaitHandle( + false, + EventResetMode.AutoReset, + eventName); + eventHandle.Set(); + Logger.LogDebug($"[EventHelper] Signaled event: {eventName}"); + return true; + } + catch (Exception ex) + { + Logger.LogError($"[EventHelper] Failed to signal event '{eventName}': {ex.Message}"); + return false; + } + } + } +} diff --git a/src/modules/powerdisplay/PowerDisplay/Configuration/AppConstants.cs b/src/modules/powerdisplay/PowerDisplay/Configuration/AppConstants.cs index 42b92ac799..4759b3bbe4 100644 --- a/src/modules/powerdisplay/PowerDisplay/Configuration/AppConstants.cs +++ b/src/modules/powerdisplay/PowerDisplay/Configuration/AppConstants.cs @@ -34,5 +34,60 @@ namespace PowerDisplay.Configuration /// public const string ExternalMonitorGlyph = "\uE7F4"; } + + /// + /// DDC/CI protocol constants + /// + public static class Ddc + { + /// + /// Retry delay between DDC/CI operations in milliseconds + /// + public const int RetryDelayMs = 100; + + /// + /// Maximum number of retries for DDC/CI operations + /// + public const int MaxRetries = 3; + + /// + /// Timeout for getting monitor capabilities in milliseconds + /// + public const int CapabilitiesTimeoutMs = 5000; + + // VCP Codes + /// VCP code for Brightness (0x10) + public const byte VcpBrightness = 0x10; + + /// VCP code for Contrast (0x12) + public const byte VcpContrast = 0x12; + + /// VCP code for Select Color Preset / Color Temperature (0x14) + public const byte VcpColorTemperature = 0x14; + + /// VCP code for Input Source (0x60) + public const byte VcpInputSource = 0x60; + } + + /// + /// Process synchronization constants + /// + public static class Process + { + /// + /// Timeout for waiting for process ready signal in milliseconds + /// + public const int StartupTimeoutMs = 5000; + + /// + /// Polling interval when waiting for process ready in milliseconds + /// + public const int ReadyPollIntervalMs = 100; + + /// + /// Fallback delay when event-based synchronization fails in milliseconds + /// + public const int FallbackDelayMs = 500; + } } } diff --git a/src/modules/powerdisplay/PowerDisplay/PowerDisplayXAML/App.xaml.cs b/src/modules/powerdisplay/PowerDisplay/PowerDisplayXAML/App.xaml.cs index acaf953ddc..6ce39c7ef8 100644 --- a/src/modules/powerdisplay/PowerDisplay/PowerDisplayXAML/App.xaml.cs +++ b/src/modules/powerdisplay/PowerDisplay/PowerDisplayXAML/App.xaml.cs @@ -26,6 +26,12 @@ namespace PowerDisplay public partial class App : Application #pragma warning restore CA1001 { + /// + /// Event name for signaling that PowerDisplay process is ready. + /// Must match the constant in C++ PowerDisplayModuleInterface. + /// + private const string ProcessReadyEventName = "Local\\PowerToys_PowerDisplay_Ready"; + private readonly ISettingsUtils _settingsUtils = new SettingsUtils(); private Window? _mainWindow; private int _powerToysRunnerPid; @@ -106,6 +112,10 @@ namespace PowerDisplay RegisterViewModelEvent(Constants.ApplyColorTemperaturePowerDisplayEvent(), vm => vm.ApplyColorTemperatureFromSettings(), "ApplyColorTemperature"); RegisterViewModelEvent(Constants.ApplyProfilePowerDisplayEvent(), vm => vm.ApplyProfileFromSettings(), "ApplyProfile"); + // Signal that process is ready to receive events + // This allows the C++ module to wait for initialization instead of using hardcoded Sleep + SignalProcessReady(); + // Monitor Runner process (backup exit mechanism) if (_powerToysRunnerPid > 0) { @@ -354,5 +364,26 @@ namespace PowerDisplay _trayIconService?.Destroy(); Environment.Exit(0); } + + /// + /// Signal that PowerDisplay process is ready to receive events. + /// Uses a ManualReset event so the C++ module can wait on it. + /// + private static void SignalProcessReady() + { + try + { + using var readyEvent = new EventWaitHandle( + false, + EventResetMode.ManualReset, + ProcessReadyEventName); + readyEvent.Set(); + Logger.LogInfo("Signaled process ready event"); + } + catch (Exception ex) + { + Logger.LogError($"Failed to signal process ready event: {ex.Message}"); + } + } } } diff --git a/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.Monitors.cs b/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.Monitors.cs index ebbe0375d4..02877f58de 100644 --- a/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.Monitors.cs +++ b/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.Monitors.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Threading.Tasks; using ManagedCommon; using Microsoft.PowerToys.Settings.UI.Library; @@ -19,7 +20,7 @@ namespace PowerDisplay.ViewModels; /// public partial class MainViewModel { - private async Task InitializeAsync() + private async Task InitializeAsync(CancellationToken cancellationToken = default) { try { @@ -27,7 +28,7 @@ public partial class MainViewModel IsScanning = true; // Discover monitors - var monitors = await _monitorManager.DiscoverMonitorsAsync(_cancellationTokenSource.Token); + var monitors = await _monitorManager.DiscoverMonitorsAsync(cancellationToken); // Update UI on the dispatcher thread _dispatcherQueue.TryEnqueue(() => diff --git a/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.Settings.cs b/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.Settings.cs index 69d3c138f0..c0df41e50e 100644 --- a/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.Settings.cs +++ b/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.Settings.cs @@ -10,6 +10,7 @@ using ManagedCommon; using Microsoft.PowerToys.Settings.UI.Library; using PowerDisplay.Common.Models; using PowerDisplay.Common.Services; +using PowerDisplay.Common.Utils; using PowerDisplay.Serialization; using PowerToys.Interop; @@ -621,21 +622,8 @@ public partial class MainViewModel /// private void SignalMonitorsRefreshEvent() { - try - { - using (var eventHandle = new System.Threading.EventWaitHandle( - false, - System.Threading.EventResetMode.AutoReset, - Constants.RefreshPowerDisplayMonitorsEvent())) - { - eventHandle.Set(); - Logger.LogInfo("Signaled refresh monitors event to Settings UI"); - } - } - catch (Exception ex) - { - Logger.LogError($"Failed to signal refresh monitors event: {ex.Message}"); - } + EventHelper.SignalEvent(Constants.RefreshPowerDisplayMonitorsEvent()); + Logger.LogInfo("Signaled refresh monitors event to Settings UI"); } /// diff --git a/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.cs b/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.cs index 5d604bdab8..6f43b786c2 100644 --- a/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.cs +++ b/src/modules/powerdisplay/PowerDisplay/ViewModels/MainViewModel.cs @@ -80,7 +80,7 @@ public partial class MainViewModel : INotifyPropertyChanged, IDisposable LoadProfiles(); // Start initial discovery - _ = InitializeAsync(); + _ = InitializeAsync(_cancellationTokenSource.Token); } public ObservableCollection Monitors diff --git a/src/modules/powerdisplay/PowerDisplay/ViewModels/MonitorViewModel.cs b/src/modules/powerdisplay/PowerDisplay/ViewModels/MonitorViewModel.cs index 145015f034..0770a12913 100644 --- a/src/modules/powerdisplay/PowerDisplay/ViewModels/MonitorViewModel.cs +++ b/src/modules/powerdisplay/PowerDisplay/ViewModels/MonitorViewModel.cs @@ -404,6 +404,13 @@ public partial class MonitorViewModel : INotifyPropertyChanged, IDisposable /// Orientation: 0=normal, 1=90°, 2=180°, 3=270° public async Task SetRotationAsync(int orientation) { + // Validate orientation range (0=normal, 1=90°, 2=180°, 3=270°) + if (orientation < 0 || orientation > 3) + { + Logger.LogWarning($"[{HardwareId}] Invalid rotation value: {orientation}. Must be 0-3."); + return; + } + // If already at this orientation, do nothing if (CurrentRotation == orientation) { diff --git a/src/modules/powerdisplay/PowerDisplayModuleInterface/Constants.h b/src/modules/powerdisplay/PowerDisplayModuleInterface/Constants.h index 007ff97a32..4d19dae4ca 100644 --- a/src/modules/powerdisplay/PowerDisplayModuleInterface/Constants.h +++ b/src/modules/powerdisplay/PowerDisplayModuleInterface/Constants.h @@ -4,4 +4,9 @@ namespace PowerDisplayConstants { // Name of the powertoy module. inline const std::wstring ModuleKey = L"PowerDisplay"; + + // Process synchronization constants + inline const wchar_t* ProcessReadyEventName = L"Local\\PowerToys_PowerDisplay_Ready"; + constexpr DWORD ProcessReadyTimeoutMs = 5000; + constexpr DWORD FallbackDelayMs = 500; } \ No newline at end of file diff --git a/src/modules/powerdisplay/PowerDisplayModuleInterface/dllmain.cpp b/src/modules/powerdisplay/PowerDisplayModuleInterface/dllmain.cpp index a691cd84cf..e23ba1d05a 100644 --- a/src/modules/powerdisplay/PowerDisplayModuleInterface/dllmain.cpp +++ b/src/modules/powerdisplay/PowerDisplayModuleInterface/dllmain.cpp @@ -185,6 +185,38 @@ private: } } + // Helper method to wait for PowerDisplay.exe process to be ready + // Uses a named event for precise synchronization instead of hardcoded Sleep + void wait_for_process_ready() + { + HANDLE readyEvent = OpenEventW(SYNCHRONIZE, FALSE, PowerDisplayConstants::ProcessReadyEventName); + if (readyEvent) + { + Logger::trace(L"Waiting for PowerDisplay process ready signal..."); + DWORD waitResult = WaitForSingleObject(readyEvent, PowerDisplayConstants::ProcessReadyTimeoutMs); + CloseHandle(readyEvent); + + if (waitResult == WAIT_OBJECT_0) + { + Logger::trace(L"PowerDisplay process ready signal received"); + } + else if (waitResult == WAIT_TIMEOUT) + { + Logger::warn(L"PowerDisplay process ready timeout after {}ms", PowerDisplayConstants::ProcessReadyTimeoutMs); + } + else + { + Logger::warn(L"WaitForSingleObject failed with error: {}", GetLastError()); + } + } + else + { + // Fallback: if cannot open event, use a shorter delay + Logger::warn(L"Could not open PowerDisplay ready event, using fallback delay"); + Sleep(PowerDisplayConstants::FallbackDelayMs); + } + } + public: PowerDisplayModule() { @@ -332,9 +364,8 @@ public: { Logger::trace(L"PowerDisplay process not running, launching before applying color temperature"); launch_process(); - // Wait for process to initialize and register event listeners - // This prevents race condition where event is set before process is ready - Sleep(1000); + // Wait for process to signal ready + wait_for_process_ready(); } if (m_hApplyColorTemperatureEvent) @@ -364,8 +395,8 @@ public: { Logger::trace(L"PowerDisplay process not running, launching before applying profile"); launch_process(); - // Wait for process to initialize and register event listeners - Sleep(1000); + // Wait for process to signal ready + wait_for_process_ready(); } if (m_hApplyProfileEvent) diff --git a/src/settings-ui/Settings.UI/ViewModels/PowerDisplayViewModel.cs b/src/settings-ui/Settings.UI/ViewModels/PowerDisplayViewModel.cs index a27a46a77f..a236b092d0 100644 --- a/src/settings-ui/Settings.UI/ViewModels/PowerDisplayViewModel.cs +++ b/src/settings-ui/Settings.UI/ViewModels/PowerDisplayViewModel.cs @@ -142,11 +142,7 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels { // Explicitly signal PowerDisplay to refresh tray icon // This is needed because set_config() doesn't signal SettingsUpdatedEvent to avoid UI refresh issues - using var eventHandle = new System.Threading.EventWaitHandle( - false, - System.Threading.EventResetMode.AutoReset, - Constants.SettingsUpdatedPowerDisplayEvent()); - eventHandle.Set(); + EventHelper.SignalEvent(Constants.SettingsUpdatedPowerDisplayEvent()); Logger.LogInfo($"ShowSystemTrayIcon changed to {value}, signaled SettingsUpdatedPowerDisplayEvent"); } } @@ -396,19 +392,8 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels /// private void SignalSettingsUpdated() { - try - { - using var eventHandle = new System.Threading.EventWaitHandle( - false, - System.Threading.EventResetMode.AutoReset, - Constants.SettingsUpdatedPowerDisplayEvent()); - eventHandle.Set(); - Logger.LogInfo("Signaled SettingsUpdatedPowerDisplayEvent for feature visibility change"); - } - catch (Exception ex) - { - Logger.LogError($"Failed to signal SettingsUpdatedPowerDisplayEvent: {ex.Message}"); - } + EventHelper.SignalEvent(Constants.SettingsUpdatedPowerDisplayEvent()); + Logger.LogInfo("Signaled SettingsUpdatedPowerDisplayEvent for feature visibility change"); } public void Launch() @@ -513,7 +498,8 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels for (int i = Monitors.Count - 1; i >= 0; i--) { var existingMonitor = Monitors[i]; - if (updatedMonitorsDict.TryGetValue(existingMonitor.InternalName, out var updatedMonitor)) + if (updatedMonitorsDict.TryGetValue(existingMonitor.InternalName, out var updatedMonitor) + && updatedMonitor != null) { // Monitor still exists - update its properties in place Logger.LogInfo($"[ReloadMonitors] Updating existing monitor: {existingMonitor.InternalName}"); @@ -661,13 +647,7 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels profile.Name)); // Signal PowerDisplay to apply profile - using (var eventHandle = new System.Threading.EventWaitHandle( - false, - System.Threading.EventResetMode.AutoReset, - Constants.ApplyProfilePowerDisplayEvent())) - { - eventHandle.Set(); - } + EventHelper.SignalEvent(Constants.ApplyProfilePowerDisplayEvent()); Logger.LogInfo($"Profile '{profile.Name}' applied successfully"); }