Observe PowerManagerClient from SystemDataProvider

This change makes SystemDataProvider an observer of PowerManagerClient.
Whenever UpdateBatteryChargeStatus is called or an event is received
from PowerManagerClient, updated BatteryChargeStatus information will be
fetched from CrosHealthd + PowerManagerClient.

Bug: 1128204
Change-Id: Icecaabf56da030734804194acc38e3716b01d8b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2431726
Commit-Queue: Bailey Berro <baileyberro@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812727}

chromeos/components/diagnostics_ui/backend/system_data_provider.cc

@@ -12,6 +12,8 @@
 #include "base/callback.h"
 #include "base/optional.h"
 #include "chromeos/components/diagnostics_ui/backend/cros_healthd_helpers.h"
+#include "chromeos/components/diagnostics_ui/backend/power_manager_client_conversions.h"
+#include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
 #include "chromeos/services/cros_healthd/public/cpp/service_connection.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd_probe.mojom.h"
 
@@ -21,6 +23,7 @@ namespace {
 
 namespace healthd = cros_healthd::mojom;
 using PhysicalCpuInfos = std::vector<healthd::PhysicalCpuInfoPtr>;
+using PowerSupplyProperties = power_manager::PowerSupplyProperties;
 using ProbeCategories = healthd::ProbeCategoryEnum;
 
 void PopulateBoardName(const healthd::SystemInfo& system_info,
@@ -58,10 +61,20 @@ void PopulateMemorySize(const healthd::MemoryInfo& memory_info,
   out_system_info.total_memory_kib = memory_info.total_memory_kib;
 }
 
+bool DoesDeviceHaveBattery(
+    const PowerSupplyProperties& power_supply_properties) {
+  return power_supply_properties.battery_state() !=
+         power_manager::PowerSupplyProperties_BatteryState_NOT_PRESENT;
+}
+
+bool DoesDeviceHaveBattery(const healthd::TelemetryInfo& telemetry_info) {
+  return GetBatteryInfo(telemetry_info) != nullptr;
+}
+
 void PopulateDeviceCapabilities(const healthd::TelemetryInfo& telemetry_info,
                                 mojom::SystemInfo& out_system_info) {
   mojom::DeviceCapabilitiesPtr capabilities = mojom::DeviceCapabilities::New();
-  capabilities->has_battery = GetBatteryInfo(telemetry_info) != nullptr;
+  capabilities->has_battery = DoesDeviceHaveBattery(telemetry_info);
   out_system_info.device_capabilities = std::move(capabilities);
 }
 
@@ -73,10 +86,38 @@ void PopulateBatteryInfo(const healthd::BatteryInfo& battery_info,
       battery_info.charge_full_design * 1000;
 }
 
+void PopulatePowerInfo(const PowerSupplyProperties& power_supply_properties,
+                       mojom::BatteryChargeStatus& out_charge_status) {
+  const mojom::BatteryState battery_state =
+      ConvertBatteryStateFromProto(power_supply_properties.battery_state());
+
+  out_charge_status.battery_state = battery_state;
+  out_charge_status.power_time =
+      ConstructPowerTime(battery_state, power_supply_properties);
+  out_charge_status.power_adapter_status =
+      ConvertPowerSourceFromProto(power_supply_properties.external_power());
+}
+
+void PopulateBatteryChargeStatus(
+    const healthd::BatteryInfo& battery_info,
+    const PowerSupplyProperties& power_supply_properties,
+    mojom::BatteryChargeStatus& out_charge_status) {
+  PopulatePowerInfo(power_supply_properties, out_charge_status);
+
+  // Multiply by 1000 to convert amps to milliamps.
+  out_charge_status.current_now_milliamps = battery_info.current_now * 1000;
+  out_charge_status.charge_now_milliamp_hours = battery_info.charge_now * 1000;
+}
+
 }  // namespace
 
-SystemDataProvider::SystemDataProvider() = default;
-SystemDataProvider::~SystemDataProvider() = default;
+SystemDataProvider::SystemDataProvider() {
+  PowerManagerClient::Get()->AddObserver(this);
+}
+
+SystemDataProvider::~SystemDataProvider() {
+  PowerManagerClient::Get()->RemoveObserver(this);
+}
 
 void SystemDataProvider::GetSystemInfo(GetSystemInfoCallback callback) {
   BindCrosHealthdProbeServiceIfNeccessary();
@@ -97,6 +138,15 @@ void SystemDataProvider::GetBatteryInfo(GetBatteryInfoCallback callback) {
                      base::Unretained(this), std::move(callback)));
 }
 
