power_manager: 2/4: Emit external brightness changes When external monitor brightness is adjusted over DDC/CI on a Chromebook or Chromebox, Ash previously received no feedback signal reporting the resulting 0-100 brightness percentage, and powerd only logged DDC/CI brightness reads and writes at VLOG(1) without the percentage or requested increment. Building on https://crrev.com/c/8541652, update system_api and powerd to report and log external display brightness updates: - Define kExternalScreenBrightnessChangedSignal (ExternalScreenBrightnessChanged) on org.chromium.PowerManager in system_api. - Log the external monitor's 0-100 brightness percentage and requested adjustment increment in ExternalDisplay::ReadBrightness() and ExternalDisplay::WriteBrightness() at LOG(INFO) when the pending cause is BacklightBrightnessChange_Cause_USER_REQUEST, and at VLOG(1) for ambient-light and other causes. - Move ToBacklightBrightnessChangeCause() to BacklightController so KeyboardBacklightController and ExternalBacklightController share the SetBacklightBrightnessRequest_Cause to BacklightBrightnessChange_Cause mapping. - Invoke BrightnessChangedCallback in ExternalDisplay after writing an updated DDC/CI brightness level (VCP 0x10), and on USER_REQUEST when clamped at the 0% or 100% boundary, so ExternalBacklightController emits kExternalScreenBrightnessChangedSignal with the percentage and cause. Because BacklightBrightnessChange carries percent and cause without a display ID, each monitor's completed write broadcasts a signal and multiple external popups in Ash show the latest reported percentage. Demo: https://screencast.googleplex.com/cast/NjU1NTk2ODU1NTk3NDY1NnxmZDBiOTFkYy0xMg Demo: https://screencast.googleplex.com/cast/NTY5OTE3MDAyNjM4OTUwNHw0Y2M1ZTQwZi01OA Bug: b:572950366 Test: power_manager_system_test and power_manager_policy_test in CQ Test: manual testing on physical HDMI monitors (see Demo links) Change-Id: I9c4d8e21a63f47b8c150e293d4a7f6b1e8204c51 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/8620418 Tested-by: Mike Ho <homi@google.com> Reviewed-by: Mike Ho <homi@google.com> Reviewed-by: Puthikorn Voravootivat <puthik@chromium.org> Auto-Submit: Mike Ho <homi@google.com> Commit-Queue: Mike Ho <homi@google.com>
diff --git a/power_manager/powerd/policy/backlight_controller.cc b/power_manager/powerd/policy/backlight_controller.cc index 4d061fd..232a2f1 100644 --- a/power_manager/powerd/policy/backlight_controller.cc +++ b/power_manager/powerd/policy/backlight_controller.cc
@@ -241,6 +241,24 @@ dbus_wrapper->EmitSignal(&signal); } +BacklightBrightnessChange_Cause +BacklightController::ToBacklightBrightnessChangeCause( + SetBacklightBrightnessRequest_Cause cause) { + switch (cause) { + case SetBacklightBrightnessRequest_Cause_USER_REQUEST: + return BacklightBrightnessChange_Cause_USER_REQUEST; + case SetBacklightBrightnessRequest_Cause_MODEL: + return BacklightBrightnessChange_Cause_MODEL; + case SetBacklightBrightnessRequest_Cause_USER_REQUEST_FROM_SETTINGS_APP: + return BacklightBrightnessChange_Cause_USER_REQUEST_FROM_SETTINGS_APP; + case SetBacklightBrightnessRequest_Cause_RESTORED_FROM_USER_PREFERENCE: + return BacklightBrightnessChange_Cause_RESTORED_FROM_USER_PREFERENCE; + case SetBacklightBrightnessRequest_Cause_BATTERY_SAVER_STATE_CHANGED: + return BacklightBrightnessChange_Cause_BATTERY_SAVER_STATE_CHANGED; + } + return BacklightBrightnessChange_Cause_OTHER; +} + // static void BacklightController::EmitAmbientLightSensorEnabledChangedSignal( system::DBusWrapperInterface* dbus_wrapper,
diff --git a/power_manager/powerd/policy/backlight_controller.h b/power_manager/powerd/policy/backlight_controller.h index 66a572b..7e533a5 100644 --- a/power_manager/powerd/policy/backlight_controller.h +++ b/power_manager/powerd/policy/backlight_controller.h
@@ -204,6 +204,11 @@ double brightness_percent, BacklightBrightnessChange_Cause cause); + // Maps a |SetBacklightBrightnessRequest_Cause| value to the corresponding + // |BacklightBrightnessChange_Cause| protobuf enum value. + static BacklightBrightnessChange_Cause ToBacklightBrightnessChangeCause( + SetBacklightBrightnessRequest_Cause cause); + // Emits a D-Bus signal announcing a ambient light sensor enabled change. static void EmitAmbientLightSensorEnabledChangedSignal( system::DBusWrapperInterface* dbus_wrapper,
diff --git a/power_manager/powerd/policy/external_backlight_controller.cc b/power_manager/powerd/policy/external_backlight_controller.cc index 4246c81..d94f745 100644 --- a/power_manager/powerd/policy/external_backlight_controller.cc +++ b/power_manager/powerd/policy/external_backlight_controller.cc
@@ -329,8 +329,10 @@ // ambient light sensors, and only if ALS-based brightness is disabled. if (ambient_light_sensor_watcher_ && !external_display_als_brightness_enabled_) { + const BacklightBrightnessChange_Cause change_cause = + ToBacklightBrightnessChangeCause(cause); for (auto& [path, pair] : external_als_displays_) { - SetBrightnessPercentForAmbientLight(pair.first, percent); + SetDisplayBrightnessPercent(pair.first, percent, change_cause); } external_display_with_ambient_light_sensor_brightness_ = percent; ++num_brightness_adjustments_in_session_; @@ -398,8 +400,12 @@ } auto delegate = std::make_unique<system::ExternalDisplay::RealDelegate>(); delegate->Init(info.i2c_path); - updated_displays.emplace( - info, std::make_unique<system::ExternalDisplay>(std::move(delegate))); + auto display = + std::make_unique<system::ExternalDisplay>(std::move(delegate)); + display->SetBrightnessChangedCallback(base::BindRepeating( + &ExternalBacklightController::OnExternalDisplayBrightnessChanged, + weak_ptr_factory_.GetWeakPtr())); + updated_displays.emplace(info, std::move(display)); } external_displays_.swap(updated_displays); } @@ -417,6 +423,15 @@ } } +void ExternalBacklightController::OnExternalDisplayBrightnessChanged( + double brightness_percent, BacklightBrightnessChange_Cause cause) { + if (dbus_wrapper_) { + EmitBrightnessChangedSignal(dbus_wrapper_, + kExternalScreenBrightnessChangedSignal, + brightness_percent, cause); + } +} + int ExternalBacklightController::CalculateAssociationScore( const base::FilePath& a, const base::FilePath& b) { std::vector<std::string> a_components = a.GetComponents(); @@ -485,14 +500,23 @@ external_als_displays_.swap(updated_ambient_light_sensors); } -void ExternalBacklightController::SetBrightnessPercentForAmbientLight( - const system::DisplayInfo& display_info, double brightness_percent) { +void ExternalBacklightController::SetDisplayBrightnessPercent( + const system::DisplayInfo& display_info, + double brightness_percent, + BacklightBrightnessChange_Cause cause) { auto display_it = external_displays_.find(display_info); if (display_it != external_displays_.end()) { - display_it->second->SetBrightness(brightness_percent); + display_it->second->SetBrightness(brightness_percent, cause); } } +void ExternalBacklightController::SetBrightnessPercentForAmbientLight( + const system::DisplayInfo& display_info, double brightness_percent) { + SetDisplayBrightnessPercent( + display_info, brightness_percent, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); +} + std::vector<std::pair<base::FilePath, system::DisplayInfo>> ExternalBacklightController:: GetAmbientLightSensorAndDisplayMatchesForTesting() { @@ -523,8 +547,9 @@ // Set displays that had ALS-based brightness enabled back to the // brightness percentage they had before. for (auto& [path, pair] : external_als_displays_) { - SetBrightnessPercentForAmbientLight( - pair.first, external_display_with_ambient_light_sensor_brightness_); + SetDisplayBrightnessPercent( + pair.first, external_display_with_ambient_light_sensor_brightness_, + BacklightBrightnessChange_Cause_USER_REQUEST_FROM_SETTINGS_APP); } } }
diff --git a/power_manager/powerd/policy/external_backlight_controller.h b/power_manager/powerd/policy/external_backlight_controller.h index d844d34..e50683a 100644 --- a/power_manager/powerd/policy/external_backlight_controller.h +++ b/power_manager/powerd/policy/external_backlight_controller.h
@@ -119,6 +119,8 @@ GetAmbientLightSensorAndDisplayMatchesForTesting(); private: + friend class ExternalBacklightControllerTest; + // Handlers for requests sent via D-Bus. void HandleIncreaseBrightnessRequest(); void HandleDecreaseBrightnessRequest(bool allow_off); @@ -141,6 +143,17 @@ // percent in the range [-100.0, 100.0]. void AdjustBrightnessByPercent(double percent_offset); + // Sets the brightness of |display_info| in |external_displays_| to + // |brightness_percent| with |cause|. + void SetDisplayBrightnessPercent(const system::DisplayInfo& display_info, + double brightness_percent, + BacklightBrightnessChange_Cause cause); + + // Emits |kExternalScreenBrightnessChangedSignal| when an external display's + // brightness percentage is updated over DDC/CI. + void OnExternalDisplayBrightnessChanged( + double brightness_percent, BacklightBrightnessChange_Cause cause); + // Compute an association score between two syspaths. The score used is a // count of how many prefix path components they share. int CalculateAssociationScore(const base::FilePath& a,
diff --git a/power_manager/powerd/policy/external_backlight_controller_test.cc b/power_manager/powerd/policy/external_backlight_controller_test.cc index de84c12..8d3e42e 100644 --- a/power_manager/powerd/policy/external_backlight_controller_test.cc +++ b/power_manager/powerd/policy/external_backlight_controller_test.cc
@@ -4,11 +4,20 @@ #include "power_manager/powerd/policy/external_backlight_controller.h" +#include <linux/i2c-dev.h> +#include <linux/i2c.h> + +#include <cstring> +#include <memory> +#include <string> +#include <utility> +#include <vector> + +#include <base/check.h> +#include <base/check_op.h> #include <base/compiler_specific.h> #include <chromeos/dbus/service_constants.h> #include <gtest/gtest.h> -#include <string> -#include <utility> #include "power_manager/common/fake_prefs.h" #include "power_manager/powerd/policy/backlight_controller_observer_stub.h" @@ -17,6 +26,7 @@ #include "power_manager/powerd/system/dbus_wrapper_stub.h" #include "power_manager/powerd/system/display/display_power_setter_stub.h" #include "power_manager/powerd/system/display/display_watcher_stub.h" +#include "power_manager/powerd/system/display/external_display.h" #include "power_manager/powerd/system/external_ambient_light_sensor_factory_stub.h" #include "power_manager/powerd/testing/test_environment.h" #include "power_manager/proto_bindings/backlight.pb.h" @@ -28,6 +38,61 @@ constexpr char kSecondDisplay[] = "/sys/devices/usb2/mydisplay"; constexpr char kSecondSensor[] = "/sys/devices/usb2/mysensor"; +// Fake ExternalDisplay::Delegate that replies with a fixed initial brightness +// and succeeds on DDC/CI writes. +class FakeExternalDisplayDelegate + : public power_manager::system::ExternalDisplay::Delegate { + public: + FakeExternalDisplayDelegate(uint16_t current_brightness, + uint16_t max_brightness) + : current_brightness_(current_brightness), + max_brightness_(max_brightness) {} + FakeExternalDisplayDelegate(const FakeExternalDisplayDelegate&) = delete; + FakeExternalDisplayDelegate& operator=(const FakeExternalDisplayDelegate&) = + delete; + ~FakeExternalDisplayDelegate() override = default; + + std::string GetName() const override { return "i2c-fake"; } + + bool PerformI2COperation(struct i2c_rdwr_ioctl_data* data) override { + CHECK(data); + CHECK_EQ(data->nmsgs, 1u); + struct i2c_msg* const msg = data->msgs; + CHECK(msg); + if (msg->flags == 0) { + return true; + } + if (msg->flags == I2C_M_RD) { + using power_manager::system::ExternalDisplay; + std::vector<uint8_t> reply = { + ExternalDisplay::kDdcDisplayAddress, + static_cast<uint8_t>(ExternalDisplay::kDdcMessageBodyLengthMask | 8), + ExternalDisplay::kDdcGetReplyCommand, + 0x0, + ExternalDisplay::kDdcBrightnessIndex, + 0x0, + static_cast<uint8_t>(max_brightness_ >> 8), + static_cast<uint8_t>(max_brightness_ & 0xff), + static_cast<uint8_t>(current_brightness_ >> 8), + static_cast<uint8_t>(current_brightness_ & 0xff), + 0x0}; + uint8_t checksum = ExternalDisplay::kDdcVirtualHostAddress; + for (size_t i = 0; i < reply.size() - 1; ++i) { + checksum ^= reply[i]; + } + reply.back() = checksum; + CHECK_EQ(static_cast<size_t>(msg->len), reply.size()); + memcpy(msg->buf, reply.data(), reply.size()); + return true; + } + return false; + } + + private: + const uint16_t current_brightness_; + const uint16_t max_brightness_; +}; + } // namespace namespace power_manager::policy { @@ -98,6 +163,25 @@ ASSERT_TRUE(dbus::MessageReader(response.get()).PopBool(enabled)); } + void AttachFakeDelegate(ExternalBacklightController* controller, + uint16_t current_brightness, + uint16_t max_brightness) { + CHECK_EQ(1u, controller->external_displays_.size()); + system::ExternalDisplay::TestApi test_api( + controller->external_displays_.begin()->second.get()); + test_api.SetDelegate(std::make_unique<FakeExternalDisplayDelegate>( + current_brightness, max_brightness)); + } + + bool TriggerDisplayTimeout(ExternalBacklightController* controller) { + if (controller->external_displays_.empty()) { + return false; + } + system::ExternalDisplay::TestApi test_api( + controller->external_displays_.begin()->second.get()); + return test_api.TriggerTimeout(); + } + std::string default_als_steps_ = "5.0 -1 600\n100.0 500 -1"; FakePrefs prefs_; @@ -133,27 +217,54 @@ // If ALS-based brightness is disabled, the brightness of external displays // with an ambient light sensor can be adjusted with an absolute request or a - // relative request. + // relative request, and emits ExternalScreenBrightnessChanged with the + // user/request cause rather than AMBIENT_LIGHT_CHANGED. + AddDisplay(kFirstDisplay); + AttachFakeDelegate(&controller_, 60, 100); + AddSensor(kFirstSensor); bool enabled = false; CallGetExternalDisplayALSBrightness(&enabled); EXPECT_TRUE(enabled); CallSetExternalDisplayALSBrightness(false); CallGetExternalDisplayALSBrightness(&enabled); EXPECT_FALSE(enabled); + ASSERT_TRUE(TriggerDisplayTimeout(&controller_)); + ASSERT_EQ(1u, dbus_wrapper_.num_sent_signals()); + test::CheckBrightnessChangedSignal( + &dbus_wrapper_, 0, kExternalScreenBrightnessChangedSignal, 100.0, + BacklightBrightnessChange_Cause_USER_REQUEST_FROM_SETTINGS_APP); + + ASSERT_TRUE(TriggerDisplayTimeout(&controller_)); test::CallSetScreenBrightness( &dbus_wrapper_, 42.0, SetBacklightBrightnessRequest_Transition_INSTANT, - SetBacklightBrightnessRequest_Cause_USER_REQUEST); + SetBacklightBrightnessRequest_Cause_USER_REQUEST_FROM_SETTINGS_APP); EXPECT_EQ(1, controller_.GetNumUserAdjustments()); EXPECT_TRUE(controller_.GetBrightnessPercent(&percent)); EXPECT_EQ(42.0, percent); + ASSERT_EQ(2u, dbus_wrapper_.num_sent_signals()); + test::CheckBrightnessChangedSignal( + &dbus_wrapper_, 1, kExternalScreenBrightnessChangedSignal, 42.0, + BacklightBrightnessChange_Cause_USER_REQUEST_FROM_SETTINGS_APP); + + ASSERT_TRUE(TriggerDisplayTimeout(&controller_)); test::CallIncreaseScreenBrightness(&dbus_wrapper_); EXPECT_TRUE(controller_.GetBrightnessPercent(&percent)); EXPECT_EQ(47.0, percent); EXPECT_EQ(2, controller_.GetNumUserAdjustments()); + ASSERT_EQ(3u, dbus_wrapper_.num_sent_signals()); + test::CheckBrightnessChangedSignal( + &dbus_wrapper_, 2, kExternalScreenBrightnessChangedSignal, 47.0, + BacklightBrightnessChange_Cause_USER_REQUEST); + + ASSERT_TRUE(TriggerDisplayTimeout(&controller_)); test::CallDecreaseScreenBrightness(&dbus_wrapper_, true /* allow_off */); EXPECT_TRUE(controller_.GetBrightnessPercent(&percent)); EXPECT_EQ(42.0, percent); EXPECT_EQ(3, controller_.GetNumUserAdjustments()); + ASSERT_EQ(4u, dbus_wrapper_.num_sent_signals()); + test::CheckBrightnessChangedSignal( + &dbus_wrapper_, 3, kExternalScreenBrightnessChangedSignal, 42.0, + BacklightBrightnessChange_Cause_USER_REQUEST); } TEST_F(ExternalBacklightControllerTest, SecondaryControllerMode) { @@ -186,15 +297,39 @@ kGetExternalDisplayALSBrightnessMethod)); // Dedicated external display brightness methods must be exported and adjust - // external monitors. + // external monitors, emitting ExternalScreenBrightnessChanged with + // USER_REQUEST via the callback bound in UpdateDisplays(). + AddDisplay(kFirstDisplay); + AttachFakeDelegate(&secondary_controller, 60, 100); EXPECT_TRUE(secondary_dbus_wrapper.IsMethodExported( kIncreaseExternalScreenBrightnessMethod)); EXPECT_TRUE(secondary_dbus_wrapper.IsMethodExported( kDecreaseExternalScreenBrightnessMethod)); test::CallIncreaseExternalScreenBrightness(&secondary_dbus_wrapper); EXPECT_EQ(1, secondary_controller.GetNumUserAdjustments()); + ASSERT_TRUE(TriggerDisplayTimeout(&secondary_controller)); + ASSERT_EQ(1u, secondary_dbus_wrapper.num_sent_signals()); + test::CheckBrightnessChangedSignal( + &secondary_dbus_wrapper, 0, kExternalScreenBrightnessChangedSignal, 65.0, + BacklightBrightnessChange_Cause_USER_REQUEST); + + ASSERT_TRUE(TriggerDisplayTimeout(&secondary_controller)); test::CallDecreaseExternalScreenBrightness(&secondary_dbus_wrapper); EXPECT_EQ(2, secondary_controller.GetNumUserAdjustments()); + ASSERT_EQ(2u, secondary_dbus_wrapper.num_sent_signals()); + test::CheckBrightnessChangedSignal( + &secondary_dbus_wrapper, 1, kExternalScreenBrightnessChangedSignal, 60.0, + BacklightBrightnessChange_Cause_USER_REQUEST); + + // Ambient-light brightness updates must emit ExternalScreenBrightnessChanged + // with AMBIENT_LIGHT_CHANGED rather than USER_REQUEST. + ASSERT_TRUE(TriggerDisplayTimeout(&secondary_controller)); + secondary_controller.SetBrightnessPercentForAmbientLight( + display_watcher_.GetDisplays()[0], 40.0); + ASSERT_EQ(3u, secondary_dbus_wrapper.num_sent_signals()); + test::CheckBrightnessChangedSignal( + &secondary_dbus_wrapper, 2, kExternalScreenBrightnessChangedSignal, 40.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); // Lifecycle and power state callbacks must safely no-op without dereferencing // a null display_power_setter_ or emitting duplicate observer notifications.
diff --git a/power_manager/powerd/policy/keyboard_backlight_controller.cc b/power_manager/powerd/policy/keyboard_backlight_controller.cc index 8c2ba1f..3238a57 100644 --- a/power_manager/powerd/policy/keyboard_backlight_controller.cc +++ b/power_manager/powerd/policy/keyboard_backlight_controller.cc
@@ -61,24 +61,6 @@ NOTREACHED() << "Unhandled transition style " << static_cast<int>(transition); } -// Map a |SetBacklightBrightnessRequest_Cause| to an equivalent -// |BacklightBrightnessChange_Cause|. -BacklightBrightnessChange_Cause ToBacklightBrightnessChangeCause( - SetBacklightBrightnessRequest_Cause cause) { - switch (cause) { - case SetBacklightBrightnessRequest_Cause_USER_REQUEST: - return BacklightBrightnessChange_Cause_USER_REQUEST; - case SetBacklightBrightnessRequest_Cause_USER_REQUEST_FROM_SETTINGS_APP: - return BacklightBrightnessChange_Cause_USER_REQUEST_FROM_SETTINGS_APP; - case SetBacklightBrightnessRequest_Cause_MODEL: - return BacklightBrightnessChange_Cause_MODEL; - case SetBacklightBrightnessRequest_Cause_RESTORED_FROM_USER_PREFERENCE: - return BacklightBrightnessChange_Cause_RESTORED_FROM_USER_PREFERENCE; - default: - return BacklightBrightnessChange_Cause_OTHER; - } -} - } // namespace KeyboardBacklightController::KeyboardBacklightController()
diff --git a/power_manager/powerd/system/display/external_display.cc b/power_manager/powerd/system/display/external_display.cc index 1b4637c..0a2ade9 100644 --- a/power_manager/powerd/system/display/external_display.cc +++ b/power_manager/powerd/system/display/external_display.cc
@@ -129,19 +129,34 @@ return true; } +void ExternalDisplay::TestApi::SetDelegate(std::unique_ptr<Delegate> delegate) { + display_->delegate_ = std::move(delegate); +} + ExternalDisplay::ExternalDisplay(std::unique_ptr<Delegate> delegate) : delegate_(std::move(delegate)) {} +void ExternalDisplay::SetBrightnessChangedCallback( + BrightnessChangedCallback callback) { + brightness_changed_callback_ = std::move(callback); +} + void ExternalDisplay::AdjustBrightnessByPercent(double percent_offset) { pending_brightness_adjustment_percent_ += percent_offset; + pending_brightness_change_cause_ = + BacklightBrightnessChange_Cause_USER_REQUEST; if (!timer_.IsRunning()) { DCHECK_EQ(State::IDLE, state_); UpdateState(); } } -void ExternalDisplay::SetBrightness(double percent) { +void ExternalDisplay::SetBrightness(double percent, + BacklightBrightnessChange_Cause cause) { pending_brightness_percent_ = percent; + if (std::abs(pending_brightness_adjustment_percent_) <= kEpsilon) { + pending_brightness_change_cause_ = cause; + } if (!timer_.IsRunning()) { DCHECK_EQ(State::IDLE, state_); UpdateState(); @@ -225,9 +240,16 @@ current_brightness_percent_ = util::ClampPercent( static_cast<double>(current_level) / max_brightness_level_ * 100.0); - VLOG(1) << "Received current brightness " << current_level << " (" - << current_brightness_percent_ << "%) and maximum brightness " - << max_level << " from " << delegate_->GetName(); + if (pending_brightness_change_cause_ == + BacklightBrightnessChange_Cause_USER_REQUEST) { + LOG(INFO) << "Received current brightness " << current_level << " (" + << current_brightness_percent_ << "%) and maximum brightness " + << max_level << " from " << delegate_->GetName(); + } else { + VLOG(1) << "Received current brightness " << current_level << " (" + << current_brightness_percent_ << "%) and maximum brightness " + << max_level << " from " << delegate_->GetName(); + } last_brightness_update_time_ = clock_.GetCurrentTime(); return true; } @@ -244,12 +266,38 @@ // mashing on a brightness key, they're probably not simultaneously hitting // the display's physical buttons. if (new_level == BrightnessPercentToLevel(current_brightness_percent_)) { + if (pending_brightness_change_cause_ == + BacklightBrightnessChange_Cause_USER_REQUEST) { + LOG(INFO) << "Not writing unchanged brightness level " << new_level + << " (" << new_percent << "%, increment " + << pending_brightness_adjustment_percent_ << "%) to " + << delegate_->GetName(); + } else { + VLOG(1) << "Not writing unchanged brightness level " << new_level << " (" + << new_percent << "%, increment " + << pending_brightness_adjustment_percent_ << "%) to " + << delegate_->GetName(); + } last_brightness_update_time_ = clock_.GetCurrentTime(); + if (brightness_changed_callback_ && + pending_brightness_change_cause_ == + BacklightBrightnessChange_Cause_USER_REQUEST) { + brightness_changed_callback_.Run(current_brightness_percent_, + pending_brightness_change_cause_); + } return false; } - VLOG(1) << "Writing brightness " << new_level << " (" << new_percent - << "%) to " << delegate_->GetName(); + if (pending_brightness_change_cause_ == + BacklightBrightnessChange_Cause_USER_REQUEST) { + LOG(INFO) << "Writing brightness " << new_level << " (" << new_percent + << "%, increment " << pending_brightness_adjustment_percent_ + << "%) to " << delegate_->GetName(); + } else { + VLOG(1) << "Writing brightness " << new_level << " (" << new_percent + << "%, increment " << pending_brightness_adjustment_percent_ + << "%) to " << delegate_->GetName(); + } std::vector<uint8_t> message; message.push_back(kDdcSetCommand); message.push_back(kDdcBrightnessIndex); @@ -262,6 +310,10 @@ current_brightness_percent_ = new_percent; last_brightness_update_time_ = clock_.GetCurrentTime(); + if (brightness_changed_callback_) { + brightness_changed_callback_.Run(current_brightness_percent_, + pending_brightness_change_cause_); + } return true; } @@ -282,6 +334,8 @@ } pending_brightness_adjustment_percent_ = 0.0; pending_brightness_percent_ = -1.0; + pending_brightness_change_cause_ = + BacklightBrightnessChange_Cause_OTHER; return; } @@ -290,6 +344,8 @@ if (!RequestBrightness()) { pending_brightness_adjustment_percent_ = 0.0; pending_brightness_percent_ = -1.0; + pending_brightness_change_cause_ = + BacklightBrightnessChange_Cause_OTHER; return; } state_ = State::WAITING_FOR_REPLY; @@ -302,6 +358,8 @@ if (!ReadBrightness()) { pending_brightness_adjustment_percent_ = 0.0; pending_brightness_percent_ = -1.0; + pending_brightness_change_cause_ = + BacklightBrightnessChange_Cause_OTHER; return; } @@ -314,6 +372,7 @@ } pending_brightness_adjustment_percent_ = 0.0; pending_brightness_percent_ = -1.0; + pending_brightness_change_cause_ = BacklightBrightnessChange_Cause_OTHER; return; } }
diff --git a/power_manager/powerd/system/display/external_display.h b/power_manager/powerd/system/display/external_display.h index a15bb4b..c7c1587 100644 --- a/power_manager/powerd/system/display/external_display.h +++ b/power_manager/powerd/system/display/external_display.h
@@ -18,6 +18,7 @@ #include <base/timer/timer.h> #include "power_manager/common/clock.h" +#include "power_manager/proto_bindings/backlight.pb.h" struct i2c_rdwr_ioctl_data; @@ -193,16 +194,27 @@ // and returns true. Otherwise, returns false. [[nodiscard]] bool TriggerTimeout(); + // Replaces |display_|'s |delegate_| for testing. + void SetDelegate(std::unique_ptr<Delegate> delegate); + private: ExternalDisplay* display_; // owned elsewhere }; + // Callback invoked with the updated brightness percentage in [0.0, 100.0] and + // the cause of the change whenever a new brightness level is successfully + // written to the display. + using BrightnessChangedCallback = base::RepeatingCallback<void( + double brightness_percent, BacklightBrightnessChange_Cause cause)>; + explicit ExternalDisplay(std::unique_ptr<Delegate> delegate); ExternalDisplay(const ExternalDisplay&) = delete; ExternalDisplay& operator=(const ExternalDisplay&) = delete; ~ExternalDisplay() = default; + void SetBrightnessChangedCallback(BrightnessChangedCallback callback); + // Adjusts the display's brightness by |offset_percent|, a linearly-calculated // percent in the range [-100.0, 100.0]. Note that the adjustment will happen // asynchronously if the display's current brightness is initially unknown. @@ -214,7 +226,7 @@ // racy if the user is simultaneously adjusting the brightness using physical // buttons on the display. This should not be used with displays that have // physical buttons for adjusting the brightness. - void SetBrightness(double percent); + void SetBrightness(double percent, BacklightBrightnessChange_Cause cause); private: enum class State { @@ -294,10 +306,16 @@ // value will be less than zero if no change is pending. double pending_brightness_percent_ = -1.0; + // Cause associated with the pending brightness change. + BacklightBrightnessChange_Cause pending_brightness_change_cause_ = + BacklightBrightnessChange_Cause_OTHER; + // Invokes UpdateState(). Used to enforce the mandatory delays between // requesting the brightness and reading the reply, and after sending a "set" // request to the display. base::OneShotTimer timer_; + + BrightnessChangedCallback brightness_changed_callback_; }; } // namespace power_manager::system
diff --git a/power_manager/powerd/system/display/external_display_test.cc b/power_manager/powerd/system/display/external_display_test.cc index f7a801d..1e6f346 100644 --- a/power_manager/powerd/system/display/external_display_test.cc +++ b/power_manager/powerd/system/display/external_display_test.cc
@@ -14,8 +14,10 @@ #include <base/check.h> #include <base/check_op.h> #include <base/compiler_specific.h> +#include <base/functional/bind.h> #include <base/logging.h> #include <base/strings/string_number_conversions.h> +#include <brillo/syslog_logging.h> #include <gtest/gtest.h> #include "power_manager/common/metrics_constants.h" @@ -31,6 +33,28 @@ return base::HexEncode(base::byte_span_from_ref(byte)); } +// Captures brillo log output into brillo::GetLog() and restores global log +// state on destruction. +class ScopedLogCapture { + public: + ScopedLogCapture() : previous_min_log_level_(logging::GetMinLogLevel()) { + logging::SetMinLogLevel(logging::LOGGING_INFO); + brillo::ClearLog(); + brillo::LogToString(true); + } + ScopedLogCapture(const ScopedLogCapture&) = delete; + ScopedLogCapture& operator=(const ScopedLogCapture&) = delete; + + ~ScopedLogCapture() { + brillo::LogToString(false); + brillo::ClearLog(); + logging::SetMinLogLevel(previous_min_log_level_); + } + + private: + const int previous_min_log_level_; +}; + // Test implementation of ExternalDisplay::Delegate. class TestDelegate : public ExternalDisplay::Delegate { public: @@ -444,7 +468,8 @@ TEST_F(ExternalDisplayTest, AbsoluteBrightness) { // Setting the brightness to 60% should result in a "get brightness" request // being sent. - display_.SetBrightness(60.0); + display_.SetBrightness(60.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); EXPECT_EQ(request_brightness_message_, delegate_->PopSentMessage()); // After the timer fires, the reply should be read and a request to set the @@ -458,7 +483,8 @@ // since no time has passed since the previous request. After the timer fires, // a new request should be sent containing the last absolute request combined // with the adjustment. - display_.SetBrightness(50.0); + display_.SetBrightness(50.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); EXPECT_EQ("", delegate_->PopSentMessage()); display_.AdjustBrightnessByPercent(35.0); EXPECT_EQ("", delegate_->PopSentMessage()); @@ -467,4 +493,196 @@ EXPECT_EQ(GetSetBrightnessMessage(85), delegate_->PopSentMessage()); } +TEST_F(ExternalDisplayTest, BrightnessChangedCallback) { + std::vector<std::pair<double, BacklightBrightnessChange_Cause>> + reported_changes; + display_.SetBrightnessChangedCallback(base::BindRepeating( + [](std::vector<std::pair<double, BacklightBrightnessChange_Cause>>* + changes, + double percent, BacklightBrightnessChange_Cause cause) { + changes->emplace_back(percent, cause); + }, + &reported_changes)); + + // Initial +10% adjustment reads 50/100 and writes 60% (USER_REQUEST). + display_.AdjustBrightnessByPercent(10.0); + EXPECT_EQ(request_brightness_message_, delegate_->PopSentMessage()); + EXPECT_TRUE(reported_changes.empty()); + + delegate_->set_reply_message(GetBrightnessReply(50, 100)); + ASSERT_TRUE(test_api_.TriggerTimeout()); + EXPECT_EQ(GetSetBrightnessMessage(60), delegate_->PopSentMessage()); + ASSERT_EQ(1u, reported_changes.size()); + EXPECT_DOUBLE_EQ(60.0, reported_changes[0].first); + EXPECT_EQ(BacklightBrightnessChange_Cause_USER_REQUEST, + reported_changes[0].second); + + // Coalesced +20% and +5% adjustment writes 85% (USER_REQUEST). + display_.AdjustBrightnessByPercent(20.0); + display_.AdjustBrightnessByPercent(5.0); + ASSERT_TRUE(test_api_.TriggerTimeout()); + EXPECT_EQ(GetSetBrightnessMessage(85), delegate_->PopSentMessage()); + ASSERT_EQ(2u, reported_changes.size()); + EXPECT_DOUBLE_EQ(85.0, reported_changes[1].first); + EXPECT_EQ(BacklightBrightnessChange_Cause_USER_REQUEST, + reported_changes[1].second); + + // Setting absolute brightness from an ambient light sensor writes 40% -> + // callback receives (40.0, AMBIENT_LIGHT_CHANGED). + ASSERT_TRUE(test_api_.TriggerTimeout()); + display_.SetBrightness(40.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); + EXPECT_EQ(GetSetBrightnessMessage(40), delegate_->PopSentMessage()); + ASSERT_EQ(3u, reported_changes.size()); + EXPECT_DOUBLE_EQ(40.0, reported_changes[2].first); + EXPECT_EQ(BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED, + reported_changes[2].second); + + // Setting absolute brightness from a user request (e.g. Settings app) writes + // 30% -> callback receives (30.0, USER_REQUEST_FROM_SETTINGS_APP). + ASSERT_TRUE(test_api_.TriggerTimeout()); + display_.SetBrightness( + 30.0, BacklightBrightnessChange_Cause_USER_REQUEST_FROM_SETTINGS_APP); + EXPECT_EQ(GetSetBrightnessMessage(30), delegate_->PopSentMessage()); + ASSERT_EQ(4u, reported_changes.size()); + EXPECT_DOUBLE_EQ(30.0, reported_changes[3].first); + EXPECT_EQ(BacklightBrightnessChange_Cause_USER_REQUEST_FROM_SETTINGS_APP, + reported_changes[3].second); + + // Coalescing a pending relative user adjustment (+10%) with an ambient-light + // SetBrightness(40.0) while the post-write timer is running preserves + // USER_REQUEST and writes 50%. + display_.AdjustBrightnessByPercent(10.0); + display_.SetBrightness(40.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); + ASSERT_TRUE(test_api_.TriggerTimeout()); + EXPECT_EQ(GetSetBrightnessMessage(50), delegate_->PopSentMessage()); + ASSERT_EQ(5u, reported_changes.size()); + EXPECT_DOUBLE_EQ(50.0, reported_changes[4].first); + EXPECT_EQ(BacklightBrightnessChange_Cause_USER_REQUEST, + reported_changes[4].second); + + // An ambient-light update that rounds to the same brightness level (50) must + // not write to the display or invoke the callback. + ASSERT_TRUE(test_api_.TriggerTimeout()); + display_.SetBrightness(50.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); + EXPECT_EQ("", delegate_->PopSentMessage()); + EXPECT_EQ(5u, reported_changes.size()); + + // Reaching 100% writes 100, and a subsequent user request at 100% must still + // invoke the callback with USER_REQUEST (so Ash shows the brightness popup at + // the boundary) without sending a redundant DDC write. + display_.AdjustBrightnessByPercent(60.0); + EXPECT_EQ(GetSetBrightnessMessage(100), delegate_->PopSentMessage()); + ASSERT_EQ(6u, reported_changes.size()); + EXPECT_DOUBLE_EQ(100.0, reported_changes[5].first); + EXPECT_EQ(BacklightBrightnessChange_Cause_USER_REQUEST, + reported_changes[5].second); + + ASSERT_TRUE(test_api_.TriggerTimeout()); + display_.AdjustBrightnessByPercent(5.0); + EXPECT_EQ("", delegate_->PopSentMessage()); + ASSERT_EQ(7u, reported_changes.size()); + EXPECT_DOUBLE_EQ(100.0, reported_changes[6].first); + EXPECT_EQ(BacklightBrightnessChange_Cause_USER_REQUEST, + reported_changes[6].second); + + // A failed write must not invoke the callback. + delegate_->set_report_write_failure(true); + display_.AdjustBrightnessByPercent(-5.0); + EXPECT_EQ(7u, reported_changes.size()); +} + +TEST_F(ExternalDisplayTest, BrightnessLoggingByCause) { + ScopedLogCapture log_capture; + + // A relative user request logs both the DDC/CI read and changed write at + // INFO. + display_.AdjustBrightnessByPercent(10.0); + EXPECT_EQ(request_brightness_message_, delegate_->PopSentMessage()); + EXPECT_EQ("", brillo::GetLog()); + + delegate_->set_reply_message(GetBrightnessReply(50, 100)); + ASSERT_TRUE(test_api_.TriggerTimeout()); + EXPECT_EQ(GetSetBrightnessMessage(60), delegate_->PopSentMessage()); + EXPECT_TRUE(brillo::FindLog( + "Received current brightness 50 (50%) and maximum brightness 100 " + "from i2c-test")); + EXPECT_TRUE(brillo::FindLog( + "Writing brightness 60 (60%, increment 10%) to i2c-test")); + + // Reaching 100% and stepping again via a user request logs the unchanged + // boundary level at INFO. + ASSERT_TRUE(test_api_.TriggerTimeout()); + display_.AdjustBrightnessByPercent(50.0); + EXPECT_EQ(GetSetBrightnessMessage(100), delegate_->PopSentMessage()); + ASSERT_TRUE(test_api_.TriggerTimeout()); + + brillo::ClearLog(); + display_.AdjustBrightnessByPercent(5.0); + EXPECT_EQ("", delegate_->PopSentMessage()); + EXPECT_TRUE(brillo::FindLog( + "Not writing unchanged brightness level 100 (100%, increment 5%) to " + "i2c-test")); + + // Explicit SetBrightness with USER_REQUEST also logs at INFO. + brillo::ClearLog(); + display_.SetBrightness(70.0, BacklightBrightnessChange_Cause_USER_REQUEST); + EXPECT_EQ(GetSetBrightnessMessage(70), delegate_->PopSentMessage()); + EXPECT_TRUE( + brillo::FindLog("Writing brightness 70 (70%, increment 0%) to i2c-test")); + ASSERT_TRUE(test_api_.TriggerTimeout()); + + // Non-USER_REQUEST updates (ambient light reads, changed writes, repeated + // unchanged levels, and Settings-app updates) do not log at INFO. + test_api_.AdvanceTime(ExternalDisplay::kCachedBrightnessValid + + base::Milliseconds(10)); + brillo::ClearLog(); + display_.SetBrightness(40.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); + EXPECT_EQ(request_brightness_message_, delegate_->PopSentMessage()); + delegate_->set_reply_message(GetBrightnessReply(70, 100)); + ASSERT_TRUE(test_api_.TriggerTimeout()); + EXPECT_EQ(GetSetBrightnessMessage(40), delegate_->PopSentMessage()); + ASSERT_TRUE(test_api_.TriggerTimeout()); + + display_.SetBrightness(40.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); + EXPECT_EQ("", delegate_->PopSentMessage()); + + display_.SetBrightness( + 30.0, BacklightBrightnessChange_Cause_USER_REQUEST_FROM_SETTINGS_APP); + EXPECT_EQ(GetSetBrightnessMessage(30), delegate_->PopSentMessage()); + ASSERT_TRUE(test_api_.TriggerTimeout()); + EXPECT_EQ("", brillo::GetLog()); + + // At VLOG(1) verbosity, non-USER_REQUEST reads, changed writes, and unchanged + // levels are logged. + logging::SetMinLogLevel(-1); + test_api_.AdvanceTime(ExternalDisplay::kCachedBrightnessValid + + base::Milliseconds(10)); + brillo::ClearLog(); + display_.SetBrightness(40.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); + EXPECT_EQ(request_brightness_message_, delegate_->PopSentMessage()); + delegate_->set_reply_message(GetBrightnessReply(30, 100)); + ASSERT_TRUE(test_api_.TriggerTimeout()); + EXPECT_EQ(GetSetBrightnessMessage(40), delegate_->PopSentMessage()); + EXPECT_TRUE(brillo::FindLog( + "Received current brightness 30 (30%) and maximum brightness 100 " + "from i2c-test")); + EXPECT_TRUE( + brillo::FindLog("Writing brightness 40 (40%, increment 0%) to i2c-test")); + ASSERT_TRUE(test_api_.TriggerTimeout()); + + brillo::ClearLog(); + display_.SetBrightness(40.0, + BacklightBrightnessChange_Cause_AMBIENT_LIGHT_CHANGED); + EXPECT_EQ("", delegate_->PopSentMessage()); + EXPECT_TRUE(brillo::FindLog( + "Not writing unchanged brightness level 40 (40%, increment 0%) to " + "i2c-test")); +} + } // namespace power_manager::system
diff --git a/system_api/dbus/power_manager/dbus-constants.h b/system_api/dbus/power_manager/dbus-constants.h index 4471525..a4b7be8 100644 --- a/system_api/dbus/power_manager/dbus-constants.h +++ b/system_api/dbus/power_manager/dbus-constants.h
@@ -82,6 +82,8 @@ // Signals emitted by powerd. const char kScreenBrightnessChangedSignal[] = "ScreenBrightnessChanged"; +const char kExternalScreenBrightnessChangedSignal[] = + "ExternalScreenBrightnessChanged"; const char kKeyboardBrightnessChangedSignal[] = "KeyboardBrightnessChanged"; const char kPeripheralBatteryStatusSignal[] = "PeripheralBatteryStatus"; const char kPowerSupplyPollSignal[] = "PowerSupplyPoll";