DeviceStatusCollector: complete migration to cros_healthd

Migrate the battery metric collection from runtime_probe
to cros_healthd.

manual testing, where I cherry-pick crrev.com/c/1779132
and checked /var/log/chrome/chrome for the expected
values.

Bug: b:128683357
Test: DeviceStatusCollectorTest.TestCrosHealthdInfo and
Change-Id: Ia0290d189d7eabf1ec8e878edcd4f56b7025bb04
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1847177Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Commit-Queue: Paul Moy <pmoy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707685}

chrome/browser/chromeos/policy/status_collector/device_status_collector.h

@@ -28,8 +28,6 @@
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chromeos/dbus/cryptohome/cryptohome_client.h"
 #include "chromeos/dbus/power/power_manager_client.h"
-#include "chromeos/dbus/runtime_probe/runtime_probe.pb.h"
-#include "chromeos/dbus/runtime_probe_client.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd.mojom.h"
 #include "components/policy/proto/device_management_backend.pb.h"
 #include "components/prefs/pref_member.h"
@@ -149,17 +147,10 @@ class DeviceStatusCollector : public StatusCollector,
   // Gets the TpmStatusInfo and passes it to TpmStatusReceiver.
   using TpmStatusFetcher = base::RepeatingCallback<void(TpmStatusReceiver)>;
 
-  // Format of the function that asynchronously receives ProbeResult / sampled
-  // data.
-  using ProbeDataReceiver = base::OnceCallback<void(
-      const base::Optional<runtime_probe::ProbeResult>&,
-      const base::circular_deque<std::unique_ptr<SampledData>>&)>;
-  // Gets the ProbeResult/sampled data and passes it to ProbeDataReceiver.
-  using ProbeDataFetcher = base::RepeatingCallback<void(ProbeDataReceiver)>;
-
   // Format of the function that asynchronously receives data from cros_healthd.
-  using CrosHealthdDataReceiver =
-      base::OnceCallback<void(chromeos::cros_healthd::mojom::TelemetryInfoPtr)>;
+  using CrosHealthdDataReceiver = base::OnceCallback<void(
+      chromeos::cros_healthd::mojom::TelemetryInfoPtr,
+      const base::circular_deque<std::unique_ptr<SampledData>>&)>;
   // Gets the data from cros_healthd and passes it to CrosHealthdDataReceiver.
   using CrosHealthdDataFetcher =
       base::RepeatingCallback<void(CrosHealthdDataReceiver)>;
@@ -240,7 +231,7 @@ class DeviceStatusCollector : public StatusCollector,
   // Callbacks used during sampling data collection, that allows to pass
   // additional data using partial function application.
   using SamplingProbeResultCallback =
-      base::OnceCallback<void(base::Optional<runtime_probe::ProbeResult>)>;
+      base::OnceCallback<void(chromeos::cros_healthd::mojom::TelemetryInfoPtr)>;
   using SamplingCallback = base::OnceCallback<void()>;
 
   // Clears the cached hardware resource usage.
@@ -296,11 +287,11 @@ class DeviceStatusCollector : public StatusCollector,
   // Callback invoked to update our cpu usage information.
   void ReceiveCPUStatistics(const std::string& statistics);
 
-  // Callback for RuntimeProbe that samples probe live data. |callback| will
+  // Callback for CrosHealthd that samples probe live data. |callback| will
   // be called once all sampling is finished.
   void SampleProbeData(std::unique_ptr<SampledData> sample,
                        SamplingProbeResultCallback callback,
-                       base::Optional<runtime_probe::ProbeResult> result);
+                       chromeos::cros_healthd::mojom::TelemetryInfoPtr result);
 
   // Callback triggered from PowerManagedClient that samples battery discharge
   // rate. |callback| will be called once all sampling is finished.
@@ -321,14 +312,11 @@ class DeviceStatusCollector : public StatusCollector,
   // cros_healthd and passes it to |callback|.
   void FetchCrosHealthdData(CrosHealthdDataReceiver callback);
 
