From d587032a0a3ab2c26fa483c2bf59f27c5741b026 Mon Sep 17 00:00:00 2001 From: Michele Bigi Date: Tue, 25 Aug 2026 23:58:37 +0200 Subject: [PATCH] Fix FM RDS: station name/RadioText never decoded, three stacked bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/tuner/status and the full FM band scan always returned an empty station_name, on every frequency, regardless of signal quality -- confirmed live on multiple >35 dB SNR channels before this fix. Three independent problems, found by reading the Si4684 datasheet (AN649 Rev.2.0) instead of guessing further: 1. FM_RDS_CONFIG (property 0x3C02) was enabled (RDSEN=1) but with both block-error thresholds at 0 ("no block errors"), so almost any real-world RDS group -- routine with ordinary multipath/noise, even on a strong signal -- got rejected from the FIFO outright. Raised to the datasheet's most tolerant recommended setting (2, "3-5 bit errors detected and corrected"). 2. FM_RDS_INTERRUPT_FIFO_COUNT (property 0x3C01) was never set at all, defaulting to 0 -- which the datasheet states disables RDSFIFOINT permanently. Si4684Driver::readFmRds()'s "received" flag reads exactly that bit (FM_RDS_STATUS RESP4 bit 0), so it could never be true, and Si4684Tuner::refreshStatus()'s RDS poll loop broke out on its first iteration every single time, before ever touching the accumulator. Set to 1 (fire as soon as one group is queued). 3. The real bug, once groups actually started arriving: RdsMetadataAccumulator::applyGroup() read the two Program Service name characters from Block C and computed the segment index as (blockB >> 1) & 0x3. Per ETSI EN 62106 §3.1.5, PS characters for group type 0 (both 0A and 0B) are always in Block D -- Block C holds alternate-frequency codes (0A) or a repeated PI code (0B), never text -- and the segment address is blockB bits[1:0] with no shift. This function had no dedicated test before now, which is how a wrong block/bit pair could ship unnoticed: the old test fixture encoded the same bug in its "expected" input. Confirmed live after all three fixes: station_name and radiotext both populate with stable, plausible content across repeated reads (not noise) on a real broadcast signal. Co-Authored-By: Claude Sonnet 5 --- .../include/core/RdsMetadataAccumulator.hpp | 10 +- .../core/src/RdsMetadataAccumulator.cpp | 11 +- Software/components/core/test/CMakeLists.txt | 4 + .../core/test/broadcast_metadata_test.cpp | 10 +- .../test/rds_metadata_accumulator_test.cpp | 126 ++++++++++++++++++ .../drivers/si4684/src/Si4684Driver.cpp | 20 ++- 6 files changed, 168 insertions(+), 13 deletions(-) create mode 100644 Software/components/core/test/rds_metadata_accumulator_test.cpp diff --git a/Software/components/core/include/core/RdsMetadataAccumulator.hpp b/Software/components/core/include/core/RdsMetadataAccumulator.hpp index 3126817..b78e955 100644 --- a/Software/components/core/include/core/RdsMetadataAccumulator.hpp +++ b/Software/components/core/include/core/RdsMetadataAccumulator.hpp @@ -47,10 +47,12 @@ public: * @brief applyGroup — ingest one RDS group (blocks A–D). * * @dname applyGroup - * @param blockA RDS block A (PI code). - * @param blockB RDS block B (group type and address). - * @param blockC RDS block C (text payload). - * @param blockD RDS block D (text payload for RT). + * @param blockA RDS block A (PI code, currently unused). + * @param blockB RDS block B (group type and segment address). + * @param blockC RDS block C (RT text for group 2 only; group 0's + * AF list/repeated PI is not text and is ignored). + * @param blockD RDS block D (PS text for group 0; RT text for + * group 2, per ETSI EN 62106 §3.1.5/§3.1.5.3). * @pubstate updates PS/RT buffers for group types 0A and 2A. * * @author Michele Bigi diff --git a/Software/components/core/src/RdsMetadataAccumulator.cpp b/Software/components/core/src/RdsMetadataAccumulator.cpp index 8ef84b2..9df17eb 100644 --- a/Software/components/core/src/RdsMetadataAccumulator.cpp +++ b/Software/components/core/src/RdsMetadataAccumulator.cpp @@ -35,12 +35,15 @@ void RdsMetadataAccumulator::applyGroup(std::uint16_t blockA, static_cast((blockB >> 12U) & 0x0FU); if (groupType == 0U) { - const std::size_t index = - static_cast((blockB >> 1U) & 0x03U); + // Group type 0 (both 0A and 0B): the two Program Service name + // characters are always in Block D. Block C differs by version -- + // 0A carries alternate-frequency codes, 0B repeats the PI code -- + // but never PS text either way (ETSI EN 62106 §3.1.5). + const std::size_t index = static_cast(blockB & 0x03U); if (index < kPsSegments) { psBuffer_[index * 2U] = - static_cast((blockC >> 8U) & 0xFFU); - psBuffer_[index * 2U + 1U] = static_cast(blockC & 0xFFU); + static_cast((blockD >> 8U) & 0xFFU); + psBuffer_[index * 2U + 1U] = static_cast(blockD & 0xFFU); psSegmentValid_[index] = true; } return; diff --git a/Software/components/core/test/CMakeLists.txt b/Software/components/core/test/CMakeLists.txt index b9dbbc6..785f35f 100644 --- a/Software/components/core/test/CMakeLists.txt +++ b/Software/components/core/test/CMakeLists.txt @@ -129,6 +129,10 @@ add_executable(bt1035_at_test bt1035_at_test.cpp) target_link_libraries(bt1035_at_test PRIVATE digiradio_core) add_test(NAME bt1035_at_test COMMAND bt1035_at_test) +add_executable(rds_metadata_accumulator_test rds_metadata_accumulator_test.cpp) +target_link_libraries(rds_metadata_accumulator_test PRIVATE digiradio_core) +add_test(NAME rds_metadata_accumulator_test COMMAND rds_metadata_accumulator_test) + add_executable(broadcast_metadata_test broadcast_metadata_test.cpp) target_link_libraries(broadcast_metadata_test PRIVATE digiradio_core) add_test(NAME broadcast_metadata_test COMMAND broadcast_metadata_test) diff --git a/Software/components/core/test/broadcast_metadata_test.cpp b/Software/components/core/test/broadcast_metadata_test.cpp index fca2b6e..6be4c9d 100644 --- a/Software/components/core/test/broadcast_metadata_test.cpp +++ b/Software/components/core/test/broadcast_metadata_test.cpp @@ -34,11 +34,13 @@ namespace { [[nodiscard]] int runRdsProgramNameTest() { + // Group type 0, segment address in blockB bits[1:0] (no shift); PS + // characters are in blockD, never blockC (ETSI EN 62106 §3.1.5). core::RdsMetadataAccumulator acc; - acc.applyGroup(0U, 0x0000U, 0x5445U, 0U); - acc.applyGroup(0U, 0x0002U, 0x5354U, 0U); - acc.applyGroup(0U, 0x0004U, 0x2020U, 0U); - acc.applyGroup(0U, 0x0006U, 0x2020U, 0U); + acc.applyGroup(0U, 0x0000U, 0U, 0x5445U); + acc.applyGroup(0U, 0x0001U, 0U, 0x5354U); + acc.applyGroup(0U, 0x0002U, 0U, 0x2020U); + acc.applyGroup(0U, 0x0003U, 0U, 0x2020U); const auto name = acc.programName(); if (!name || name->value() != "TEST") { diff --git a/Software/components/core/test/rds_metadata_accumulator_test.cpp b/Software/components/core/test/rds_metadata_accumulator_test.cpp new file mode 100644 index 0000000..6542c00 --- /dev/null +++ b/Software/components/core/test/rds_metadata_accumulator_test.cpp @@ -0,0 +1,126 @@ +/** + * @file rds_metadata_accumulator_test.cpp + * @brief Host tests for RdsMetadataAccumulator against ETSI EN 62106 + * group 0B/2A block layouts. + * + * DigiRadio firmware — https://github.com/manvalan/DigiRadio + * + * Copyright 2026 Michele Bigi + * SPDX-License-Identifier: Apache-2.0 + * + * @author Michele Bigi + * @date 2026-08-25 + */ + +#include "core/RdsMetadataAccumulator.hpp" + +#include +#include + +namespace { + +/* + * Group type 0B (block B bit 11 set), segment address in bits[1:0]: the two + * Program Service characters for that segment live in Block D (high byte + * first), never Block C -- Block C for group 0 carries alternate-frequency + * codes (0A) or a repeated PI code (0B), not text. This is the exact bug + * fixed 2026-08-25 (characters were being read from Block C, which made + * every FM station name silently fail to accumulate). + */ +[[nodiscard]] int runProgramServiceNameTest() +{ + core::RdsMetadataAccumulator acc; + + // "RADIO101" across the 4 PS segments, group type 0B. + struct + { + std::uint16_t segment; + char c0; + char c1; + } segments[] = { + {0U, 'R', 'A'}, + {1U, 'D', 'I'}, + {2U, 'O', '1'}, + {3U, '0', '1'}, + }; + + for (const auto &seg : segments) { + const std::uint16_t blockB = + static_cast(0x0800U | seg.segment); + const std::uint16_t blockD = static_cast( + (static_cast(seg.c0) << 8) | + static_cast(seg.c1)); + // Block C deliberately holds garbage (a repeated-PI-like value that + // is NOT the expected text) to prove the decoder ignores it for + // group 0, rather than happening to read the right bytes by luck. + acc.applyGroup(0x1234U, blockB, 0xBEEFU, blockD); + } + + const auto name = acc.programName(); + if (!name) { + std::cerr << "expected a program name after 4 PS segments\n"; + return EXIT_FAILURE; + } + if (name->value() != "RADIO101") { + std::cerr << "program name mismatch: got '" << name->value() + << "'\n"; + return EXIT_FAILURE; + } + return EXIT_SUCCESS; +} + +[[nodiscard]] int runIncompleteSegmentsTest() +{ + core::RdsMetadataAccumulator acc; + acc.applyGroup(0x1234U, 0x0800U, 0xBEEFU, 0x5241U); // segment 0 only + if (acc.programName()) { + std::cerr << "program name should be absent with 3 segments " + "missing\n"; + return EXIT_FAILURE; + } + return EXIT_SUCCESS; +} + +/* + * Group type 2A: RadioText characters split across Block C (2 chars) and + * Block D (2 chars) for the same segment -- unlike group 0, Block C really + * does carry text here, so this is a regression guard against ever + * "fixing" group 2 the same way group 0 needed fixing. + */ +[[nodiscard]] int runRadiotextTest() +{ + core::RdsMetadataAccumulator acc; + const std::uint16_t blockB = 0x2000U; // group type 2, version A, seg 0 + const std::uint16_t blockC = + (static_cast('T') << 8) | static_cast('e'); + const std::uint16_t blockD = + (static_cast('s') << 8) | static_cast('t'); + acc.applyGroup(0x1234U, blockB, blockC, blockD); + + const auto rt = acc.radiotext(); + if (!rt) { + std::cerr << "expected radiotext after one 2A segment\n"; + return EXIT_FAILURE; + } + if (rt->value() != "Test") { + std::cerr << "radiotext mismatch: got '" << rt->value() << "'\n"; + return EXIT_FAILURE; + } + return EXIT_SUCCESS; +} + +} // namespace + +int main() +{ + if (runProgramServiceNameTest() != EXIT_SUCCESS) { + return EXIT_FAILURE; + } + if (runIncompleteSegmentsTest() != EXIT_SUCCESS) { + return EXIT_FAILURE; + } + if (runRadiotextTest() != EXIT_SUCCESS) { + return EXIT_FAILURE; + } + return EXIT_SUCCESS; +} diff --git a/Software/components/drivers/si4684/src/Si4684Driver.cpp b/Software/components/drivers/si4684/src/Si4684Driver.cpp index 5934402..4ff7390 100644 --- a/Software/components/drivers/si4684/src/Si4684Driver.cpp +++ b/Software/components/drivers/si4684/src/Si4684Driver.cpp @@ -66,6 +66,11 @@ constexpr std::uint16_t kSi4684I2sOutEnable = 0x8002U; /** Si4684 volume: 0=mute, 63=max (AN649 AUDIO_ANALOG_VOLUME). */ constexpr std::uint8_t kSi4684VolumeMax = 63U; constexpr std::uint16_t kPropFmRdsConfig = 0x3C02U; +/** AN649 §0x3C01 FM_RDS_INTERRUPT_FIFO_COUNT: DEPTH[7:0], groups needed + * before RDSFIFOINT (FM_RDS_STATUS RESP4 bit0, our readFmRds() "received" + * bit) ever sets. Default 0 disables it permanently regardless of + * FM_RDS_CONFIG -- must be nonzero for any RDS group to ever be seen. */ +constexpr std::uint16_t kPropFmRdsInterruptFifoCount = 0x3C01U; /** AN649 FM_AUDIO_DE_EMPHASIS (0x3900): 0=75us/US (chip default), 1=50us/ * Europe, 2=disabled. FM seek band/spacing above is already the European * 87.5-107.9 MHz/100 kHz plan, so the chip must not stay on its 75us/US @@ -557,9 +562,22 @@ std::expected Si4684Driver::configureAfterBoot( return set; } } - if (auto rds = setProperty(kPropFmRdsConfig, 0x0001U); !rds) { + // AN649 §0x3C02 FM_RDS_CONFIG: BLETHB[7:6]/BLETHCD[5:4] block-error + // thresholds, RDSEN[0]. 0x0001 (thresholds at 0, "no block errors") + // rejected almost every real-world group -- any bit error at all + // (routine with multipath/noise, even on a strong signal) dropped + // the group from the FIFO, so accumulated station names/RadioText + // never completed. 0x00A1 keeps both thresholds at the datasheet's + // most tolerant recommended setting (2 = "3-5 bit errors detected + // and corrected"), still discarding uncorrectable (3) groups. + if (auto rds = setProperty(kPropFmRdsConfig, 0x00A1U); !rds) { return rds; } + if (auto rdsFifo = + setProperty(kPropFmRdsInterruptFifoCount, 0x0001U); + !rdsFifo) { + return rdsFifo; + } if (auto deEmph = setProperty(kPropFmAudioDeEmphasis, kFmAudioDeEmphasisEurope); !deEmph) {