Commit ec986c82 authored by Paul Moy's avatar Paul Moy Committed by Commit Bot

cros_healthd: remove external parameters from two battery routines

Now that the battery health and capacity routines have good default
parameter values, and can look up board-specific parameters from
cros_config, we no longer need to expose these parameters to the
end user. This will make the routine easier to run effectively,
because the user doesn't need to know what good values for the
parameters are.

Bug: chromium:1131609
Change-Id: Ie4a11fefbce13b4777144c06e67139b91f53f839
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2477287
Commit-Queue: Paul Moy <pmoy@chromium.org>
Reviewed-by: default avatarOleh Lamzin <lamzin@google.com>
Reviewed-by: default avatarDaniel Cheng <dcheng@chromium.org>
Reviewed-by: default avatarMaksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819397}
parent ec812a1f
......@@ -137,56 +137,19 @@ void DeviceCommandRunRoutineJob::RunImpl(CallbackWithResult succeeded_callback,
switch (routine_enum_) {
case chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::
kBatteryCapacity: {
constexpr char kLowMahFieldName[] = "lowMah";
constexpr char kHighMahFieldName[] = "highMah";
base::Optional<int> low_mah = params_dict_.FindIntKey(kLowMahFieldName);
base::Optional<int> high_mah = params_dict_.FindIntKey(kHighMahFieldName);
// The battery capacity routine expects two integers >= 0.
if (!low_mah.has_value() || !high_mah.has_value() ||
low_mah.value() < 0 || high_mah.value() < 0) {
SYSLOG(ERROR) << "Invalid parameters for BatteryCapacity routine.";
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(failed_callback),
std::make_unique<Payload>(
MakeInvalidParametersResponse())));
break;
}
chromeos::cros_healthd::ServiceConnection::GetInstance()
->RunBatteryCapacityRoutine(
low_mah.value(), high_mah.value(),
base::BindOnce(
&DeviceCommandRunRoutineJob::OnCrosHealthdResponseReceived,
weak_ptr_factory_.GetWeakPtr(), std::move(succeeded_callback),
std::move(failed_callback)));
->RunBatteryCapacityRoutine(base::BindOnce(
&DeviceCommandRunRoutineJob::OnCrosHealthdResponseReceived,
weak_ptr_factory_.GetWeakPtr(), std::move(succeeded_callback),
std::move(failed_callback)));
break;
}
case chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryHealth: {
constexpr char kMaximumCycleCountFieldName[] = "maximumCycleCount";
constexpr char kPercentBatteryWearAllowedFieldName[] =
"percentBatteryWearAllowed";
base::Optional<int> maximum_cycle_count =
params_dict_.FindIntKey(kMaximumCycleCountFieldName);
base::Optional<int> percent_battery_wear_allowed =
params_dict_.FindIntKey(kPercentBatteryWearAllowedFieldName);
// The battery health routine expects two integers >= 0.
if (!maximum_cycle_count.has_value() ||
!percent_battery_wear_allowed.has_value() ||
maximum_cycle_count.value() < 0 ||
percent_battery_wear_allowed.value() < 0) {
SYSLOG(ERROR) << "Invalid parameters for BatteryHealth routine.";
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(failed_callback),
std::make_unique<Payload>(
MakeInvalidParametersResponse())));
break;
}
chromeos::cros_healthd::ServiceConnection::GetInstance()
->RunBatteryHealthRoutine(
maximum_cycle_count.value(), percent_battery_wear_allowed.value(),
base::BindOnce(
&DeviceCommandRunRoutineJob::OnCrosHealthdResponseReceived,
weak_ptr_factory_.GetWeakPtr(), std::move(succeeded_callback),
std::move(failed_callback)));
->RunBatteryHealthRoutine(base::BindOnce(
&DeviceCommandRunRoutineJob::OnCrosHealthdResponseReceived,
weak_ptr_factory_.GetWeakPtr(), std::move(succeeded_callback),
std::move(failed_callback)));
break;
}
case chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kUrandom: {
......
......@@ -38,17 +38,6 @@ constexpr char kRoutineEnumFieldName[] = "routineEnum";
// payload.
constexpr char kParamsFieldName[] = "params";
// String constants identifying the parameter fields for the battery capacity
// routine.
constexpr char kLowMahFieldName[] = "lowMah";
constexpr char kHighMahFieldName[] = "highMah";
// String constants identifying the parameter fields for the battery health
// routine.
constexpr char kMaximumCycleCountFieldName[] = "maximumCycleCount";
constexpr char kPercentBatteryWearAllowedFieldName[] =
"percentBatteryWearAllowed";
// String constants identifying the parameter fields for the routine.
constexpr char kLengthSecondsFieldName[] = "lengthSeconds";
......@@ -279,14 +268,14 @@ TEST_F(DeviceCommandRunRoutineJobTest, CommandPayloadMissingParamDict) {
EXPECT_EQ(RemoteCommandJob::INVALID, job->status());
}
// Note that the battery capacity routine has no parameters, so it's enough to
// ensure the routine can be run.
TEST_F(DeviceCommandRunRoutineJobTest, RunBatteryCapacityRoutineSuccess) {
auto run_routine_response =
chromeos::cros_healthd::mojom::RunRoutineResponse::New(kId, kStatus);
chromeos::cros_healthd::FakeCrosHealthdClient::Get()
->SetRunRoutineResponseForTesting(run_routine_response);
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kLowMahFieldName, kPositiveInt);
params_dict.SetIntKey(kHighMahFieldName, kPositiveInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryCapacity,
std::move(params_dict),
......@@ -298,82 +287,14 @@ TEST_F(DeviceCommandRunRoutineJobTest, RunBatteryCapacityRoutineSuccess) {
})));
}
// Test that leaving out the lowMah parameter causes the battery capacity
// routine to fail.
TEST_F(DeviceCommandRunRoutineJobTest, RunBatteryCapacityRoutineMissingLowMah) {
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kHighMahFieldName, kPositiveInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryCapacity,
std::move(params_dict),
base::BindLambdaForTesting([](RemoteCommandJob* job) {
EXPECT_EQ(job->status(), RemoteCommandJob::FAILED);
std::unique_ptr<std::string> payload = job->GetResultPayload();
EXPECT_TRUE(payload);
EXPECT_EQ(CreateInvalidParametersFailurePayload(), *payload);
})));
}
// Test that leaving out the highMah parameter causes the battery capacity
// routine to fail.
TEST_F(DeviceCommandRunRoutineJobTest,
RunBatteryCapacityRoutineMissingHighMah) {
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kLowMahFieldName, kPositiveInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryCapacity,
std::move(params_dict),
base::BindLambdaForTesting([](RemoteCommandJob* job) {
EXPECT_EQ(job->status(), RemoteCommandJob::FAILED);
std::unique_ptr<std::string> payload = job->GetResultPayload();
EXPECT_TRUE(payload);
EXPECT_EQ(CreateInvalidParametersFailurePayload(), *payload);
})));
}
// Test that a negative lowMah parameter causes the battery capacity routine to
// fail.
TEST_F(DeviceCommandRunRoutineJobTest, RunBatteryCapacityRoutineInvalidLowMah) {
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kLowMahFieldName, kNegativeInt);
params_dict.SetIntKey(kHighMahFieldName, kPositiveInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryCapacity,
std::move(params_dict),
base::BindLambdaForTesting([](RemoteCommandJob* job) {
EXPECT_EQ(job->status(), RemoteCommandJob::FAILED);
std::unique_ptr<std::string> payload = job->GetResultPayload();
EXPECT_TRUE(payload);
EXPECT_EQ(CreateInvalidParametersFailurePayload(), *payload);
})));
}
// Test that a negative highMah parameter causes the battery capacity routine to
// fail.
TEST_F(DeviceCommandRunRoutineJobTest,
RunBatteryCapacityRoutineInvalidHighMah) {
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kLowMahFieldName, kPositiveInt);
params_dict.SetIntKey(kHighMahFieldName, kNegativeInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryCapacity,
std::move(params_dict),
base::BindLambdaForTesting([](RemoteCommandJob* job) {
EXPECT_EQ(job->status(), RemoteCommandJob::FAILED);
std::unique_ptr<std::string> payload = job->GetResultPayload();
EXPECT_TRUE(payload);
EXPECT_EQ(CreateInvalidParametersFailurePayload(), *payload);
})));
}
// Note that the battery health routine has no parameters, so it's enough to
// ensure the routine can be run.
TEST_F(DeviceCommandRunRoutineJobTest, RunBatteryHealthRoutineSuccess) {
auto run_routine_response =
chromeos::cros_healthd::mojom::RunRoutineResponse::New(kId, kStatus);
chromeos::cros_healthd::FakeCrosHealthdClient::Get()
->SetRunRoutineResponseForTesting(run_routine_response);
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kMaximumCycleCountFieldName, kPositiveInt);
params_dict.SetIntKey(kPercentBatteryWearAllowedFieldName, kPositiveInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryHealth,
std::move(params_dict),
......@@ -385,76 +306,6 @@ TEST_F(DeviceCommandRunRoutineJobTest, RunBatteryHealthRoutineSuccess) {
})));
}
// Test that leaving out the maximumCycleCount parameter causes the battery
// health routine to fail.
TEST_F(DeviceCommandRunRoutineJobTest,
RunBatteryHealthRoutineMissingMaximumCycleCount) {
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kPercentBatteryWearAllowedFieldName, kPositiveInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryHealth,
std::move(params_dict),
base::BindLambdaForTesting([](RemoteCommandJob* job) {
EXPECT_EQ(job->status(), RemoteCommandJob::FAILED);
std::unique_ptr<std::string> payload = job->GetResultPayload();
EXPECT_TRUE(payload);
EXPECT_EQ(CreateInvalidParametersFailurePayload(), *payload);
})));
}
// Test that leaving out the percentBatteryWearAllowed parameter causes the
// battery health routine to fail.
TEST_F(DeviceCommandRunRoutineJobTest,
RunBatteryHealthRoutineMissingPercentBatteryWearAllowed) {
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kMaximumCycleCountFieldName, kPositiveInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryHealth,
std::move(params_dict),
base::BindLambdaForTesting([](RemoteCommandJob* job) {
EXPECT_EQ(job->status(), RemoteCommandJob::FAILED);
std::unique_ptr<std::string> payload = job->GetResultPayload();
EXPECT_TRUE(payload);
EXPECT_EQ(CreateInvalidParametersFailurePayload(), *payload);
})));
}
// Test that a negative maximumCycleCount parameter causes the battery health
// routine to fail.
TEST_F(DeviceCommandRunRoutineJobTest,
RunBatteryHealthRoutineInvalidMaximumCycleCount) {
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kMaximumCycleCountFieldName, kNegativeInt);
params_dict.SetIntKey(kPercentBatteryWearAllowedFieldName, kPositiveInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryHealth,
std::move(params_dict),
base::BindLambdaForTesting([](RemoteCommandJob* job) {
EXPECT_EQ(job->status(), RemoteCommandJob::FAILED);
std::unique_ptr<std::string> payload = job->GetResultPayload();
EXPECT_TRUE(payload);
EXPECT_EQ(CreateInvalidParametersFailurePayload(), *payload);
})));
}
// Test that a negative percentBatteryWearAllowed parameter causes the battery
// health routine to fail.
TEST_F(DeviceCommandRunRoutineJobTest,
RunBatteryHealthRoutineInvalidPercentBatteryWearAllowed) {
base::Value params_dict(base::Value::Type::DICTIONARY);
params_dict.SetIntKey(kMaximumCycleCountFieldName, kPositiveInt);
params_dict.SetIntKey(kPercentBatteryWearAllowedFieldName, kNegativeInt);
EXPECT_TRUE(RunJob(
chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kBatteryHealth,
std::move(params_dict),
base::BindLambdaForTesting([](RemoteCommandJob* job) {
EXPECT_EQ(job->status(), RemoteCommandJob::FAILED);
std::unique_ptr<std::string> payload = job->GetResultPayload();
EXPECT_TRUE(payload);
EXPECT_EQ(CreateInvalidParametersFailurePayload(), *payload);
})));
}
TEST_F(DeviceCommandRunRoutineJobTest, RunUrandomRoutineSuccess) {
auto run_routine_response =
chromeos::cros_healthd::mojom::RunRoutineResponse::New(kId, kStatus);
......
......@@ -64,12 +64,12 @@ void DiagnosticsService::GetRoutineUpdate(
std::move(callback)));
}
// TODO(b/171327161): Remove |low_mah| and |high_mah| from this routine.
void DiagnosticsService::RunBatteryCapacityRoutine(
uint32_t low_mah,
uint32_t high_mah,
RunBatteryCapacityRoutineCallback callback) {
GetService()->RunBatteryCapacityRoutine(
low_mah, high_mah,
base::BindOnce(
[](health::mojom::DiagnosticsService::
RunBatteryCapacityRoutineCallback callback,
......@@ -79,12 +79,13 @@ void DiagnosticsService::RunBatteryCapacityRoutine(
std::move(callback)));
}
// TODO(b/171327161): Remove |maximum_cycle_count| and
// |percent_battery_wear_allowed| from this routine.
void DiagnosticsService::RunBatteryHealthRoutine(
uint32_t maximum_cycle_count,
uint32_t percent_battery_wear_allowed,
RunBatteryHealthRoutineCallback callback) {
GetService()->RunBatteryHealthRoutine(
maximum_cycle_count, percent_battery_wear_allowed,
base::BindOnce(
[](health::mojom::DiagnosticsService::RunBatteryHealthRoutineCallback
callback,
......
......@@ -76,8 +76,6 @@ void FakeCrosHealthdService::RunUrandomRoutine(
}
void FakeCrosHealthdService::RunBatteryCapacityRoutine(
uint32_t low_mah,
uint32_t high_mah,
RunBatteryCapacityRoutineCallback callback) {
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE,
......@@ -86,8 +84,6 @@ void FakeCrosHealthdService::RunBatteryCapacityRoutine(
}
void FakeCrosHealthdService::RunBatteryHealthRoutine(
uint32_t maximum_cycle_count,
uint32_t percent_battery_wear_allowed,
RunBatteryHealthRoutineCallback callback) {
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE,
......
......@@ -58,12 +58,8 @@ class FakeCrosHealthdService final
void RunUrandomRoutine(uint32_t length_seconds,
RunUrandomRoutineCallback callback) override;
void RunBatteryCapacityRoutine(
uint32_t low_mah,
uint32_t high_mah,
RunBatteryCapacityRoutineCallback callback) override;
void RunBatteryHealthRoutine(
uint32_t maximum_cycle_count,
uint32_t percent_battery_wear_allowed,
RunBatteryHealthRoutineCallback callback) override;
void RunSmartctlCheckRoutine(
RunSmartctlCheckRoutineCallback callback) override;
......
......@@ -43,13 +43,9 @@ class ServiceConnectionImpl : public ServiceConnection {
mojom::CrosHealthdDiagnosticsService::RunUrandomRoutineCallback callback)
override;
void RunBatteryCapacityRoutine(
uint32_t low_mah,
uint32_t high_mah,
mojom::CrosHealthdDiagnosticsService::RunBatteryCapacityRoutineCallback
callback) override;
void RunBatteryHealthRoutine(
uint32_t maximum_cycle_count,
uint32_t percent_battery_wear_allowed,
mojom::CrosHealthdDiagnosticsService::RunBatteryHealthRoutineCallback
callback) override;
void RunSmartctlCheckRoutine(
......@@ -226,25 +222,21 @@ void ServiceConnectionImpl::RunUrandomRoutine(
}
void ServiceConnectionImpl::RunBatteryCapacityRoutine(
uint32_t low_mah,
uint32_t high_mah,
mojom::CrosHealthdDiagnosticsService::RunBatteryCapacityRoutineCallback
callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
BindCrosHealthdDiagnosticsServiceIfNeeded();
cros_healthd_diagnostics_service_->RunBatteryCapacityRoutine(
low_mah, high_mah, std::move(callback));
std::move(callback));
}
void ServiceConnectionImpl::RunBatteryHealthRoutine(
uint32_t maximum_cycle_count,
uint32_t percent_battery_wear_allowed,
mojom::CrosHealthdDiagnosticsService::RunBatteryHealthRoutineCallback
callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
BindCrosHealthdDiagnosticsServiceIfNeeded();
cros_healthd_diagnostics_service_->RunBatteryHealthRoutine(
maximum_cycle_count, percent_battery_wear_allowed, std::move(callback));
std::move(callback));
}
void ServiceConnectionImpl::RunSmartctlCheckRoutine(
......
......@@ -65,8 +65,6 @@ class ServiceConnection {
// src/chromeos/service/cros_healthd/public/mojom/cros_healthd.mojom for
// details.
virtual void RunBatteryCapacityRoutine(
uint32_t low_mah,
uint32_t high_mah,
mojom::CrosHealthdDiagnosticsService::RunBatteryCapacityRoutineCallback
callback) = 0;
......@@ -74,8 +72,6 @@ class ServiceConnection {
// src/chromeos/service/cros_healthd/public/mojom/cros_healthd.mojom for
// details.
virtual void RunBatteryHealthRoutine(
uint32_t maximum_cycle_count,
uint32_t percent_battery_wear_allowed,
mojom::CrosHealthdDiagnosticsService::RunBatteryHealthRoutineCallback
callback) = 0;
......
......@@ -323,7 +323,6 @@ TEST_F(CrosHealthdServiceConnectionTest, RunBatteryCapacityRoutine) {
FakeCrosHealthdClient::Get()->SetRunRoutineResponseForTesting(response);
bool callback_done = false;
ServiceConnection::GetInstance()->RunBatteryCapacityRoutine(
/*low_mah=*/1001, /*high_mah=*/120345,
base::BindOnce(
[](bool* callback_done, mojom::RunRoutineResponsePtr response) {
EXPECT_EQ(response, MakeRunRoutineResponse());
......@@ -340,7 +339,6 @@ TEST_F(CrosHealthdServiceConnectionTest, RunBatteryHealthRoutine) {
FakeCrosHealthdClient::Get()->SetRunRoutineResponseForTesting(response);
bool callback_done = false;
ServiceConnection::GetInstance()->RunBatteryHealthRoutine(
/*maximum_cycle_count=*/2, /*percent_battery_wear_allowed=*/90,
base::BindOnce(
[](bool* callback_done, mojom::RunRoutineResponsePtr response) {
EXPECT_EQ(response, MakeRunRoutineResponse());
......
......@@ -81,38 +81,30 @@ interface CrosHealthdDiagnosticsService {
=> (RunRoutineResponse response);
// Requests that the BatteryCapacity routine is created and started on the
// platform. This routine checks the battery's design capacity against the
// inputs. The routine will pass iff the design capacity of the battery read
// from the platform is inclusively within these bounds. This routine is only
// available if GetAvailableRoutines returned kBatteryCapactity.
//
// The request:
// * |low_mah| - lower bound for the battery's design capacity (mAh).
// * |high_mah| - upper bound for the battery's design capacity (mAh).
// platform. This routine checks the battery's design capacity against inputs
// configured in cros_config. If no configuration data is present in
// cros_config, the routine will fall back to fleet-wide default values of
// [1000, 10000]. The routine will pass iff the design capacity of the battery
// read from the platform is inclusively within these bounds. This routine is
// only available if GetAvailableRoutines returned kBatteryCapactity.
//
// The response:
// * |response| - contains a unique identifier and status for the created
// routine.
RunBatteryCapacityRoutine(uint32 low_mah, uint32 high_mah)
=> (RunRoutineResponse response);
RunBatteryCapacityRoutine() => (RunRoutineResponse response);
// Requests that the BatteryHealth routine is created and started on the
// platform. This routine checks the cycle count and percent wear of the
// battery. This routine is only available if GetAvailableRoutines returned
// kBatteryHealth.
//
// The request:
// * |maximum_cycle_count| - maximum cycle count allowed for the routine to
// pass.
// * |percent_battery_wear_allowed| - maximum percent battery wear allowed for
// the routine to pass.
// battery against inputs configured in cros_config. If no configuration data
// is present in cros_config, the routine will fall back to fleet-wide default
// values of 1000 for the maximum allowable cycle count and 50% for maximum
// battery wear percentage allowed. This routine is only available if
// GetAvailableRoutines returned kBatteryHealth.
//
// The response:
// * |response| - contains a unique identifier and status for the created
// routine.
RunBatteryHealthRoutine(uint32 maximum_cycle_count,
uint32 percent_battery_wear_allowed)
=> (RunRoutineResponse response);
RunBatteryHealthRoutine() => (RunRoutineResponse response);
// Requests that the SmartctlCheck routine is created and started on the
// platform. This routine checks available spare NVMe capacity against the
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment