Repository navigation
Conversation
- Use stack buffers instead of new[]/delete in set_pll, set_ms and set_vcxo (drops malloc/free/operator new from the link) - Merge select_r_div and select_r_div_ms67 into one helper parameterized by minimum frequency - Share p1/p2/p3 register packing (pack_reg_set) and fraction-to-register math (frac_to_reg) - Compute CLK0-5 register addresses arithmetically in set_ms/ms_div; CLK6/7 stay special-cased and out-of-range clocks still write nothing - Collapse the duplicated CLK6/CLK7 peer-clock branch in set_freq - Loop the per-clock setup in reset() Public API, RFRAC_DENOM and SI5351_FREQ_MULT are unchanged. Host simulation of 1,060 call sequences showed identical I2C transcripts, register state and return codes vs master. ATmega328P (Uno, -Os -flto) flash savings measured at about 2.0-3.1 KB depending on the sketch. Not yet verified on hardware.
When a CLK0-CLK5 Multisynth is switched from DIVBY4 mode (outputs above 150 MHz) to a normal divider without a subsequent PLL reset, the output stays silent. set_freq() only resets the PLL on its >100 MHz path, so e.g. 80 MHz -> 156 MHz -> 80 MHz on the same clock, 155 MHz -> 96 MHz, or reset() after a DIVBY4 frequency followed by set_freq(512 kHz) all leave the clock without output. reset() does not clear the MSx_DIVBY4 bits in reg 44/52/..., so the stale state survives it. set_ms() already reads the register holding MSx_DIVBY4 for its read-modify-write, so use that value to detect the DIVBY4 -> normal transition and reset the PLL assigned to that clock once, after all Multisynth registers are written. This covers set_freq() (both paths), set_freq_manual() and direct set_ms() calls. No reset is added in any other case, so ordinary tuning remains glitch-free. Public API is unchanged. Fixes #65
The >100 MHz path of set_freq() moves the PLL to a frequency chosen for that output (e.g. 4 x 155 MHz = 620 MHz) and leaves it there. A later <=100 MHz set_freq() on a clock using that PLL then reuses the low PLL, which can need a fractional Multisynth ratio below 8, outside the valid range (8 + 1/1048575 .. 2048). On hardware, 155 MHz -> 96 MHz on CLK0 (ratio 620/96 = 6.458) produced an unstable output near 88.5 MHz. In the <=100 MHz path, when the required ratio would be below 8 and no other output on that PLL is above 100 MHz, set the PLL back to the fixed 800 MHz and recalculate every output on it, then reset the PLL, reusing the existing >100 MHz re-plan code. Ordinary tuning with the PLL at 800 MHz never takes this branch, so no extra resets are added there. If another output holds the PLL above 100 MHz the behaviour is unchanged. Refs #65
When set_freq() re-plans a PLL (the >100 MHz path or the ratio<8 re-plan) it rewrites every Multisynth on that PLL and then resets the PLL. If one of those Multisynths was leaving DIVBY4 mode, set_ms() also issued its own reset in the middle of the rewrites, so the PLL was reset twice, the first time before all MS registers were written. Defer set_ms()'s DIVBY4-exit reset while set_freq() is re-planning (file-static flag) so only the final reset, after all PLL and MS writes, is issued. Direct set_ms(), set_freq_manual() and the normal <=100 MHz path are unchanged. Refs #65
If a <=100 MHz set_freq() would need a Multisynth ratio below 8 because another output above 100 MHz holds the PLL (issue #65: 155 MHz on CLK0, then 99 MHz on CLK2 gives MS2 = 620/99 = 6.26), the PLL cannot be re-planned without breaking the other output. In that case move the output to the other PLL with set_ms_source(): - if the other PLL already gives a ratio >= 8, use it unchanged (no PLL write, so outputs already on it are not disturbed and no reset is issued, except the usual one when leaving DIVBY4); - otherwise, if no other output (CLK0-CLK7) uses it, re-plan it at 800 MHz, write the MS and reset it once after both; - otherwise (both PLLs constrained) keep the previous behaviour. set_freq_manual() is unchanged: the caller chose the PLL there. CLK6/CLK7 handling is unchanged. Steady-state tuning adds no resets. Refs #65
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR does two things in
src/si5351.cppandsrc/si5351.h:The public API does not change. No signatures, enums, structs or defines in the
public:part ofsi5351.hchanged. The only header change is private: two privateselect_r_divhelpers became one. Frequencies are still in Hz × 100 andRFRAC_DENOMis unchanged.Fixes #65
Motivation
malloc/freeandoperator new/deletefor three 20-byte scratch buffers.reset()in between) was silent on hardware.set_freq()on that PLL then reused it as is, which can need a fractional MS ratio below 8. AN619 only allows 8 to 2048. On hardware, 155 → 96 MHz on CLK0 gave an unstable ~88.5 MHz instead of 96 MHz (620/96 = 6.46). The third scenario reported in pll not recalculated ( 150 MHz boundary ?? ) #65 is the same problem across two outputs: 155 MHz on CLK0, then 99 MHz on CLK2, so MS2 = 620/99 = 6.26.Changes
1. Flash-size refactor (51e6650): no behavior change
set_pll,set_ms,set_vcxonew uint8_t[20]/deletereplaced by an 8-byte stack array. This drops the heap from the library entirely and also fixes the olddeletevsdelete[]mismatch.select_r_div(private)select_r_div()andselect_r_div_ms67()merged into one function that takes the minimum frequency. Same band thresholds.pack_reg_setset_ms,ms_divbase + clk * 8instead of aswitchper clock. CLK6/7 are still handled separately (single-byte MS registers and shared R-divider register 92). An out-of-range clk still writes nothing, as before.set_freqpeerindex. The existing asymmetry is kept on purpose: CLK6's first set always programs PLLB, CLK7's usespll_assignment[clk].frac_to_regpll_calcandmultisynth_calc.reset()resetset_freqmultisynth_calccalls.Risk: low. The math, register values and I2C write order are unchanged. See Verification.
2. Reset the PLL when a Multisynth leaves DIVBY4 (d052ddd)
What:
set_ms()reads the MS parameter byte it already reads for the read-modify-write. If DIVBY4 was set and the new setting doesn't use it, it resets the PLL that the clock is actually on (pll_assignment[clk]) after the MS writes.Why: this is the #65 failure. Without a PLL reset after leaving DIVBY4, the Multisynth stays silent.
Coverage: this is in
set_ms(), so it covers every path:set_freq()below and above 100 MHzset_freq_manual()set_ms()callsreset()followed byset_freq(), sincereset()leaves the DIVBY4 bits setBehavior impact: at most one extra PLL reset, and only on that transition. Normal tuning never triggers it.
Risk: a reset glitches every output on that PLL. That only happens on a DIVBY4 exit, which already glitches that output.
3. Re-plan the PLL when a ≤100 MHz output would need an MS ratio below 8 (9d086d2)
What: in the ≤100 MHz path of
set_freq(), if the current PLL frequency is less than 8 × the output frequency, and no other output above 100 MHz holds that PLL, the PLL is re-planned to 800 MHz (SI5351_PLL_FIXED). Every output on it gets recalculated, and the PLL is reset after the writes. This is what the >100 MHz path already did.Why: fixes 155 → 96 MHz and similar cases where the PLL was left low by an earlier >100 MHz setting.
Behavior impact: when this triggers, other outputs on the same PLL get rewritten and glitch once, but stay at their frequencies. It never triggers during normal tuning, because the PLL is already at 800 MHz there.
Risk: low. It only triggers when the old code would have written an out-of-spec ratio.
4. Move a ≤100 MHz output to the other PLL when its own PLL is held (b8f7988)
What: when the case in change 3 applies but another output above 100 MHz holds the PLL (the #65 155 MHz CLK0 + 99 MHz CLK2 case),
set_freq()moves the output to the other PLL with the existingset_ms_source(). That updates bothpll_assignment[]and the MS_SRC bit. Then:Not changed:
set_freq_manual()(the caller picks the PLL there), the CLK6/CLK7 logic, and the PLL assignment rules for MS6/7.Behavior impact: after a move, the output stays on the other PLL until
reset()orset_ms_source(). Later tuning of that output on the new PLL is glitch-free. A later >100 MHz request on the moved output can now succeed where v2.2.0 returned 1, because its new PLL isn't held.Risk: see Known limitations. A moved output is now exposed to later direct changes of the other PLL.
5. Issue only one PLL reset per re-plan (34c486c)
What: when
set_freq()re-plans a PLL (the >100 MHz path, or changes 3 and 4) and one of the Multisynths it rewrites is leaving DIVBY4,set_ms()used to issue its own reset in the middle of the rewrites, followed by the normal reset at the end. A file-static flag now defers theset_ms()reset whileset_freq()is re-planning, so there is exactly one reset, after all PLL and MS writes.Behavior impact: in the test suite, 64 duplicate resets are gone. Every affected call still ends with a single reset as its last write. Direct
set_ms(),set_freq_manual()and the plain ≤100 MHz path are unchanged.Cost: 1 byte of RAM and about 18 bytes of flash.
Flash size
My own measurement on hardware: my test program went from 15464 to 13516 bytes with the refactor alone (51e6650).
arduino-cli,
arduino:avr:uno(ATmega328P), arduino:avr 1.8.8, default-Os -flto. "Sketch uses" bytes (.text + .data), delta vs v2.2.0, and static RAM (.data + .bss):The #65 fixes give back part of the refactor savings. Most of the cost is in
set_freq(): 64-bit compares and the extra paths are expensive on AVR, and the PLL move adds about 430–490 bytes.malloc/free/new/deleteare no longer linked from the library.Verification
Simulator equivalence. I built an out-of-tree host harness, not part of this PR:
set_freq_manual,set_correction,set_vcxo,reset/initResults:
set_freq_manualcalls with a low PLL, which are the caller's choiceHardware (Si5351A, my bench):
Si5351_Issue65_Verifysketch on 9d086d2: all steps passed, covering 80→156→80, 155→96, 200→10, CLK1 on PLLB 160→14, andset_freq_manual175→14 on CLK2 and CLK0, with steady tuning showing no resets.Known limitations / not addressed here
set_freq_manual()at a 600 MHz PLL. It isn't re-planned out from under that output.set_pll(),set_vcxo(), or the firstset_freq()on CLK6/CLK7, which programs PLLB. The library has never recalculated CLK0–5 outputs on a PLL changed that way. This was already true for anything on that PLL, but a move can now put an output there implicitly. The Si5351A 10-MSOP (CLK0–2 only) can't hit the CLK6/7 case.set_pll()/set_vcxo()but has no output tracked on it, and it's too low for the move, the move re-plans it to 800 MHz.set_freq_manual()is not re-planned or moved. A manual PLL that gives a ratio below 8 is still written as asked.set_freq_manual()on CLK6/CLK7 truncates P1 into the single-byte divider register.set_freq()on CLK6/CLK7 does not enable the output (reg 3). It needs an explicitoutput_enable().set_freq(0, clk)divides by zero.multisynth67_calc()rejects a ratio,set_freq()still writes an uninitialized byte to reg 90/91.pll_calc()correction formula left-shifts a negative value (UBSan). The result is as intended on GCC/AVR.