From c1763a679659cdb1a50f7b4640b6c3f659f1d666 Mon Sep 17 00:00:00 2001 From: Tomasz Wasilczyk Date: Tue, 25 Jul 2017 10:01:17 -0700 Subject: [PATCH] Fix remaining broadcastradio 1.1 VTS TODOs. This includes: - cover all AM/FM bands, not just first one - fix flakiness on late callback dereference - fix 1.0 tuneComplete check - move utils includes into separate subdirectories Bug: b/36864490 Test: VTS Change-Id: I6e2427ac29abd6278c9783cf83b4df05195ac7ea --- broadcastradio/1.1/default/Tuner.cpp | 2 +- broadcastradio/1.1/default/Tuner.h | 2 +- broadcastradio/1.1/default/VirtualProgram.cpp | 2 +- broadcastradio/1.1/default/VirtualRadio.cpp | 2 +- .../1.1/tests/WorkerThread_test.cpp | 2 +- broadcastradio/1.1/utils/Android.bp | 2 +- broadcastradio/1.1/utils/Utils.cpp | 22 ++- broadcastradio/1.1/utils/WorkerThread.cpp | 2 +- .../broadcastradio-utils}/Utils.h | 7 + .../broadcastradio-utils}/WorkerThread.h | 0 broadcastradio/1.1/vts/functional/Android.bp | 16 +- .../VtsHalBroadcastradioV1_1TargetTest.cpp | 141 ++++++++++++------ broadcastradio/1.1/vts/utils/Android.bp | 28 ++++ .../{functional => utils}/call-barrier.cpp | 2 +- .../broadcastradio-vts-utils}/call-barrier.h | 0 .../broadcastradio-vts-utils}/mock-timeout.h | 0 broadcastradio/Android.bp | 1 + 17 files changed, 160 insertions(+), 71 deletions(-) rename broadcastradio/1.1/utils/{ => include/broadcastradio-utils}/Utils.h (96%) rename broadcastradio/1.1/utils/{ => include/broadcastradio-utils}/WorkerThread.h (100%) create mode 100644 broadcastradio/1.1/vts/utils/Android.bp rename broadcastradio/1.1/vts/{functional => utils}/call-barrier.cpp (95%) rename broadcastradio/1.1/vts/{functional => utils/include/broadcastradio-vts-utils}/call-barrier.h (100%) rename broadcastradio/1.1/vts/{functional => utils/include/broadcastradio-vts-utils}/mock-timeout.h (100%) diff --git a/broadcastradio/1.1/default/Tuner.cpp b/broadcastradio/1.1/default/Tuner.cpp index 0a452088fd..133593ed85 100644 --- a/broadcastradio/1.1/default/Tuner.cpp +++ b/broadcastradio/1.1/default/Tuner.cpp @@ -20,7 +20,7 @@ #include "BroadcastRadio.h" #include "Tuner.h" -#include +#include #include namespace android { diff --git a/broadcastradio/1.1/default/Tuner.h b/broadcastradio/1.1/default/Tuner.h index 2ab4f407aa..2222e5a877 100644 --- a/broadcastradio/1.1/default/Tuner.h +++ b/broadcastradio/1.1/default/Tuner.h @@ -18,9 +18,9 @@ #include "VirtualRadio.h" -#include #include #include +#include namespace android { namespace hardware { diff --git a/broadcastradio/1.1/default/VirtualProgram.cpp b/broadcastradio/1.1/default/VirtualProgram.cpp index 1f3b69310b..4c6b3b1b59 100644 --- a/broadcastradio/1.1/default/VirtualProgram.cpp +++ b/broadcastradio/1.1/default/VirtualProgram.cpp @@ -15,7 +15,7 @@ */ #include "VirtualProgram.h" -#include +#include #include "resources.h" diff --git a/broadcastradio/1.1/default/VirtualRadio.cpp b/broadcastradio/1.1/default/VirtualRadio.cpp index 89b2b0ae2d..692e7bc91c 100644 --- a/broadcastradio/1.1/default/VirtualRadio.cpp +++ b/broadcastradio/1.1/default/VirtualRadio.cpp @@ -15,7 +15,7 @@ */ #include "VirtualRadio.h" -#include +#include namespace android { namespace hardware { diff --git a/broadcastradio/1.1/tests/WorkerThread_test.cpp b/broadcastradio/1.1/tests/WorkerThread_test.cpp index a0e0ebb27c..ed36de3e85 100644 --- a/broadcastradio/1.1/tests/WorkerThread_test.cpp +++ b/broadcastradio/1.1/tests/WorkerThread_test.cpp @@ -14,7 +14,7 @@ * limitations under the License. */ -#include +#include #include namespace { diff --git a/broadcastradio/1.1/utils/Android.bp b/broadcastradio/1.1/utils/Android.bp index 73c66809a7..e80d133dca 100644 --- a/broadcastradio/1.1/utils/Android.bp +++ b/broadcastradio/1.1/utils/Android.bp @@ -27,7 +27,7 @@ cc_library_static { "Utils.cpp", "WorkerThread.cpp", ], - export_include_dirs: ["."], + export_include_dirs: ["include"], shared_libs: [ "android.hardware.broadcastradio@1.1", ], diff --git a/broadcastradio/1.1/utils/Utils.cpp b/broadcastradio/1.1/utils/Utils.cpp index f8a4479046..8bb7691794 100644 --- a/broadcastradio/1.1/utils/Utils.cpp +++ b/broadcastradio/1.1/utils/Utils.cpp @@ -16,7 +16,7 @@ #define LOG_TAG "BroadcastRadioDefault.utils" //#define LOG_NDEBUG 0 -#include "Utils.h" +#include #include @@ -208,6 +208,26 @@ bool isDigital(const ProgramSelector& sel) { } // namespace utils } // namespace V1_1 + +namespace V1_0 { + +bool operator==(const BandConfig& l, const BandConfig& r) { + if (l.type != r.type) return false; + if (l.antennaConnected != r.antennaConnected) return false; + if (l.lowerLimit != r.lowerLimit) return false; + if (l.upperLimit != r.upperLimit) return false; + if (l.spacings != r.spacings) return false; + if (l.type == Band::AM || l.type == Band::AM_HD) { + return l.ext.am == r.ext.am; + } else if (l.type == Band::FM || l.type == Band::FM_HD) { + return l.ext.fm == r.ext.fm; + } else { + ALOGW("Unsupported band config type: %s", toString(l.type).c_str()); + return false; + } +} + +} // namespace V1_0 } // namespace broadcastradio } // namespace hardware } // namespace android diff --git a/broadcastradio/1.1/utils/WorkerThread.cpp b/broadcastradio/1.1/utils/WorkerThread.cpp index a3ceaa1bfa..bfcbb390e8 100644 --- a/broadcastradio/1.1/utils/WorkerThread.cpp +++ b/broadcastradio/1.1/utils/WorkerThread.cpp @@ -17,7 +17,7 @@ #define LOG_TAG "WorkerThread" //#define LOG_NDEBUG 0 -#include "WorkerThread.h" +#include #include diff --git a/broadcastradio/1.1/utils/Utils.h b/broadcastradio/1.1/utils/include/broadcastradio-utils/Utils.h similarity index 96% rename from broadcastradio/1.1/utils/Utils.h rename to broadcastradio/1.1/utils/include/broadcastradio-utils/Utils.h index cd86ffaadc..a7da9fea4e 100644 --- a/broadcastradio/1.1/utils/Utils.h +++ b/broadcastradio/1.1/utils/include/broadcastradio-utils/Utils.h @@ -66,6 +66,13 @@ bool isDigital(const ProgramSelector& sel); } // namespace utils } // namespace V1_1 + +namespace V1_0 { + +bool operator==(const BandConfig& l, const BandConfig& r); + +} // namespace V1_0 + } // namespace broadcastradio } // namespace hardware } // namespace android diff --git a/broadcastradio/1.1/utils/WorkerThread.h b/broadcastradio/1.1/utils/include/broadcastradio-utils/WorkerThread.h similarity index 100% rename from broadcastradio/1.1/utils/WorkerThread.h rename to broadcastradio/1.1/utils/include/broadcastradio-utils/WorkerThread.h diff --git a/broadcastradio/1.1/vts/functional/Android.bp b/broadcastradio/1.1/vts/functional/Android.bp index c1360198a1..6e5c84cde1 100644 --- a/broadcastradio/1.1/vts/functional/Android.bp +++ b/broadcastradio/1.1/vts/functional/Android.bp @@ -31,7 +31,8 @@ cc_test { ], static_libs: [ "VtsHalHidlTargetTestBase", - "broadcastradio-vts-call-barrier", + "android.hardware.broadcastradio@1.1-utils-lib", + "android.hardware.broadcastradio@1.1-vts-utils-lib", "libgmock", ], cflags: [ @@ -40,16 +41,3 @@ cc_test { "-g", ], } - -cc_library_static { - name: "broadcastradio-vts-call-barrier", - srcs: [ - "call-barrier.cpp", - ], - export_include_dirs: ["."], - cflags: [ - "-Wall", - "-Wextra", - "-Werror", - ], -} diff --git a/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp b/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp index d20452bce5..c6bc344a8d 100644 --- a/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp +++ b/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp @@ -17,8 +17,15 @@ #define LOG_TAG "broadcastradio.vts" #include +#include +#include +#include +#include +#include #include -#include +#include +#include +#include #include #include #include @@ -27,14 +34,6 @@ #include -#include -#include -#include -#include -#include - -#include "mock-timeout.h" - namespace android { namespace hardware { namespace broadcastradio { @@ -57,6 +56,9 @@ using V1_0::MetaData; using V1_0::MetadataKey; using V1_0::MetadataType; +using std::chrono::steady_clock; +using std::this_thread::sleep_for; + static constexpr auto kConfigTimeout = 10s; static constexpr auto kConnectModuleTimeout = 1s; static constexpr auto kTuneTimeout = 30s; @@ -91,10 +93,8 @@ class BroadcastRadioHalTest : public ::testing::VtsHalHidlTargetTestBase, virtual void SetUp() override; virtual void TearDown() override; - // TODO(b/36864490): check all bands for good test conditions (ie. FM is more likely to have - // any stations on the list, so don't pick AM blindly). - bool openTuner(unsigned band); - const BandConfig& getBand(unsigned idx); + bool openTuner(); + bool nextBand(); bool getProgramList(std::function& list)> cb); Class radioClass; @@ -105,9 +105,33 @@ class BroadcastRadioHalTest : public ::testing::VtsHalHidlTargetTestBase, sp mCallback = new TunerCallbackMock(); private: + const BandConfig& getBand(unsigned idx); + + unsigned currentBandIndex = 0; hidl_vec mBands; }; +/** + * Clears strong pointer and waits until the object gets destroyed. + * + * @param ptr The pointer to get cleared. + * @param timeout Time to wait for other references. + */ +template +static void clearAndWait(sp& ptr, std::chrono::milliseconds timeout) { + wp wptr = ptr; + ptr.clear(); + auto limit = steady_clock::now() + timeout; + while (wptr.promote() != nullptr) { + constexpr auto step = 10ms; + if (steady_clock::now() + step > limit) { + FAIL() << "Pointer was not released within timeout"; + break; + } + sleep_for(step); + } +} + void BroadcastRadioHalTest::SetUp() { radioClass = GetParam(); @@ -153,10 +177,10 @@ void BroadcastRadioHalTest::SetUp() { void BroadcastRadioHalTest::TearDown() { mTuner.clear(); mRadioModule.clear(); - // TODO(b/36864490): wait (with timeout) until mCallback has only one reference + clearAndWait(mCallback, 1s); } -bool BroadcastRadioHalTest::openTuner(unsigned band) { +bool BroadcastRadioHalTest::openTuner() { EXPECT_EQ(nullptr, mTuner.get()); if (radioClass == Class::AM_FM) { @@ -169,7 +193,8 @@ bool BroadcastRadioHalTest::openTuner(unsigned band) { if (result != Result::OK) return; mTuner = ITuner::castFrom(tuner); }; - auto hidlResult = mRadioModule->openTuner(getBand(band), true, mCallback, openCb); + currentBandIndex = 0; + auto hidlResult = mRadioModule->openTuner(getBand(0), true, mCallback, openCb); EXPECT_TRUE(hidlResult.isOk()); EXPECT_EQ(Result::OK, halResult); @@ -210,6 +235,21 @@ const BandConfig& BroadcastRadioHalTest::getBand(unsigned idx) { return band; } +bool BroadcastRadioHalTest::nextBand() { + if (currentBandIndex + 1 >= mBands.size()) return false; + currentBandIndex++; + + BandConfig bandCb; + EXPECT_TIMEOUT_CALL(*mCallback, configChange, Result::OK, _) + .WillOnce(DoAll(SaveArg<1>(&bandCb), testing::Return(ByMove(Void())))); + auto hidlResult = mTuner->setConfiguration(getBand(currentBandIndex)); + EXPECT_EQ(Result::OK, hidlResult); + EXPECT_TIMEOUT_CALL_WAIT(*mCallback, configChange, kConfigTimeout); + EXPECT_EQ(getBand(currentBandIndex), bandCb); + + return true; +} + bool BroadcastRadioHalTest::getProgramList( std::function& list)> cb) { ProgramListResult getListResult = ProgramListResult::NOT_INITIALIZED; @@ -244,11 +284,7 @@ bool BroadcastRadioHalTest::getProgramList( EXPECT_EQ(ProgramListResult::OK, getListResult); } - if (isListEmpty) { - printSkipped("Program list is empty."); - return false; - } - return true; + return !isListEmpty; } /** @@ -263,13 +299,13 @@ bool BroadcastRadioHalTest::getProgramList( */ TEST_P(BroadcastRadioHalTest, OpenTunerTwice) { if (skipped) return; - ASSERT_TRUE(openTuner(0)); - Result halResult = Result::NOT_INITIALIZED; - auto openCb = [&](Result result, const sp&) { halResult = result; }; - auto hidlResult = mRadioModule->openTuner(getBand(0), true, mCallback, openCb); - ASSERT_TRUE(hidlResult.isOk()); - ASSERT_EQ(Result::OK, halResult); + ASSERT_TRUE(openTuner()); + + auto secondTuner = mTuner; + mTuner.clear(); + + ASSERT_TRUE(openTuner()); } /** @@ -283,17 +319,25 @@ TEST_P(BroadcastRadioHalTest, OpenTunerTwice) { */ TEST_P(BroadcastRadioHalTest, TuneFromProgramList) { if (skipped) return; - ASSERT_TRUE(openTuner(0)); + ASSERT_TRUE(openTuner()); ProgramInfo firstProgram; - auto getCb = [&](const hidl_vec& list) { - // don't copy the whole list out, it might be heavy - firstProgram = list[0]; - }; - if (!getProgramList(getCb)) return; + bool foundAny = false; + do { + auto getCb = [&](const hidl_vec& list) { + // don't copy the whole list out, it might be heavy + firstProgram = list[0]; + }; + if (getProgramList(getCb)) foundAny = true; + } while (nextBand()); + if (HasFailure()) return; + if (!foundAny) { + printSkipped("Program list is empty."); + return; + } ProgramSelector selCb; - EXPECT_CALL(*mCallback, tuneComplete(_, _)); + EXPECT_CALL(*mCallback, tuneComplete(_, _)).Times(0); EXPECT_TIMEOUT_CALL(*mCallback, tuneComplete_1_1, Result::OK, _) .WillOnce(DoAll(SaveArg<1>(&selCb), testing::Return(ByMove(Void())))); auto tuneResult = mTuner->tune_1_1(firstProgram.selector); @@ -304,7 +348,7 @@ TEST_P(BroadcastRadioHalTest, TuneFromProgramList) { TEST_P(BroadcastRadioHalTest, CancelAnnouncement) { if (skipped) return; - ASSERT_TRUE(openTuner(0)); + ASSERT_TRUE(openTuner()); auto hidlResult = mTuner->cancelAnnouncement(); EXPECT_EQ(Result::OK, hidlResult); @@ -336,23 +380,24 @@ TEST_P(BroadcastRadioHalTest, GetNoImage) { */ TEST_P(BroadcastRadioHalTest, OobImagesOnly) { if (skipped) return; - ASSERT_TRUE(openTuner(0)); + ASSERT_TRUE(openTuner()); std::vector imageIds; - ProgramInfo firstProgram; - auto getCb = [&](const hidl_vec& list) { - for (auto&& program : list) { - for (auto&& entry : program.base.metadata) { - EXPECT_NE(MetadataType::RAW, entry.type); - if (entry.key != MetadataKey::ICON && entry.key != MetadataKey::ART) continue; - EXPECT_NE(0, entry.intValue); - EXPECT_EQ(0u, entry.rawValue.size()); - if (entry.intValue != 0) imageIds.push_back(entry.intValue); + do { + auto getCb = [&](const hidl_vec& list) { + for (auto&& program : list) { + for (auto&& entry : program.base.metadata) { + EXPECT_NE(MetadataType::RAW, entry.type); + if (entry.key != MetadataKey::ICON && entry.key != MetadataKey::ART) continue; + EXPECT_NE(0, entry.intValue); + EXPECT_EQ(0u, entry.rawValue.size()); + if (entry.intValue != 0) imageIds.push_back(entry.intValue); + } } - } - }; - if (!getProgramList(getCb)) return; + }; + getProgramList(getCb); + } while (nextBand()); if (imageIds.size() == 0) { printSkipped("No images found"); diff --git a/broadcastradio/1.1/vts/utils/Android.bp b/broadcastradio/1.1/vts/utils/Android.bp new file mode 100644 index 0000000000..0c7e2a4433 --- /dev/null +++ b/broadcastradio/1.1/vts/utils/Android.bp @@ -0,0 +1,28 @@ +// +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +cc_library_static { + name: "android.hardware.broadcastradio@1.1-vts-utils-lib", + srcs: [ + "call-barrier.cpp", + ], + export_include_dirs: ["include"], + cflags: [ + "-Wall", + "-Wextra", + "-Werror", + ], +} diff --git a/broadcastradio/1.1/vts/functional/call-barrier.cpp b/broadcastradio/1.1/vts/utils/call-barrier.cpp similarity index 95% rename from broadcastradio/1.1/vts/functional/call-barrier.cpp rename to broadcastradio/1.1/vts/utils/call-barrier.cpp index fede297ad1..d8c47162ed 100644 --- a/broadcastradio/1.1/vts/functional/call-barrier.cpp +++ b/broadcastradio/1.1/vts/utils/call-barrier.cpp @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include "call-barrier.h" +#include namespace android { namespace hardware { diff --git a/broadcastradio/1.1/vts/functional/call-barrier.h b/broadcastradio/1.1/vts/utils/include/broadcastradio-vts-utils/call-barrier.h similarity index 100% rename from broadcastradio/1.1/vts/functional/call-barrier.h rename to broadcastradio/1.1/vts/utils/include/broadcastradio-vts-utils/call-barrier.h diff --git a/broadcastradio/1.1/vts/functional/mock-timeout.h b/broadcastradio/1.1/vts/utils/include/broadcastradio-vts-utils/mock-timeout.h similarity index 100% rename from broadcastradio/1.1/vts/functional/mock-timeout.h rename to broadcastradio/1.1/vts/utils/include/broadcastradio-vts-utils/mock-timeout.h diff --git a/broadcastradio/Android.bp b/broadcastradio/Android.bp index a5ad5e7af7..8c65bf6013 100644 --- a/broadcastradio/Android.bp +++ b/broadcastradio/Android.bp @@ -8,4 +8,5 @@ subdirs = [ "1.1/tests", "1.1/utils", "1.1/vts/functional", + "1.1/vts/utils", ]