Skip to content

feat: recover power mode after sleep/shutdown/reboot#208

Open
xionglinlin wants to merge 1 commit into
linuxdeepin:masterfrom
xionglinlin:master
Open

feat: recover power mode after sleep/shutdown/reboot#208
xionglinlin wants to merge 1 commit into
linuxdeepin:masterfrom
xionglinlin:master

Conversation

@xionglinlin
Copy link
Copy Markdown
Contributor

When performing shutdown, reboot, hibernate or suspend operations, switch to performance mode before the operation, and restore the user's configured power mode upon wake or next startup. This ensures smooth transitions and optimal performance during these critical operations.

Key changes:

  1. Add setTlpMode() helper function to set power mode via D-Bus
  2. Add recoverySystemPowerMode() to restore user's configured power mode after wake
  3. Call setTlpMode(Performance) before shutdown, reboot, hibernate, and suspend
  4. Connect to PrepareForSleep signal to restore mode after sleep wake
  5. On startup, schedule recovery of power mode via timer

Log: Optimized power management behavior during system suspend/ hibernate/reboot/shutdown

Influence:

  1. Test shutdown: verify system switches to performance mode before shutdown
  2. Test reboot: verify performance mode is set before reboot
  3. Test hibernate: verify performance mode is set before hibernation
  4. Test suspend: verify performance mode is set before suspend
  5. Test wake from sleep: verify user's original power mode is restored
  6. Test startup: verify power mode is restored to user's configuration
  7. Test with/without battery: verify low battery mode handling on battery ≤20%
  8. Verify D-Bus communication with org.deepin.dde.Power1 service
  9. Test error handling when D-Bus calls fail

feat: 恢复休眠/关机/重启后的电源模式

在执行关机、重启、休眠或挂起操作前,先将系统切换到高性能模式,操作完成后
(唤醒或下次启动)再恢复为用户之前配置的电源模式。这确保了这些关键操作期
间的流畅过渡和最佳性能。

主要变更:

  1. 添加 setTlpMode() 辅助函数,通过 D-Bus 设置电源模式
  2. 添加 recoverySystemPowerMode() 恢复用户配置的电源模式
  3. 在关机、重启、休眠和挂起前调用 setTlpMode(Performance)
  4. 连接 PrepareForSleep 信号,在睡眠唤醒后恢复模式
  5. 在启动时通过定时器调度恢复电源模式

Log: 优化系统挂起/休眠/重启/关机时的电源管理行为

Influence:

  1. 测试关机:验证关机前是否切换为高性能模式
  2. 测试重启:验证重启前是否设置为高性能模式
  3. 测试休眠:验证休眠前是否设置为高性能模式
  4. 测试挂起:验证挂起前是否设置为高性能模式
  5. 测试从睡眠唤醒:验证是否恢复用户原始电源模式
  6. 测试启动:验证电源模式是否恢复为用户配置
  7. 测试有/无电池情况:验证电池电量≤20%时的低电量模式处理
  8. 验证与 org.deepin.dde.Power1 服务的 D-Bus 通信
  9. 测试 D-Bus 调用失败时的错误处理

PMS: TASK-389737
Change-Id: I30122c303b2b61a4912a554ccfb0d75dce3747a1

When performing shutdown, reboot, hibernate or suspend operations,
switch to performance mode before the operation, and restore the user's
configured power mode upon wake or next startup. This ensures smooth
transitions and optimal performance during these critical operations.

Key changes:
1. Add `setTlpMode()` helper function to set power mode via D-Bus
2. Add `recoverySystemPowerMode()` to restore user's configured power
mode after wake
3. Call `setTlpMode(Performance)` before shutdown, reboot, hibernate,
and suspend
4. Connect to `PrepareForSleep` signal to restore mode after sleep wake
5. On startup, schedule recovery of power mode via timer

Log: Optimized power management behavior during system suspend/
hibernate/reboot/shutdown

Influence:
1. Test shutdown: verify system switches to performance mode before
shutdown
2. Test reboot: verify performance mode is set before reboot
3. Test hibernate: verify performance mode is set before hibernation
4. Test suspend: verify performance mode is set before suspend
5. Test wake from sleep: verify user's original power mode is restored
6. Test startup: verify power mode is restored to user's configuration
7. Test with/without battery: verify low battery mode handling on
battery ≤20%
8. Verify D-Bus communication with org.deepin.dde.Power1 service
9. Test error handling when D-Bus calls fail

