From 0d4207d97ab73bb2a1d193ad08489370705abbfb Mon Sep 17 00:00:00 2001 From: Weilin Xu Date: Fri, 9 Dec 2022 00:37:44 +0000 Subject: [PATCH] Improve DAB support in broadcast radio AIDL HAL SID in DAB_SID_EXT was extended to 32-bit to support DMB radio. The documatation for DAB program selector and program info was updated to make a DAB station is uniquely specified. AIDL HAL definition requirement for identifier, program selector and info was enforced in the AIDL utils class. The default implementation and VTS were also updated correspondingly. Bug: 261912181 Test: atest VtsHalBroadcastradioAidlTargetTest Change-Id: Ic420955340f0c77370106e736410d7125536e62d --- .../broadcastradio/IdentifierType.aidl | 5 +- .../hardware/broadcastradio/ProgramInfo.aidl | 2 +- .../broadcastradio/ProgramSelector.aidl | 3 +- .../aidl/default/BroadcastRadio.cpp | 18 ++++--- .../aidl/default/VirtualProgram.cpp | 2 +- broadcastradio/aidl/default/VirtualRadio.cpp | 32 +++++++----- .../VtsHalBroadcastradioAidlTargetTest.cpp | 52 ++++++++++++++++--- broadcastradio/common/utilsaidl/Utils.cpp | 48 +++++++++++------ .../include/broadcastradio-utils-aidl/Utils.h | 2 +- 9 files changed, 117 insertions(+), 47 deletions(-) diff --git a/broadcastradio/aidl/android/hardware/broadcastradio/IdentifierType.aidl b/broadcastradio/aidl/android/hardware/broadcastradio/IdentifierType.aidl index 0484d02a9e..646c502660 100644 --- a/broadcastradio/aidl/android/hardware/broadcastradio/IdentifierType.aidl +++ b/broadcastradio/aidl/android/hardware/broadcastradio/IdentifierType.aidl @@ -110,10 +110,11 @@ enum IdentifierType { HD_STATION_NAME, /** - * 28bit compound primary identifier for Digital Audio Broadcasting. + * 44bit compound primary identifier for Digital Audio Broadcasting and + * Digital Multimeida Broadcasting. * * Consists of (from the LSB): - * - 16bit: SId; + * - 32bit: SId; * - 8bit: ECC code; * - 4bit: SCIdS. * diff --git a/broadcastradio/aidl/android/hardware/broadcastradio/ProgramInfo.aidl b/broadcastradio/aidl/android/hardware/broadcastradio/ProgramInfo.aidl index 3e2c9cca7d..d6502394ec 100644 --- a/broadcastradio/aidl/android/hardware/broadcastradio/ProgramInfo.aidl +++ b/broadcastradio/aidl/android/hardware/broadcastradio/ProgramInfo.aidl @@ -120,7 +120,7 @@ parcelable ProgramInfo { * * Only physical identifiers are valid: * - AMFM_FREQUENCY_KHZ; - * - DAB_ENSEMBLE; + * - DAB_FREQUENCY_KHZ; * - DRMO_FREQUENCY_KHZ; * - SXM_CHANNEL; * - VENDOR_*; diff --git a/broadcastradio/aidl/android/hardware/broadcastradio/ProgramSelector.aidl b/broadcastradio/aidl/android/hardware/broadcastradio/ProgramSelector.aidl index 8bd3fd41e4..93b0e12674 100644 --- a/broadcastradio/aidl/android/hardware/broadcastradio/ProgramSelector.aidl +++ b/broadcastradio/aidl/android/hardware/broadcastradio/ProgramSelector.aidl @@ -51,7 +51,8 @@ parcelable ProgramSelector { * - analogue AM/FM: AMFM_FREQUENCY_KHZ; * - FM RDS: RDS_PI; * - HD Radio: HD_STATION_ID_EXT; - * - DAB: DAB_SID_EXT; + * - DAB/DMB: DAB_SID_EXT (when used, DAB_ENSEMBLE and DAB_FREQUENCY_KHZ + * must present in secondaryIds); * - Digital Radio Mondiale: DRMO_SERVICE_ID; * - SiriusXM: SXM_SERVICE_ID; * - vendor-specific: VENDOR_START..VENDOR_END. diff --git a/broadcastradio/aidl/default/BroadcastRadio.cpp b/broadcastradio/aidl/default/BroadcastRadio.cpp index 42e550e75a..36520fb387 100644 --- a/broadcastradio/aidl/default/BroadcastRadio.cpp +++ b/broadcastradio/aidl/default/BroadcastRadio.cpp @@ -589,22 +589,28 @@ binder_status_t BroadcastRadio::cmdTune(int fd, const char** args, uint32_t numA } ProgramSelector sel = {}; if (isDab) { - if (numArgs != 4) { + if (numArgs != 5) { dprintf(fd, - "Invalid number of arguments: please provide --tune dab \n"); + "Invalid number of arguments: please provide " + "--tune dab \n"); return STATUS_BAD_VALUE; } int sid; if (!utils::parseArgInt(string(args[2]), &sid)) { - dprintf(fd, "Non-integer sid provided with tune: %s\n", string(args[2]).c_str()); + dprintf(fd, "Non-integer sid provided with tune: %s\n", args[2]); return STATUS_BAD_VALUE; } int ensemble; if (!utils::parseArgInt(string(args[3]), &ensemble)) { - dprintf(fd, "Non-integer ensemble provided with tune: %s\n", string(args[3]).c_str()); + dprintf(fd, "Non-integer ensemble provided with tune: %s\n", args[3]); return STATUS_BAD_VALUE; } - sel = utils::makeSelectorDab(sid, ensemble); + int freq; + if (!utils::parseArgInt(string(args[4]), &freq)) { + dprintf(fd, "Non-integer frequency provided with tune: %s\n", args[4]); + return STATUS_BAD_VALUE; + } + sel = utils::makeSelectorDab(sid, ensemble, freq); } else { if (numArgs != 3) { dprintf(fd, "Invalid number of arguments: please provide --tune amfm \n"); @@ -612,7 +618,7 @@ binder_status_t BroadcastRadio::cmdTune(int fd, const char** args, uint32_t numA } int freq; if (!utils::parseArgInt(string(args[2]), &freq)) { - dprintf(fd, "Non-integer frequency provided with tune: %s\n", string(args[2]).c_str()); + dprintf(fd, "Non-integer frequency provided with tune: %s\n", args[2]); return STATUS_BAD_VALUE; } sel = utils::makeSelectorAmfm(freq); diff --git a/broadcastradio/aidl/default/VirtualProgram.cpp b/broadcastradio/aidl/default/VirtualProgram.cpp index 0df0a82fa4..4fe6567d17 100644 --- a/broadcastradio/aidl/default/VirtualProgram.cpp +++ b/broadcastradio/aidl/default/VirtualProgram.cpp @@ -53,7 +53,7 @@ VirtualProgram::operator ProgramInfo() const { break; case IdentifierType::DAB_SID_EXT: info.logicallyTunedTo = selectId(IdentifierType::DAB_SID_EXT); - info.physicallyTunedTo = selectId(IdentifierType::DAB_ENSEMBLE); + info.physicallyTunedTo = selectId(IdentifierType::DAB_FREQUENCY_KHZ); break; case IdentifierType::DRMO_SERVICE_ID: info.logicallyTunedTo = selectId(IdentifierType::DRMO_SERVICE_ID); diff --git a/broadcastradio/aidl/default/VirtualRadio.cpp b/broadcastradio/aidl/default/VirtualRadio.cpp index 851543b0aa..126bcff621 100644 --- a/broadcastradio/aidl/default/VirtualRadio.cpp +++ b/broadcastradio/aidl/default/VirtualRadio.cpp @@ -53,15 +53,18 @@ const VirtualRadio& VirtualRadio::getAmFmRadio() { static VirtualRadio amFmRadioMock( "AM/FM radio mock", { - {makeSelectorAmfm(94900), "Wild 94.9", "Drake ft. Rihanna", "Too Good"}, - {makeSelectorAmfm(96500), "KOIT", "Celine Dion", "All By Myself"}, - {makeSelectorAmfm(97300), "Alice@97.3", "Drops of Jupiter", "Train"}, - {makeSelectorAmfm(99700), "99.7 Now!", "The Chainsmokers", "Closer"}, - {makeSelectorAmfm(101300), "101-3 KISS-FM", "Justin Timberlake", "Rock Your Body"}, - {makeSelectorAmfm(103700), "iHeart80s @ 103.7", "Michael Jackson", "Billie Jean"}, - {makeSelectorAmfm(106100), "106 KMEL", "Drake", "Marvins Room"}, - {makeSelectorAmfm(700), "700 AM", "Artist700", "Title700"}, - {makeSelectorAmfm(1700), "1700 AM", "Artist1700", "Title1700"}, + {makeSelectorAmfm(/* frequency= */ 94900), "Wild 94.9", "Drake ft. Rihanna", + "Too Good"}, + {makeSelectorAmfm(/* frequency= */ 96500), "KOIT", "Celine Dion", "All By Myself"}, + {makeSelectorAmfm(/* frequency= */ 97300), "Alice@97.3", "Drops of Jupiter", "Train"}, + {makeSelectorAmfm(/* frequency= */ 99700), "99.7 Now!", "The Chainsmokers", "Closer"}, + {makeSelectorAmfm(/* frequency= */ 101300), "101-3 KISS-FM", "Justin Timberlake", + "Rock Your Body"}, + {makeSelectorAmfm(/* frequency= */ 103700), "iHeart80s @ 103.7", "Michael Jackson", + "Billie Jean"}, + {makeSelectorAmfm(/* frequency= */ 106100), "106 KMEL", "Drake", "Marvins Room"}, + {makeSelectorAmfm(/* frequency= */ 700), "700 AM", "Artist700", "Title700"}, + {makeSelectorAmfm(/* frequency= */ 1700), "1700 AM", "Artist1700", "Title1700"}, }); // clang-format on return amFmRadioMock; @@ -73,9 +76,14 @@ const VirtualRadio& VirtualRadio::getDabRadio() { static VirtualRadio dabRadioMock( "DAB radio mock", { - {makeSelectorDab(0xA00001u, 0x0001u), "BBC Radio 1", "Khalid", "Talk"}, - {makeSelectorDab(0xB00001u, 0x1001u), "Classic FM", "Jean Sibelius", "Andante Festivo"}, - {makeSelectorDab(0xB00002u, 0x1001u), "Absolute Radio", "Coldplay", "Clocks"}, + {makeSelectorDab(/* sidExt= */ 0xA000000001u, /* ensemble= */ 0x0001u, + /* freq= */ 225648), "BBC Radio 1", "Khalid", "Talk"}, + {makeSelectorDab(/* sidExt= */ 0xB000000001u, /* ensemble= */ 0x1001u, + /* freq= */ 222064), "Classic FM", "Jean Sibelius", "Andante Festivo"}, + {makeSelectorDab(/* sidExt= */ 0xB000000002u, /* ensemble= */ 0x1002u, + /* freq= */ 227360), "Absolute Radio", "Coldplay", "Clocks"}, + {makeSelectorDab(/* sidExt= */ 0xB000000002u, /* ensemble= */ 0x1002u, + /* freq= */ 222064), "Absolute Radio", "Coldplay", "Clocks"}, }); // clang-format on return dabRadioMock; diff --git a/broadcastradio/aidl/vts/src/VtsHalBroadcastradioAidlTargetTest.cpp b/broadcastradio/aidl/vts/src/VtsHalBroadcastradioAidlTargetTest.cpp index 5a568464ec..356673f715 100644 --- a/broadcastradio/aidl/vts/src/VtsHalBroadcastradioAidlTargetTest.cpp +++ b/broadcastradio/aidl/vts/src/VtsHalBroadcastradioAidlTargetTest.cpp @@ -46,6 +46,7 @@ namespace { using ::aidl::android::hardware::broadcastradio::utils::makeIdentifier; using ::aidl::android::hardware::broadcastradio::utils::makeSelectorAmfm; +using ::aidl::android::hardware::broadcastradio::utils::makeSelectorDab; using ::aidl::android::hardware::broadcastradio::utils::resultToInt; using ::ndk::ScopedAStatus; using ::ndk::SharedRefBase; @@ -110,8 +111,7 @@ bool supportsFM(const AmFmRegionConfig& config) { class TunerCallbackMock : public BnTunerCallback { public: TunerCallbackMock(); - - MOCK_METHOD2(onTuneFailed, ScopedAStatus(Result, const ProgramSelector&)); + ScopedAStatus onTuneFailed(Result result, const ProgramSelector& selector) override; MOCK_TIMEOUT_METHOD1(onCurrentProgramInfoChangedMock, ScopedAStatus(const ProgramInfo&)); ScopedAStatus onCurrentProgramInfoChanged(const ProgramInfo& info) override; ScopedAStatus onProgramListUpdated(const ProgramListChunk& chunk) override; @@ -154,6 +154,12 @@ TunerCallbackMock::TunerCallbackMock() { EXPECT_CALL(*this, onAntennaStateChange(false)).Times(0); } +ScopedAStatus TunerCallbackMock::onTuneFailed(Result result, const ProgramSelector& selector) { + LOG(DEBUG) << "Tune failed for selector" << selector.toString(); + EXPECT_TRUE(result == Result::CANCELED); + return ndk::ScopedAStatus::ok(); +} + ScopedAStatus TunerCallbackMock::onCurrentProgramInfoChanged(const ProgramInfo& info) { for (const auto& id : info.selector) { EXPECT_NE(id.type, IdentifierType::INVALID); @@ -175,7 +181,7 @@ ScopedAStatus TunerCallbackMock::onCurrentProgramInfoChanged(const ProgramInfo& IdentifierType physically = info.physicallyTunedTo.type; // ditto (see "logically" above) EXPECT_TRUE(physically == IdentifierType::AMFM_FREQUENCY_KHZ || - physically == IdentifierType::DAB_ENSEMBLE || + physically == IdentifierType::DAB_FREQUENCY_KHZ || physically == IdentifierType::DRMO_FREQUENCY_KHZ || physically == IdentifierType::SXM_CHANNEL || (physically >= IdentifierType::VENDOR_START && @@ -593,10 +599,43 @@ TEST_P(BroadcastRadioHalTest, DabTune) { ASSERT_TRUE(halResult.isOk()); ASSERT_NE(config.size(), 0U); - // TODO(245787803): use a DAB frequency that can actually be tuned to. + auto programList = getProgramList(); + + if (!programList) { + printSkipped("Empty DAB station list, tune cannot be performed"); + return; + } + ProgramSelector sel = {}; - int64_t freq = config[config.size() / 2].frequencyKhz; - sel.primaryId = makeIdentifier(IdentifierType::DAB_FREQUENCY_KHZ, freq); + uint64_t freq = 0; + bool dabStationPresent = false; + for (auto&& programInfo : *programList) { + if (!utils::hasId(programInfo.selector, IdentifierType::DAB_FREQUENCY_KHZ)) { + continue; + } + for (auto&& config_entry : config) { + if (config_entry.frequencyKhz == + utils::getId(programInfo.selector, IdentifierType::DAB_FREQUENCY_KHZ, 0)) { + freq = config_entry.frequencyKhz; + break; + } + } + // Do not trigger a tune request if the programList entry does not contain + // a valid DAB frequency. + if (freq == 0) { + continue; + } + int64_t dabSidExt = utils::getId(programInfo.selector, IdentifierType::DAB_SID_EXT, 0); + int64_t dabEns = utils::getId(programInfo.selector, IdentifierType::DAB_ENSEMBLE, 0); + sel = makeSelectorDab(dabSidExt, (int32_t)dabEns, freq); + dabStationPresent = true; + break; + } + + if (!dabStationPresent) { + printSkipped("No DAB stations in the list, tune cannot be performed"); + return; + } // try tuning ProgramInfo infoCb = {}; @@ -623,7 +662,6 @@ TEST_P(BroadcastRadioHalTest, DabTune) { vector freqs = bcutils::getAllIds(infoCb.selector, IdentifierType::DAB_FREQUENCY_KHZ); EXPECT_NE(freqs.end(), find(freqs.begin(), freqs.end(), freq)) << "DAB freq " << freq << " kHz is not sent back by callback."; - ; } /** diff --git a/broadcastradio/common/utilsaidl/Utils.cpp b/broadcastradio/common/utilsaidl/Utils.cpp index 52c7b40c7a..ad823661db 100644 --- a/broadcastradio/common/utilsaidl/Utils.cpp +++ b/broadcastradio/common/utilsaidl/Utils.cpp @@ -136,7 +136,9 @@ bool tunesTo(const ProgramSelector& a, const ProgramSelector& b) { return getHdSubchannel(b) == 0 && haveEqualIds(a, b, IdentifierType::AMFM_FREQUENCY_KHZ); case IdentifierType::DAB_SID_EXT: - return haveEqualIds(a, b, IdentifierType::DAB_SID_EXT); + return haveEqualIds(a, b, IdentifierType::DAB_SID_EXT) && + haveEqualIds(a, b, IdentifierType::DAB_ENSEMBLE) && + haveEqualIds(a, b, IdentifierType::DAB_FREQUENCY_KHZ); case IdentifierType::DRMO_SERVICE_ID: return haveEqualIds(a, b, IdentifierType::DRMO_SERVICE_ID); case IdentifierType::SXM_SERVICE_ID: @@ -241,8 +243,8 @@ bool isValid(const ProgramIdentifier& id) { break; } case IdentifierType::DAB_SID_EXT: { - int64_t sid = val & 0xFFFF; // 16bit - val >>= 16; + int64_t sid = val & 0xFFFFFFFF; // 32bit + val >>= 32; int64_t ecc = val & 0xFF; // 8bit expect(sid != 0u, "DAB SId != 0"); expect(ecc >= 0xA0u && ecc <= 0xF6u, "Invalid ECC, see ETSI TS 101 756 V2.1.1"); @@ -277,13 +279,35 @@ bool isValid(const ProgramIdentifier& id) { } bool isValid(const ProgramSelector& sel) { - // iterate through primaryId and secondaryIds - for (auto it = begin(sel); it != end(sel); it++) { + if (sel.primaryId.type != IdentifierType::AMFM_FREQUENCY_KHZ && + sel.primaryId.type != IdentifierType::RDS_PI && + sel.primaryId.type != IdentifierType::HD_STATION_ID_EXT && + sel.primaryId.type != IdentifierType::DAB_SID_EXT && + sel.primaryId.type != IdentifierType::DRMO_SERVICE_ID && + sel.primaryId.type != IdentifierType::SXM_SERVICE_ID && + (sel.primaryId.type < IdentifierType::VENDOR_START || + sel.primaryId.type > IdentifierType::VENDOR_END)) { + return false; + } + if (!isValid(sel.primaryId)) { + return false; + } + + bool isDab = sel.primaryId.type == IdentifierType::DAB_SID_EXT; + bool hasDabEnsemble = false; + bool hasDabFrequency = false; + for (auto it = sel.secondaryIds.begin(); it != sel.secondaryIds.end(); it++) { if (!isValid(*it)) { return false; } + if (isDab && it->type == IdentifierType::DAB_ENSEMBLE) { + hasDabEnsemble = true; + } + if (isDab && it->type == IdentifierType::DAB_FREQUENCY_KHZ) { + hasDabFrequency = true; + } } - return true; + return !isDab || (hasDabEnsemble && hasDabFrequency); } ProgramIdentifier makeIdentifier(IdentifierType type, int64_t value) { @@ -296,16 +320,12 @@ ProgramSelector makeSelectorAmfm(int32_t frequency) { return sel; } -ProgramSelector makeSelectorDab(int32_t sidExt, int32_t ensemble) { +ProgramSelector makeSelectorDab(int64_t sidExt, int32_t ensemble, int64_t freq) { ProgramSelector sel = {}; - // TODO(243686545): Have a helper function to create the sidExt instead of - // passing the whole identifier here. Something like makeDabSidExt. sel.primaryId = makeIdentifier(IdentifierType::DAB_SID_EXT, sidExt); vector secondaryIds = { makeIdentifier(IdentifierType::DAB_ENSEMBLE, ensemble), - // TODO(243686545): Include frequency here when the helper method to - // translate between ensemble and frequency is implemented. - }; + makeIdentifier(IdentifierType::DAB_FREQUENCY_KHZ, freq)}; sel.secondaryIds = std::move(secondaryIds); return sel; } @@ -330,10 +350,6 @@ bool satisfies(const ProgramFilter& filter, const ProgramSelector& sel) { } } - if (!filter.includeCategories && sel.primaryId.type == IdentifierType::DAB_ENSEMBLE) { - return false; - } - return true; } diff --git a/broadcastradio/common/utilsaidl/include/broadcastradio-utils-aidl/Utils.h b/broadcastradio/common/utilsaidl/include/broadcastradio-utils-aidl/Utils.h index 8ea6319ce7..beebd037c2 100644 --- a/broadcastradio/common/utilsaidl/include/broadcastradio-utils-aidl/Utils.h +++ b/broadcastradio/common/utilsaidl/include/broadcastradio-utils-aidl/Utils.h @@ -138,7 +138,7 @@ bool isValid(const ProgramSelector& sel); ProgramIdentifier makeIdentifier(IdentifierType type, int64_t value); ProgramSelector makeSelectorAmfm(int32_t frequency); -ProgramSelector makeSelectorDab(int32_t sidExt, int32_t ensemble); +ProgramSelector makeSelectorDab(int64_t sidExt, int32_t ensemble, int64_t freq); bool satisfies(const ProgramFilter& filter, const ProgramSelector& sel);