Fix FM RDS: station name/RadioText never decoded, three stacked bugs

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 <noreply@anthropic.com>
2026-08-25 23:58:37 +02:00
parent 5d7d4919a0
commit d587032a0a
6 changed files with 168 additions and 13 deletions
@@ -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
@@ -35,12 +35,15 @@ void RdsMetadataAccumulator::applyGroup(std::uint16_t blockA,
        static_cast<std::uint8_t>((blockB >> 12U) & 0x0FU);

    if (groupType == 0U) {
-        const std::size_t index =
-            static_cast<std::size_t>((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<std::size_t>(blockB & 0x03U);
        if (index < kPsSegments) {
            psBuffer_[index * 2U] =
-                static_cast<char>((blockC >> 8U) & 0xFFU);
-            psBuffer_[index * 2U + 1U] = static_cast<char>(blockC & 0xFFU);
+                static_cast<char>((blockD >> 8U) & 0xFFU);
+            psBuffer_[index * 2U + 1U] = static_cast<char>(blockD & 0xFFU);
            psSegmentValid_[index] = true;
        }
        return;
@@ -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)
@@ -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") {
@@ -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 <cstdlib>
+#include <iostream>
+
+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<std::uint16_t>(0x0800U | seg.segment);
+        const std::uint16_t blockD = static_cast<std::uint16_t>(
+            (static_cast<std::uint16_t>(seg.c0) << 8) |
+            static_cast<std::uint16_t>(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<std::uint16_t>('T') << 8) | static_cast<std::uint16_t>('e');
+    const std::uint16_t blockD =
+        (static_cast<std::uint16_t>('s') << 8) | static_cast<std::uint16_t>('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;
+}
@@ -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<void, Si4684Error> 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) {