Address Broadcast Radio HAL review notes.

Bug: b/64229617
Test: instrumentation, VTS
Change-Id: I4009b33eaea6df585f514711f22dfb7fec5ad379

6 files changed, 57 insertions, 41 deletions

diff --git a/broadcastradio/1.1/IBroadcastRadio.hal b/broadcastradio/1.1/IBroadcastRadio.hal
index 9bde361bd..6e7002d78 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 cc2e58db3..034f9c6d6 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.
@@ -122,19 +122,6 @@ interface ITuner extends @1.0::ITuner {
         generates (ProgramListResult result, vec<ProgramInfo> 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.
      *
      * User may disable digital playback for FM HD Radio or hybrid FM/DAB with
@@ -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 b1c5b01d5..2e593b0fa 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 f48a8db36..1e6b9daae 100644
--- a/broadcastradio/1.1/default/Tuner.cpp
+++ b/broadcastradio/1.1/default/Tuner.cpp
@@ -248,10 +248,10 @@ Return<Result> Tuner::tune(uint32_t channel, uint32_t subChannel) {
         lock_guard<mutex> lk(mMut);
         band = mAmfmConfig.type;
     }
-    return tune_1_1(utils::make_selector(band, channel, subChannel));
+    return tuneByProgramSelector(utils::make_selector(band, channel, subChannel));
 }
 
-Return<Result> Tuner::tune_1_1(const ProgramSelector& sel) {
+Return<Result> Tuner::tuneByProgramSelector(const ProgramSelector& sel) {
     ALOGV("%s(%s)", __func__, toString(sel).c_str());
     lock_guard<mutex> lk(mMut);
     if (mIsClosed) return Result::NOT_INITIALIZED;
@@ -336,6 +336,15 @@ Return<void> Tuner::getProgramList(const hidl_string& filter, getProgramList_cb
     return {};
 }
 
+Return<Result> Tuner::setAnalogForced(bool isForced) {
+    ALOGV("%s", __func__);
+    lock_guard<mutex> lk(mMut);
+    if (mIsClosed) return Result::NOT_INITIALIZED;
+
+    mIsAnalogForced = isForced;
+    return Result::OK;
+}
+
 Return<void> Tuner::isAnalogForced(isAnalogForced_cb _hidl_cb) {
     ALOGV("%s", __func__);
     lock_guard<mutex> lk(mMut);
@@ -348,15 +357,6 @@ Return<void> Tuner::isAnalogForced(isAnalogForced_cb _hidl_cb) {
     return {};
 }
 
-Return<Result> Tuner::setAnalogForced(bool isForced) {
-    ALOGV("%s", __func__);
-    lock_guard<mutex> 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 c4efe6ec4..f375a84a3 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<Result> scan(V1_0::Direction direction, bool skipSubChannel) override;
     virtual Return<Result> step(V1_0::Direction direction, bool skipSubChannel) override;
     virtual Return<Result> tune(uint32_t channel, uint32_t subChannel) override;
-    virtual Return<Result> tune_1_1(const ProgramSelector& program) override;
+    virtual Return<Result> tuneByProgramSelector(const ProgramSelector& program) override;
     virtual Return<Result> cancel() override;
     virtual Return<Result> cancelAnnouncement() override;
     virtual Return<void> getProgramInformation(getProgramInformation_cb _hidl_cb) override;
@@ -47,8 +47,8 @@ struct Tuner : public ITuner {
     virtual Return<ProgramListResult> startBackgroundScan() override;
     virtual Return<void> getProgramList(const hidl_string& filter,
                                         getProgramList_cb _hidl_cb) override;
-    virtual Return<void> isAnalogForced(isAnalogForced_cb _hidl_cb) override;
     virtual Return<Result> setAnalogForced(bool isForced) override;
+    virtual Return<void> 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 bd2e0a724..41cea275b 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<void>(bool));
     MOCK_TIMEOUT_METHOD1(backgroundScanComplete, Return<void>(ProgramListResult));
     MOCK_METHOD0(programListChanged, Return<void>());
-    MOCK_METHOD0(programInfoChanged, Return<void>());
+    MOCK_METHOD0(currentProgramInfoChanged, Return<void>());
 };
 
 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);