feat: 恢复休眠/关机/重启后的电源模式

在执行关机、重启、休眠或挂起操作前,先将系统切换到高性能模式,操作完成后
(唤醒或下次启动)再恢复为用户之前配置的电源模式。这确保了这些关键操作期
间的流畅过渡和最佳性能。

主要变更:
1. 添加 `setTlpMode()` 辅助函数,通过 D-Bus 设置电源模式
2. 添加 `recoverySystemPowerMode()` 恢复用户配置的电源模式
3. 在关机、重启、休眠和挂起前调用 `setTlpMode(Performance)`
4. 连接 `PrepareForSleep` 信号,在睡眠唤醒后恢复模式
5. 在启动时通过定时器调度恢复电源模式

Log: 优化系统挂起/休眠/重启/关机时的电源管理行为

Influence:
1. 测试关机:验证关机前是否切换为高性能模式
2. 测试重启:验证重启前是否设置为高性能模式
3. 测试休眠:验证休眠前是否设置为高性能模式
4. 测试挂起:验证挂起前是否设置为高性能模式
5. 测试从睡眠唤醒:验证是否恢复用户原始电源模式
6. 测试启动:验证电源模式是否恢复为用户配置
7. 测试有/无电池情况:验证电池电量≤20%时的低电量模式处理
8. 验证与 org.deepin.dde.Power1 服务的 D-Bus 通信
9. 测试 D-Bus 调用失败时的错误处理

PMS: TASK-389737
Change-Id: I30122c303b2b61a4912a554ccfb0d75dce3747a1
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @xionglinlin, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff代码。这段代码主要是在会话管理器中增加了对系统电源模式(TLP Mode)的控制,特别是在休眠、重启、关机等操作前将模式切换为高性能,并在唤醒或初始化时尝试恢复之前的电源模式。

整体逻辑思路是清晰的,但在代码性能、逻辑严谨性、代码安全性和代码质量方面存在一些需要改进的地方。以下是详细的审查意见:

1. 代码性能

  • 严重阻塞主线程: recoverySystemPowerMode() 函数中连续使用了三次 inter.call("Get", ...) 进行同步DBus调用。DBus的同步调用会阻塞当前线程等待远程服务响应,而 SessionManager 通常运行在主线程(UI线程)中,连续三次同步调用会导致主线程卡顿,甚至引发ANR(应用无响应)。
    • 改进意见: 应当使用 asyncCall 替代 call,并通过 QDBusPendingCallWatcher 监听异步返回结果;或者如果底层服务支持,直接通过 org.freedesktop.DBus.PropertiesGetAll 方法一次性获取所有属性,减少IPC往返次数。
  • 重复构造 DBus 接口: setTlpModerecoverySystemPowerMode 每次调用都会创建一个新的 QDBusInterface 对象,这涉及服务名解析、规则匹配等开销。
    • 改进意见: 考虑将 org.deepin.dde.Power1 的DBus Interface作为 SessionManager 的成员变量(类似代码中已有的 m_login1ManagerInter),在初始化时创建一次,后续复用。

2. 语法与逻辑

  • 休眠/关机前设置高性能模式的逻辑存疑:RequestSuspendRequestHibernateRequestRebootRequestShutdown 中,代码在执行操作前调用了 setTlpMode(Performance)
    • 对于 Suspend/Hibernate:系统即将进入睡眠,硬件很快会断电或进入低功耗状态,此时将电源模式切为高性能意义不大,反而可能引起短暂的功耗飙升。
    • 对于 Reboot/Shutdown:系统即将关闭,所有进程都会被杀死,此时修改电源模式除了增加关机耗时外,没有任何持久化效果。
    • 改进意见: 请确认产品需求。如果是为了加速“休眠镜像写入磁盘”的速度,可以在注释中明确说明;否则建议移除这些不必要的状态变更。
  • recoverySystemPowerMode 中的业务逻辑漏洞: 代码判断 if (mode == PowerSave) 且有电池且电量 <= 20.0 时,将 mode 改为 LowBattery。但这里获取的 mode 是从DBus属性读到的当前系统记忆的模式,还是用户设置的目标模式?如果在唤醒时,系统当前处于高性能模式,而记忆的模式是节能模式,这段逻辑会直接将系统设为节能模式,忽略了唤醒瞬间可能需要短暂高性能的过渡期。
    • 改进意见: 建议在唤醒后延迟几秒再恢复电源模式,以免系统刚唤醒时因性能不足导致卡顿。

