Add class to support reacting to power state changes - #809
Conversation
📝 WalkthroughWalkthroughA new PowerMonitor utility is introduced to monitor system power events via IOKit. It includes a public enum defining power event types, a factory-created class using PassKey pattern for construction, IOKit integration for system power notifications, and comprehensive unit tests. Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant PM as PowerMonitor::Create
participant IOKit as IOKit/System
participant DQ as Dispatch Queue
participant Callback as Power Event Callback
User->>PM: Create(PowerEventBlock callback)
PM->>IOKit: IORegisterForSystemPower()
IOKit->>PM: Returns io_connect_t, IONotificationPortRef, io_object_t
PM->>DQ: CFRunLoopAddSource(notification_port)
PM->>User: Returns std::unique_ptr<PowerMonitor>
IOKit->>PM: PowerCallback(power_message)
PM->>PM: HandlePowerEvent(message_type)
PM->>IOKit: IOAllowPowerChange(if needed)
PM->>DQ: async dispatch to queue
DQ->>Callback: Invoke with PowerEvent enum
Callback->>User: Power state notification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Source/common/PowerMonitor.mm (2)
97-103:IOAllowPowerChangeis called after the user callback — a slow callback delays the power transition.For both
kIOMessageCanSystemSleepandkIOMessageSystemWillSleep, the callback runs beforeIOAllowPowerChange. If the callback performs expensive work (disk flush, network call, etc.), it delays the kernel's power state transition for that duration. This is probably intentional (giving the caller time to prepare), but it's worth documenting that the callback should complete promptly, or the system sleep can be delayed (the kernel has a ~30 s timeout forkIOMessageCanSystemSleepresponses).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/common/PowerMonitor.mm` around lines 97 - 103, The power-change acknowledgement is currently issued after invoking callback_ for kIOMessageCanSystemSleep and kIOMessageSystemWillSleep which lets a slow callback delay the kernel power transition; move the IOAllowPowerChange(connect_, reinterpret_cast<long>(message_argument)) call to occur immediately when handling both kIOMessageCanSystemSleep and kIOMessageSystemWillSleep (before calling callback_), and add a brief comment near callback_ and PowerEvent noting that callbacks must return promptly (they may block system sleep / have kernel timeouts).
95-110:kIOMessageCanSystemSleepalways allows sleep — intentional?Currently,
HandlePowerEventunconditionally callsIOAllowPowerChangeforkIOMessageCanSystemSleep. This means the callback is notified but cannot deny sleep (i.e.,IOCancelPowerChangeis never called). If that's a deliberate simplification, a brief comment would be helpful for future maintainers. If callers may eventually need to veto idle sleep, thePowerEventBlocksignature would need to return a decision value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/common/PowerMonitor.mm` around lines 95 - 110, HandlePowerEvent currently unconditionally calls IOAllowPowerChange for kIOMessageCanSystemSleep so callers cannot veto sleep; either document this deliberate behavior with a clear comment in HandlePowerEvent next to the kIOMessageCanSystemSleep case, or change the PowerEventBlock (callback_) signature to return a decision (e.g., bool) and update HandlePowerEvent to call IOAllowPowerChange when callback_ returns true and IOCancelPowerChange when it returns false (adjust all call sites and tests to the new PowerEventBlock signature).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Source/common/PowerMonitor.mm`:
- Around line 97-103: The power-change acknowledgement is currently issued after
invoking callback_ for kIOMessageCanSystemSleep and kIOMessageSystemWillSleep
which lets a slow callback delay the kernel power transition; move the
IOAllowPowerChange(connect_, reinterpret_cast<long>(message_argument)) call to
occur immediately when handling both kIOMessageCanSystemSleep and
kIOMessageSystemWillSleep (before calling callback_), and add a brief comment
near callback_ and PowerEvent noting that callbacks must return promptly (they
may block system sleep / have kernel timeouts).
- Around line 95-110: HandlePowerEvent currently unconditionally calls
IOAllowPowerChange for kIOMessageCanSystemSleep so callers cannot veto sleep;
either document this deliberate behavior with a clear comment in
HandlePowerEvent next to the kIOMessageCanSystemSleep case, or change the
PowerEventBlock (callback_) signature to return a decision (e.g., bool) and
update HandlePowerEvent to call IOAllowPowerChange when callback_ returns true
and IOCancelPowerChange when it returns false (adjust all call sites and tests
to the new PowerEventBlock signature).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
Source/common/BUILDSource/common/PowerMonitor.hSource/common/PowerMonitor.mmSource/common/PowerMonitorTest.mm
Allow callers to provide a block that gets called during power state changes in order to perform some operation.
Part of SNT-261