From c1d89888b97fc37a82ae060db66f189db4444a20 Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Sat, 28 Apr 2018 02:31:25 -0700 Subject: [PATCH 1/2] Audio V4 VTS: test setter even if getter is NOT_SUPPORTED Previously, the setter of optional properties were not tested if the getter was not supported. This was an issue as the framework rarely gets, most often only sets as it consider having the ownership of those properties. Thus implementation tend to only support set. As a result optional setter were not tested although called by the framework. For example the default impl setTtyMode has a bug that was not detected because the Pixel HAL does not support getTtyMode. This patch makes sure to always test getter and setter. Test: atest VtsHalAudioV4_0TargetTest Bug: 71486871 Change-Id: Ib0b03fb6eb77cc0428a33eb463166dd7c339bd3d Signed-off-by: Kevin Rocard --- .../include/utility/PrettyPrintAudioTypes.h | 1 + .../functional/AudioPrimaryHidlHalTest.cpp | 113 ++++++++++-------- 2 files changed, 61 insertions(+), 53 deletions(-) diff --git a/audio/common/all-versions/test/utility/include/utility/PrettyPrintAudioTypes.h b/audio/common/all-versions/test/utility/include/utility/PrettyPrintAudioTypes.h index 05239accfb..abc2ff5f82 100644 --- a/audio/common/all-versions/test/utility/include/utility/PrettyPrintAudioTypes.h +++ b/audio/common/all-versions/test/utility/include/utility/PrettyPrintAudioTypes.h @@ -41,6 +41,7 @@ namespace audio { inline void PrintTo(const T& val, ::std::ostream* os) { *os << toString(val); } namespace AUDIO_HAL_VERSION { +DEFINE_GTEST_PRINT_TO(IPrimaryDevice::TtyMode) DEFINE_GTEST_PRINT_TO(Result) } // namespace AUDIO_HAL_VERSION diff --git a/audio/core/4.0/vts/functional/AudioPrimaryHidlHalTest.cpp b/audio/core/4.0/vts/functional/AudioPrimaryHidlHalTest.cpp index 0bf32b5788..c764ea625e 100644 --- a/audio/core/4.0/vts/functional/AudioPrimaryHidlHalTest.cpp +++ b/audio/core/4.0/vts/functional/AudioPrimaryHidlHalTest.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -51,6 +52,7 @@ using std::initializer_list; using std::string; using std::to_string; using std::vector; +using std::list; using ::android::sp; using ::android::hardware::Return; @@ -104,6 +106,7 @@ using namespace ::android::hardware::audio::common::test::utility; static auto okOrNotSupported = {Result::OK, Result::NOT_SUPPORTED}; static auto okOrNotSupportedOrInvalidArgs = {Result::OK, Result::NOT_SUPPORTED, Result::INVALID_ARGUMENTS}; +static auto invalidArgsOrNotSupported = {Result::INVALID_ARGUMENTS, Result::NOT_SUPPORTED}; class AudioHidlTestEnvironment : public ::Environment { public: @@ -212,53 +215,59 @@ TEST_F(AudioPrimaryHidlTest, Init) { template class AccessorPrimaryHidlTest : public AudioPrimaryHidlTest { protected: - /** Test a property getter and setter. */ - template - void testAccessors(const string& propertyName, const vector& valuesToTest, - Setter setter, Getter getter, const vector& invalidValues = {}) { - Property initialValue; // Save initial value to restore it at the end - // of the test - ASSERT_OK((device.get()->*getter)(returnIn(res, initialValue))); - ASSERT_OK(res); + enum Optionality { REQUIRED, OPTIONAL }; + struct Initial { // Initial property value + Initial(Property value, Optionality check = REQUIRED) : value(value), check(check) {} + Property value; + Optionality check; // If this initial value should be checked + }; + /** Test a property getter and setter. + * The getter and/or the setter may return NOT_SUPPORTED if optionality == OPTIONAL. + */ + template + void testAccessors(const string& propertyName, const Initial expectedInitial, + list valuesToTest, Setter setter, Getter getter, + const vector& invalidValues = {}) { + const auto expectedResults = {Result::OK, + optionality == OPTIONAL ? Result::NOT_SUPPORTED : Result::OK}; + Property initialValue = expectedInitial.value; + ASSERT_OK((device.get()->*getter)(returnIn(res, initialValue))); + ASSERT_RESULT(expectedResults, res); + if (res == Result::OK && expectedInitial.check == REQUIRED) { + EXPECT_EQ(expectedInitial.value, initialValue); + } + + valuesToTest.push_front(expectedInitial.value); + valuesToTest.push_back(initialValue); for (Property setValue : valuesToTest) { SCOPED_TRACE("Test " + propertyName + " getter and setter for " + testing::PrintToString(setValue)); - ASSERT_OK((device.get()->*setter)(setValue)); + auto ret = (device.get()->*setter)(setValue); + ASSERT_RESULT(expectedResults, ret); + if (ret == Result::NOT_SUPPORTED) { + doc::partialTest(propertyName + " setter is not supported"); + break; + } Property getValue; // Make sure the getter returns the same value just set ASSERT_OK((device.get()->*getter)(returnIn(res, getValue))); - ASSERT_OK(res); + ASSERT_RESULT(expectedResults, res); + if (res == Result::NOT_SUPPORTED) { + doc::partialTest(propertyName + " getter is not supported"); + continue; + } EXPECT_EQ(setValue, getValue); } for (Property invalidValue : invalidValues) { SCOPED_TRACE("Try to set " + propertyName + " with the invalid value " + testing::PrintToString(invalidValue)); - EXPECT_RESULT(Result::INVALID_ARGUMENTS, (device.get()->*setter)(invalidValue)); + EXPECT_RESULT(invalidArgsOrNotSupported, (device.get()->*setter)(invalidValue)); } - ASSERT_OK((device.get()->*setter)(initialValue)); // restore initial value - } - - /** Test the getter and setter of an optional feature. */ - template - void testOptionalAccessors(const string& propertyName, const vector& valuesToTest, - Setter setter, Getter getter, - const vector& invalidValues = {}) { - doc::test("Test the optional " + propertyName + " getters and setter"); - { - SCOPED_TRACE("Test feature support by calling the getter"); - Property initialValue; - ASSERT_OK((device.get()->*getter)(returnIn(res, initialValue))); - if (res == Result::NOT_SUPPORTED) { - doc::partialTest(propertyName + " getter is not supported"); - return; - } - ASSERT_OK(res); // If it is supported it must succeed - } - // The feature is supported, test it - testAccessors(propertyName, valuesToTest, setter, getter, invalidValues); + // Restore initial value + EXPECT_RESULT(expectedResults, (device.get()->*setter)(initialValue)); } }; @@ -266,24 +275,22 @@ using BoolAccessorPrimaryHidlTest = AccessorPrimaryHidlTest; TEST_F(BoolAccessorPrimaryHidlTest, MicMuteTest) { doc::test("Check that the mic can be muted and unmuted"); - testAccessors("mic mute", {true, false, true}, &IDevice::setMicMute, &IDevice::getMicMute); + testAccessors("mic mute", Initial{false}, {true}, &IDevice::setMicMute, &IDevice::getMicMute); // TODO: check that the mic is really muted (all sample are 0) } TEST_F(BoolAccessorPrimaryHidlTest, MasterMuteTest) { - doc::test( - "If master mute is supported, try to mute and unmute the master " - "output"); - testOptionalAccessors("master mute", {true, false, true}, &IDevice::setMasterMute, - &IDevice::getMasterMute); + doc::test("If master mute is supported, try to mute and unmute the master output"); + testAccessors("master mute", Initial{false}, {true}, &IDevice::setMasterMute, + &IDevice::getMasterMute); // TODO: check that the master volume is really muted } using FloatAccessorPrimaryHidlTest = AccessorPrimaryHidlTest; TEST_F(FloatAccessorPrimaryHidlTest, MasterVolumeTest) { doc::test("Test the master volume if supported"); - testOptionalAccessors( - "master volume", {0, 0.5, 1}, &IDevice::setMasterVolume, &IDevice::getMasterVolume, + testAccessors( + "master volume", Initial{1}, {0, 0.5}, &IDevice::setMasterVolume, &IDevice::getMasterVolume, {-0.1, 1.1, NAN, INFINITY, -INFINITY, 1 + std::numeric_limits::epsilon()}); // TODO: check that the master volume is really changed } @@ -957,7 +964,6 @@ TEST_IO_STREAM(close, "Make sure a stream can be closed", ASSERT_OK(closeStream( TEST_IO_STREAM(closeTwice, "Make sure a stream can not be closed twice", ASSERT_OK(closeStream()); ASSERT_RESULT(Result::INVALID_STATE, closeStream())) -static auto invalidArgsOrNotSupported = {Result::INVALID_ARGUMENTS, Result::NOT_SUPPORTED}; static void testCreateTooBigMmapBuffer(IStream* stream) { MmapBufferInfo info; Result res; @@ -1415,35 +1421,36 @@ TEST_F(AudioPrimaryHidlTest, updateRotation) { TEST_F(BoolAccessorPrimaryHidlTest, BtScoNrecEnabled) { doc::test("Query and set the BT SCO NR&EC state"); - testOptionalAccessors("BtScoNrecEnabled", {true, false, true}, - &IPrimaryDevice::setBtScoNrecEnabled, - &IPrimaryDevice::getBtScoNrecEnabled); + testAccessors("BtScoNrecEnabled", Initial{false, OPTIONAL}, {true}, + &IPrimaryDevice::setBtScoNrecEnabled, + &IPrimaryDevice::getBtScoNrecEnabled); } TEST_F(BoolAccessorPrimaryHidlTest, setGetBtScoWidebandEnabled) { doc::test("Query and set the SCO whideband state"); - testOptionalAccessors("BtScoWideband", {true, false, true}, - &IPrimaryDevice::setBtScoWidebandEnabled, - &IPrimaryDevice::getBtScoWidebandEnabled); + testAccessors("BtScoWideband", Initial{false, OPTIONAL}, {true}, + &IPrimaryDevice::setBtScoWidebandEnabled, + &IPrimaryDevice::getBtScoWidebandEnabled); } TEST_F(BoolAccessorPrimaryHidlTest, setGetBtHfpEnabled) { doc::test("Query and set the BT HFP state"); - testOptionalAccessors("BtHfpEnabled", {true, false, true}, &IPrimaryDevice::setBtHfpEnabled, - &IPrimaryDevice::getBtHfpEnabled); + testAccessors("BtHfpEnabled", Initial{false, OPTIONAL}, {true}, + &IPrimaryDevice::setBtHfpEnabled, &IPrimaryDevice::getBtHfpEnabled); } using TtyModeAccessorPrimaryHidlTest = AccessorPrimaryHidlTest; TEST_F(TtyModeAccessorPrimaryHidlTest, setGetTtyMode) { doc::test("Query and set the TTY mode state"); - testOptionalAccessors("TTY mode", {TtyMode::OFF, TtyMode::HCO, TtyMode::VCO, TtyMode::FULL}, - &IPrimaryDevice::setTtyMode, &IPrimaryDevice::getTtyMode); + testAccessors("TTY mode", Initial{TtyMode::OFF}, + {TtyMode::HCO, TtyMode::VCO, TtyMode::FULL}, + &IPrimaryDevice::setTtyMode, &IPrimaryDevice::getTtyMode); } TEST_F(BoolAccessorPrimaryHidlTest, setGetHac) { doc::test("Query and set the HAC state"); - testOptionalAccessors("HAC", {true, false, true}, &IPrimaryDevice::setHacEnabled, - &IPrimaryDevice::getHacEnabled); + testAccessors("HAC", Initial{false}, {true}, &IPrimaryDevice::setHacEnabled, + &IPrimaryDevice::getHacEnabled); } ////////////////////////////////////////////////////////////////////////////// From 912bd4bf2ff9397c40df18c29406af4579b07921 Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Wed, 31 Jan 2018 22:20:16 -0800 Subject: [PATCH 2/2] Audio HAL HIDL wrapper: Fix incorrect conversion of TTY Mode The TTYMode enum numeric value was converted to a char* instead of its literal value and vice versa. Instead convert it to/from its literal value. Eg: TTYMode::FULL -> "FULL" instead of "3" "FULL" -> 0 as atoi conversion would fail Test: atest VtsHalAudioV4_0TargetTest Bug: 71486871 Change-Id: I29bbf6bf3b5532269afcc5d39ea10eff2871bdea Signed-off-by: Kevin Rocard Signed-off-by: Jungyee Yoo --- .../all-versions/default/ParametersUtil.h | 1 + .../default/ParametersUtil.impl.h | 6 +++ .../all-versions/default/PrimaryDevice.impl.h | 48 +++++++++++++++++-- 3 files changed, 51 insertions(+), 4 deletions(-) diff --git a/audio/core/all-versions/default/include/core/all-versions/default/ParametersUtil.h b/audio/core/all-versions/default/include/core/all-versions/default/ParametersUtil.h index a27ac25d43..35ff1105ea 100644 --- a/audio/core/all-versions/default/include/core/all-versions/default/ParametersUtil.h +++ b/audio/core/all-versions/default/include/core/all-versions/default/ParametersUtil.h @@ -36,6 +36,7 @@ using ::android::hardware::hidl_vec; class ParametersUtil { public: + Result setParam(const char* name, const char* value); Result getParam(const char* name, bool* value); Result getParam(const char* name, int* value); Result getParam(const char* name, String8* value, AudioParameter context = {}); diff --git a/audio/core/all-versions/default/include/core/all-versions/default/ParametersUtil.impl.h b/audio/core/all-versions/default/include/core/all-versions/default/ParametersUtil.impl.h index afff2b6d2a..34bc53c7a7 100644 --- a/audio/core/all-versions/default/include/core/all-versions/default/ParametersUtil.impl.h +++ b/audio/core/all-versions/default/include/core/all-versions/default/ParametersUtil.impl.h @@ -112,6 +112,12 @@ std::unique_ptr ParametersUtil::getParams(const AudioParameter& return std::unique_ptr(new AudioParameter(paramsAndValues)); } +Result ParametersUtil::setParam(const char* name, const char* value) { + AudioParameter param; + param.add(String8(name), String8(value)); + return setParams(param); +} + Result ParametersUtil::setParam(const char* name, bool value) { AudioParameter param; param.add(String8(name), String8(value ? AudioParameter::valueOn : AudioParameter::valueOff)); diff --git a/audio/core/all-versions/default/include/core/all-versions/default/PrimaryDevice.impl.h b/audio/core/all-versions/default/include/core/all-versions/default/PrimaryDevice.impl.h index 61ffbe00c4..f269dd4f91 100644 --- a/audio/core/all-versions/default/include/core/all-versions/default/PrimaryDevice.impl.h +++ b/audio/core/all-versions/default/include/core/all-versions/default/PrimaryDevice.impl.h @@ -208,16 +208,56 @@ Return PrimaryDevice::setBtScoWidebandEnabled(bool enabled) { return mDevice->setParam(AUDIO_PARAMETER_KEY_BT_SCO_WB, enabled); } +static const char* convertTtyModeFromHIDL(IPrimaryDevice::TtyMode mode) { + switch (mode) { + case IPrimaryDevice::TtyMode::OFF: + return AUDIO_PARAMETER_VALUE_TTY_OFF; + case IPrimaryDevice::TtyMode::VCO: + return AUDIO_PARAMETER_VALUE_TTY_VCO; + case IPrimaryDevice::TtyMode::HCO: + return AUDIO_PARAMETER_VALUE_TTY_HCO; + case IPrimaryDevice::TtyMode::FULL: + return AUDIO_PARAMETER_VALUE_TTY_FULL; + default: + return nullptr; + } +} +static IPrimaryDevice::TtyMode convertTtyModeToHIDL(const char* halMode) { + if (strcmp(halMode, AUDIO_PARAMETER_VALUE_TTY_OFF) == 0) + return IPrimaryDevice::TtyMode::OFF; + else if (strcmp(halMode, AUDIO_PARAMETER_VALUE_TTY_VCO) == 0) + return IPrimaryDevice::TtyMode::VCO; + else if (strcmp(halMode, AUDIO_PARAMETER_VALUE_TTY_HCO) == 0) + return IPrimaryDevice::TtyMode::HCO; + else if (strcmp(halMode, AUDIO_PARAMETER_VALUE_TTY_FULL) == 0) + return IPrimaryDevice::TtyMode::FULL; + return IPrimaryDevice::TtyMode(-1); +} + Return PrimaryDevice::getTtyMode(getTtyMode_cb _hidl_cb) { - int halMode; + String8 halMode; Result retval = mDevice->getParam(AUDIO_PARAMETER_KEY_TTY_MODE, &halMode); - TtyMode mode = retval == Result::OK ? TtyMode(halMode) : TtyMode::OFF; - _hidl_cb(retval, mode); + if (retval != Result::OK) { + _hidl_cb(retval, TtyMode::OFF); + return Void(); + } + TtyMode mode = convertTtyModeToHIDL(halMode); + if (mode == TtyMode(-1)) { + ALOGE("HAL returned invalid TTY value: %s", halMode.c_str()); + _hidl_cb(Result::INVALID_STATE, TtyMode::OFF); + return Void(); + } + _hidl_cb(Result::OK, mode); return Void(); } Return PrimaryDevice::setTtyMode(IPrimaryDevice::TtyMode mode) { - return mDevice->setParam(AUDIO_PARAMETER_KEY_TTY_MODE, static_cast(mode)); + const char* modeStr = convertTtyModeFromHIDL(mode); + if (modeStr == nullptr) { + ALOGW("Can not set an invalid TTY value: %d", mode); + return Result::INVALID_ARGUMENTS; + } + return mDevice->setParam(AUDIO_PARAMETER_KEY_TTY_MODE, modeStr); } Return PrimaryDevice::getHacEnabled(getHacEnabled_cb _hidl_cb) {