3. 代码安全与健壮性

  • 硬编码的魔数: if (batteryCapacity <= 20.0) 中的 20.0 是一个硬编码的魔数。
    • 改进意见: 应当将其定义为常量,例如 static const double LowBatteryThreshold = 20.0;,以便未来维护和适应不同硬件阈值。
  • DBus返回值解析缺乏防御性编程: tlpMsg.arguments().value(0).value<QDBusVariant>().variant().toString() 这种链式调用非常危险。如果DBus服务异常返回了空参数、类型不匹配(例如返回了Int而不是QDBusVariant),会导致程序崩溃(Segfault)。
    • 改进意见: 必须分步解析,并严格校验参数类型和 QVariant::canConvert<T>()
  • 异步调用的错误被忽略: setTlpMode 中使用了 asyncCall,但直接 Q_UNUSED(reply),如果设置模式失败,没有任何日志记录,极难排查问题。
    • 改进意见: 使用 QDBusPendingCallWatcher 监听异步调用结果,失败时输出 qWarning

4. 代码质量

  • 拼写错误: recoverySystemPowerMode 语义为“恢复”,正确的英语单词应为 recover,建议函数名改为 recoverSystemPowerMode
  • 全局静态常量的定义规范: static const QString Performance = ... 定义在头文件中会被每个包含此编译单元的地方持有独立拷贝(虽然这里在cpp中影响不大),且 QString 的初始化有开销。
    • 改进意见: 建议使用 QLatin1StringQStringLiteral,并使用命名空间或类内的 static constexpr 修饰。
  • Lambda捕获建议: connect(m_login1ManagerInter, ... , [=](bool sleep) 中使用了默认值捕获 [=]
    • 改进意见: 现代C++建议显式捕获,避免悬空指针风险,此处应改为 [this](bool sleep)

🚀 改进后的代码建议

基于以上分析,我为你重构了相关代码:

sessionmanager.h 修改:

// 在 private 或 public slots: 区域
void setTlpMode(const QString &mode);
void recoverSystemPowerMode(); // 修正拼写

sessionmanager.cpp 修改:

// 常量定义优化
namespace PowerMode {
    static const QLatin1String Performance = QLatin1String("performance");
    static const QLatin1String PowerSave = QLatin1String("powersave");
    static const QLatin1String LowBattery = QLatin1String("lowBattery");
    static const double LowBatteryThreshold = 20.0; // 避免魔数
}

// ... 其他代码 ...

void SessionManager::initConnections()
{
    // ...
    // 修改为显式捕获 [this]
    connect(m_login1ManagerInter, &org::freedesktop::login1::Manager::PrepareForSleep, this, [this](bool sleep) {
        qDebug() << "system is preparing for sleep: " << sleep;
        if (!sleep) {
            // 建议唤醒后延迟恢复,避免唤醒瞬间性能不足卡顿
            QTimer::singleShot(3000, this, [this]() {
                recoverSystemPowerMode();
            });
        }
    });
    // ...
}

// 休眠/关机接口,建议重新评估是否真的需要设置 Performance
void SessionManager::RequestSuspend()
{
    // 如果产品需求明确要求休眠前切高性能以加速内存转储,可保留,否则建议移除
    // setTlpMode(PowerMode::Performance);
    // ...
}

// setTlpMode 增加异步错误处理
void SessionManager::setTlpMode(const QString &mode)
{
    qInfo() << "setTlpMode mode:" << mode;
    QDBusInterface inter("org.deepin.dde.Power1", "/org/deepin/dde/Power1", 
                         "org.deepin.dde.Power1", QDBusConnection::systemBus());
    
    QDBusPendingCall asyncCall = inter.asyncCall("SetTlpMode", mode);
    QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(asyncCall, this);
    QObject::connect(watcher, &QDBusPendingCallWatcher::finished, this, [mode](QDBusPendingCallWatcher *watcher) {
        QDBusPendingReply<> reply = *watcher;
        if (reply.isError()) {
            qWarning() << "Failed to setTlpMode to" << mode << ", error:" << reply.error().message();
        }
        watcher->deleteLater();
    });
}

// recoverSystemPowerMode 性能及安全性优化
void SessionManager::recoverSystemPowerMode()
{
    qInfo() << "recoverSystemPowerMode";
    
    // 建议:如果可能,使用 m_powerInter 替代局部 QDBusInterface 的反复构造
    QDBusInterface inter("org.deepin.dde.Power1", "/org/deepin/dde/Power1",
                         "org.freedesktop.DBus.Properties", QDBusConnection::systemBus());

    // 1. 使用同步调用时,必须做极其严格的类型和有效性检查
    QDBusMessage tlpMsg = inter.call("Get", "org.deepin.dde.Power1", "TlpMode");
    if (tlpMsg.type() != QDBusMessage::ReplyMessage || tlpMsg.arguments().isEmpty()) {
        qWarning() << "Get DBus property TlpMode failed:" << tlpMsg.errorMessage();
        return;
    }
    
    QVariant tlpVariant = tlpMsg.arguments().value(0).value<QDBusVariant>().variant();
    if (!tlpVariant.canConvert<QString>()) {
        qWarning() << "TlpMode type mismatch!";
        return;
    }
    QString tlpMode = tlpVariant.toString();

    QDBusMessage modeMsg = inter.call("Get", "org.deepin.dde.Power1", "Mode");
    if (modeMsg.type() != QDBusMessage::ReplyMessage || modeMsg.arguments().isEmpty()) {
        qWarning() << "Get DBus property Mode failed:" << modeMsg.errorMessage();
        return;
    }
    
    QVariant modeVariant = modeMsg.arguments().value(0).value<QDBusVariant>().variant();
    if (!modeVariant.canConvert<QString>()) {
        qWarning() << "Mode type mismatch!";
        return;
    }
    QString mode = modeVariant.toString();

    qInfo() << "DBus property of TlpMode:" << tlpMode << ", Mode:" << mode;

    if (mode == tlpMode) {
        return;
    }

    // 2. 逻辑优化
    if (mode == PowerMode::PowerSave) {
        QDBusMessage batteryMsg = inter.call("Get", "org.deepin.dde.Power1", "HasBattery");
        if (batteryMsg.type() == QDBusMessage::ReplyMessage && !batteryMsg.arguments().isEmpty()) {
            QVariant batteryVariant = batteryMsg.arguments().value(0).value<QDBusVariant>().variant();
            if (batteryVariant.canConvert<bool>()) {
                bool hasBattery = batteryVariant.toBool();
                if (!hasBattery) {
                    setTlpMode(mode);
                    return;
                }

                QDBusMessage capacityMsg = inter.call("Get", "org.deepin.dde.Power1", "BatteryCapacity");
                if (capacityMsg.type() == QDBusMessage::ReplyMessage && !capacityMsg.arguments().isEmpty()) {
                    QVariant capacityVariant = capacityMsg.arguments().value(0).value<QDBusVariant>().variant();
                    if (capacityVariant.canConvert<double>()) {
                        double batteryCapacity = capacityVariant.toDouble();
                        if (batteryCapacity <= PowerMode::LowBatteryThreshold) {
                            mode = PowerMode::LowBattery;
                        }
                    } else {
                        qWarning() << "BatteryCapacity type mismatch!";
                    }
                }
            } else {
                qWarning() << "HasBattery type mismatch!";
            }
        }
    }

    setTlpMode(mode);
}

void SessionManager::init()
{
    // ...
    QTimer::singleShot(0, this, [this] { 
        recoverSystemPowerMode(); 
    });
    // ...
}

重点总结:

  1. 一定要修复 recoverySystemPowerMode 中的同步DBus调用阻塞主线程的问题(如果业务不能改异步,至少要确保防御性编程不崩溃)。
  2. 一定要修复 QDBusVariant 链式解析可能导致的空指针/类型错误崩溃
  3. 请重新评估关机/重启前设置高性能模式的必要性。

Copy link
Copy Markdown
Contributor

@mhduiy mhduiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #208

功能目标明确:在关机/重启/休眠/挂起前切换高性能模式,唤醒/启动后恢复用户配置。以下是具体问题:


🔴 需要修复的问题

1. sessionmanager.cpp - setTlpMode 异步调用导致竞态

void SessionManager::setTlpMode(const QString &mode)
{
    QDBusInterface inter(...);
    QDBusPendingReply<> reply = inter.asyncCall("SetTlpMode", mode);
    Q_UNUSED(reply)
}

void SessionManager::RequestShutdown()
{
    setTlpMode(Performance);  // 异步,不等待结果
    shutdown(true);           // 立即执行关机
}

问题setTlpMode 是异步调用,RequestShutdown/RequestReboot/RequestSuspend 不等待 TLP 模式设置完成就立即执行关机/重启/挂起。在实际场景中,关机流程可能在 TLP 模式切换完成之前就已经终止了 DDE 服务,导致高性能模式设置失败——整个功能的核心目标可能无法达成。
建议:使用 QDBusPendingCallWatcher 等待调用完成后再执行关机流程,或使用同步 call() 方式(考虑到这是关机前的一次性操作,短暂阻塞可接受)。

2. sessionmanager.cpp - recoverySystemPowerMode 同步 D-Bus 调用过多

QDBusMessage tlpMsg = inter.call("Get", "org.deepin.dde.Power1", "TlpMode");   // 同步
QDBusMessage modeMsg = inter.call("Get", "org.deepin.dde.Power1", "Mode");     // 同步
QDBusMessage batteryMsg = inter.call("Get", ..., "HasBattery");                 // 同步
QDBusMessage capacityMsg = inter.call("Get", ..., "BatteryCapacity");           // 同步

问题:最多 4 次同步 D-Bus 调用,每次调用可能耗时数十毫秒到数秒。在启动路径 (init()) 中调用时,会拖慢启动速度。在 PrepareForSleep(false) 唤醒回调中调用时,D-Bus 服务可能尚未完全就绪。
建议

  • 启动时:考虑延迟恢复或使用 QTimer::singleShot 增加重试
  • 唤醒时:先检查 D-Bus 服务是否可用,失败后延迟重试

3. sessionmanager.cpp - 电源模式字符串硬编码

static const QString Performance = QStringLiteral("performance");
static const QString PowerSave = QStringLiteral("powersave");
static const QString LowBattery = QStringLiteral("lowBattery");

问题:这些字符串必须与 dde-daemon 中的电源模式常量保持一致。如果 dde-daemon 修改了这些值,此处会静默失败。
建议:考虑通过 D-Bus 接口查询可用的模式列表,或在注释中明确标注依赖关系。LowBattery 是 dde-daemon 内部使用的值,是否应该通过 TlpMode 直接使用 dde-daemon 暴露的 Mode 属性而非自己做 battery 判断?

4. sessionmanager.cpp:567 - 启动时的 recoverySystemPowerMode 时机问题

QTimer::singleShot(0, this, [this] { recoverySystemPowerMode(); });

问题QTimer::singleShot(0) 只保证在当前事件循环迭代完成后执行,但不能保证 org.deepin.dde.Power1 的 D-Bus 服务已经就绪。在冷启动场景下,系统服务可能尚未注册到 D-Bus。
建议:添加重试机制,例如失败后每秒重试,最多 3-5 次。


🟡 建议改进

5. 电池电量阈值硬编码

if (batteryCapacity <= 20.0) {
    mode = LowBattery;
}

建议:提取为常量,并添加注释说明此阈值应与 dde-daemon 中的低电量阈值保持一致。

6. recoverySystemPowerMode 缺少成功日志

建议:在函数末尾添加 qInfo() << "Recovered power mode to:" << mode;,方便排查问题。

7. 行尾有多余空格

    QTimer::singleShot(0, this, [this] { 

建议:清理行尾空格。

8. PrepareForSleep 信号连接位置

connect(m_login1ManagerInter, &org::freedesktop::login1::Manager::PrepareForSleep, [=](bool sleep) {

建议:此 lambda 捕获了 this(通过 [=]),但这里应该使用 [this] 显式捕获,避免意外捕获其他变量。虽然当前没有其他局部变量,但 [=] 是不良实践。


🟢 整体评价

  • 功能设计思路正确:操作前切高性能,唤醒后恢复
  • PrepareForSleep 信号的使用是正确的唤醒恢复方案
  • 代码结构清晰,新增的两个方法职责单一
  • 核心风险:异步调用导致 TLP 模式可能来不及设置就关机(第 1 点),建议优先修复

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fly602, mhduiy, xionglinlin

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants