Skip to content

Fix AGC attack path to never increase gain on saturation (#145)#154

Open
lekkalaharsha wants to merge 12 commits into
NawfalMotii79:developfrom
lekkalaharsha:main
Open

Fix AGC attack path to never increase gain on saturation (#145)#154
lekkalaharsha wants to merge 12 commits into
NawfalMotii79:developfrom
lekkalaharsha:main

Conversation

@lekkalaharsha

@lekkalaharsha lekkalaharsha commented May 29, 2026

Copy link
Copy Markdown

Summary

  • The `else` branch in the AGC attack guard (`agc_base_gain = min_gain`) could increase gain when `min_gain > agc_base_gain`, driving the LNA into a higher-gain state during a saturation event — unsafe hardware behavior.
  • Replaced with a saturating subtract: the attack path does nothing when `agc_base_gain` is already at or below the floor, guaranteeing gain never increases on saturation.
  • Added `configure()` with `min_gain <= max_gain` validation and automatic `agc_base_gain` clamping to the new range.

Closes #145.

Changes

  • `ADAR1000_AGC.cpp` — rewrite attack path with saturating subtract; add `configure()` implementation
  • `ADAR1000_AGC.h` — declare `configure()`
  • `tests/test_agc_outer_loop.cpp` — two new regression tests (`test_saturation_never_increases_gain`, `test_configure_validation`); all 15/15 pass

Test plan

  • `cd 9_Firmware/9_1_Microcontroller/tests && make clean && make` — all 15 AGC tests pass
  • `cd 9_Firmware/9_2_FPGA && bash run_regression.sh` — no FPGA regressions
  • `uv run ruff check .` — no lint errors

NawfalMotii79 and others added 10 commits April 29, 2026 21:53
Added GitHub badges for stars, forks, watchers, license, and hardware.
…79#145)

The else branch of the attack guard could set agc_base_gain = min_gain
even when min_gain > agc_base_gain, raising LNA gain during a saturation
event. Replace with a saturating subtract that does nothing when already
at or below the floor. Add configure() with min/max validation and
agc_base_gain clamping. Two regression tests added (15/15 pass).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…le (NawfalMotii79#145)

With holdoff_frames == 0 the recovery condition (holdoff_counter >= holdoff_frames)
is permanently true, bypassing the intended throttle and causing gain-up on every
non-saturated frame. configure() now rejects zero with DIAG_WARN and leaves all
fields unchanged. Regression case added to test_configure_validation.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lekkalaharsha

Copy link
Copy Markdown
Author

Hi @soufianebouaddis, applied your holdoff_frames == 0 feedback — commit c5cb5a5.

What changed:

  • configure() now rejects holdoff_frames == 0 and returns false with no fields written — same pattern as the existing min_gain > max_gain guard
  • Added DIAG_WARN on both rejection paths so invalid runtime configs leave a trace even if the caller ignores the return value
  • Added regression case in test_configure_validation: configure(..., 0) == false and verifies holdoff_frames is left unchanged
  • Updated the configure() doc comment in the header to document the new constraint

All 15/15 tests pass.

NawfalMotii79#145)

Three additional validation guards in configure() identified by @soufianebouaddis:

- gain_step_down == 0: silently disables AGC attack (gain never decreases on
  saturation). Now rejected with DIAG_WARN and false return.
- gain_step_up == 0: silently disables AGC recovery (gain never increases after
  saturation clears, AGC locks at floor permanently). Now rejected.
- max_gain > 127: exceeds the 7-bit ADAR1000 VGA register range. Now rejected.

All three guards follow the existing atomic-reject pattern: no fields are
written on a rejected call, so partial configuration is impossible.

Three regression tests added (tests 16/17/18 in test_agc_outer_loop.cpp):
- test_configure_rejects_zero_step_down
- test_configure_rejects_zero_step_up
- test_configure_rejects_max_gain_out_of_range (boundary: 127 accepted, 128+ rejected)

All 18/18 tests pass.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lekkalaharsha

Copy link
Copy Markdown
Author

Hi @soufianebouaddis, applied all three guards — commit f05ab2c.

Changes:

  • gain_step_down == 0 → rejected; attack path becomes permanently disabled (gain never decreases on saturation)
  • gain_step_up == 0 → rejected; recovery path becomes permanently disabled (AGC locks at floor after first saturation)
  • max_gain > 127 → rejected; exceeds the 7-bit ADAR1000 VGA register range

All three follow the existing atomic-reject pattern — no fields are written on a rejected call, so partial configuration is impossible regardless of call order.

Three regression tests added (test_configure_rejects_zero_step_down, test_configure_rejects_zero_step_up, test_configure_rejects_max_gain_out_of_range). The boundary test verifies max_gain == 127 is accepted and max_gain == 128+ is rejected.

18/18 tests pass. PR #154 is ready for re-review.

@lekkalaharsha

Copy link
Copy Markdown
Author

@NawfalMotii79@soufianebouaddis has approved this PR. All five configure() guards are verified, implementation and docstring agree, and the atomic-reject pattern is intact across every path.

Ready to merge. Please squash merge into develop — the current PR title and description are suitable as the final commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants