Skip to content

Saturate EMA sum to PROD_W range to prevent arithmetic overflow on sustained inputs#2

Open
Abraxas3d wants to merge 1 commit into
mainfrom
fix/sum-saturation
Open

Saturate EMA sum to PROD_W range to prevent arithmetic overflow on sustained inputs#2
Abraxas3d wants to merge 1 commit into
mainfrom
fix/sum-saturation

Conversation

@Abraxas3d
Copy link
Copy Markdown
Member

@Abraxas3d Abraxas3d commented May 17, 2026

Summary

The combinational sum assignment in lowpass_ema.vhd sums two shift_left'd signed terms without saturation. When sustained high-amplitude inputs drive the accumulator past +/- (2^(PROD_W-1) - 1), the result wraps. Once wrapped, the cascade output flips sign and the EMA settles into a stable wrong-sign equilibrium with no path to recovery. The slow time constant that gives the EMA its smoothing power also locks it into the wrong half-plane.

This PR adds an EXTRA_W = 4 wider intermediate signal (sum_wide) for the combinational add, then clamps to +/- (2^(PROD_W-1) - 1) before assigning to sum. Behavior is bit-exact identical for in-range values. It clamps gracefully at the limits.

Background

We're integrating lowpass_ema as the inner stage of a cascaded power-detection EMA in the 64-channel polyphase channelizer for Haifuraiya. It will also be used for SIC-MDT, AMSAT-UK's FunCube+ mission.

Our typical config (PD per channel): ALPHA_W=18, PROD_W=43, SUM_SHIFT_W=18, MULT_DATA_SHIFT=0, MULT_SUM_SHIFT=1, with two EMAs in cascade. The outer (slow) EMA uses alpha2 = 2^-12 for ~4096-frame time constant on power readings.

Note: This PR is independent of and complementary to #1 (data_ena gating), which open sepparately. The two fixes address unrelated concerns and should not mess each other up.

The bug, demonstrated

During a testbench run with sustained DC input applied to a channel, we observed channel 0's reported power register reading back as 1,785,562,731 (unsigned). Reinterpreted as signed 32-bit: -2,509,404,565 this is super clearly wrong for a power magnitude.

We probed u_ema_2/sum (43-bit signed) on the affected channel at 10 µs intervals across the 1 ms test:

time     sum_MSB    sum (binary, MSB-leading)
10us     0          0000000000000000000000000000000000000000000   (initialized)
...
260us    0          0111000101011110111100000110100010100110100   (climbing, near +max)
270us    0          0111100110111110100100100101000010010100000   (≈ +4.14×10¹²)
280us    1          1000001001000100010110011001000100111100110   ← WRAP to negative
290us    1          1000101101101111110101011111010100010101010   (now negative)
...
410us    1          1111110101000111001000011110000010111011000   (climbing back)
420us    0          0000011110111111000100101101011000101001100   ← crossed zero
...
550us    0          0111110011110111001101110100101100011001010   (≈ +4.4×10¹², near max)
560us    1          1000010111100110111011001010110000111010110   ← second WRAP
...
1000us   1          1101010011001010010011110110110010000100010   (ended negative)

The accumulator was actively oscillating through wraparound every ~280 µs under sustained DC. The test sampled sum at a moment when MSB was 1, which is why downstream code read back a negative-interpreted-as-large-unsigned value.

Root cause: shift_left(mult_sum, MULT_SUM_SHIFT) plus shift_left(mult_data, MULT_DATA_SHIFT) exceeds the 43-bit signed range when both summerands are near their respective maxima. With sustained high-amplitude integration, this happens repeatedly. This sustained high-amplitude integration is a harsh use case, but it can happen.

The fix

src/lowpass_ema.vhd adds saturation in the combinational sum computation:

-- Architecture declarations
CONSTANT EXTRA_W   : NATURAL := 4;  -- guard bits: max shift_left + 1 for the add
CONSTANT SAT_W     : NATURAL := PROD_W + EXTRA_W;
CONSTANT SAT_MAX_V : signed(SAT_W -1 DOWNTO 0) := to_signed(2**(PROD_W-1) - 1, SAT_W);
CONSTANT SAT_MIN_V : signed(SAT_W -1 DOWNTO 0) := to_signed(-(2**(PROD_W-1)),  SAT_W);
SIGNAL sum_wide    : signed(SAT_W -1 DOWNTO 0);

-- In statement region (replaces the original sum <= ... assignment)
sum_wide <= shift_left(resize(mult_data, SAT_W), MULT_DATA_SHIFT) +
            shift_left(resize(mult_sum,  SAT_W), MULT_SUM_SHIFT);

sum <= to_signed(2**(PROD_W-1) - 1, PROD_W) WHEN sum_wide > SAT_MAX_V ELSE
       to_signed(-(2**(PROD_W-1)),  PROD_W) WHEN sum_wide < SAT_MIN_V ELSE
       resize(sum_wide, PROD_W);

Synthesis cost: one comparison and a 2:1 mux per sum bit. Negligible on Zynq UltraScale+ or iCE40UP5K (our two current targets).

Verification

Same testbench, same input pattern, post-fix probe of sum MSB:

10us – 1000us: MSB = 0 throughout the ENTIRE simulation

No wraps. No sign flips. The accumulator clamps cleanly at the upper limit when input pushes it past +(2^(PROD_W-1) - 1), and the EMA continues to behave as a low-pass filter at the saturation boundary.

End-to-end result: 9/9 testbench assertions pass across the full 64-channel polyphase channelizer with 64 instances of this lowpass_ema module in production cascade configuration.

Notes for Matthew et al

  • EXTRA_W = 4 is chosen conservatively to absorb any reasonable combination of MULT_DATA_SHIFT and MULT_SUM_SHIFT (max 3 bits each in our config) plus the +1 bit for the addition. Maybe this should be parameterized as well?? Or leave it up to implementers to compute it from the existing shift parameters? Not sure what to do about this.

  • The original sum <= ... line is preserved as a comment immediately above the new code so we can see exactly what's being replaced without flipping between commits. It can be removed if this all looks reasonable and it's too much clutter.

  • I have not changed any timing or interface sum keeps the same type, width, and combinational nature.

The combinational sum assignment summed two shift_left'd signed terms
without saturation, which could wrap past +/- 2^(PROD_W-1) when
sustained high-amplitude inputs drove the accumulator past the signed
PROD_W-bit range. Once wrapped, the cascade output flipped negative and
the EMA settled into a stable wrong-sign equilibrium with no recovery.

Symptom in our use case (polyphase channelizer power detector, AMSAT-UK
FunCube+ MDT-SIC): channel-0 EMA-2 sum oscillated between near +2^42
and near -2^42 every ~280us under sustained DC input, ending tests in
a negative-pinned state that contaminated subsequent measurements.

Fix: compute the sum in EXTRA_W=4 wider intermediate (sum_wide),
then clamp to +/- (2^(PROD_W-1) - 1) before assigning to sum.
Preserves bit-exact behavior for in-range values; clamps gracefully
at the limits.

Verified: full 1ms simulation across 64 EMA cascades shows MSB stays 0
throughout, no wrap events, all 9 testbench assertions pass.
@Abraxas3d Abraxas3d self-assigned this May 17, 2026
@Abraxas3d Abraxas3d added bug Something isn't working enhancement New feature or request labels May 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant