From 5617d552dffa0e4a7f8ae960609f1a188b01fb78 Mon Sep 17 00:00:00 2001 From: Jason <83615043+JJassonn69@users.noreply.github.com> Date: Thu, 23 Apr 2026 04:43:02 +0545 Subject: [PATCH] fix(fpga): frame-boundary atomic commit for USB timing regs C-4: GUI Waveform Timing opcodes (0x10..0x14) write one parameter per USB packet with arbitrary inter-opcode latency. Previously each opcode landed directly on a live register consumed by radar_mode_controller, so a mid-scan reconfiguration exposed the RX FSM to torn sets (e.g. new long_chirp with old long_listen), and a value below the running timer truncated the current chirp and corrupted one range-Doppler frame. Fix: split each of the five timing regs into pending (USB write target) and live (FSM consumer). radar_mode_controller exports a new cfg_commit_strobe that is HIGH while scanning is idle (S_IDLE) and pulses for 1 cycle at every elevation/azimuth boundary in S_ADVANCE. radar_system_top snapshots pending -> live on the strobe, guaranteeing the FSM always sees a self-consistent set. No CDC added: strobe is in clk_100m same as the USB-cmd path. Regression: tb_radar_mode_controller adds 3 checks for strobe semantics (idle-HIGH, scanning-LOW, one pulse per elevation boundary). 32/32 full regression PASS; Radar Mode Controller TB grows from 78 to 81 checks. --- 9_Firmware/9_2_FPGA/radar_mode_controller.v | 33 +++++++- 9_Firmware/9_2_FPGA/radar_receiver_final.v | 11 ++- 9_Firmware/9_2_FPGA/radar_system_top.v | 82 ++++++++++++++----- .../9_2_FPGA/tb/tb_radar_mode_controller.v | 25 +++++- 4 files changed, 128 insertions(+), 23 deletions(-) diff --git a/9_Firmware/9_2_FPGA/radar_mode_controller.v b/9_Firmware/9_2_FPGA/radar_mode_controller.v index 9c4c7ec..4b20c3f 100644 --- a/9_Firmware/9_2_FPGA/radar_mode_controller.v +++ b/9_Firmware/9_2_FPGA/radar_mode_controller.v @@ -97,7 +97,15 @@ module radar_mode_controller #( // Status output wire scanning, // 1 = scan in progress - output wire scan_complete // pulse when full scan done + output wire scan_complete, // pulse when full scan done + + // Timing-config commit strobe (see "Timing-Config Commit" note below). + // Held HIGH while scanning is idle (S_IDLE) and pulsed for 1 clock at + // every elevation/azimuth boundary so radar_system_top can atomically + // copy its USB-written pending timing regs into the live regs the FSM + // reads. Guarantees no torn (cfg_long_chirp, cfg_long_listen, ...) + // reconfiguration mid-frame. + output wire cfg_commit_strobe `ifdef FORMAL , @@ -128,6 +136,29 @@ assign fv_scan_state = scan_state; assign fv_timer = timer; `endif +// ============================================================================ +// Timing-Config Commit Strobe +// +// radar_system_top uses this to snapshot pending USB-written timing regs +// (opcodes 0x10..0x14) into the live regs this FSM consumes. The strobe +// is ASSERTED whenever it is safe to commit new timing: +// (a) FSM is in S_IDLE (no chirp underway) -> held HIGH continuously +// so an opcode written before the first trigger takes effect on +// the first chirp. +// (b) FSM is in S_ADVANCE at the elevation/azimuth boundary (i.e., +// chirp_count has reached the last chirp of the current elevation) +// -> pulsed for the single S_ADVANCE cycle. The next frame of +// CHIRPS_PER_ELEVATION chirps runs with the newly-committed set. +// +// Combinational (no added FF). scan_state and chirp_count are registers +// in the same clock domain, so this wire is glitch-free relative to the +// destination register's clock edge. Downstream is a synchronous-enable +// FF in radar_system_top, which captures cleanly on the rising edge. +// ============================================================================ +assign cfg_commit_strobe = + (scan_state == S_IDLE) || + ((scan_state == S_ADVANCE) && (chirp_count >= cfg_chirps_per_elev - 1)); + // Edge detection for STM32 pass-through reg stm32_new_chirp_prev; reg stm32_new_elevation_prev; diff --git a/9_Firmware/9_2_FPGA/radar_receiver_final.v b/9_Firmware/9_2_FPGA/radar_receiver_final.v index 4b7053b..9929474 100644 --- a/9_Firmware/9_2_FPGA/radar_receiver_final.v +++ b/9_Firmware/9_2_FPGA/radar_receiver_final.v @@ -105,7 +105,13 @@ module radar_receiver_final ( // silent sample drop between the 400 MHz CIC output and the 100 MHz // FIR input; stays high until the next reset. OR'd into the GPIO // diagnostic bit at the top level. - output wire ddc_cic_fir_overrun + output wire ddc_cic_fir_overrun, + + // C-4: timing-config commit strobe from rmc. radar_system_top uses + // this to atomically snapshot pending USB-written timing regs + // (opcodes 0x10..0x14) into the live regs this RX chain consumes, + // eliminating the mid-frame torn-reconfig hazard. + output wire cfg_commit_strobe ); // ========== INTERNAL SIGNALS ========== @@ -216,7 +222,8 @@ radar_mode_controller rmc ( .elevation_count(rmc_elevation_count), .azimuth_count(rmc_azimuth_count), .scanning(rmc_scanning), - .scan_complete(rmc_scan_complete) + .scan_complete(rmc_scan_complete), + .cfg_commit_strobe(cfg_commit_strobe) ); wire clk_400m; diff --git a/9_Firmware/9_2_FPGA/radar_system_top.v b/9_Firmware/9_2_FPGA/radar_system_top.v index f5daa51..6fd71a4 100644 --- a/9_Firmware/9_2_FPGA/radar_system_top.v +++ b/9_Firmware/9_2_FPGA/radar_system_top.v @@ -217,6 +217,10 @@ wire rx_range_decim_watchdog; // CIC→FIR CDC overrun sticky (audit F-1.2). High = at least one baseband // sample has been silently dropped between the 400 MHz CIC and 100 MHz FIR. wire rx_ddc_cic_fir_overrun; +// C-4: timing-config commit strobe from rmc (clk_100m domain). HIGH in +// S_IDLE and pulsed at elevation/azimuth boundaries. Used to latch the +// pending USB-written timing regs into the live regs the RX FSM reads. +wire cfg_commit_strobe; // Data packing for USB wire [31:0] usb_range_profile; @@ -254,12 +258,27 @@ reg [3:0] host_gain_shift; // These override the compile-time defaults in radar_mode_controller when // written via USB command. Defaults match the parameter values in // radar_mode_controller.v so behavior is unchanged until the host writes them. -reg [15:0] host_long_chirp_cycles; // Opcode 0x10 (default 3000) -reg [15:0] host_long_listen_cycles; // Opcode 0x11 (default 13700) -reg [15:0] host_guard_cycles; // Opcode 0x12 (default 17540) -reg [15:0] host_short_chirp_cycles; // Opcode 0x13 (default 50) -reg [15:0] host_short_listen_cycles; // Opcode 0x14 (default 17450) -reg [5:0] host_chirps_per_elev; // Opcode 0x15 (default 32) +// C-4: Split each timing reg into pending (USB write target) and live +// (FSM consumer). The RX radar_mode_controller pulses cfg_commit_strobe +// at every elevation/azimuth boundary (and holds it HIGH in S_IDLE), +// and the live regs snapshot the pending set on that strobe. This +// guarantees that a multi-opcode GUI reconfiguration (0x10 → 0x11 → +// 0x12, separated by USB round-trips) can never be observed +// mid-reconfiguration by the FSM: between frames the pending set is +// either entirely old or entirely new. Without this, a mid-chirp update +// to cfg_long_chirp_cycles below the current timer would immediately +// truncate the chirp and corrupt that frame's range-Doppler map. +reg [15:0] host_long_chirp_cycles_pending; // Opcode 0x10 (default 3000) +reg [15:0] host_long_listen_cycles_pending; // Opcode 0x11 (default 13700) +reg [15:0] host_guard_cycles_pending; // Opcode 0x12 (default 17540) +reg [15:0] host_short_chirp_cycles_pending; // Opcode 0x13 (default 50) +reg [15:0] host_short_listen_cycles_pending; // Opcode 0x14 (default 17450) +reg [15:0] host_long_chirp_cycles; // Live: committed at frame boundary +reg [15:0] host_long_listen_cycles; // Live: committed at frame boundary +reg [15:0] host_guard_cycles; // Live: committed at frame boundary +reg [15:0] host_short_chirp_cycles; // Live: committed at frame boundary +reg [15:0] host_short_listen_cycles; // Live: committed at frame boundary +reg [5:0] host_chirps_per_elev; // Opcode 0x15 (default 32) — has dedicated clamp logic reg host_status_request; // Opcode 0xFF (self-clearing pulse) // Fix 4: Doppler/chirps mismatch protection @@ -596,7 +615,9 @@ radar_receiver_final rx_inst ( .mti_saturation_count_out(rx_mti_saturation_count), // Range-bin decimator watchdog (audit F-6.4) .range_decim_watchdog(rx_range_decim_watchdog), - .ddc_cic_fir_overrun(rx_ddc_cic_fir_overrun) + .ddc_cic_fir_overrun(rx_ddc_cic_fir_overrun), + // C-4: timing-reg commit strobe from rmc (live regs update only when high) + .cfg_commit_strobe(cfg_commit_strobe) ); // ============================================================================ @@ -968,12 +989,18 @@ always @(posedge clk_100m_buf or negedge sys_reset_n) begin host_detect_threshold <= 16'd10000; // Default threshold host_stream_control <= `RP_STREAM_CTRL_DEFAULT; // Default: all streams, mag-only mode host_gain_shift <= 4'd0; // Default: pass-through (no gain change) - // Gap 2: chirp timing defaults (match radar_mode_controller parameters) - host_long_chirp_cycles <= 16'd3000; - host_long_listen_cycles <= 16'd13700; - host_guard_cycles <= 16'd17540; - host_short_chirp_cycles <= 16'd50; - host_short_listen_cycles <= 16'd17450; + // Gap 2: chirp timing defaults — initialize pending AND live to the + // same default so pre-first-trigger behavior matches the old code. + host_long_chirp_cycles_pending <= 16'd3000; + host_long_listen_cycles_pending <= 16'd13700; + host_guard_cycles_pending <= 16'd17540; + host_short_chirp_cycles_pending <= 16'd50; + host_short_listen_cycles_pending <= 16'd17450; + host_long_chirp_cycles <= 16'd3000; + host_long_listen_cycles <= 16'd13700; + host_guard_cycles <= 16'd17540; + host_short_chirp_cycles <= 16'd50; + host_short_listen_cycles <= 16'd17450; host_chirps_per_elev <= 6'd32; host_status_request <= 1'b0; chirps_mismatch_error <= 1'b0; @@ -1005,12 +1032,15 @@ always @(posedge clk_100m_buf or negedge sys_reset_n) begin 8'h02: host_trigger_pulse <= 1'b1; 8'h03: host_detect_threshold <= usb_cmd_value; 8'h04: host_stream_control <= usb_cmd_value[5:0]; - // Gap 2: chirp timing configuration - 8'h10: host_long_chirp_cycles <= usb_cmd_value; - 8'h11: host_long_listen_cycles <= usb_cmd_value; - 8'h12: host_guard_cycles <= usb_cmd_value; - 8'h13: host_short_chirp_cycles <= usb_cmd_value; - 8'h14: host_short_listen_cycles <= usb_cmd_value; + // Gap 2: chirp timing configuration — writes land in the + // PENDING set. The live set atomically snapshots pending + // on cfg_commit_strobe (S_IDLE or elevation boundary). + // See C-4 note at the timing-reg declaration. + 8'h10: host_long_chirp_cycles_pending <= usb_cmd_value; + 8'h11: host_long_listen_cycles_pending <= usb_cmd_value; + 8'h12: host_guard_cycles_pending <= usb_cmd_value; + 8'h13: host_short_chirp_cycles_pending <= usb_cmd_value; + 8'h14: host_short_listen_cycles_pending <= usb_cmd_value; 8'h15: begin // Fix 4: Clamp chirps_per_elev to the fixed Doppler frame size. // If host requests a different value, clamp and set error flag. @@ -1056,6 +1086,20 @@ always @(posedge clk_100m_buf or negedge sys_reset_n) begin default: ; endcase end + + // C-4: atomic commit of pending → live timing regs. The strobe is + // HIGH while the RX FSM is in S_IDLE and pulses for 1 clk_100m + // cycle at every elevation/azimuth boundary. Live regs are only + // updated on the strobe, so the FSM always sees a self-consistent + // set even when the GUI sends opcodes 0x10..0x14 as separate USB + // packets with arbitrary inter-opcode latency. + if (cfg_commit_strobe) begin + host_long_chirp_cycles <= host_long_chirp_cycles_pending; + host_long_listen_cycles <= host_long_listen_cycles_pending; + host_guard_cycles <= host_guard_cycles_pending; + host_short_chirp_cycles <= host_short_chirp_cycles_pending; + host_short_listen_cycles <= host_short_listen_cycles_pending; + end end end diff --git a/9_Firmware/9_2_FPGA/tb/tb_radar_mode_controller.v b/9_Firmware/9_2_FPGA/tb/tb_radar_mode_controller.v index 0b0f829..766fe98 100644 --- a/9_Firmware/9_2_FPGA/tb/tb_radar_mode_controller.v +++ b/9_Firmware/9_2_FPGA/tb/tb_radar_mode_controller.v @@ -44,6 +44,7 @@ module tb_radar_mode_controller; wire [5:0] azimuth_count; wire scanning; wire scan_complete; + wire cfg_commit_strobe; // ── Test bookkeeping ─────────────────────────────────────── integer pass_count; @@ -60,6 +61,7 @@ module tb_radar_mode_controller; integer elevation_toggles; integer azimuth_toggles; integer scan_completes; + integer commit_pulses; // Saved values for toggle checks reg saved_mc_new_chirp; @@ -104,7 +106,8 @@ module tb_radar_mode_controller; .elevation_count (elevation_count), .azimuth_count (azimuth_count), .scanning (scanning), - .scan_complete (scan_complete) + .scan_complete (scan_complete), + .cfg_commit_strobe (cfg_commit_strobe) ); // ── Check task ───────────────────────────────────────────── @@ -173,6 +176,12 @@ module tb_radar_mode_controller; check(azimuth_count === 6'd0, "azimuth_count=0 after reset"); check(scanning === 1'b0, "scanning=0 after reset"); check(scan_complete === 1'b0, "scan_complete=0 after reset"); + + // C-4: cfg_commit_strobe must be HIGH while idle so a timing-reg + // opcode written before the first trigger is latched into the + // live set immediately (no stale default on first chirp). + check(cfg_commit_strobe === 1'b1, + "cfg_commit_strobe=1 while idle (pre-scan commit window)"); reset_n = 1; @(posedge clk); #1; @@ -249,11 +258,18 @@ module tb_radar_mode_controller; elevation_toggles = 0; azimuth_toggles = 0; scan_completes = 0; + commit_pulses = 0; // Check: scanning starts immediately @(posedge clk); #1; check(scanning === 1'b1, "Scanning starts immediately in auto mode"); + // C-4: once scanning is active, cfg_commit_strobe must drop low + // (no longer in S_IDLE). Sample immediately after the IDLE→chirp + // transition. It will pulse again only at elevation boundaries. + check(cfg_commit_strobe === 1'b0, + "cfg_commit_strobe=0 once scan has left IDLE"); + // Run for enough cycles to complete one full scan for (i = 0; i < 15000; i = i + 1) begin @(posedge clk); #1; @@ -266,6 +282,8 @@ module tb_radar_mode_controller; azimuth_toggles = azimuth_toggles + 1; if (scan_complete) scan_completes = scan_completes + 1; + if (cfg_commit_strobe) + commit_pulses = commit_pulses + 1; mc_new_chirp_prev = mc_new_chirp; mc_new_elevation_prev = mc_new_elevation; @@ -288,6 +306,11 @@ module tb_radar_mode_controller; check(chirp_toggles >= SIM_CHIRPS * SIM_ELEVATIONS * SIM_AZIMUTHS, "At least 24 chirp toggles in full scan"); + // C-4: one commit pulse per elevation boundary (commit window). + // With SIM_ELEVATIONS*SIM_AZIMUTHS elevations visited per scan, + // the counter should see at least that many pulses. + check(commit_pulses >= SIM_ELEVATIONS * SIM_AZIMUTHS, + "cfg_commit_strobe pulses at every elevation boundary"); check(scan_completes >= 1, "At least 1 scan completion detected"); check(elevation_toggles >= SIM_AZIMUTHS,