From b8a2afe4469586e9ed68d3c886a94eea130cd99f Mon Sep 17 00:00:00 2001 From: Tomasz Wasilczyk Date: Tue, 1 Aug 2017 10:52:18 -0700 Subject: [PATCH] Address Broadcast Radio HAL review notes. Bug: b/64229617 Test: instrumentation, VTS Change-Id: I4009b33eaea6df585f514711f22dfb7fec5ad379 --- broadcastradio/1.1/IBroadcastRadio.hal | 13 +++++++++ broadcastradio/1.1/ITuner.hal | 28 +++++++++---------- broadcastradio/1.1/ITunerCallback.hal | 25 +++++++++-------- broadcastradio/1.1/default/Tuner.cpp | 22 +++++++-------- broadcastradio/1.1/default/Tuner.h | 4 +-- .../VtsHalBroadcastradioV1_1TargetTest.cpp | 6 ++-- 6 files changed, 57 insertions(+), 41 deletions(-) diff --git a/broadcastradio/1.1/IBroadcastRadio.hal b/broadcastradio/1.1/IBroadcastRadio.hal index 9bde361bdb..6e7002d785 100644 --- a/broadcastradio/1.1/IBroadcastRadio.hal +++ b/broadcastradio/1.1/IBroadcastRadio.hal @@ -40,6 +40,19 @@ interface IBroadcastRadio extends @1.0::IBroadcastRadio { * The data should be a valid PNG, JPEG, GIF or BMP file. * Invalid format must be handled gracefully as if the image was missing. * + * The image identifier may become invalid after some time from passing it + * with metadata struct (due to resource cleanup at the HAL implementation). + * However, it must remain valid for a currently tuned program at least + * until currentProgramInfoChanged or programListChanged is called and + * metadata changes for the current program. + * + * There is still a race condition possible (if the HAL deletes the old + * image immediately after notifying about the new one) between + * currentProgramInfoChanged callback propagating through the framework and + * the HAL implementation removing previous image. In such case, client + * application may expect the new currentProgramInfoChanged callback with + * updated image identifier. + * * @param id Identifier of an image; * value of 0 is reserved and should be treated as invalid image. * @return image A binary blob with image data diff --git a/broadcastradio/1.1/ITuner.hal b/broadcastradio/1.1/ITuner.hal index cc2e58db39..034f9c6d6c 100644 --- a/broadcastradio/1.1/ITuner.hal +++ b/broadcastradio/1.1/ITuner.hal @@ -39,7 +39,7 @@ interface ITuner extends @1.0::ITuner { * INVALID_ARGUMENTS if invalid arguments are passed. * NOT_INITIALIZED if another error occurs. */ - tune_1_1(ProgramSelector program) generates (Result result); + tuneByProgramSelector(ProgramSelector program) generates (Result result); /** * Cancels announcement. @@ -121,19 +121,6 @@ interface ITuner extends @1.0::ITuner { getProgramList(string filter) generates (ProgramListResult result, vec programList); - /** - * Checks, if the analog playback is forced, see setAnalogForced. - * - * The isForced value is only valid if result was OK. - * - * @return result OK if the call succeeded and isForced is valid. - * INVALID_STATE if the switch is not supported at current - * configuration. - * NOT_INITIALIZED if any other error occurs. - * @return isForced true if analog is forced, false otherwise. - */ - isAnalogForced() generates (Result result, bool isForced); - /** * Forces the analog playback for the supporting radio technology. * @@ -150,4 +137,17 @@ interface ITuner extends @1.0::ITuner { * NOT_INITIALIZED if any other error occurs. */ setAnalogForced(bool isForced) generates (Result result); + + /** + * Checks, if the analog playback is forced, see setAnalogForced. + * + * The isForced value is only valid if result was OK. + * + * @return result OK if the call succeeded and isForced is valid. + * INVALID_STATE if the switch is not supported at current + * configuration. + * NOT_INITIALIZED if any other error occurs. + * @return isForced true if analog is forced, false otherwise. + */ + isAnalogForced() generates (Result result, bool isForced); }; diff --git a/broadcastradio/1.1/ITunerCallback.hal b/broadcastradio/1.1/ITunerCallback.hal index b1c5b01d5b..2e593b0fad 100644 --- a/broadcastradio/1.1/ITunerCallback.hal +++ b/broadcastradio/1.1/ITunerCallback.hal @@ -29,9 +29,9 @@ interface ITunerCallback extends @1.0::ITunerCallback { * Method called by the HAL when a tuning operation completes * following a step(), scan() or tune() command. * - * This callback supersedes V1_0::tuneComplete. For performance reasons, - * the 1.0 callback may not be called when HAL implementation detects 1.1 - * client (by casting V1_0::ITunerCallback to V1_1::ITunerCallback). + * This callback supersedes V1_0::tuneComplete. + * The 1.0 callback must not be called when HAL implementation detects + * 1.1 client (by casting V1_0::ITunerCallback to V1_1::ITunerCallback). * * @param result OK if tune succeeded or TIMEOUT in case of time out. * @param selector A ProgramSelector structure describing the tuned station. @@ -41,9 +41,9 @@ interface ITunerCallback extends @1.0::ITunerCallback { /** * Method called by the HAL when a frequency switch occurs. * - * This callback supersedes V1_0::afSwitch. For performance reasons, - * the 1.0 callback may not be called when HAL implementation detects 1.1 - * client (by casting V1_0::ITunerCallback to V1_1::ITunerCallback). + * This callback supersedes V1_0::afSwitch. + * The 1.0 callback must not be called when HAL implementation detects + * 1.1 client (by casting V1_0::ITunerCallback to V1_1::ITunerCallback). * * @param selector A ProgramSelector structure describing the tuned station. */ @@ -73,6 +73,9 @@ interface ITunerCallback extends @1.0::ITunerCallback { * call it immediately, ie. it may wait for a short time to accumulate * multiple list change notifications into a single event. * + * This callback is only for notifying about insertions and deletions, + * not about metadata changes. + * * It may be triggered either by an explicitly issued background scan, * or a scan issued by the device internally. * @@ -89,10 +92,10 @@ interface ITunerCallback extends @1.0::ITunerCallback { * * This may be called together with tuneComplete_1_1 or afSwitch_1_1. * - * This callback supersedes V1_0::tuneComplete, V1_0::afSwitch and - * newMetadata. For performance reasons, these callbacks may not be called - * when HAL implementation detects 1.1 client (by casting - * V1_0::ITunerCallback to V1_1::ITunerCallback). + * This callback supersedes V1_0::newMetadata and partly V1_0::tuneComplete + * and V1_0::afSwitch. + * 1.0 callbacks must not be called when HAL implementation detects + * 1.1 client (by casting V1_0::ITunerCallback to V1_1::ITunerCallback). */ - oneway programInfoChanged(); + oneway currentProgramInfoChanged(); }; diff --git a/broadcastradio/1.1/default/Tuner.cpp b/broadcastradio/1.1/default/Tuner.cpp index f48a8db369..1e6b9daae0 100644 --- a/broadcastradio/1.1/default/Tuner.cpp +++ b/broadcastradio/1.1/default/Tuner.cpp @@ -248,10 +248,10 @@ Return Tuner::tune(uint32_t channel, uint32_t subChannel) { lock_guard lk(mMut); band = mAmfmConfig.type; } - return tune_1_1(utils::make_selector(band, channel, subChannel)); + return tuneByProgramSelector(utils::make_selector(band, channel, subChannel)); } -Return Tuner::tune_1_1(const ProgramSelector& sel) { +Return Tuner::tuneByProgramSelector(const ProgramSelector& sel) { ALOGV("%s(%s)", __func__, toString(sel).c_str()); lock_guard lk(mMut); if (mIsClosed) return Result::NOT_INITIALIZED; @@ -336,6 +336,15 @@ Return Tuner::getProgramList(const hidl_string& filter, getProgramList_cb return {}; } +Return Tuner::setAnalogForced(bool isForced) { + ALOGV("%s", __func__); + lock_guard lk(mMut); + if (mIsClosed) return Result::NOT_INITIALIZED; + + mIsAnalogForced = isForced; + return Result::OK; +} + Return Tuner::isAnalogForced(isAnalogForced_cb _hidl_cb) { ALOGV("%s", __func__); lock_guard lk(mMut); @@ -348,15 +357,6 @@ Return Tuner::isAnalogForced(isAnalogForced_cb _hidl_cb) { return {}; } -Return Tuner::setAnalogForced(bool isForced) { - ALOGV("%s", __func__); - lock_guard lk(mMut); - if (mIsClosed) return Result::NOT_INITIALIZED; - - mIsAnalogForced = isForced; - return Result::OK; -} - } // namespace implementation } // namespace V1_1 } // namespace broadcastradio diff --git a/broadcastradio/1.1/default/Tuner.h b/broadcastradio/1.1/default/Tuner.h index c4efe6ec4c..f375a84a37 100644 --- a/broadcastradio/1.1/default/Tuner.h +++ b/broadcastradio/1.1/default/Tuner.h @@ -39,7 +39,7 @@ struct Tuner : public ITuner { virtual Return scan(V1_0::Direction direction, bool skipSubChannel) override; virtual Return step(V1_0::Direction direction, bool skipSubChannel) override; virtual Return tune(uint32_t channel, uint32_t subChannel) override; - virtual Return tune_1_1(const ProgramSelector& program) override; + virtual Return tuneByProgramSelector(const ProgramSelector& program) override; virtual Return cancel() override; virtual Return cancelAnnouncement() override; virtual Return getProgramInformation(getProgramInformation_cb _hidl_cb) override; @@ -47,8 +47,8 @@ struct Tuner : public ITuner { virtual Return startBackgroundScan() override; virtual Return getProgramList(const hidl_string& filter, getProgramList_cb _hidl_cb) override; - virtual Return isAnalogForced(isAnalogForced_cb _hidl_cb) override; virtual Return setAnalogForced(bool isForced) override; + virtual Return isAnalogForced(isAnalogForced_cb _hidl_cb) override; private: std::mutex mMut; diff --git a/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp b/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp index bd2e0a7241..41cea275b0 100644 --- a/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp +++ b/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp @@ -84,7 +84,7 @@ struct TunerCallbackMock : public ITunerCallback { MOCK_METHOD1(backgroundScanAvailable, Return(bool)); MOCK_TIMEOUT_METHOD1(backgroundScanComplete, Return(ProgramListResult)); MOCK_METHOD0(programListChanged, Return()); - MOCK_METHOD0(programInfoChanged, Return()); + MOCK_METHOD0(currentProgramInfoChanged, Return()); }; class BroadcastRadioHalTest : public ::testing::VtsHalHidlTargetTestBase, @@ -315,7 +315,7 @@ TEST_P(BroadcastRadioHalTest, OpenTunerTwice) { * - getProgramList either succeeds or returns NOT_STARTED/NOT_READY status; * - if the program list is NOT_STARTED, startBackgroundScan makes it completed * within a full scan timeout and the next getProgramList call succeeds; - * - if the program list is not empty, tune_1_1 call succeeds. + * - if the program list is not empty, tuneByProgramSelector call succeeds. */ TEST_P(BroadcastRadioHalTest, TuneFromProgramList) { if (skipped) return; @@ -340,7 +340,7 @@ TEST_P(BroadcastRadioHalTest, TuneFromProgramList) { 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); + auto tuneResult = mTuner->tuneByProgramSelector(firstProgram.selector); ASSERT_EQ(Result::OK, tuneResult); EXPECT_TIMEOUT_CALL_WAIT(*mCallback, tuneComplete_1_1, kTuneTimeout); EXPECT_EQ(firstProgram.selector.primaryId, selCb.primaryId);