Merge pull request #95 from joyshmitz/test/agc-debounce-enforce

test(cross-layer): enforce 2-frame DIG_6 debounce guard on outerAgc.enabled (follow-up to #93)

9_Firmware/9_1_Microcontroller/9_1_1_C_Cpp_Libraries/ADAR1000_AGC.cpp

@@ -18,7 +18,7 @@ ADAR1000_AGC::ADAR1000_AGC()
     , min_gain(0)
     , max_gain(127)
     , holdoff_frames(4)
-    , enabled(true)
+    , enabled(false)
     , holdoff_counter(0)
     , last_saturated(false)
     , saturation_event_count(0)

9_Firmware/9_1_Microcontroller/9_1_3_C_Cpp_Code/main.cpp

@@ -2180,9 +2180,24 @@ int main(void)
 
       runRadarPulseSequence();
 
-      /* [AGC] Outer-loop AGC: read FPGA saturation flag (DIG_5 / PD13),
-       * adjust ADAR1000 VGA common gain once per radar frame (~258 ms).
-       * Only run when AGC is enabled — otherwise leave VGA gains untouched. */
+      /* [AGC] Outer-loop AGC: sync enable from FPGA via DIG_6 (PD14),
+       * then read saturation flag (DIG_5 / PD13) and adjust ADAR1000 VGA
+       * common gain once per radar frame (~258 ms).
+       * FPGA register host_agc_enable is the single source of truth —
+       * DIG_6 propagates it to MCU every frame.
+       * 2-frame confirmation debounce: only change outerAgc.enabled when
+       * two consecutive frames read the same DIG_6 value. Prevents a
+       * single-sample glitch from causing a spurious AGC state transition.
+       * Added latency: 1 extra frame (~258 ms), acceptable for control plane. */
+      {
+          bool dig6_now = (HAL_GPIO_ReadPin(FPGA_DIG6_GPIO_Port,
+                                            FPGA_DIG6_Pin) == GPIO_PIN_SET);
+          static bool dig6_prev = false;  // matches boot default (AGC off)
+          if (dig6_now == dig6_prev) {
+              outerAgc.enabled = dig6_now;
+          }
+          dig6_prev = dig6_now;
+      }
       if (outerAgc.enabled) {
           bool sat = HAL_GPIO_ReadPin(FPGA_DIG5_SAT_GPIO_Port,
                                       FPGA_DIG5_SAT_Pin) == GPIO_PIN_SET;

9_Firmware/9_1_Microcontroller/tests/test_agc_outer_loop.cpp

@@ -50,7 +50,7 @@ static void test_defaults()
     assert(agc.min_gain == 0);
     assert(agc.max_gain == 127);
     assert(agc.holdoff_frames == 4);
-    assert(agc.enabled == true);
+    assert(agc.enabled == false);  // disabled by default — FPGA DIG_6 is source of truth
     assert(agc.holdoff_counter == 0);
     assert(agc.last_saturated == false);
     assert(agc.saturation_event_count == 0);
@@ -67,6 +67,7 @@ static void test_defaults()
 static void test_saturation_reduces_gain()
 {
     ADAR1000_AGC agc;
+    agc.enabled = true;  // default is OFF; enable for this test
     uint8_t initial = agc.agc_base_gain;  // 30
 
     agc.update(true);  // saturation
@@ -82,6 +83,7 @@ static void test_saturation_reduces_gain()
 static void test_holdoff_prevents_early_gain_up()
 {
     ADAR1000_AGC agc;
+    agc.enabled = true;  // default is OFF; enable for this test
     agc.update(true);  // saturate once -> gain = 26
     uint8_t after_sat = agc.agc_base_gain;
 
@@ -101,6 +103,7 @@ static void test_holdoff_prevents_early_gain_up()
 static void test_recovery_after_holdoff()
 {
     ADAR1000_AGC agc;
+    agc.enabled = true;  // default is OFF; enable for this test
     agc.update(true);  // saturate -> gain = 26
     uint8_t after_sat = agc.agc_base_gain;
 
@@ -119,6 +122,7 @@ static void test_recovery_after_holdoff()
 static void test_min_gain_clamp()
 {
     ADAR1000_AGC agc;
+    agc.enabled = true;  // default is OFF; enable for this test
     agc.min_gain = 10;
     agc.agc_base_gain = 12;
     agc.gain_step_down = 4;
@@ -136,6 +140,7 @@ static void test_min_gain_clamp()
 static void test_max_gain_clamp()
 {
     ADAR1000_AGC agc;
+    agc.enabled = true;  // default is OFF; enable for this test
     agc.max_gain = 32;
     agc.agc_base_gain = 31;
     agc.gain_step_up = 2;
@@ -226,6 +231,7 @@ static void test_apply_gain_spi()
 static void test_reset_preserves_config()
 {
     ADAR1000_AGC agc;
+    agc.enabled = true;  // default is OFF; enable for this test
     agc.agc_base_gain = 42;
     agc.gain_step_down = 8;
     agc.cal_offset[3] = -5;
@@ -255,6 +261,7 @@ static void test_reset_preserves_config()
 static void test_saturation_counter()
 {
     ADAR1000_AGC agc;
+    agc.enabled = true;  // default is OFF; enable for this test
 
     for (int i = 0; i < 10; ++i) {
         agc.update(true);
@@ -274,6 +281,7 @@ static void test_saturation_counter()
 static void test_mixed_sequence()
 {
     ADAR1000_AGC agc;
+    agc.enabled = true;  // default is OFF; enable for this test
     agc.agc_base_gain = 30;
     agc.gain_step_down = 4;
     agc.gain_step_up = 1;

9_Firmware/9_2_FPGA/constraints/xc7a50t_ftg256.xdc

@@ -224,7 +224,7 @@ set_property IOSTANDARD LVCMOS33 [get_ports {stm32_mixers_enable}]
 
 # DIG_5 = H11, DIG_6 = G12, DIG_7 = H12 — FPGA→STM32 status outputs
 # DIG_5: AGC saturation flag (PD13 on STM32)
-# DIG_6: reserved (PD14)
+# DIG_6: AGC enable flag (PD14) — mirrors FPGA host_agc_enable to STM32
 # DIG_7: reserved (PD15)
 set_property PACKAGE_PIN H11 [get_ports {gpio_dig5}]
 set_property PACKAGE_PIN G12 [get_ports {gpio_dig6}]

9_Firmware/9_2_FPGA/radar_system_top.v

@@ -130,7 +130,7 @@ module radar_system_top (
     // FPGA→STM32 GPIO outputs (DIG_5..DIG_7 on 50T board)
     // Used by STM32 outer AGC loop to read saturation state without USB polling.
     output wire gpio_dig5,          // DIG_5 (H11→PD13): AGC saturation flag (1=clipping detected)
-    output wire gpio_dig6,          // DIG_6 (G12→PD14): reserved (tied low)
+    output wire gpio_dig6,          // DIG_6 (G12→PD14): AGC enable flag (mirrors host_agc_enable)
     output wire gpio_dig7           // DIG_7 (H12→PD15): reserved (tied low)
 );
 
@@ -1037,9 +1037,11 @@ assign system_status = status_reg;
 // ============================================================================
 // DIG_5: AGC saturation flag — high when per-frame saturation_count > 0.
 //        STM32 reads PD13 to detect clipping and adjust ADAR1000 VGA gain.
-// DIG_6, DIG_7: Reserved (tied low for future use).
+// DIG_6: AGC enable flag — mirrors host_agc_enable so STM32 outer-loop AGC
+//        tracks the FPGA register as single source of truth.
+// DIG_7: Reserved (tied low for future use).
 assign gpio_dig5 = (rx_agc_saturation_count != 8'd0);
-assign gpio_dig6 = 1'b0;
+assign gpio_dig6 = host_agc_enable;
 assign gpio_dig7 = 1'b0;
 
 // ============================================================================

9_Firmware/tests/cross_layer/test_cross_layer_contract.py

@@ -27,6 +27,7 @@ layers agree (because both could be wrong).
 from __future__ import annotations
 
 import os
+import re
 import struct
 import subprocess
 import tempfile
@@ -369,6 +370,188 @@ class TestTier1ResetDefaults:
             )
 
 
+class TestTier1AgcCrossLayerInvariant:
+    """
+    Verify AGC enable/disable is consistent across FPGA, MCU, and GUI layers.
+
+    System-level invariant: the FPGA register host_agc_enable is the single
+    source of truth for AGC state. It propagates to MCU via DIG_6 GPIO and
+    to GUI via status word 4 bit[11]. At boot, all layers must agree AGC=OFF.
+    At runtime, the MCU must read DIG_6 every frame to sync its outer-loop AGC.
+    """
+
+    def test_fpga_dig6_drives_agc_enable(self):
+        """FPGA must drive gpio_dig6 from host_agc_enable, NOT tied low."""
+        rtl = (cp.FPGA_DIR / "radar_system_top.v").read_text()
+        # Must find: assign gpio_dig6 = host_agc_enable;
+        assert re.search(
+            r'assign\s+gpio_dig6\s*=\s*host_agc_enable\s*;', rtl
+        ), "gpio_dig6 must be driven by host_agc_enable (not tied low)"
+        # Must NOT have the old tied-low pattern
+        assert not re.search(
+            r"assign\s+gpio_dig6\s*=\s*1'b0\s*;", rtl
+        ), "gpio_dig6 must NOT be tied low — it carries AGC enable"
+
+    def test_fpga_agc_enable_boot_default_off(self):
+        """FPGA host_agc_enable must reset to 0 (AGC off at boot)."""
+        v_defaults = cp.parse_verilog_reset_defaults()
+        assert "host_agc_enable" in v_defaults, (
+            "host_agc_enable not found in reset block"
+        )
+        assert v_defaults["host_agc_enable"] == 0, (
+            f"host_agc_enable reset default is {v_defaults['host_agc_enable']}, "
+            "expected 0 (AGC off at boot)"
+        )
+
+    def test_mcu_agc_constructor_default_off(self):
+        """MCU ADAR1000_AGC constructor must default enabled=false."""
+        agc_cpp = (cp.MCU_LIB_DIR / "ADAR1000_AGC.cpp").read_text()
+        # The constructor initializer list must have enabled(false)
+        assert re.search(
+            r'enabled\s*\(\s*false\s*\)', agc_cpp
+        ), "ADAR1000_AGC constructor must initialize enabled(false)"
+        assert not re.search(
+            r'enabled\s*\(\s*true\s*\)', agc_cpp
+        ), "ADAR1000_AGC constructor must NOT initialize enabled(true)"
+
+    def test_mcu_reads_dig6_before_agc_gate(self):
+        """MCU main loop must read DIG_6 GPIO to sync outerAgc.enabled."""
+        main_cpp = (cp.MCU_CODE_DIR / "main.cpp").read_text()
+        # DIG_6 must be read via HAL_GPIO_ReadPin
+        assert re.search(
+            r'HAL_GPIO_ReadPin\s*\(\s*FPGA_DIG6', main_cpp,
+        ), "main.cpp must read DIG_6 GPIO via HAL_GPIO_ReadPin"
+        # outerAgc.enabled must be assigned from the DIG_6 reading
+        # (may be indirect via debounce variable like dig6_now)
+        assert re.search(
+            r'outerAgc\.enabled\s*=', main_cpp,
+        ), "main.cpp must assign outerAgc.enabled from DIG_6 state"
+
+    def test_boot_invariant_all_layers_agc_off(self):
+        """
+        At boot, all three layers must agree: AGC is OFF.
+        - FPGA: host_agc_enable resets to 0 -> DIG_6 low
+        - MCU: ADAR1000_AGC.enabled defaults to false
+        - GUI: reads status word 4 bit[11] = 0 -> reports MANUAL
+        """
+        # FPGA
+        v_defaults = cp.parse_verilog_reset_defaults()
+        assert v_defaults.get("host_agc_enable") == 0
+
+        # MCU
+        agc_cpp = (cp.MCU_LIB_DIR / "ADAR1000_AGC.cpp").read_text()
+        assert re.search(r'enabled\s*\(\s*false\s*\)', agc_cpp)
+
+        # GUI: status word 4 bit[11] is host_agc_enable, which resets to 0.
+        # Verify the GUI parses bit[11] of status word 4 as the AGC flag.
+        gui_py = (cp.GUI_DIR / "radar_protocol.py").read_text()
+        assert re.search(
+            r'words\[4\].*>>\s*11|status_words\[4\].*>>\s*11',
+            gui_py,
+        ), "GUI must parse AGC status from words[4] bit[11]"
+
+    def test_status_word4_agc_bit_matches_dig6_source(self):
+        """
+        Status word 4 bit[11] and DIG_6 must both derive from host_agc_enable.
+        This guarantees the GUI status display can never lie about MCU AGC state.
+        """
+        rtl = (cp.FPGA_DIR / "radar_system_top.v").read_text()
+
+        # DIG_6 driven by host_agc_enable
+        assert re.search(
+            r'assign\s+gpio_dig6\s*=\s*host_agc_enable\s*;', rtl
+        )
+
+        # Status word 4 must contain host_agc_enable (may be named
+        # status_agc_enable at the USB interface port boundary).
+        # Also verify the top-level wiring connects them.
+        usb_ft2232h = (cp.FPGA_DIR / "usb_data_interface_ft2232h.v").read_text()
+        usb_ft601 = (cp.FPGA_DIR / "usb_data_interface.v").read_text()
+
+        # USB interfaces use the port name status_agc_enable
+        found_in_ft2232h = "status_agc_enable" in usb_ft2232h
+        found_in_ft601 = "status_agc_enable" in usb_ft601
+
+        assert found_in_ft2232h or found_in_ft601, (
+            "status_agc_enable must appear in at least one USB interface's "
+            "status word to guarantee GUI status matches DIG_6"
+        )
+
+        # Verify top-level wiring: status_agc_enable port is connected
+        # to host_agc_enable (same signal that drives DIG_6)
+        assert re.search(
+            r'\.status_agc_enable\s*\(\s*host_agc_enable\s*\)', rtl
+        ), (
+            "Top-level must wire .status_agc_enable(host_agc_enable) "
+            "so status word and DIG_6 derive from the same signal"
+        )
+
+    def test_mcu_dig6_debounce_guards_enable_assignment(self):
+        """
+        MCU must apply a 2-frame confirmation debounce before mutating
+        outerAgc.enabled from DIG_6 reads. A naive assignment straight from
+        the latest GPIO sample would let a single-cycle glitch flip the AGC
+        state for one frame — defeating the debounce claim in the PR body.
+        """
+        main_cpp = (cp.MCU_CODE_DIR / "main.cpp").read_text()
+
+        # (1) Current-frame DIG_6 sample must be captured in a local variable
+        # so it can be compared against the previous-frame value.
+        now_match = re.search(
+            r'(bool|int|uint8_t)\s+(\w*dig6\w*)\s*=\s*[^;]*?'
+            r'HAL_GPIO_ReadPin\s*\(\s*FPGA_DIG6[^;]*;',
+            main_cpp,
+            re.DOTALL,
+        )
+        assert now_match, (
+            "DIG_6 read must be stored in a local variable (e.g. `dig6_now`) "
+            "so the current sample can be compared against the previous frame"
+        )
+        now_var = now_match.group(2)
+
+        # (2) Previous-frame state must persist across iterations via static
+        # storage, and must default to false (matches FPGA boot: AGC off).
+        prev_match = re.search(
+            r'static\s+(bool|int|uint8_t)\s+(\w*dig6\w*)\s*=\s*(false|0)\s*;',
+            main_cpp,
+        )
+        assert prev_match, (
+            "A static previous-frame variable (e.g. "
+            "`static bool dig6_prev = false;`) must exist, initialized to "
+            "false so the debounce starts in sync with the FPGA boot default"
+        )
+        prev_var = prev_match.group(2)
+        assert prev_var != now_var, (
+            f"Current and previous DIG_6 variables must be distinct "
+            f"(both are '{now_var}')"
+        )
+
+        # (3) outerAgc.enabled assignment must be gated by now == prev.
+        guarded_assign = re.search(
+            rf'if\s*\(\s*{now_var}\s*==\s*{prev_var}\s*\)\s*\{{[^}}]*?'
+            rf'outerAgc\.enabled\s*=\s*{now_var}\s*;',
+            main_cpp,
+            re.DOTALL,
+        )
+        assert guarded_assign, (
+            f"`outerAgc.enabled = {now_var};` must be inside "
+            f"`if ({now_var} == {prev_var}) {{ ... }}` — the confirmation "
+            "guard that absorbs single-sample GPIO glitches. A naive "
+            "assignment without this guard reintroduces the glitch bug."
+        )
+
+        # (4) Previous-frame variable must advance each frame.
+        prev_update = re.search(
+            rf'{prev_var}\s*=\s*{now_var}\s*;',
+            main_cpp,
+        )
+        assert prev_update, (
+            f"`{prev_var} = {now_var};` must run each frame so the "
+            "debounce window slides forward; without it the guard is "
+            "stuck and enable changes never confirm"
+        )
+
+
 class TestTier1DataPacketLayout:
     """Verify data packet byte layout matches between Python and Verilog."""