Fix Si4684 pitch distortion: uncalibrated crystal (CTUN/XTAL_FREQ)

Root cause of FM/DAB "stonata" audio: xtalCtun=31/nominal XTAL_FREQ
defaults were never measured against this board's actual crystal
(Abracon ABM8-19.200MHZ-10-1-U-T, CL=10pF, two external 15pF load
caps). Fixed via CTUN trim by ear (31->0) plus a new live calibration
loop that reads the Si4684's own FM_RSQ FREQOFF field (broadcast
carriers are GPS-locked, so it's a free precision frequency reference)
to trim XTAL_FREQ with no lab equipment. Converged to CTUN=0,
XTAL_FREQ=19199750 Hz, residual -3/-4 ppm cross-checked on two
stations.

New tools/infrastructure (kept, not one-off):
- Si4684Driver::recalibrateXtal() + POST /api/tuner/xtal-calibrate:
  live crystal re-trim without an ESP32 reflash.
- FM_RSQ FREQOFF exposed as "freqoff_ppm" in GET /api/tuner/status.
- tools/si4684_xtal_calibration.py: automates the trim loop.
- tools/si4684_antenna_calibration.py: AN851 Appendix A ANTCAP/VARM/VARB
  sweep tool (same session, separate calibration).
- CONFIG_ESP32_I2S_TEST_TONE (off by default): isolates Si4684-specific
  audio issues from shared-downstream ones by writing a tone directly
  over the I2S bus the Si4684 also uses.
- SIGMA_WRITE_REGISTER_BLOCK/sigma_safeload_block now retry on NACK and
  verify via read-back instead of firing I2C writes blind.
- FM de-emphasis set to European 50us (was left at the US 75us default).
- DAB_ACF_ENABLE restored to its previous 0x0000 with a citation
  explaining why (tested the datasheet default of 0x0003, made things
  audibly worse).

