DeviceStatusCollector: add ProbeError to BatteryInfo request

Update cros_healthd telemetry handling code to check for and log errors
from BatteryInfo request.

Bug: chromium:1041153
Change-Id: Ic88d6146cfd81468d7023f02f7e4d0671e05a68f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2153603
Commit-Queue: Trent Begin <tbegin@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Jesse Schettler <jschettler@chromium.org>
Reviewed-by: Paul Moy <pmoy@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762074}

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

@@ -870,32 +870,40 @@ class DeviceStatusCollectorState : public StatusCollectorState {
       }
     }
 
-    const auto& battery_info = probe_result->battery_info;
-    if (!battery_info.is_null()) {
-      em::PowerStatus* const power_status_out =
-          response_params_.device_status->mutable_power_status();
-      em::BatteryInfo* const battery_info_out =
-          power_status_out->add_batteries();
-      battery_info_out->set_serial(battery_info->serial_number);
-      battery_info_out->set_manufacturer(battery_info->vendor);
-      battery_info_out->set_cycle_count(battery_info->cycle_count);
-      battery_info_out->set_technology(battery_info->technology);
-      // Convert Ah to mAh:
-      battery_info_out->set_design_capacity(
-          std::lround(battery_info->charge_full_design * 1000));
-      battery_info_out->set_full_charge_capacity(
-          std::lround(battery_info->charge_full * 1000));
-      // Convert V to mV:
-      battery_info_out->set_design_min_voltage(
-          std::lround(battery_info->voltage_min_design * 1000));
-      const auto& smart_info = battery_info->smart_battery_info;
-      if (!smart_info.is_null())
-        battery_info_out->set_manufacture_date(smart_info->manufacture_date);
-
-      for (const std::unique_ptr<SampledData>& sample_data : samples) {
-        auto it = sample_data->battery_samples.find(battery_info->model_name);
-        if (it != sample_data->battery_samples.end())
-          battery_info_out->add_samples()->CheckTypeAndMergeFrom(it->second);
+    // Process BatteryResult.
+    const auto& battery_result = probe_result->battery_result;
+    if (!battery_result.is_null()) {
+      if (battery_result->is_error()) {
+        LOG(ERROR) << "cros_healthd: Error getting battery info: "
+                   << battery_result->get_error()->msg;
+      } else if (!battery_result->get_battery_info().is_null()) {
+        const auto& battery_info = battery_result->get_battery_info();
+        em::PowerStatus* const power_status_out =
+            response_params_.device_status->mutable_power_status();
+        em::BatteryInfo* const battery_info_out =
+            power_status_out->add_batteries();
+        battery_info_out->set_serial(battery_info->serial_number);
+        battery_info_out->set_manufacturer(battery_info->vendor);
+        battery_info_out->set_cycle_count(battery_info->cycle_count);
+        battery_info_out->set_technology(battery_info->technology);
+        // Convert Ah to mAh:
+        battery_info_out->set_design_capacity(
+            std::lround(battery_info->charge_full_design * 1000));
+        battery_info_out->set_full_charge_capacity(
+            std::lround(battery_info->charge_full * 1000));
+        // Convert V to mV:
+        battery_info_out->set_design_min_voltage(
+            std::lround(battery_info->voltage_min_design * 1000));
+        if (battery_info->manufacture_date) {
+          battery_info_out->set_manufacture_date(
+              battery_info->manufacture_date.value());
+        }
+
+        for (const std::unique_ptr<SampledData>& sample_data : samples) {
+          auto it = sample_data->battery_samples.find(battery_info->model_name);
+          if (it != sample_data->battery_samples.end())
+            battery_info_out->add_samples()->CheckTypeAndMergeFrom(it->second);
+        }
       }
     }
 
@@ -1480,25 +1488,30 @@ void DeviceStatusCollector::SampleProbeData(
   if (result.is_null())
     return;
 
-  const auto& battery = result->battery_info;
-  if (!battery.is_null()) {
-    enterprise_management::BatterySample battery_sample;
-    battery_sample.set_timestamp(sample->timestamp.ToJavaTime());
-    // Convert V to mV:
-    battery_sample.set_voltage(std::lround(battery->voltage_now * 1000));
-    // Convert Ah to mAh:
-    battery_sample.set_remaining_capacity(
-        std::lround(battery->charge_now * 1000));
-    // Convert A to mA:
-    battery_sample.set_current(std::lround(battery->current_now * 1000));
-    battery_sample.set_status(battery->status);
-    // Convert 0.1 Kelvin to Celsius:
-    const auto& smart_info = battery->smart_battery_info;
-    if (!smart_info.is_null()) {
-      battery_sample.set_temperature(
-          (smart_info->temperature - kZeroCInDeciKelvin) / 10);
+  const auto& battery_result = result->battery_result;
+  if (!battery_result.is_null()) {
+    if (battery_result->is_error()) {
+      LOG(ERROR) << "cros_healthd: Error getting battery info: "
+                 << battery_result->get_error()->msg;
+    } else if (!battery_result->get_battery_info().is_null()) {
+      const auto& battery = battery_result->get_battery_info();
+      enterprise_management::BatterySample battery_sample;
+      battery_sample.set_timestamp(sample->timestamp.ToJavaTime());
+      // Convert V to mV:
+      battery_sample.set_voltage(std::lround(battery->voltage_now * 1000));
+      // Convert Ah to mAh:
+      battery_sample.set_remaining_capacity(
+          std::lround(battery->charge_now * 1000));
+      // Convert A to mA:
+      battery_sample.set_current(std::lround(battery->current_now * 1000));
+      battery_sample.set_status(battery->status);
+      // Convert 0.1 Kelvin to Celsius:
+      if (battery->temperature) {
+        battery_sample.set_temperature(
+            (battery->temperature->value - kZeroCInDeciKelvin) / 10);
+      }
+      sample->battery_samples[battery->model_name] = battery_sample;
     }
-    sample->battery_samples[battery->model_name] = battery_sample;
   }
 
   SamplingCallback completion_callback;

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

@@ -443,14 +443,18 @@ void GetEmptyCrosHealthdData(
 void GetFakeCrosHealthdData(
     policy::DeviceStatusCollector::CrosHealthdDataReceiver receiver) {
   // Create fake TelemetryInfo.
-  chromeos::cros_healthd::mojom::SmartBatteryInfo smart_battery_info(
-      kFakeSmartBatteryManufactureDate, kFakeSmartBatteryTemperature);
-  chromeos::cros_healthd::mojom::BatteryInfo battery_info(
-      kFakeBatteryCycleCount, kFakeBatteryVoltageNow, kFakeBatteryVendor,
-      kFakeBatterySerial, kFakeBatteryChargeFullDesign, kFakeBatteryChargeFull,
-      kFakeBatteryVoltageMinDesign, kFakeBatteryModel, kFakeBatteryChargeNow,
-      kFakeBatteryCurrentNow, kFakeBatteryTechnology, kFakeBatteryStatus,
-      smart_battery_info.Clone());
+  auto battery_result =
+      chromeos::cros_healthd::mojom::BatteryResult::NewBatteryInfo(
+          chromeos::cros_healthd::mojom::BatteryInfo::New(
+              kFakeBatteryCycleCount, kFakeBatteryVoltageNow,
+              kFakeBatteryVendor, kFakeBatterySerial,
+              kFakeBatteryChargeFullDesign, kFakeBatteryChargeFull,
+              kFakeBatteryVoltageMinDesign, kFakeBatteryModel,
+              kFakeBatteryChargeNow, kFakeBatteryCurrentNow,
+              kFakeBatteryTechnology, kFakeBatteryStatus,
+              kFakeSmartBatteryManufactureDate,
+              chromeos::cros_healthd::mojom::UInt64Value::New(
+                  kFakeSmartBatteryTemperature)));
   auto vpd_result = chromeos::cros_healthd::mojom::CachedVpdResult::NewVpdInfo(
       chromeos::cros_healthd::mojom::CachedVpdInfo::New(kFakeSkuNumber));
   chromeos::cros_healthd::mojom::CpuInfo cpu_info(
@@ -493,7 +497,7 @@ void GetFakeCrosHealthdData(
   auto fan_result = chromeos::cros_healthd::mojom::FanResult::NewFanInfo(
       std::move(fan_vector));
   chromeos::cros_healthd::mojom::TelemetryInfo fake_info(
-      battery_info.Clone(), std::move(block_device_result),
+      std::move(battery_result), std::move(block_device_result),
       std::move(vpd_result), std::move(cpu_result), std::move(timezone_result),
       std::move(memory_result), std::move(backlight_result),
       std::move(fan_result));
@@ -511,7 +515,7 @@ void GetFakeCrosHealthdData(
   fake_battery_sample.set_status(kFakeBatteryStatus);
   auto sample = std::make_unique<policy::SampledData>();
   sample->cpu_samples[fake_cpu_temp_sample.cpu_label()] = fake_cpu_temp_sample;
-  sample->battery_samples[battery_info.model_name] = fake_battery_sample;
+  sample->battery_samples[kFakeBatteryModel] = fake_battery_sample;
   base::circular_deque<std::unique_ptr<policy::SampledData>> samples;
   samples.push_back(std::move(sample));
 

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

@@ -86,17 +86,16 @@ mojom::NonRemovableBlockDeviceResultPtr MakeNonRemovableBlockDeviceResult() {
       std::move(info));
 }
 
-mojom::BatteryInfoPtr MakeBatteryInfo() {
-  mojom::SmartBatteryInfoPtr smart_info = mojom::SmartBatteryInfo::New(
-      "2018-08-06" /* manufacture_date */, 981729 /* temperature */);
-  return mojom::BatteryInfo::New(
+mojom::BatteryResultPtr MakeBatteryResult() {
+  return mojom::BatteryResult::NewBatteryInfo(mojom::BatteryInfo::New(
       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 */, "battery_model" /* model_name */,
       5.123 /* charge_now */, 98.123 /* current_now */,
       "battery_technology" /* technology */, "battery_status" /* status */,
-      std::move(smart_info));
+      "2018-08-06" /* manufacture_date */,
+      mojom::UInt64Value::New(981729) /* temperature */));
 }
 
 mojom::CachedVpdResultPtr MakeCachedVpdResult() {
@@ -147,7 +146,7 @@ mojom::FanResultPtr MakeFanResult() {
 
 mojom::TelemetryInfoPtr MakeTelemetryInfo() {
   return mojom::TelemetryInfo::New(
-      MakeBatteryInfo() /* battery_info */,
+      MakeBatteryResult() /* battery_result */,
       MakeNonRemovableBlockDeviceResult() /* block_device_result */,
       MakeCachedVpdResult() /* vpd_result */, MakeCpuResult() /* cpu_result */,
       MakeTimezoneResult() /* timezone_result */,

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

@@ -51,13 +51,20 @@ struct ProbeError {
   string msg;
 };
 
-// Information related to a Smart Battery, as defined in
-// http://sbs-forum.org/specs/sbdat110.pdf.
-struct SmartBatteryInfo {
-  // Manufacture date converted to yyyy-mm-dd format.
-  string manufacture_date;
-  // Temperature in 0.1K.
-  uint64 temperature;
+// Optional uint64 field. Since primitives numeric types cannot be optional,
+// wrap uint64 in a struct that can be nulled.
+struct UInt64Value {
+  // The value of the uint64.
+  uint64 value;
+};
+
+// Battery probe result. Can either be populated with the BatteryInfo or an
+// error retrieving the information.
+union BatteryResult {
+  // Valid BatteryInfo. Null value if a battery is not present.
+  BatteryInfo? battery_info;
+  // The error that occurred attempting to retrieve the BatteryInfo.
+  ProbeError error;
 };
 
 // Information related to the main battery.
@@ -86,9 +93,14 @@ struct BatteryInfo {
   string technology;
   // Status of the battery
   string status;
-  // Information related to a Smart Battery. Included when the main battery is a
-  // Smart Battery.
-  SmartBatteryInfo? smart_battery_info;
+
+  // The fields below are optionally included if the main battery is a Smart
+  // Battery as defined in http://sbs-forum.org/specs/sbdat110.pdf.
+
+  // Manufacture date converted to yyyy-mm-dd format.
+  string? manufacture_date;
+  // Temperature in 0.1K. Included when the main battery is a Smart Battery.
+  UInt64Value? temperature;
 };
 
 // Non-removable block device probe result. Can either be populated with the
@@ -235,7 +247,7 @@ struct FanInfo {
 struct TelemetryInfo {
   // Information about the device's main battery. Only present when kBattery was
   // included in the categories input to ProbeTelemetryInfo.
-  BatteryInfo? battery_info;
+  BatteryResult? battery_result;
   // Information about all of the device's non-removable block devices. Only
   // present when kNonRemovableBlockDevices was included in the categories input
   // to ProbeTelemetryInfo.