From c71624f8ed57962fe85656aac5b687d8394dfdc0 Mon Sep 17 00:00:00 2001 From: Tomasz Wasilczyk Date: Fri, 22 Dec 2017 10:54:34 -0800 Subject: [PATCH] Prepare a best-effort workaround for HD Radio station id abuse. In theory, 32bit HD Radio station ID + subchannel index (parts of HD_STATION_ID_EXT) is a globally unique identifier. It allows broadcast radio framework to determine which programs are the same and allow the application to match entries from favourite list and the program list provided by the tuner. However, some broadcasters don't perform equipment setup correctly and don't set station ID. As a result, there are some stations with conflicting IDs. As a workaround to treat these stations separately in a given location, FM frequency was added as a part of HD_STATION_ID_EXT. This still doesn't solve the global uniqueness problem: user might save KCQW 105.5 (sid=0) in California, travel to Nevada and find KNAB 105.5 (sid=0). It turns out there is no reliable identifier that might identify the station globally. As a workaround, shortened station name is added for double-checking. This is a best-effort fix, so it's not required for such misbehaving stations to get correctly identified in every corner case. Bug: 69958777 Test: VTS Change-Id: Id11243096f1cde7fdda5cb70a7248d1831985cdd --- broadcastradio/2.0/types.hal | 25 +++++++- broadcastradio/2.0/vts/functional/Android.bp | 3 + .../VtsHalBroadcastradioV2_0TargetTest.cpp | 59 +++++++++++++++---- broadcastradio/common/tests/Android.bp | 7 +++ .../common/tests/ProgramIdentifier_test.cpp | 40 +++++++++++++ broadcastradio/common/utils2x/Android.bp | 3 + broadcastradio/common/utils2x/Utils.cpp | 43 ++++++++++++++ .../include/broadcastradio-utils-2x/Utils.h | 6 ++ 8 files changed, 173 insertions(+), 13 deletions(-) create mode 100644 broadcastradio/common/tests/ProgramIdentifier_test.cpp diff --git a/broadcastradio/2.0/types.hal b/broadcastradio/2.0/types.hal index 38a5709a29..a4474c8c06 100644 --- a/broadcastradio/2.0/types.hal +++ b/broadcastradio/2.0/types.hal @@ -466,7 +466,12 @@ enum IdentifierType : uint32_t { * Consists of (from the LSB): * - 32bit: Station ID number; * - 4bit: HD Radio subchannel; - * - 18bit: AMFM_FREQUENCY. // TODO(b/69958777): is it necessary? + * - 18bit: AMFM_FREQUENCY. + * + * While station ID number should be unique globally, it sometimes get + * abused by broadcasters (i.e. not being set at all). To ensure local + * uniqueness, AMFM_FREQUENCY was added here. Global uniqueness is + * a best-effort - see HD_STATION_NAME. * * HD Radio subchannel is a value in range 0-7. * This index is 0-based (where 0 is MPS and 1..7 are SPS), @@ -477,6 +482,22 @@ enum IdentifierType : uint32_t { */ HD_STATION_ID_EXT, + /** + * 64bit additional identifier for HD Radio. + * + * Due to Station ID abuse, some HD_STATION_ID_EXT identifiers may be not + * globally unique. To provide a best-effort solution, a short version of + * station name may be carried as additional identifier and may be used + * by the tuner hardware to double-check tuning. + * + * The name is limited to the first 8 A-Z0-9 characters (lowercase letters + * must be converted to uppercase). Encoded in little-endian ASCII: + * the first character of the name is the LSB. + * + * For example: "Abc" is encoded as 0x434241. + */ + HD_STATION_NAME, + /** * 28bit compound primary identifier for Digital Audio Broadcasting. * @@ -492,7 +513,7 @@ enum IdentifierType : uint32_t { * The remaining bits should be set to zeros when writing on the chip side * and ignored when read. */ - DAB_SID_EXT = HD_STATION_ID_EXT + 2, + DAB_SID_EXT, /** 16bit */ DAB_ENSEMBLE, diff --git a/broadcastradio/2.0/vts/functional/Android.bp b/broadcastradio/2.0/vts/functional/Android.bp index 6017b15291..6940bca49e 100644 --- a/broadcastradio/2.0/vts/functional/Android.bp +++ b/broadcastradio/2.0/vts/functional/Android.bp @@ -17,6 +17,9 @@ cc_test { name: "VtsHalBroadcastradioV2_0TargetTest", defaults: ["VtsHalTargetTestDefaults"], + cppflags: [ + "-std=c++1z", + ], srcs: ["VtsHalBroadcastradioV2_0TargetTest.cpp"], static_libs: [ "android.hardware.broadcastradio@2.0", diff --git a/broadcastradio/2.0/vts/functional/VtsHalBroadcastradioV2_0TargetTest.cpp b/broadcastradio/2.0/vts/functional/VtsHalBroadcastradioV2_0TargetTest.cpp index 87ac93415e..151e089af4 100644 --- a/broadcastradio/2.0/vts/functional/VtsHalBroadcastradioV2_0TargetTest.cpp +++ b/broadcastradio/2.0/vts/functional/VtsHalBroadcastradioV2_0TargetTest.cpp @@ -30,6 +30,7 @@ #include #include +#include #include namespace android { @@ -96,6 +97,7 @@ class BroadcastRadioHalTest : public ::testing::VtsHalHidlTargetTestBase { bool openSession(); bool getAmFmRegionConfig(bool full, AmFmRegionConfig* config); + std::optional getProgramList(); sp mModule; Properties mProperties; @@ -178,6 +180,25 @@ bool BroadcastRadioHalTest::getAmFmRegionConfig(bool full, AmFmRegionConfig* con return halResult == Result::OK; } +std::optional BroadcastRadioHalTest::getProgramList() { + EXPECT_TIMEOUT_CALL(*mCallback, onProgramListReady).Times(AnyNumber()); + + auto startResult = mSession->startProgramListUpdates({}); + if (startResult == Result::NOT_SUPPORTED) { + printSkipped("Program list not supported"); + return nullopt; + } + EXPECT_EQ(Result::OK, startResult); + if (startResult != Result::OK) return nullopt; + + EXPECT_TIMEOUT_CALL_WAIT(*mCallback, onProgramListReady, timeout::programListScan); + + auto stopResult = mSession->stopProgramListUpdates(); + EXPECT_TRUE(stopResult.isOk()); + + return mCallback->mProgramList; +} + /** * Test session opening. * @@ -645,19 +666,35 @@ TEST_F(BroadcastRadioHalTest, SetConfigFlags) { TEST_F(BroadcastRadioHalTest, GetProgramList) { ASSERT_TRUE(openSession()); - EXPECT_TIMEOUT_CALL(*mCallback, onProgramListReady).Times(AnyNumber()); + getProgramList(); +} - auto startResult = mSession->startProgramListUpdates({}); - if (startResult == Result::NOT_SUPPORTED) { - printSkipped("Program list not supported"); - return; +/** + * Test HD_STATION_NAME correctness. + * + * Verifies that if a program on the list contains HD_STATION_NAME identifier: + * - the program provides station name in its metadata; + * - the identifier matches the name; + * - there is only one identifier of that type. + */ +TEST_F(BroadcastRadioHalTest, HdRadioStationNameId) { + ASSERT_TRUE(openSession()); + + auto list = getProgramList(); + if (!list) return; + + for (auto&& program : *list) { + auto nameIds = utils::getAllIds(program.selector, IdentifierType::HD_STATION_NAME); + EXPECT_LE(nameIds.size(), 1u); + if (nameIds.size() == 0) continue; + + auto name = utils::getMetadataString(program, MetadataKey::PROGRAM_NAME); + if (!name) name = utils::getMetadataString(program, MetadataKey::RDS_PS); + ASSERT_TRUE(name.has_value()); + + auto expectedId = utils::make_hdradio_station_name(*name); + EXPECT_EQ(expectedId.value, nameIds[0]); } - ASSERT_EQ(Result::OK, startResult); - - EXPECT_TIMEOUT_CALL_WAIT(*mCallback, onProgramListReady, timeout::programListScan); - - auto stopResult = mSession->stopProgramListUpdates(); - EXPECT_TRUE(stopResult.isOk()); } // TODO(b/70939328): test ProgramInfo's currentlyTunedId and diff --git a/broadcastradio/common/tests/Android.bp b/broadcastradio/common/tests/Android.bp index 512c02e496..f6a3b6f4d9 100644 --- a/broadcastradio/common/tests/Android.bp +++ b/broadcastradio/common/tests/Android.bp @@ -22,6 +22,9 @@ cc_test { "-Wextra", "-Werror", ], + cppflags: [ + "-std=c++1z", + ], srcs: [ "CommonXX_test.cpp", ], @@ -43,8 +46,12 @@ cc_test { "-Wextra", "-Werror", ], + cppflags: [ + "-std=c++1z", + ], srcs: [ "IdentifierIterator_test.cpp", + "ProgramIdentifier_test.cpp", ], static_libs: [ "android.hardware.broadcastradio@common-utils-2x-lib", diff --git a/broadcastradio/common/tests/ProgramIdentifier_test.cpp b/broadcastradio/common/tests/ProgramIdentifier_test.cpp new file mode 100644 index 0000000000..51ad0145ee --- /dev/null +++ b/broadcastradio/common/tests/ProgramIdentifier_test.cpp @@ -0,0 +1,40 @@ +/* + * 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. + */ + +#include +#include + +#include + +namespace { + +namespace utils = android::hardware::broadcastradio::utils; + +TEST(ProgramIdentifierTest, hdRadioStationName) { + auto verify = [](std::string name, uint64_t nameId) { + auto id = utils::make_hdradio_station_name(name); + EXPECT_EQ(nameId, id.value) << "Failed to convert '" << name << "'"; + }; + + verify("", 0); + verify("Abc", 0x434241); + verify("Some Station 1", 0x54415453454d4f53); + verify("Station1", 0x314e4f4954415453); + verify("!@#$%^&*()_+", 0); + verify("-=[]{};':\"0", 0x30); +} + +} // anonymous namespace diff --git a/broadcastradio/common/utils2x/Android.bp b/broadcastradio/common/utils2x/Android.bp index c6b94afb0f..aab94f2a1f 100644 --- a/broadcastradio/common/utils2x/Android.bp +++ b/broadcastradio/common/utils2x/Android.bp @@ -23,6 +23,9 @@ cc_library_static { "-Wextra", "-Werror", ], + cppflags: [ + "-std=c++1z", + ], srcs: [ "Utils.cpp", ], diff --git a/broadcastradio/common/utils2x/Utils.cpp b/broadcastradio/common/utils2x/Utils.cpp index d825a7a000..6fe95549d8 100644 --- a/broadcastradio/common/utils2x/Utils.cpp +++ b/broadcastradio/common/utils2x/Utils.cpp @@ -245,6 +245,15 @@ bool isValid(const ProgramIdentifier& id) { expect(freq < 10000000u, "f < 10GHz"); break; } + case IdentifierType::HD_STATION_NAME: { + while (val > 0) { + auto ch = static_cast(val & 0xFF); + val >>= 8; + expect((ch >= '0' && ch <= '9') || (ch >= 'A' && ch <= 'Z'), + "HD_STATION_NAME does not match [A-Z0-9]+"); + } + break; + } case IdentifierType::DAB_SID_EXT: { auto sid = val & 0xFFFF; // 16bit val >>= 16; @@ -367,6 +376,40 @@ void updateProgramList(ProgramInfoSet& list, const ProgramListChunk& chunk) { } } +std::optional getMetadataString(const V2_0::ProgramInfo& info, + const V2_0::MetadataKey key) { + auto isKey = [key](const V2_0::Metadata& item) { + return static_cast(item.key) == key; + }; + + auto it = std::find_if(info.metadata.begin(), info.metadata.end(), isKey); + if (it == info.metadata.end()) return std::nullopt; + + return it->stringValue; +} + +V2_0::ProgramIdentifier make_hdradio_station_name(const std::string& name) { + constexpr size_t maxlen = 8; + + std::string shortName; + shortName.reserve(maxlen); + + auto&& loc = std::locale::classic(); + for (char ch : name) { + if (!std::isalnum(ch, loc)) continue; + shortName.push_back(std::toupper(ch, loc)); + if (shortName.length() >= maxlen) break; + } + + uint64_t val = 0; + for (auto rit = shortName.rbegin(); rit != shortName.rend(); ++rit) { + val <<= 8; + val |= static_cast(*rit); + } + + return make_identifier(IdentifierType::HD_STATION_NAME, val); +} + } // namespace utils } // namespace broadcastradio } // namespace hardware diff --git a/broadcastradio/common/utils2x/include/broadcastradio-utils-2x/Utils.h b/broadcastradio/common/utils2x/include/broadcastradio-utils-2x/Utils.h index e3134f7b88..5e51941634 100644 --- a/broadcastradio/common/utils2x/include/broadcastradio-utils-2x/Utils.h +++ b/broadcastradio/common/utils2x/include/broadcastradio-utils-2x/Utils.h @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -146,6 +147,11 @@ typedef std::unordered_set getMetadataString(const V2_0::ProgramInfo& info, + const V2_0::MetadataKey key); + +V2_0::ProgramIdentifier make_hdradio_station_name(const std::string& name); + } // namespace utils } // namespace broadcastradio } // namespace hardware