See docs/si4684-rf-investigation-report.md's 2026-08-23 entry for the
full elimination chain and docs/TODO.md for follow-up work (persisting
calibration results to EEPROM instead of requiring a firmware edit).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-23 20:30:42 +02:00
co-authored by Claude Sonnet 5
parent a91c63fc31
commit fae2305164
24 changed files with 1309 additions and 29 deletions
@@ -68,6 +68,7 @@ namespace adau1701
{
const unsigned char deviceAddr =
static_cast<unsigned char>(pins_.i2cAddr7 << 1);
std::size_t index = 0U;
for (const core::RegisterWrite &write : program.writes())
{
const auto data = write.data();
@@ -75,12 +76,35 @@ namespace adau1701
{
return std::unexpected(Adau1701Error::DownloadFailed);
}
SIGMA_WRITE_REGISTER_BLOCK(
deviceAddr,
write.address(),
static_cast<unsigned int>(data.size()),
const_cast<ADI_REG_TYPE *>(
reinterpret_cast<const ADI_REG_TYPE *>(data.data())));
auto *bytes = const_cast<ADI_REG_TYPE *>(
reinterpret_cast<const ADI_REG_TYPE *>(data.data()));
const auto length = static_cast<unsigned int>(data.size());
if (SIGMA_WRITE_REGISTER_BLOCK(deviceAddr, write.address(), length,
bytes) != 0)
{
ESP_LOGE(kTag,
"program write #%u failed (addr=0x%04X len=%u) "
"after retries",
static_cast<unsigned>(index),
static_cast<unsigned>(write.address()),
static_cast<unsigned>(length));
return std::unexpected(Adau1701Error::DownloadFailed);
}
// Diagnostic only: an ACKed write that still reads back wrong
// would otherwise be invisible (see the SIGMA_WRITE_REGISTER_BLOCK
// comment in SigmaStudioFW.c). Logged, not fatal -- some
// addresses in this range may be self-clearing/status bits
// that legitimately don't read back what was written.
if (sigma_verify_block(write.address(), bytes, length) != 0)
{
ESP_LOGW(kTag,
"program write #%u read-back mismatch (addr=0x%04X "
"len=%u) -- ACKed but did not land as written",
static_cast<unsigned>(index),
static_cast<unsigned>(write.address()),
static_cast<unsigned>(length));
}
++index;
}
return {};
}
@@ -158,19 +182,69 @@ namespace adau1701
return replay;
}
// Diagnostic: log EQ band 0's live Param RAM contents. This band is
// the fixed high-pass (SigmaStudio band 1) and applyEq() always
// skips it -- no runtime code path ever safeloads it, so whatever
// landed here at program-load time is what plays, permanently,
// regardless of the audio-profile API (which reports a fictional
// value for it). Read straight from the chip rather than trusting
// the compiled source, in case the two have ever diverged.
{
const unsigned baseAddr = paramAddrEqBandBase(0U);
for (unsigned i = 0U; i < 5U; ++i)
{
unsigned char raw[4U] = {0U, 0U, 0U, 0U};
if (sigma_i2c_read(baseAddr + i, raw, sizeof(raw)) == 0)
{
const std::int32_t fixpoint =
static_cast<std::int32_t>(
(static_cast<std::uint32_t>(raw[0]) << 24) |
(static_cast<std::uint32_t>(raw[1]) << 16) |
(static_cast<std::uint32_t>(raw[2]) << 8) |
static_cast<std::uint32_t>(raw[3]));
ESP_LOGI(kTag,
"EQ band0 param[%u] addr=0x%04X raw=0x%08X "
"value=%f",
i, baseAddr + i,
static_cast<unsigned>(fixpoint),
static_cast<double>(fixpoint) /
static_cast<double>(1U << 23));
}
else
{
ESP_LOGW(kTag, "EQ band0 param[%u] read-back failed", i);
}
}
}
// SerialInputRegister (0x081F) override: bit3 IBP=1, matching the
// BCLK edge the Si4684's I2S output actually changes data on
// (compiled DSP program default is 0x00 = IBP=0, which produced
// pure static on a strong locked signal). ILP=1 was also tried
// (0x18) and made it worse (pure white noise again) — IBP alone
// (0x08) is the correct override, confirmed live: real, recognizable
// music instead of static/noise on a locked FM station.
// music instead of static/noise on a locked FM station. Redundant
// with the R3_HWCONFIGURATION program write above (SigmaStudio's
// own export already places 0x08 at this address) — kept as a
// belt-and-braces re-assert in case that specific chunk silently
// failed; now checked/verified like everything else instead of
// fire-and-forget.
{
const unsigned char deviceAddr =
static_cast<unsigned char>(pins_.i2cAddr7 << 1);
ADI_REG_TYPE serialInFix = 0x08U;
SIGMA_WRITE_REGISTER_BLOCK(deviceAddr, 0x081FU, 1U,
&serialInFix);
if (SIGMA_WRITE_REGISTER_BLOCK(deviceAddr, 0x081FU, 1U,
&serialInFix) != 0)
{
ESP_LOGE(kTag, "SerialInputRegister override failed after retries");
return std::unexpected(Adau1701Error::DownloadFailed);
}
if (sigma_verify_block(0x081FU, &serialInFix, 1U) != 0)
{
ESP_LOGW(kTag,
"SerialInputRegister read-back mismatch -- ACKed "
"but did not land as 0x08");
}
}
booted_ = true;
@@ -14,12 +14,14 @@
#include "SigmaStudioFW.h"
#include "driver/i2c_master.h"
#include "esp_log.h"
#include "freertos/FreeRTOS.h"
#include "freertos/semphr.h"
#include "freertos/task.h"
#include <string.h>
static const char* kTag = "SigmaStudioFW";
static i2c_master_dev_handle_t s_dev = NULL;
static SemaphoreHandle_t s_lock = NULL;
@@ -73,14 +75,45 @@ static unsigned int sigmaWordSize(unsigned int address)
return 4U;
}
void SIGMA_WRITE_REGISTER_BLOCK(unsigned char devAddress,
unsigned int address,
unsigned int length,
ADI_REG_TYPE* pData)
/*
* The boot-time program/param replay was previously fire-and-forget: the
* i2c_master_transmit result was discarded outright, so a single transient
* NACK anywhere in the hundreds of chunked writes that make up a DSP
* program load silently left that one chunk (a gain, a filter, a mux)
* at its power-on-reset RAM contents instead of the SigmaStudio-designed
* value -- indistinguishable at the time from a correct load, but capable
* of producing exactly a persistent, localized audio artifact rather than
* gross silence. Give it the same retry-on-NACK reliability the runtime
* safeload path already got in sigma_i2c_write (see its comment) and make
* failure observable instead of silent.
*/
static int sigmaTransmitChunk(unsigned int addr, const ADI_REG_TYPE* payload,
unsigned int chunkBytes)
{
unsigned char buf[2U + 64U];
buf[0] = (unsigned char)((addr >> 8) & 0xFFU);
buf[1] = (unsigned char)(addr & 0xFFU);
memcpy(buf + 2U, payload, chunkBytes);
static const int kMaxAttempts = 3;
for (int attempt = 0; attempt < kMaxAttempts; ++attempt) {
if (i2c_master_transmit(s_dev, buf, (size_t)(2U + chunkBytes), 1000) ==
ESP_OK) {
return 0;
}
vTaskDelay(pdMS_TO_TICKS(2));
}
return -1;
}
int SIGMA_WRITE_REGISTER_BLOCK(unsigned char devAddress,
unsigned int address,
unsigned int length,
ADI_REG_TYPE* pData)
{
(void)devAddress;
if (s_dev == NULL || pData == NULL || length == 0U) {
return;
return -1;
}
enum { kChunkBytesMax = 64U };
@@ -96,15 +129,48 @@ void SIGMA_WRITE_REGISTER_BLOCK(unsigned char devAddress,
const unsigned int chunk =
remaining > chunkBytes ? chunkBytes : remaining;
const unsigned int words = chunk / wordSize;
unsigned char buf[2U + kChunkBytesMax];
buf[0] = (unsigned char)((addr >> 8) & 0xFFU);
buf[1] = (unsigned char)(addr & 0xFFU);
memcpy(buf + 2U, cursor, chunk);
i2c_master_transmit(s_dev, buf, (size_t)(2U + chunk), 1000);
if (sigmaTransmitChunk(addr, cursor, chunk) != 0) {
return -1;
}
addr += words;
cursor += chunk;
remaining -= chunk;
}
return 0;
}
int sigma_verify_block(unsigned int address, const ADI_REG_TYPE* expected,
unsigned int length)
{
if (s_dev == NULL || expected == NULL || length == 0U) {
return -1;
}
enum { kChunkBytesMax = 64U };
const unsigned int wordSize = sigmaWordSize(address);
const unsigned int wordsPerChunk = kChunkBytesMax / wordSize;
const unsigned int chunkBytes = wordsPerChunk * wordSize;
unsigned int addr = address;
unsigned int remaining = length;
const ADI_REG_TYPE* cursor = expected;
while (remaining > 0U) {
const unsigned int chunk =
remaining > chunkBytes ? chunkBytes : remaining;
const unsigned int words = chunk / wordSize;
unsigned char readBack[kChunkBytesMax];
if (sigma_i2c_read(addr, readBack, chunk) != 0) {
return -1;
}
if (memcmp(readBack, cursor, chunk) != 0) {
return -1;
}
addr += words;
cursor += chunk;
remaining -= chunk;
}
return 0;
}
int sigma_i2c_read(unsigned int reg, unsigned char* data, unsigned int length)
@@ -198,6 +264,37 @@ int sigma_safeload_param(unsigned int paramAddr, int fixpoint)
return sigma_safeload_block(1U, addrs, values);
}
/*
* Diagnostic only, mirrors the boot-replay read-back in
* SIGMA_WRITE_REGISTER_BLOCK: a safeload can ACK every transaction and
* still not land as intended (address/data written to the wrong Param RAM
* slot, a stale value left from a previous session's committed-but-later-
* garbled write, etc). This is the runtime path applied on every EQ/gain
* change and on the stored-profile replay at boot -- unlike the one-time
* program load, it was previously unverified. Log-only: some callers pass
* addresses this can't independently confirm are readable Param RAM (vs.
* a write-only control register), so a mismatch here is a strong signal,
* not a hard boot/apply-time failure.
*/
static void sigmaVerifyParam(unsigned int paramAddr, int fixpoint)
{
unsigned char readBack[4U];
if (sigma_i2c_read(paramAddr, readBack, sizeof(readBack)) != 0) {
ESP_LOGW(kTag, "safeload verify: read-back failed for param 0x%04X",
paramAddr);
return;
}
const int actual = (int)(((unsigned int)readBack[0] << 24) |
((unsigned int)readBack[1] << 16) |
((unsigned int)readBack[2] << 8) |
(unsigned int)readBack[3]);
if (actual != fixpoint) {
ESP_LOGW(kTag,
"safeload verify: param 0x%04X mismatch, wrote %d read %d",
paramAddr, fixpoint, actual);
}
}
int sigma_safeload_block(unsigned char count, const unsigned int* paramAddrs,
const int* fixpoints)
{
@@ -218,7 +315,14 @@ int sigma_safeload_block(unsigned char count, const unsigned int* paramAddrs,
}
}
return sigma_trigger_safeload();
if (sigma_trigger_safeload() != 0) {
return -1;
}
for (unsigned char i = 0U; i < count; ++i) {
sigmaVerifyParam(paramAddrs[i], fixpoints[i]);
}
return 0;
}
int sigma_safeload_raw_block(unsigned char count, const unsigned int* paramAddrs,
@@ -112,6 +112,16 @@ public:
* verified live (72 = 720 uA startup bias).
* @param xtalCtun POWER_UP ARG8 CTUN, 0-63 (AN649 §Command 0x01);
* default matches the values already verified live.
* @param xtalFreqHz POWER_UP ARG4-7 XTAL_FREQ in Hz (AN649 §Command
* 0x01); default 19,200,000 (nominal crystal
* frequency). Deliberately overriding this away
* from the crystal's true nominal value is a valid
* software calibration trick: it tells the chip's
* internal PLL math "the crystal actually runs at
* this rate," compensating any real physical
* offset (from load-cap mismatch etc) without
* touching CTUN. See FM_RSQ_STATUS FREQOFF for the
* measurement this is meant to null out.
* @return Ok on success, or Si4684Error.
* @pubstate writes booted_ and loadedBand_ on success.
*
@@ -120,7 +130,28 @@ public:
*/
[[nodiscard]] std::expected<void, Si4684Error> boot(
Si4684Band band, std::uint8_t xtalIbias = 72U,
std::uint8_t xtalCtun = 31U);
std::uint8_t xtalCtun = 31U, std::uint32_t xtalFreqHz = 19200000U);
/**
* @brief recalibrateXtal — re-run boot() with new crystal parameters.
*
* @dname recalibrateXtal
* @param xtalIbias New POWER_UP ARG3 IBIAS.
* @param xtalCtun New POWER_UP ARG8 CTUN.
* @param xtalFreqHz New POWER_UP ARG4-7 XTAL_FREQ in Hz.
* @return Ok on success, or Si4684Error::NotBooted if never booted.
* @pubstate forces booted_=false then re-runs boot() for the currently
* loaded band -- full RSTB# pulse + patch/image reload, same
* as a cold boot, just without an ESP32 restart. Diagnostic:
* lets a calibration script iterate crystal parameters live
* over HTTP instead of a firmware rebuild+reflash per value.
*
* @author Michele Bigi
* @date 2026-08-23
*/
[[nodiscard]] std::expected<void, Si4684Error> recalibrateXtal(
std::uint8_t xtalIbias, std::uint8_t xtalCtun,
std::uint32_t xtalFreqHz);
/**
* @brief isBooted — query whether boot completed successfully.
@@ -178,6 +178,24 @@ public:
[[nodiscard]] std::expected<void, core::TunerError> setVolume(
std::uint8_t level) override;
/**
* @brief recalibrateXtal — re-run driver boot() with new crystal
* parameters, without an ESP32 restart.
*
* @dname recalibrateXtal
* @param ibias New POWER_UP ARG3 IBIAS.
* @param ctun New POWER_UP ARG8 CTUN.
* @param xtalFreqHz New POWER_UP ARG4-7 XTAL_FREQ in Hz.
* @return Ok on success, or a mapped TunerError.
* @pubstate delegates to driver_.recalibrateXtal(); caller must re-tune
* afterwards, this only reboots the chip.
*
* @author Michele Bigi
* @date 2026-08-23
*/
[[nodiscard]] std::expected<void, core::TunerError> recalibrateXtal(
std::uint8_t ibias, std::uint8_t ctun, std::uint32_t xtalFreqHz);
private:
/**
* @brief mapError — translate Si4684Error to core::TunerError.
@@ -98,6 +98,12 @@ struct Si4684FmRsq {
std::int8_t snrDb; ///< SNR in dB.
bool valid; ///< RSQ valid flag from the chip.
bool stereo; ///< Stereo pilot detected.
/** AN649 FM_RSQ_STATUS RESP8 FREQOFF: signed offset in units of 2 PPM
* (range -128..127, i.e. -256..+254 PPM). Crystal calibration signal:
* every locked station's carrier reads the same PPM error when the
* XTAL_FREQ/CTUN reference is off, since real broadcast transmitters
* are themselves GPS/rubidium-locked. */
std::int8_t freqOffBppm;
};
/**
@@ -40,6 +40,8 @@ constexpr std::size_t kSpiReplyLeadIn = 1U;
/** FM_RSQ_STATUS field indices with kSpiReplyLeadIn (AN649 RESP510). */
constexpr std::size_t kFmRsqOffValid = 6U;
constexpr std::size_t kFmRsqOffReadFreq = 7U;
/** AN649 FM_RSQ_STATUS RESP8 FREQOFF: signed offset in 2 PPM units. */
constexpr std::size_t kFmRsqOffFreqOff = 9U;
constexpr std::size_t kFmRsqOffRssi = 10U;
constexpr std::size_t kFmRsqOffSnr = 11U;
@@ -64,6 +66,12 @@ 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 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
* default -- that under-de-emphasizes treble on every station. */
constexpr std::uint16_t kPropFmAudioDeEmphasis = 0x3900U;
constexpr std::uint16_t kFmAudioDeEmphasisEurope = 0x0001U;
/** AN649 FM valid tune properties (defaults RSSI 17 dBµV, SNR 10 dB). */
constexpr std::uint16_t kPropFmValidRssiThreshold = 0x3202U;
constexpr std::uint16_t kPropFmValidSnrThreshold = 0x3204U;
@@ -486,6 +494,16 @@ std::expected<void, Si4684Error> Si4684Driver::configureAfterBoot(
{0xB302U, 0x0000U},
{0xB303U, 0x0000U},
{0xB401U, 0x0002U},
// AN649 Property 0xB500 DAB_ACF_ENABLE: bit0 SOFTMUTE_ENABLE,
// bit1 COMF_NOISE_ENABLE, datasheet default 0x0003 (both on).
// Tried 0x0003 on 2026-08-23 hoping it would mask the periodic
// hiss/unintelligible-voice symptom on real DAB reception --
// live A/B test made it worse (COMF_NOISE injects synthetic
// noise on every brief signal-quality dip, and frequent
// softmute engagement chopped up speech). PE5PVB's independent
// SI4684-DAB-Receiver project also explicitly disables this
// (Set_Property(0xB500, 0x0000)), matching what real testing
// shows here -- disabled deliberately, not an oversight.
{0xB500U, 0x0000U},
};
for (const auto& prop : kDabProps) {
@@ -542,6 +560,11 @@ std::expected<void, Si4684Error> Si4684Driver::configureAfterBoot(
if (auto rds = setProperty(kPropFmRdsConfig, 0x0001U); !rds) {
return rds;
}
if (auto deEmph = setProperty(kPropFmAudioDeEmphasis,
kFmAudioDeEmphasisEurope);
!deEmph) {
return deEmph;
}
// AN649 §0x3202/0x3204: lower seek/tune validity for weak lab antennas.
if (auto rssi = setProperty(kPropFmValidRssiThreshold,
kFmValidRssiThresholdDbuV);
@@ -597,7 +620,8 @@ std::expected<void, Si4684Error> Si4684Driver::configureAfterBoot(
}
std::expected<void, Si4684Error> Si4684Driver::boot(
Si4684Band band, std::uint8_t xtalIbias, std::uint8_t xtalCtun)
Si4684Band band, std::uint8_t xtalIbias, std::uint8_t xtalCtun,
std::uint32_t xtalFreqHz)
{
if (booted_ && loadedBand_ == band) {
return {};
@@ -672,10 +696,19 @@ std::expected<void, Si4684Error> Si4684Driver::boot(
}
// ARG2=0x17(CLK_MODE=crystal,TR_SIZE), ARG3=IBIAS, ARG4-7=XTAL_FREQ
// 19.2 MHz (0x0124F800), ARG8=CTUN, ARG9=0x10 (fixed bit4=1 per AN649
// (little-endian, nominal 19.2 MHz = 0x0124F800; may be intentionally
// offset from nominal as a software crystal calibration -- see the
// xtalFreqHz doc comment), ARG8=CTUN, ARG9=0x10 (fixed bit4=1 per AN649
// §Command 0x01), ARG10-15=0 (AN649 POWER_UP argument table).
std::uint8_t powerUp[] = {
0x17, xtalIbias, 0x00, 0xf8, 0x24, 0x01, xtalCtun, 0x10,
0x17,
xtalIbias,
static_cast<std::uint8_t>(xtalFreqHz & 0xFFU),
static_cast<std::uint8_t>((xtalFreqHz >> 8) & 0xFFU),
static_cast<std::uint8_t>((xtalFreqHz >> 16) & 0xFFU),
static_cast<std::uint8_t>((xtalFreqHz >> 24) & 0xFFU),
xtalCtun,
0x10,
0x00, 0x00, 0x00, 0x18, 0x00, 0x00,
};
if (auto pu = writeCommand(Command::PowerUp, powerUp, sizeof(powerUp));
@@ -745,6 +778,17 @@ std::expected<void, Si4684Error> Si4684Driver::boot(
return {};
}
std::expected<void, Si4684Error> Si4684Driver::recalibrateXtal(
std::uint8_t xtalIbias, std::uint8_t xtalCtun, std::uint32_t xtalFreqHz)
{
if (auto ready = ensureBooted(); !ready) {
return ready;
}
const Si4684Band band = loadedBand_;
booted_ = false;
return boot(band, xtalIbias, xtalCtun, xtalFreqHz);
}
bool Si4684Driver::isBooted() const noexcept
{
return booted_;
@@ -920,6 +964,7 @@ std::expected<Si4684FmRsq, Si4684Error> Si4684Driver::readFmRsq()
static_cast<std::int8_t>(raw[kFmRsqOffSnr]),
freqInBand && chipValid,
false,
static_cast<std::int8_t>(raw[kFmRsqOffFreqOff]),
};
return rsq;
}
@@ -169,6 +169,7 @@ std::expected<core::TunerStatus, core::TunerError> Si4684Tuner::readStatus()
status.locked = rsq->valid;
status.fmRssiDbuV = rsq->rssiDbuV;
status.fmSnrDb = rsq->snrDb;
status.fmFreqOffBppm = rsq->freqOffBppm;
status.fmStereo = rsq->stereo;
status.fmChipReadFrequency = rsq->frequency;
// Keep commanded frequency when chip READFREQ is stale (stuck at band
@@ -342,4 +343,14 @@ std::expected<void, core::TunerError> Si4684Tuner::setVolume(std::uint8_t level)
return {};
}
std::expected<void, core::TunerError> Si4684Tuner::recalibrateXtal(
std::uint8_t ibias, std::uint8_t ctun, std::uint32_t xtalFreqHz)
{
if (auto result = driver_.recalibrateXtal(ibias, ctun, xtalFreqHz);
!result) {
return std::unexpected(mapError(result.error()));
}
return {};
}
} // namespace si4684