+void SystemDataProvider::PowerChanged(const PowerSupplyProperties& proto) {
+  // Fetch updated data from CrosHealthd
+  BindCrosHealthdProbeServiceIfNeccessary();
+  probe_service_->ProbeTelemetryInfo(
+      {ProbeCategories::kBattery},
+      base::BindOnce(&SystemDataProvider::OnBatteryChargeStatusUpdated,
+                     base::Unretained(this), proto));
+}
+
 void SystemDataProvider::OnSystemInfoProbeResponse(
     GetSystemInfoCallback callback,
     healthd::TelemetryInfoPtr info_ptr) {
@@ -164,6 +214,58 @@ void SystemDataProvider::OnBatteryInfoProbeResponse(
   std::move(callback).Run(std::move(battery_info));
 }
 
+void SystemDataProvider::UpdateBatteryChargeStatus() {
+  // Fetch updated data from PowerManagerClient
+  base::Optional<PowerSupplyProperties> properties =
+      PowerManagerClient::Get()->GetLastStatus();
+
+  // Fetch updated data from CrosHealthd
+  BindCrosHealthdProbeServiceIfNeccessary();
+
+  probe_service_->ProbeTelemetryInfo(
+      {ProbeCategories::kBattery},
+      base::BindOnce(&SystemDataProvider::OnBatteryChargeStatusUpdated,
+                     base::Unretained(this), properties));
+}
+
+void SystemDataProvider::OnBatteryChargeStatusUpdated(
+    const base::Optional<PowerSupplyProperties>& power_supply_properties,
+    healthd::TelemetryInfoPtr info_ptr) {
+  mojom::BatteryChargeStatusPtr battery_charge_status =
+      mojom::BatteryChargeStatus::New();
+
+  if (info_ptr.is_null()) {
+    LOG(ERROR) << "Null response from croshealthd::ProbeTelemetryInfo.";
+    NotifyBatteryChargeStatusObservers(battery_charge_status);
+    return;
+  }
+
+  if (!power_supply_properties.has_value()) {
+    LOG(ERROR) << "Null response from power_manager_client::GetLastStatus.";
+    NotifyBatteryChargeStatusObservers(battery_charge_status);
+    return;
+  }
+
+  if (!DoesDeviceHaveBattery(*info_ptr) ||
+      !DoesDeviceHaveBattery(*power_supply_properties)) {
+    DCHECK_EQ(DoesDeviceHaveBattery(*info_ptr),
+              DoesDeviceHaveBattery(*power_supply_properties))
+        << "Sources should not disagree about whether there is a battery.";
+    NotifyBatteryChargeStatusObservers(battery_charge_status);
+    return;
+  }
+
+  PopulateBatteryChargeStatus(*diagnostics::GetBatteryInfo(*info_ptr),
+                              *power_supply_properties,
+                              *battery_charge_status.get());
+  NotifyBatteryChargeStatusObservers(battery_charge_status);
+}
+
+void SystemDataProvider::NotifyBatteryChargeStatusObservers(
+    const mojom::BatteryChargeStatusPtr& battery_charge_status) {
+  NOTIMPLEMENTED();  // Implemented in subsequent CL.
+}
+
 void SystemDataProvider::BindCrosHealthdProbeServiceIfNeccessary() {
   if (!probe_service_ || !probe_service_.is_connected()) {
     cros_healthd::ServiceConnection::GetInstance()->GetProbeService(

chromeos/components/diagnostics_ui/backend/system_data_provider.h

@@ -5,14 +5,19 @@
 #ifndef CHROMEOS_COMPONENTS_DIAGNOSTICS_UI_BACKEND_SYSTEM_DATA_PROVIDER_H_
 #define CHROMEOS_COMPONENTS_DIAGNOSTICS_UI_BACKEND_SYSTEM_DATA_PROVIDER_H_
 
+#include <memory>
+
+#include "base/optional.h"
 #include "chromeos/components/diagnostics_ui/mojom/system_data_provider.mojom.h"
+#include "chromeos/dbus/power/power_manager_client.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd.mojom.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
 namespace chromeos {
 namespace diagnostics {
 
-class SystemDataProvider : public mojom::SystemDataProvider {
+class SystemDataProvider : public mojom::SystemDataProvider,
+                           public PowerManagerClient::Observer {
  public:
   SystemDataProvider();
 
@@ -25,6 +30,9 @@ class SystemDataProvider : public mojom::SystemDataProvider {
   void GetSystemInfo(GetSystemInfoCallback callback) override;
   void GetBatteryInfo(GetBatteryInfoCallback callback) override;
 
+  // PowerManagerClient::Observer:
+  void PowerChanged(const power_manager::PowerSupplyProperties& proto) override;
+
  private:
   void BindCrosHealthdProbeServiceIfNeccessary();
 
@@ -38,6 +46,16 @@ class SystemDataProvider : public mojom::SystemDataProvider {
       GetBatteryInfoCallback callback,
       cros_healthd::mojom::TelemetryInfoPtr info_ptr);
 
+  void UpdateBatteryChargeStatus();
+
+  void NotifyBatteryChargeStatusObservers(
+      const mojom::BatteryChargeStatusPtr& battery_charge_status);
+
+  void OnBatteryChargeStatusUpdated(
+      const base::Optional<power_manager::PowerSupplyProperties>&
+          power_supply_properties,
+      cros_healthd::mojom::TelemetryInfoPtr info_ptr);
+
   mojo::Remote<cros_healthd::mojom::CrosHealthdProbeService> probe_service_;
 };
 

chromeos/components/diagnostics_ui/backend/system_data_provider_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/time/time.h"
 #include "chromeos/dbus/cros_healthd/cros_healthd_client.h"
 #include "chromeos/dbus/cros_healthd/fake_cros_healthd_client.h"
+#include "chromeos/dbus/power/fake_power_manager_client.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd.mojom.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd_probe.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -144,12 +145,15 @@ void SetCrosHealthdBatteryInfoResponse(const std::string& vendor,
 class SystemDataProviderTest : public testing::Test {
  public:
   SystemDataProviderTest() {
+    chromeos::PowerManagerClient::InitializeFake();
     chromeos::CrosHealthdClient::InitializeFake();
     system_data_provider_ = std::make_unique<SystemDataProvider>();
   }
 
   ~SystemDataProviderTest() override {
+    system_data_provider_.reset();
     chromeos::CrosHealthdClient::Shutdown();
+    chromeos::PowerManagerClient::Shutdown();
     base::RunLoop().RunUntilIdle();
   }
 

chromeos/components/diagnostics_ui/mojom/system_data_provider.mojom

@@ -4,6 +4,8 @@
 
 module chromeos.diagnostics.mojom;
 
+import "mojo/public/mojom/base/string16.mojom";
+
 // Contains the capabilities of the device.
 struct DeviceCapabilities {
   bool has_battery;
@@ -42,6 +44,17 @@ enum BatteryState {
   kFull,
 };
 
+// Contains information about the charging state of the battery.
+struct BatteryChargeStatus {
+  // String containing a localized time duration. If the time is unreliable
+  // the string will be empty.
+  mojo_base.mojom.String16 power_time;
+  int32 current_now_milliamps;
+  uint32 charge_now_milliamp_hours;
+  BatteryState battery_state;
+  ExternalPowerSource power_adapter_status;
+};
+
 // Provides telemetric information about the system. This API is exposed to the
 // Diagnostics SWA.
 interface SystemDataProvider {