From ea2615ef841f2aeec55c469024f188dfe7537493 Mon Sep 17 00:00:00 2001 From: Jason <83615043+JJassonn69@users.noreply.github.com> Date: Wed, 29 Apr 2026 18:36:31 +0545 Subject: [PATCH] =?UTF-8?q?doppler:=20gate=20S=5FIDLE=E2=86=92S=5FACCUMULA?= =?UTF-8?q?TE=20on=20frame=5Fstart=5Fpulse=20(AUDIT-S3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-fix S_IDLE had two independent if-branches: one for frame_start_pulse (resets pointers) and one for data_valid (transitions to S_ACCUMULATE). A data_valid arriving before frame_start_pulse would advance the FSM with whatever pointers happened to be live, and the BRAM write block would write the sample into mem_write_addr = (write_chirp_index*RANGE_BINS) + 0. In current operation the race is benign — end-of-S_ACCUMULATE always zeros write_chirp_index/write_range_bin (line 287-288) and the MF pipeline latency (~165 µs) is millions of cycles longer than the frame_start CDC latency (~50 ns), so frame_start always arrives first. But the FSM relies on an undocumented system-level invariant; a future code path that leaves pointers stale on entry to S_IDLE would silently corrupt the first sample. Fix: add a `frame_armed` register set when frame_start_pulse arrives in S_IDLE, cleared on transition to S_ACCUMULATE. Both the FSM transition and the BRAM write block gate on `(frame_start_pulse || frame_armed)`. The OR admits the same-cycle case where both arrive together (write to addr 0 still resolves correctly because both blocks use the same gate). Verification: tb_doppler_frame_start_gate 21/21 PASS, quick regression 32/32 PASS (was 31/31; +1 new test, 0 regressions). tb_doppler_realdata (full FFT pipeline) still passes — gate transparent to normal operation. --- 9_Firmware/9_2_FPGA/doppler_processor.v | 37 ++- 9_Firmware/9_2_FPGA/run_regression.sh | 4 + .../9_2_FPGA/tb/tb_doppler_frame_start_gate.v | 275 ++++++++++++++++++ 3 files changed, 311 insertions(+), 5 deletions(-) create mode 100644 9_Firmware/9_2_FPGA/tb/tb_doppler_frame_start_gate.v diff --git a/9_Firmware/9_2_FPGA/doppler_processor.v b/9_Firmware/9_2_FPGA/doppler_processor.v index 82cb521..47d53bc 100644 --- a/9_Firmware/9_2_FPGA/doppler_processor.v +++ b/9_Firmware/9_2_FPGA/doppler_processor.v @@ -134,6 +134,17 @@ reg frame_buffer_full; reg [9:0] chirps_received; reg [1:0] chirp_state; +// AUDIT-S3 fix: arm-on-frame-start gating. Set when frame_start_pulse arrives +// in S_IDLE; cleared when the FSM transitions to S_ACCUMULATE. Prevents stale +// data_valid from prior MF pipeline residue from advancing S_IDLE → S_ACCUMULATE +// before the new frame is officially started, which would write the first +// sample(s) into addr 0 of the previous frame's buffer if write_chirp_index +// happened to be non-zero. The pointer-reset invariant (line 287-288 always +// zeros pointers at end of S_ACCUMULATE) makes this race benign in current +// operation, but the gate makes the FSM robust against future code paths +// that might leave pointers stale on entry to S_IDLE. +reg frame_armed; + // Sub-frame tracking reg current_sub_frame; // 0=processing long, 1=processing short @@ -246,6 +257,7 @@ always @(posedge clk or negedge reset_n) begin range_bin <= 0; sub_frame <= 0; current_sub_frame <= 0; + frame_armed <= 0; end else begin doppler_valid <= 0; fft_input_valid <= 0; @@ -262,14 +274,24 @@ always @(posedge clk or negedge reset_n) begin write_range_bin <= 0; frame_buffer_full <= 0; chirps_received <= 0; + frame_armed <= 1; // AUDIT-S3: arm on frame_start_pulse end - - if (data_valid && !frame_buffer_full) begin + + // AUDIT-S3 fix: only transition to S_ACCUMULATE when armed, + // i.e., when this frame has been officially started by a + // frame_start_pulse. Pre-fix code accepted any data_valid in + // S_IDLE and could race with a missing/late frame_start_pulse. + // (frame_start_pulse || frame_armed) admits the same-cycle case + // where both pulse and data_valid arrive together — write to + // addr 0 still resolves correctly because the BRAM write block + // uses the same gate. + if ((frame_start_pulse || frame_armed) && data_valid && !frame_buffer_full) begin state <= S_ACCUMULATE; write_range_bin <= 1; + frame_armed <= 0; // disarm; S_ACCUMULATE handles its own pointers end end - + S_ACCUMULATE: begin if (data_valid) begin if (write_range_bin < RANGE_BINS - 1) begin @@ -393,14 +415,19 @@ always @(posedge clk) begin case (state) S_IDLE: begin - if (data_valid && !frame_buffer_full) begin + // AUDIT-S3 fix: gate BRAM write on frame_armed so stale + // data_valid arriving before frame_start_pulse cannot + // overwrite addr 0 of the buffer. Same gate as the FSM's + // S_IDLE → S_ACCUMULATE transition above, so the two blocks + // stay coherent. + if ((frame_start_pulse || frame_armed) && data_valid && !frame_buffer_full) begin mem_we <= 1; mem_waddr_r <= mem_write_addr; mem_wdata_i <= range_data[15:0]; mem_wdata_q <= range_data[31:16]; end end - + S_ACCUMULATE: begin if (data_valid) begin mem_we <= 1; diff --git a/9_Firmware/9_2_FPGA/run_regression.sh b/9_Firmware/9_2_FPGA/run_regression.sh index 20a3e1e..03ee2fc 100755 --- a/9_Firmware/9_2_FPGA/run_regression.sh +++ b/9_Firmware/9_2_FPGA/run_regression.sh @@ -554,6 +554,10 @@ run_test "FT2232H Frame Drop Counter (AUDIT-C12)" \ tb/tb_ft2232h_frame_drop.vvp \ tb/tb_ft2232h_frame_drop.v usb_data_interface_ft2232h.v +run_test "Doppler Frame-Start Gate (AUDIT-S3)" \ + tb/tb_doppler_frame_start_gate.vvp \ + tb/tb_doppler_frame_start_gate.v doppler_processor.v xfft_16.v fft_engine.v + echo "" # =========================================================================== diff --git a/9_Firmware/9_2_FPGA/tb/tb_doppler_frame_start_gate.v b/9_Firmware/9_2_FPGA/tb/tb_doppler_frame_start_gate.v new file mode 100644 index 0000000..518c37f --- /dev/null +++ b/9_Firmware/9_2_FPGA/tb/tb_doppler_frame_start_gate.v @@ -0,0 +1,275 @@ +`timescale 1ns / 1ps + +// ============================================================================ +// tb_doppler_frame_start_gate.v — AUDIT-S3 regression +// ============================================================================ +// Verifies the post-fix gating in doppler_processor_optimized's S_IDLE state. +// +// AUDIT-S3 (pre-fix bug): S_IDLE had two independent if-branches — one for +// frame_start_pulse (resets pointers) and one for data_valid (transitions to +// S_ACCUMULATE). A data_valid arriving BEFORE frame_start_pulse would +// transition the FSM with whatever pointers happened to be set, and the BRAM +// write block would write into addr (write_chirp_index*RANGE_BINS+0). +// +// Post-fix: a `frame_armed` register is set on frame_start_pulse and cleared +// on transition to S_ACCUMULATE. Both the FSM transition and the BRAM write +// are gated on `(frame_start_pulse || frame_armed)`. data_valid arriving +// before frame_start_pulse is silently dropped at the input. +// +// Tests: +// T1 data_valid alone (no frame_start) → state stays S_IDLE, mem_we=0 +// T2 frame_start_pulse alone (no data_valid) → state stays S_IDLE, +// frame_armed=1, mem_we=0 +// T3 frame_start, then data_valid 5 cycles later (normal MF latency case) +// → state→S_ACCUMULATE on the data_valid cycle, mem_we=1, mem_waddr_r=0 +// T4 frame_start and data_valid on the SAME cycle +// → state→S_ACCUMULATE this cycle, mem_we=1, mem_waddr_r=0 +// T5 long burst of data_valid in S_IDLE before any frame_start_pulse +// → state stays S_IDLE for the entire burst, mem_we never asserts +// ============================================================================ + +`include "radar_params.vh" + +module tb_doppler_frame_start_gate; + +localparam CLK_PERIOD = 10.0; // 100 MHz + +// Tiny config for fast simulation. We only exercise the S_IDLE gating; the +// rest of the FSM (S_ACCUMULATE, FFT pipeline) is not driven to completion. +localparam DOPPLER_FFT_SIZE = 16; // unchanged +localparam RANGE_BINS = 8; +localparam CHIRPS_PER_FRAME = 4; +localparam CHIRPS_PER_SUBFRAME = 2; +localparam DATA_WIDTH = 16; + +reg clk; +reg reset_n; +reg [31:0] range_data; +reg data_valid; +reg new_chirp_frame; + +wire [31:0] doppler_output; +wire doppler_valid; +wire [4:0] doppler_bin; +wire [`RP_RANGE_BIN_WIDTH_MAX-1:0] range_bin; +wire sub_frame; +wire processing_active; +wire frame_complete; +wire [3:0] dut_status; + +doppler_processor_optimized #( + .DOPPLER_FFT_SIZE(DOPPLER_FFT_SIZE), + .RANGE_BINS(RANGE_BINS), + .CHIRPS_PER_FRAME(CHIRPS_PER_FRAME), + .CHIRPS_PER_SUBFRAME(CHIRPS_PER_SUBFRAME), + .DATA_WIDTH(DATA_WIDTH) +) dut ( + .clk(clk), + .reset_n(reset_n), + .range_data(range_data), + .data_valid(data_valid), + .new_chirp_frame(new_chirp_frame), + .doppler_output(doppler_output), + .doppler_valid(doppler_valid), + .doppler_bin(doppler_bin), + .range_bin(range_bin), + .sub_frame(sub_frame), + .processing_active(processing_active), + .frame_complete(frame_complete), + .status(dut_status) +); + +// ---------------------------------------------------------------------------- +// Hierarchical refs to internal state +// ---------------------------------------------------------------------------- +wire [2:0] dut_state = dut.state; +wire dut_armed = dut.frame_armed; +wire dut_mem_we = dut.mem_we; +wire [`RP_DOPPLER_MEM_ADDR_W-1:0] dut_mem_waddr = dut.mem_waddr_r; + +localparam [2:0] S_IDLE = 3'b000; +localparam [2:0] S_ACCUMULATE = 3'b001; + +// ---------------------------------------------------------------------------- +// Clock +// ---------------------------------------------------------------------------- +initial clk = 0; +always #(CLK_PERIOD / 2) clk = ~clk; + +// ---------------------------------------------------------------------------- +// Test bookkeeping +// ---------------------------------------------------------------------------- +integer pass_count = 0; +integer fail_count = 0; +integer test_num = 0; + +task check; + input cond; + input [255:0] label; + begin + test_num = test_num + 1; + if (cond) begin + $display("[PASS %0d] %0s", test_num, label); + pass_count = pass_count + 1; + end else begin + $display("[FAIL %0d] %0s (state=%0d armed=%0b mem_we=%0b waddr=%0d)", + test_num, label, dut_state, dut_armed, dut_mem_we, dut_mem_waddr); + fail_count = fail_count + 1; + end + end +endtask + +task do_reset; + begin + reset_n = 0; + data_valid = 0; + new_chirp_frame = 0; + range_data = 32'h0; + repeat (4) @(posedge clk); + #1 reset_n = 1; + @(posedge clk); #1; + end +endtask + +// ---------------------------------------------------------------------------- +// Stimulus +// ---------------------------------------------------------------------------- +initial begin + $dumpfile("tb_doppler_frame_start_gate.vcd"); + $dumpvars(0, tb_doppler_frame_start_gate); + + $display("\n=== AUDIT-S3 regression: doppler S_IDLE frame_start gating ===\n"); + + // ============================================================ + // T1: data_valid alone (no frame_start) → no transition, no write + // ============================================================ + do_reset; + range_data = 32'hAAAA_BBBB; + data_valid = 1; + @(posedge clk); #1; + check(dut_state == S_IDLE, "T1.a: state stays S_IDLE under data_valid w/o frame_start"); + check(dut_armed == 1'b0, "T1.b: frame_armed remains 0"); + check(dut_mem_we == 1'b0, "T1.c: mem_we does NOT fire"); + + // Hold for 5 more cycles to confirm no late transition / no write + repeat (5) @(posedge clk); + #1; + check(dut_state == S_IDLE, "T1.d: state still S_IDLE after 5 more cycles"); + check(dut_mem_we == 1'b0, "T1.e: mem_we still 0 after 5 more cycles"); + + data_valid = 0; + @(posedge clk); #1; + + // ============================================================ + // T2: frame_start_pulse alone, no data_valid → frame_armed=1, no write + // ============================================================ + do_reset; + new_chirp_frame = 1; // edge-detector inside DUT will fire one-shot + @(posedge clk); #1; + new_chirp_frame = 0; + @(posedge clk); #1; // frame_start_pulse fires this cycle + check(dut_armed == 1'b1, "T2.a: frame_armed set after frame_start_pulse"); + check(dut_state == S_IDLE, "T2.b: state stays S_IDLE without data_valid"); + check(dut_mem_we == 1'b0, "T2.c: mem_we does NOT fire (no data_valid)"); + + // Wait some cycles — armed should persist + repeat (10) @(posedge clk); + #1; + check(dut_armed == 1'b1, "T2.d: frame_armed persists across idle cycles"); + check(dut_state == S_IDLE, "T2.e: state still S_IDLE"); + + // ============================================================ + // T3: armed (from T2), then data_valid → transition + write to addr 0 + // ============================================================ + range_data = 32'h1234_5678; + data_valid = 1; + @(posedge clk); #1; + check(dut_state == S_ACCUMULATE, "T3.a: state→S_ACCUMULATE on first data_valid after frame_start"); + check(dut_armed == 1'b0, "T3.b: frame_armed cleared on transition"); + check(dut_mem_we == 1'b1, "T3.c: mem_we asserted on transition"); + check(dut_mem_waddr == 0, "T3.d: first sample writes to addr 0"); + + data_valid = 0; + @(posedge clk); #1; + + // ============================================================ + // T4: same-cycle frame_start_pulse + data_valid → transition + write + // ============================================================ + do_reset; + new_chirp_frame = 1; + @(posedge clk); #1; // 1st cycle of new_chirp_frame=1 + new_chirp_frame = 0; + range_data = 32'hDEAD_BEEF; + data_valid = 1; + // On THIS cycle: new_chirp_frame_d1=1 (latched last cycle), new_chirp_frame=0 + // → frame_start_pulse = 0 (XOR is 1 only on rising edge) + // We need to time the data_valid to coincide with the rising-edge cycle. + // So actually: drive new_chirp_frame=1 simultaneously with data_valid=1 + // for ONE cycle, starting from a state where new_chirp_frame_d1=0. + @(posedge clk); #1; // give the FSM a cycle to settle (T4 prep) + do_reset; // reclean + range_data = 32'hDEAD_BEEF; + data_valid = 1; + new_chirp_frame = 1; + @(posedge clk); #1; // first cycle: new_chirp_frame_d1 still 0 + // → frame_start_pulse fires THIS cycle. + // Same cycle as data_valid → transition+write. + check(dut_state == S_ACCUMULATE, "T4.a: same-cycle pulse+data → state→S_ACCUMULATE"); + check(dut_mem_we == 1'b1, "T4.b: same-cycle → mem_we fires"); + check(dut_mem_waddr == 0, "T4.c: same-cycle → write to addr 0"); + check(dut_armed == 1'b0, "T4.d: same-cycle → frame_armed disarmed"); + + new_chirp_frame = 0; + data_valid = 0; + @(posedge clk); #1; + + // ============================================================ + // T5: long data_valid burst with no frame_start → no transition, no write + // (regression for the audit's specific concern) + // ============================================================ + do_reset; + range_data = 32'hCAFE_F00D; + data_valid = 1; + begin : t5_burst + integer i; + integer mem_we_count; + mem_we_count = 0; + for (i = 0; i < 100; i = i + 1) begin + @(posedge clk); #1; + if (dut_mem_we) mem_we_count = mem_we_count + 1; + if (dut_state != S_IDLE) begin + $display("[FAIL] T5: state left S_IDLE at cycle %0d (state=%0d)", i, dut_state); + fail_count = fail_count + 1; + disable t5_burst; + end + end + check(mem_we_count == 0, "T5.a: 100 cycles of data_valid w/o frame_start → 0 BRAM writes"); + check(dut_state == S_IDLE, "T5.b: state still S_IDLE after 100-cycle burst"); + check(dut_armed == 1'b0, "T5.c: frame_armed never set"); + end + data_valid = 0; + @(posedge clk); #1; + + // ============================================================ + // Summary + // ============================================================ + $display("\n=== AUDIT-S3 frame-start gate regression ==="); + $display(" PASSED: %0d / %0d", pass_count, test_num); + $display(" FAILED: %0d / %0d", fail_count, test_num); + if (fail_count == 0) + $display(" ** ALL TESTS PASSED **"); + else + $display(" ** SOME TESTS FAILED **"); + $display(""); + + #20 $finish; +end + +// Timeout safety +initial begin + #500_000; + $display("[FAIL] Watchdog timeout — TB hung"); + $finish; +end + +endmodule