-  // ProbeDataReceiver interface implementation, fetches data from
-  // RuntimeProbe passes it to |callback| via OnProbeDataFetched().
-  void FetchProbeData(ProbeDataReceiver callback);
-
-  // Callback for RuntimeProbe that performs final sampling and
+  // Callback for CrosHealthd that performs final sampling and
   // actually invokes |callback|.
-  void OnProbeDataFetched(ProbeDataReceiver callback,
-                          base::Optional<runtime_probe::ProbeResult> reply);
+  void OnProbeDataFetched(
+      CrosHealthdDataReceiver callback,
+      chromeos::cros_healthd::mojom::TelemetryInfoPtr reply);
 
   // Callback invoked when reporting users pref is changed.
   void ReportingUsersChanged();
@@ -403,8 +391,6 @@ class DeviceStatusCollector : public StatusCollector,
 
   TpmStatusFetcher tpm_status_fetcher_;
 
-  ProbeDataFetcher probe_data_fetcher_;
-
   EMMCLifetimeFetcher emmc_lifetime_fetcher_;
 
   StatefulPartitionInfoFetcher stateful_partition_info_fetcher_;
@@ -416,9 +402,6 @@ class DeviceStatusCollector : public StatusCollector,
   // Power manager client. Used to listen to power changed events.
   chromeos::PowerManagerClient* const power_manager_;
 
-  // Runtime probe client. Used to fetch hardware data.
-  chromeos::RuntimeProbeClient* const runtime_probe_;
-
   // The most recent CPU readings.
   uint64_t last_cpu_active_ = 0;
   uint64_t last_cpu_idle_ = 0;

chrome/browser/chromeos/policy/status_collector/device_status_collector_browsertest.cc

@@ -359,35 +359,35 @@ em::StatefulPartitionInfo GetFakeStatefulPartitionInfo(
 void GetEmptyCrosHealthdData(
     policy::DeviceStatusCollector::CrosHealthdDataReceiver receiver) {
   chromeos::cros_healthd::mojom::TelemetryInfoPtr empty_info;
-  std::move(receiver).Run(std::move(empty_info));
+  base::circular_deque<std::unique_ptr<policy::SampledData>> empty_samples;
+  std::move(receiver).Run(std::move(empty_info), empty_samples);
 }
 
 void GetFakeCrosHealthdData(
-    const std::string& sku_number,
-    const std::string& path,
-    int size,
-    const std::string& type,
-    uint8_t manufacturer_id,
-    const std::string& name,
-    int serial,
+    const chromeos::cros_healthd::mojom::BatteryInfo& battery_info,
+    const chromeos::cros_healthd::mojom::CachedVpdInfo& cached_vpd_info,
+    const chromeos::cros_healthd::mojom::NonRemovableBlockDeviceInfo&
+        storage_info,
+    const em::CPUTempInfo& cpu_sample,
+    const em::BatterySample& battery_sample,
     policy::DeviceStatusCollector::CrosHealthdDataReceiver receiver) {
-  chromeos::cros_healthd::mojom::NonRemovableBlockDeviceInfoPtr vector_init[] =
-      {chromeos::cros_healthd::mojom::NonRemovableBlockDeviceInfo::New(
-          path, size, type, manufacturer_id, name, serial)};
+  std::vector<chromeos::cros_healthd::mojom::NonRemovableBlockDeviceInfoPtr>
+      storage_vector;
+  storage_vector.push_back(storage_info.Clone());
+  base::Optional<std::vector<
+      chromeos::cros_healthd::mojom::NonRemovableBlockDeviceInfoPtr>>
+      block_device_info(std::move(storage_vector));
   chromeos::cros_healthd::mojom::TelemetryInfo fake_info(
-      chromeos::cros_healthd::mojom::BatteryInfo::New(), /* battery_info */
-      base::Optional<std::vector<
-          chromeos::cros_healthd::mojom::NonRemovableBlockDeviceInfoPtr>>(
-          std::vector<
-              chromeos::cros_healthd::mojom::NonRemovableBlockDeviceInfoPtr>{
-              std::make_move_iterator(std::begin(vector_init)),
-              std::make_move_iterator(
-                  std::end(vector_init))}), /* block_device_info */
-      chromeos::cros_healthd::mojom::CachedVpdInfo::New(
-          sku_number) /* vpd_info */
-  );
+      battery_info.Clone(), std::move(block_device_info),
+      cached_vpd_info.Clone());
 
-  std::move(receiver).Run(fake_info.Clone());
+  auto sample = std::make_unique<policy::SampledData>();
+  sample->cpu_samples[cpu_sample.cpu_label()] = cpu_sample;
+  sample->battery_samples[battery_info.model_name] = battery_sample;
+  base::circular_deque<std::unique_ptr<policy::SampledData>> samples;
+  samples.push_back(std::move(sample));
+
+  std::move(receiver).Run(fake_info.Clone(), samples);
 }
 
 }  // namespace
