Fix remaining Si4684 ARG-offset bugs: seek, DAB services, RSQ/RDS/DIGRAD acks
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0178rASQ6ZETPMUamvpoR2KR
This commit is contained in:
Claude Sonnet 5
@@ -408,7 +408,8 @@ private:
|
||||
[[nodiscard]] std::expected<void, Si4684Error> readRaw(
|
||||
std::span<std::uint8_t> buffer);
|
||||
[[nodiscard]] std::expected<void, Si4684Error> 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<void, Si4684Error> hostLoadBlob(
|
||||
const core::IFirmwareBlobReader& blob, std::size_t chunkPayload);
|
||||
[[nodiscard]] std::expected<void, Si4684Error> configureAfterBoot(
|
||||
|
||||
@@ -247,9 +247,11 @@ std::expected<void, Si4684Error> 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<void, Si4684Error> Si4684Driver::readRaw(
|
||||
}
|
||||
|
||||
std::expected<void, Si4684Error> 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<std::uint8_t, kSpiBufferSize> buffer = {};
|
||||
buffer[0] = static_cast<std::uint8_t>(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<core::FrequencyKHz, Si4684Error> 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<std::uint8_t>(((seekUp ? 1U : 0U) << 1U)
|
||||
| (wrapBand ? 1U : 0U));
|
||||
const std::uint8_t args[] = {
|
||||
0x00U,
|
||||
seekFlags,
|
||||
0x00U,
|
||||
0x00U,
|
||||
@@ -851,9 +856,9 @@ std::expected<Si4684FmRsq, Si4684Error> 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<std::uint8_t, 23> raw = {};
|
||||
@@ -904,8 +909,10 @@ std::expected<Si4684FmRdsStatus, Si4684Error> 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<std::uint8_t>((statusOnly ? 0x08U : 0x00U)
|
||||
static_cast<std::uint8_t>((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<void, Si4684Error> 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<std::uint8_t>(type),
|
||||
0x00U,
|
||||
0x00U,
|
||||
static_cast<std::uint8_t>(serviceId & 0xFFU),
|
||||
@@ -1152,8 +1164,8 @@ std::expected<void, Si4684Error> Si4684Driver::startDabService(
|
||||
static_cast<std::uint8_t>((componentId >> 16) & 0xFFU),
|
||||
static_cast<std::uint8_t>(componentId >> 24),
|
||||
};
|
||||
if (auto cmd =
|
||||
writeCommand(Command::StartDigitalService, args, sizeof(args));
|
||||
if (auto cmd = writeCommand(Command::StartDigitalService, args,
|
||||
sizeof(args), static_cast<std::uint8_t>(type));
|
||||
!cmd) {
|
||||
return std::unexpected(Si4684Error::CommandFailed);
|
||||
}
|
||||
@@ -1168,8 +1180,10 @@ std::expected<void, Si4684Error> 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<std::uint8_t>(type),
|
||||
0x00U,
|
||||
0x00U,
|
||||
static_cast<std::uint8_t>(serviceId & 0xFFU),
|
||||
@@ -1181,8 +1195,8 @@ std::expected<void, Si4684Error> Si4684Driver::stopDabService(
|
||||
static_cast<std::uint8_t>((componentId >> 16) & 0xFFU),
|
||||
static_cast<std::uint8_t>(componentId >> 24),
|
||||
};
|
||||
if (auto cmd =
|
||||
writeCommand(Command::StopDigitalService, args, sizeof(args));
|
||||
if (auto cmd = writeCommand(Command::StopDigitalService, args,
|
||||
sizeof(args), static_cast<std::uint8_t>(type));
|
||||
!cmd) {
|
||||
return std::unexpected(Si4684Error::CommandFailed);
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user