From f5fe92f93e9217ed7fa5c61e23aec61a53494f84 Mon Sep 17 00:00:00 2001 From: Michele Bigi Date: Sun, 16 Aug 2026 00:33:46 +0200 Subject: [PATCH] Fix remaining Si4684 ARG-offset bugs: seek, DAB services, RSQ/RDS/DIGRAD acks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit writeCommand() had no way to send a non-zero ARG1, since it always hardcoded ARG1=0x00 before the caller's payload. Every command whose real ARG1 needed to carry a flag (STCACK, INTACK, SERTYPE, DIGRAD/EVENT ack) or whose payload needed to start at ARG1 instead of ARG2 was silently broken: - seekFm(): SEEKUP/WRAP never reached the chip (always ARG2=0x00), so hardware seek always searched down/no-wrap; masked by the existing 100 kHz software-step fallback in Si4684Tuner. - startDabService()/stopDabService(): SERVICE_ID/COMPONENT_ID shifted one byte right of their real ARG4-11 positions, with SERTYPE landing where the spec requires a fixed 0x00. - readDabServiceData(): same shift, plus STATUS_ONLY was bit3 (0x08) instead of the correct bit4 (0x10). - clearFmStc(), readFmRsq(), readFmRds(), fetchDabServiceList(), readDabDigRadStatus(), readDabEventStatus(): these AN649 commands have only ARG1 and no ARG2 at all, so the old two-argument writeCommand() could never carry their ack/status flags — clearFmStc()'s STCACK never fired in this driver's history (masked by FM_TUNE_FREQ/FM_SEEK_START auto-clearing STC per their own spec). writeCommand() gains a fourth parameter, arg1 (default 0x00, preserving every already-correct call site); each caller above now passes its flag through arg1 instead of the payload array. Confirmed live: first locked:true and first genuine hardware seek (not software-fallback) in this driver's history — 87.5 -> 98.3 MHz, RSSI +12 dBuV, SNR +14 dB. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0178rASQ6ZETPMUamvpoR2KR --- .../si4684/include/si4684/Si4684Driver.hpp | 3 +- .../drivers/si4684/src/Si4684Driver.cpp | 72 ++++++++++-------- .../docs/si4684-rf-investigation-report.md | 74 +++++++++++++++++-- 3 files changed, 114 insertions(+), 35 deletions(-) diff --git a/Software/components/drivers/si4684/include/si4684/Si4684Driver.hpp b/Software/components/drivers/si4684/include/si4684/Si4684Driver.hpp index 7f6dce1..7966702 100644 --- a/Software/components/drivers/si4684/include/si4684/Si4684Driver.hpp +++ b/Software/components/drivers/si4684/include/si4684/Si4684Driver.hpp @@ -408,7 +408,8 @@ private: [[nodiscard]] std::expected readRaw( std::span buffer); [[nodiscard]] std::expected writeCommand( - Command cmd, const std::uint8_t* payload, std::size_t length); + Command cmd, const std::uint8_t* payload, std::size_t length, + std::uint8_t arg1 = 0x00U); [[nodiscard]] std::expected hostLoadBlob( const core::IFirmwareBlobReader& blob, std::size_t chunkPayload); [[nodiscard]] std::expected configureAfterBoot( diff --git a/Software/components/drivers/si4684/src/Si4684Driver.cpp b/Software/components/drivers/si4684/src/Si4684Driver.cpp index 70c1ca3..de6c5dc 100644 --- a/Software/components/drivers/si4684/src/Si4684Driver.cpp +++ b/Software/components/drivers/si4684/src/Si4684Driver.cpp @@ -247,9 +247,11 @@ std::expected Si4684Driver::clearFmStc() if (auto band = ensureBand(Si4684Band::Fm); !band) { return band; } - // AN649 FM_RSQ_STATUS ARG1 STCACK=1 clears a latched STCINT. - const std::uint8_t args[] = {0x01U}; - if (auto cmd = writeCommand(Command::FmRsqStatus, args, sizeof(args)); + // AN649 Command 0x32 FM_RSQ_STATUS has a single argument, ARG1, whose + // bit 0 is STCACK (clears a latched STCINT). No ARG2+ exists for this + // command. + constexpr std::uint8_t kStcAck = 0x01U; + if (auto cmd = writeCommand(Command::FmRsqStatus, nullptr, 0U, kStcAck); !cmd) { return cmd; } @@ -295,14 +297,15 @@ std::expected Si4684Driver::readRaw( } std::expected Si4684Driver::writeCommand( - Command cmd, const std::uint8_t* payload, std::size_t length) + Command cmd, const std::uint8_t* payload, std::size_t length, + std::uint8_t arg1) { if (length + 2U > kSpiBufferSize) { return std::unexpected(Si4684Error::CommandFailed); } std::array buffer = {}; buffer[0] = static_cast(cmd); - buffer[1] = 0x00U; + buffer[1] = arg1; if (payload != nullptr && length > 0U) { std::memcpy(buffer.data() + 2U, payload, length); } @@ -805,12 +808,14 @@ std::expected Si4684Driver::seekFm( } const bool seekUp = direction == core::SeekDirection::Up; const bool wrapBand = wrap == SeekBandWrap::Wrap; - // AN649 FM_SEEK_START: ARG1=tune/injection, ARG2=SEEKUP|WRAP. + // AN649 Command 0x31 FM_SEEK_START: ARG1=tune_mode/injection (default + // 0x00 here), ARG2=SEEKUP|WRAP, ARG3=0x00 fixed, ARG4=ANTCAP[7:0], + // ARG5=ANTCAP[15:8]. writeCommand() supplies ARG1, so this array starts + // at ARG2. const std::uint8_t seekFlags = static_cast(((seekUp ? 1U : 0U) << 1U) | (wrapBand ? 1U : 0U)); const std::uint8_t args[] = { - 0x00U, seekFlags, 0x00U, 0x00U, @@ -851,9 +856,9 @@ std::expected Si4684Driver::readFmRsq() if (auto band = ensureBand(Si4684Band::Fm); !band) { return std::unexpected(band.error()); } - const std::uint8_t args[] = {0x00U}; - if (auto cmd = writeCommand(Command::FmRsqStatus, args, sizeof(args)); - !cmd) { + // AN649 Command 0x32 FM_RSQ_STATUS has a single argument, ARG1 (all + // ack/cancel bits 0 here — a plain status read). No ARG2+ exists. + if (auto cmd = writeCommand(Command::FmRsqStatus, nullptr, 0U); !cmd) { return std::unexpected(Si4684Error::CommandFailed); } std::array raw = {}; @@ -904,8 +909,10 @@ std::expected Si4684Driver::readFmRds() if (auto band = ensureBand(Si4684Band::Fm); !band) { return std::unexpected(band.error()); } - const std::uint8_t args[] = {0x01U}; - if (auto cmd = writeCommand(Command::FmRdsStatus, args, sizeof(args)); + // AN649 Command 0x34 FM_RDS_STATUS has a single argument, ARG1: bit0 + // INTACK (clear RDSINT). No ARG2+ exists. + constexpr std::uint8_t kIntAck = 0x01U; + if (auto cmd = writeCommand(Command::FmRdsStatus, nullptr, 0U, kIntAck); !cmd) { return std::unexpected(Si4684Error::CommandFailed); } @@ -931,12 +938,13 @@ Si4684Driver::readDabServiceData(bool statusOnly, bool ack) return std::unexpected(band.error()); } + // AN649 Command 0x84 GET_DIGITAL_SERVICE_DATA has a single argument, + // ARG1: bit4 STATUS_ONLY, bit0 ACK. No ARG2+ exists. const std::uint8_t arg1 = - static_cast((statusOnly ? 0x08U : 0x00U) + static_cast((statusOnly ? 0x10U : 0x00U) | (ack ? 0x01U : 0x00U)); - const std::uint8_t args[] = {arg1}; if (auto cmd = - writeCommand(Command::GetDigitalServiceData, args, sizeof(args)); + writeCommand(Command::GetDigitalServiceData, nullptr, 0U, arg1); !cmd) { return std::unexpected(Si4684Error::CommandFailed); } @@ -1029,9 +1037,11 @@ Si4684Driver::readDabDigRadStatus() if (auto band = ensureBand(Si4684Band::Dab); !band) { return std::unexpected(band.error()); } - const std::uint8_t args[] = {0x01U}; + // AN649 Command 0xB2 DAB_DIGRAD_STATUS has a single argument, ARG1: + // bit0 STC_ACK (clears the STC interrupt). No ARG2+ exists. + constexpr std::uint8_t kStcAck = 0x01U; if (auto cmd = - writeCommand(Command::DabDigRadStatus, args, sizeof(args)); + writeCommand(Command::DabDigRadStatus, nullptr, 0U, kStcAck); !cmd) { return std::unexpected(Si4684Error::CommandFailed); } @@ -1054,9 +1064,9 @@ Si4684Driver::readDabEventStatus() if (auto band = ensureBand(Si4684Band::Dab); !band) { return std::unexpected(band.error()); } - const std::uint8_t args[] = {0x00U}; - if (auto cmd = - writeCommand(Command::DabGetEventStatus, args, sizeof(args)); + // AN649 Command 0xB3 DAB_GET_EVENT_STATUS has a single argument, + // ARG1=EVENT_ACK (0 here — plain status read). No ARG2+ exists. + if (auto cmd = writeCommand(Command::DabGetEventStatus, nullptr, 0U); !cmd) { return std::unexpected(Si4684Error::CommandFailed); } @@ -1078,9 +1088,9 @@ Si4684Driver::fetchDabServiceList() return std::unexpected(band.error()); } - const std::uint8_t args[] = {0x00U}; - if (auto cmd = writeCommand(Command::GetDigitalServiceList, args, - sizeof(args)); + // AN649 Command 0x80 GET_DIGITAL_SERVICE_LIST has a single argument, + // ARG1=SERTYPE (0 = complete DAB/DMB service list). No ARG2+ exists. + if (auto cmd = writeCommand(Command::GetDigitalServiceList, nullptr, 0U); !cmd) { return std::unexpected(Si4684Error::CommandFailed); } @@ -1139,8 +1149,10 @@ std::expected Si4684Driver::startDabService( if (auto band = ensureBand(Si4684Band::Dab); !band) { return band; } + // AN649 Command 0x81 START_DIGITAL_SERVICE: ARG1=SERTYPE, ARG2-3=0x00 + // fixed, ARG4-7=SERVICE_ID (LE32), ARG8-11=COMP_ID (LE32). writeCommand() + // supplies ARG1 (=type), so this array starts at ARG2. const std::uint8_t args[] = { - static_cast(type), 0x00U, 0x00U, static_cast(serviceId & 0xFFU), @@ -1152,8 +1164,8 @@ std::expected Si4684Driver::startDabService( static_cast((componentId >> 16) & 0xFFU), static_cast(componentId >> 24), }; - if (auto cmd = - writeCommand(Command::StartDigitalService, args, sizeof(args)); + if (auto cmd = writeCommand(Command::StartDigitalService, args, + sizeof(args), static_cast(type)); !cmd) { return std::unexpected(Si4684Error::CommandFailed); } @@ -1168,8 +1180,10 @@ std::expected Si4684Driver::stopDabService( if (auto band = ensureBand(Si4684Band::Dab); !band) { return band; } + // AN649 Command 0x82 STOP_DIGITAL_SERVICE: same layout as + // START_DIGITAL_SERVICE (ARG1=SERTYPE, ARG2-3=0x00 fixed, ARG4-7= + // SERVICE_ID LE32, ARG8-11=COMP_ID LE32). const std::uint8_t args[] = { - static_cast(type), 0x00U, 0x00U, static_cast(serviceId & 0xFFU), @@ -1181,8 +1195,8 @@ std::expected Si4684Driver::stopDabService( static_cast((componentId >> 16) & 0xFFU), static_cast(componentId >> 24), }; - if (auto cmd = - writeCommand(Command::StopDigitalService, args, sizeof(args)); + if (auto cmd = writeCommand(Command::StopDigitalService, args, + sizeof(args), static_cast(type)); !cmd) { return std::unexpected(Si4684Error::CommandFailed); } diff --git a/Software/docs/si4684-rf-investigation-report.md b/Software/docs/si4684-rf-investigation-report.md index 0bbcf0b..b86a91c 100644 --- a/Software/docs/si4684-rf-investigation-report.md +++ b/Software/docs/si4684-rf-investigation-report.md @@ -421,11 +421,75 @@ comments. The lesson: `writeCommand()`'s implicit ARG1 prepend is an easy trap for future commands — any new caller must remember its array starts at ARG2, not ARG1. -**Follow-up**: get a proper antenna connected and confirm an actual station -lock (`valid=1`) on both FM and DAB; audit other `writeCommand()` call sites -in `Si4684Driver.cpp` for the same off-by-one pattern (POWER_UP and -HOST_LOAD were checked and are correct; FM_SEEK, property writes, and RSQ/ -DIGRAD status reads have not yet been re-verified against AN649 page text). +**Follow-up, completed same session**: audited every remaining +`writeCommand()` call site in `Si4684Driver.cpp` against the AN649 page text +(not driver comments) and found the identical bug pattern repeated in +several more places — `writeCommand()`'s implicit `ARG1=0x00` prepend was +either swallowing a real ARG1 value the caller needed, or shifting a whole +multi-byte struct one slot right: + +- **`seekFm()` (FM_SEEK_START, 0x31)**: `SEEKUP`/`WRAP` (real ARG2) were + never sent — the chip always saw `ARG2=0x00`, so hardware seek always + searched down with no wrap regardless of what was requested. This is why + every seek in this report's earlier captures fell through to the + `hitech95`-inspired 100 kHz software-step fallback in `Si4684Tuner` + instead of using the chip's real seek. +- **`startDabService()`/`stopDabService()` (0x81/0x82)**: `SERVICE_ID` and + `COMPONENT_ID` (8 bytes, real ARG4-11) were shifted one byte right into + ARG5-12, with `SERTYPE` landing in ARG2 (spec: fixed `0x00`) instead of + ARG1. Playing a specific DAB service would have started the wrong + service/component or failed outright — not yet observed in practice only + because tuning itself never worked before this session. +- **`readDabServiceData()` (GET_DIGITAL_SERVICE_DATA, 0x84)**: same + ARG1-only shift as below, plus the `STATUS_ONLY` bit was coded as `0x08` + (bit 3) instead of the correct `0x10` (bit 4) per the AN649 bit table. +- **`clearFmStc()`, `readFmRsq()`, `readFmRds()`, `fetchDabServiceList()`, + `readDabDigRadStatus()`, `readDabEventStatus()`**: all six commands + (FM_RSQ_STATUS 0x32, FM_RDS_STATUS 0x34, GET_DIGITAL_SERVICE_LIST 0x80, + DAB_DIGRAD_STATUS 0xB2, DAB_GET_EVENT_STATUS 0xB3) have **only ARG1** in + the AN649 spec — no ARG2 exists at all. Passing anything through the old + `writeCommand(cmd, payload, length)` two-argument form for these could + only ever send a spurious extra byte while the intended ARG1 flag (STCACK, + INTACK, SERTYPE, DIGRAD ack, EVENT_ACK) silently landed nowhere, since + `writeCommand()` had no way to set ARG1 to anything but a hardcoded + `0x00`. `clearFmStc()`'s STCACK never fired in this driver's entire + history — masked because `FM_TUNE_FREQ`/`FM_SEEK_START` already + auto-clear STC per their own AN649 documentation. + +Fixed by giving `writeCommand()` a fourth parameter, `std::uint8_t arg1 = +0x00U` (default preserves every already-correct call site), and updating +each caller above to either pass its flag byte through `arg1` with no +payload (for the ARG1-only commands) or drop the erroneous leading array +element (for the multi-arg commands whose ARG1 is legitimately always +`0x00`, e.g. `seekFm`'s default tune mode). + +**Confirmed live after this round of fixes** — first `locked: true` and +first hardware (non-software-fallback) seek in this entire investigation: + +``` +POST /api/tuner/tune {"band":"fm","frequency_khz":87500} +POST /api/tuner/seek {"direction":"up"} +-> {"frequency_khz":98300} +GET /api/tuner/status +-> {"locked":true,"fm":{"frequency_khz":98300,"rssi_dbuv":12,"snr_db":14,"stereo":false}} +``` + +The seek jumped directly from 87.5 to 98.3 MHz in one hardware search (not +100 kHz software steps), landing on a real, locked station at a plausible +RSSI/SNR. `bt1035` also came back to `true` in `/api/health` during this +same session (cause not yet diagnosed — see the BT1035 section below; +unrelated to this fix, separate chip). + +DAB was swept across 7 frequency-table indices (5, 10, 15, 20, 25, 30, 35) +after the fix — `fic_quality`/`cnr_db` stayed at 0 on all of them, no lock +yet. The DAB_TUNE_FREQ/DAB service-list/service-start fixes are verified +correct against the AN649 spec text the same way the FM fix was, but do not +yet have an empirical lock to point to, unlike FM. Not treated as a red +flag — no DAB antenna tuning has been attempted yet, and the default +European frequency table may not match active local multiplexes at these +particular indices. **Next step**: sweep the full DAB frequency table (not +just 7 samples) with a real antenna and confirm a lock the same way FM was +confirmed. **Unrelated finding from the same session, logged for completeness**: BT1035 began failing boot deterministically (`no spontaneous UART bytes after