@@ -2399,13 +2399,64 @@ TEST_F(DeviceStatusCollectorTest, TestStatefulPartitionInfo) {
 TEST_F(DeviceStatusCollectorTest, TestCrosHealthdInfo) {
   // Create a fake response from cros_healthd and populate it with some
   // arbitrary values.
+
+  // Cached VPD test values.
   constexpr char kFakeSkuNumber[] = "fake_sku_number";
-  constexpr char kFakePath[] = "fake_path";
-  constexpr int kFakeSize = 123;
-  constexpr char kFakeType[] = "fake_type";
-  constexpr uint8_t kFakeManfid = 2;
-  constexpr char kFakeName[] = "fake_name";
-  constexpr int kFakeSerial = 789;
+
+  // Storage test values.
+  constexpr char kFakeStoragePath[] = "fake_storage_path";
+  constexpr int kFakeStorageSize = 123;
+  constexpr char kFakeStorageType[] = "fake_storage_type";
+  constexpr uint8_t kFakeStorageManfid = 2;
+  constexpr char kFakeStorageName[] = "fake_storage_name";
+  constexpr int kFakeStorageSerial = 789;
+
+  // Battery test values.
+  constexpr int kFakeCycleCount = 3;
+  constexpr int kExpectedVoltageNow = 12574;                        // (mV)
+  constexpr double kFakeVoltageNow = kExpectedVoltageNow / 1000.0;  // (V)
+  constexpr char kFakeBatteryVendor[] = "fake_battery_vendor";
+  constexpr char kFakeBatterySerial[] = "fake_battery_serial";
+  constexpr int kExpectedChargeFullDesign = 5275;  // (mAh)
+  constexpr double kFakeChargeFullDesign =
+      kExpectedChargeFullDesign / 1000.0;                           // (Ah)
+  constexpr int kExpectedChargeFull = 5292;                         // (mAh)
+  constexpr double kFakeChargeFull = kExpectedChargeFull / 1000.0;  // (Ah)
+  constexpr int kExpectedVoltageMinDesign = 11550;                  // (mV)
+  constexpr double kFakeVoltageMinDesign =
+      kExpectedVoltageMinDesign / 1000.0;  // (V)
+  constexpr int kFakeManufactureDateSmart = 19718;
+  constexpr int kFakeTemperatureSmart = 3004;
+  constexpr char kFakeBatteryModel[] = "fake_battery_model";
+  constexpr int kExpectedChargeNow = 5281;                        // (mAh)
+  constexpr double kFakeChargeNow = kExpectedChargeNow / 1000.0;  // (Ah)
+
+  // CPU Temperature test values.
+  constexpr char kFakeCpuLabel[] = "fake_cpu_label";
+  constexpr int kFakeCpuTemp = 91832;
+  constexpr int kFakeCpuTimestamp = 912;
+
+  chromeos::cros_healthd::mojom::BatteryInfo battery_info(
+      kFakeCycleCount, kFakeVoltageNow, kFakeBatteryVendor, kFakeBatterySerial,
+      kFakeChargeFullDesign, kFakeChargeFull, kFakeVoltageMinDesign,
+      kFakeManufactureDateSmart, kFakeTemperatureSmart, kFakeBatteryModel,
+      kFakeChargeNow);
+  chromeos::cros_healthd::mojom::CachedVpdInfo cached_vpd_info(kFakeSkuNumber);
+  chromeos::cros_healthd::mojom::NonRemovableBlockDeviceInfo storage_info(
+      kFakeStoragePath, kFakeStorageSize, kFakeStorageType, kFakeStorageManfid,
+      kFakeStorageName, kFakeStorageSerial);
+
+  // Create a fake sample to test with.
+  em::CPUTempInfo fake_cpu_temp_sample;
+  fake_cpu_temp_sample.set_cpu_label(kFakeCpuLabel);
+  fake_cpu_temp_sample.set_cpu_temp(kFakeCpuTemp);
+  fake_cpu_temp_sample.set_timestamp(kFakeCpuTimestamp);
+  em::BatterySample fake_battery_sample;
+  // Convert from V to mV.
+  fake_battery_sample.set_voltage(kExpectedVoltageNow);
+  // Convert from Ah to mAh.
+  fake_battery_sample.set_remaining_capacity(kExpectedChargeNow);
+  fake_battery_sample.set_temperature(kFakeTemperatureSmart);
 
   RestartStatusCollector(
       base::BindRepeating(&GetEmptyVolumeInfo),
@@ -2415,9 +2466,9 @@ TEST_F(DeviceStatusCollectorTest, TestCrosHealthdInfo) {
       base::BindRepeating(&GetEmptyTpmStatus),
       base::BindRepeating(&GetEmptyEMMCLifetimeEstimation),
       base::BindRepeating(&GetEmptyStatefulPartitionInfo),
-      base::BindRepeating(&GetFakeCrosHealthdData, kFakeSkuNumber, kFakePath,
-                          kFakeSize, kFakeType, kFakeManfid, kFakeName,
-                          kFakeSerial));
+      base::BindRepeating(&GetFakeCrosHealthdData, battery_info,
+                          cached_vpd_info, storage_info, fake_cpu_temp_sample,
+                          fake_battery_sample));
 
   // If neither kReportDevicePowerStatus nor kReportDeviceStorageStatus are set,
   // expect that the data from cros_healthd isn't present in the protobuf.
@@ -2426,6 +2477,7 @@ TEST_F(DeviceStatusCollectorTest, TestCrosHealthdInfo) {
   scoped_testing_cros_settings_.device_settings()->SetBoolean(
       chromeos::kReportDeviceStorageStatus, false);
   GetStatus();
+  EXPECT_FALSE(device_status_.has_power_status());
   EXPECT_FALSE(device_status_.has_storage_status());
   EXPECT_FALSE(device_status_.has_system_status());
 
@@ -2436,20 +2488,45 @@ TEST_F(DeviceStatusCollectorTest, TestCrosHealthdInfo) {
   scoped_testing_cros_settings_.device_settings()->SetBoolean(
       chromeos::kReportDeviceStorageStatus, true);
   GetStatus();
+
+  // Check that the CPU temperature samples are stored correctly.
+  ASSERT_EQ(device_status_.cpu_temp_infos_size(), 1);
+  const auto& cpu_sample = device_status_.cpu_temp_infos(0);
+  EXPECT_EQ(cpu_sample.cpu_label(), kFakeCpuLabel);
+  EXPECT_EQ(cpu_sample.cpu_temp(), kFakeCpuTemp);
+  EXPECT_EQ(cpu_sample.timestamp(), kFakeCpuTimestamp);
+
+  // Verify the battery data.
+  ASSERT_TRUE(device_status_.has_power_status());
+  ASSERT_EQ(device_status_.power_status().batteries_size(), 1);
+  const auto& battery = device_status_.power_status().batteries(0);
+  EXPECT_EQ(battery.serial(), kFakeBatterySerial);
+  EXPECT_EQ(battery.manufacturer(), kFakeBatteryVendor);
+  EXPECT_EQ(battery.design_capacity(), kExpectedChargeFullDesign);
+  EXPECT_EQ(battery.full_charge_capacity(), kExpectedChargeFull);
+  EXPECT_EQ(battery.cycle_count(), kFakeCycleCount);
+  EXPECT_EQ(battery.design_min_voltage(), kExpectedVoltageMinDesign);
+  EXPECT_EQ(battery.manufacture_date(), "2018-08-06");
+
+  // Verify the battery sample data.
+  ASSERT_EQ(battery.samples_size(), 1);
+  const auto& battery_sample = battery.samples(0);
+  EXPECT_EQ(battery_sample.voltage(), kExpectedVoltageNow);
+  EXPECT_EQ(battery_sample.remaining_capacity(), kExpectedChargeNow);
+  EXPECT_EQ(battery_sample.temperature(), kFakeTemperatureSmart);
+
+  // Verify the storage data.
   ASSERT_TRUE(device_status_.has_storage_status());
   ASSERT_EQ(device_status_.storage_status().disks_size(), 1);
   const auto& disk = device_status_.storage_status().disks(0);
-  ASSERT_TRUE(disk.has_size());
-  EXPECT_EQ(disk.size(), kFakeSize);
-  ASSERT_TRUE(disk.has_type());
-  EXPECT_EQ(disk.type(), kFakeType);
-  ASSERT_TRUE(disk.has_manufacturer());
-  EXPECT_EQ(disk.manufacturer(), base::NumberToString(kFakeManfid));
-  ASSERT_TRUE(disk.has_model());
-  EXPECT_EQ(disk.model(), kFakeName);
-  ASSERT_TRUE(disk.has_serial());
-  EXPECT_EQ(disk.serial(), base::NumberToString(kFakeSerial));
-  ASSERT_TRUE(device_status_.system_status().has_vpd_sku_number());
+  EXPECT_EQ(disk.size(), kFakeStorageSize);
+  EXPECT_EQ(disk.type(), kFakeStorageType);
+  EXPECT_EQ(disk.manufacturer(), base::NumberToString(kFakeStorageManfid));
+  EXPECT_EQ(disk.model(), kFakeStorageName);
+  EXPECT_EQ(disk.serial(), base::NumberToString(kFakeStorageSerial));
+
+  // Verify the Cached VPD.
+  ASSERT_TRUE(device_status_.has_system_status());
   EXPECT_EQ(device_status_.system_status().vpd_sku_number(), kFakeSkuNumber);
 }
 

chromeos/services/cros_healthd/public/cpp/service_connection_unittest.cc

@@ -45,7 +45,9 @@ mojom::BatteryInfoPtr MakeBatteryInfo() {
       2 /* cycle_count */, 12.9 /* voltage_now */,
       "battery_vendor" /* vendor */, "serial_number" /* serial_number */,
       5.275 /* charge_full_design */, 5.292 /* charge_full */,
-      11.55 /* voltage_min_design */, 51785890 /* manufacture_date_smart */);
+      11.55 /* voltage_min_design */, 51785890 /* manufacture_date_smart */,
+      /*temperature smart=*/981729, /*model_name=*/"battery_model",
+      /*charge_now=*/5.123);
 }
 
 mojom::CachedVpdInfoPtr MakeCachedVpdInfo() {

chromeos/services/cros_healthd/public/mojom/cros_healthd_probe.mojom

@@ -38,6 +38,13 @@ struct BatteryInfo {
   // http://sbs-forum.org/specs/sbdat110.pdf. The value is calculated by
   // ((year-1980) * 512 + month * 32 + day).
   int64 manufacture_date_smart;
+  // Temperature in 0.1°K as Smart Battery Temperature defined in
+  // http://sbs-forum.org/specs/sbdat110.pdf
+  uint64 temperature_smart;
+  // Model name.
+  string model_name;
+  // Current battery charge (Ah)
+  double charge_now;
 };
 
 // Information related to a specific non-removable block device.