Fix RTL audit findings across utility modules - #4
Open
aca-logimentor wants to merge 11 commits into
Open
aca-logimentor wants to merge 11 commits into
aca-logimentor wants to merge 11 commits into
Conversation
- f_smallest(t_natural_arr) always returned 0 (accumulator started at 0) - f_div_ceil_2pwr rounded up by unit increments capped at 64 iterations, returning non-power-of-two values for large ratios - f_div_ceil(time, time) truncated sub-ns remainders when computing the mod - f_is_power_of_two failed on 0 and relied on to_signed truncation - f_string_format lost the carry when the fraction rounded up to 10**precision - f_string_substr bounds check was inverted, flagging valid substrings - the unsupported-operation assert in f_vector_tree used 'assert true' and could never fire; it also referenced the pre-rename package name
The third toggle synchronizer stage sampled the first stage instead of the second, and the edge detector XORed against the first stage, which samples the asynchronous toggle directly: a metastable event could double or drop the measurement window. The reported frequency also missed the cycle that closes the window (off by one), and the ready synchronizer chain was not reset, so a reset while the toggle was high produced a spurious output update. The testbench now checks the second, steady-state window, whose count does not depend on the reset phase.
bitsum_o was f_ceil_log2(g_din_w) bits wide, but counting the ones of an all-ones input yields g_din_w, which needs f_ceil_log2(g_din_w + 1) bits: for power-of-two input widths the result wrapped to zero.
- g_pulse_overlength = 1 deasserted the output on every idle cycle because the counter idles at 0 and the compare was not qualified by the enable - g_pulse_length = 1 never produced a pulse (0 < 0 is never true); the output is now active for the whole enable window - stretch mode now adds exactly g_pulse_overlength cycles after the input falling edge (was one cycle short) and is retriggered by a new rising edge instead of cutting the output mid-pulse - the input pulse is defined active high; g_out_level only sets the output polarity, and the zero-overlength passthrough honours it - the fixed-length window ignores edges arriving while it is running - the testbench drives an active-high input, checks mode-dependent widths, and skews scheduled transitions off the clock edges so sampling is unambiguous with the combinational input stage
- both pulse counters restarted on every active input cycle, so pulses wider than one clock were delayed from their trailing edge; they now trigger on the leading edge only - reset drove the outputs to '0', which is the active level when the configured pulse polarity is low; reset now drives the inactive level - delay_var: dv_o was never driven for g_delay_max = 1 and in the pulse architecture; a runtime delay of 1 now works in pulse mode; unsupported g_arch_type values (including the never-implemented mem architecture) are rejected at elaboration; superseded commented-out code removed - the delay_var testbench drove the DUT output dv_o instead of dv_i and never checked dv_o; both fixed
Counting down, the counter started from 0 and underflowed through the full register range before ever reaching the watchdog compare. The down direction is now symmetric to up: reset loads g_wd_timer-1, the counter runs down to 0, pulses timer_o and reloads. g_wd_timer is validated against the counter width at elaboration. The testbench load test carried its stimulus commented out and never drove load_i; it now loads a value and checks that counting resumes from it in both directions.
Add async_reg/shreg_extract attributes to the hand-rolled synchronizer chains (ccd_resync, ccd_sync_pulse, async_reset, clock_measure via its own commit, clock_mux) so synthesis keeps the flops discrete and adjacent instead of mapping them to SRL primitives. The clock_mux anti-glitch keep attribute was boolean-typed, which Vivado silently ignores; it is now a string. ccd_resync's meta-levels guard is a hard failure like the other guards. ccd_sync_pulse validates g_delay_len. ccd_sync_bus parks its request edge detector high across reset so a request held asserted through the reset release is not seen as a new rising edge.
- counter: down direction and load tests enabled, watchdog and load tests get explicit configurations - pulse_stretch: active-low output and unit lengths enabled - delay_pulse: multi-cycle input pulses enabled - delay_var: pulse delay of 1 and g_delay_max = 1 enabled - bitsum: power-of-two input widths exercise the all-ones count - document the removed mem architecture in the user guide and record the behavioral changes in the changelog
async_reg/shreg_extract are Xilinx-only and are silently ignored by other tools, so the synchronizer chains now also carry syn_preserve/syn_srlstyle for Synplify Pro and Lattice LSE, and altera_attribute synchronizer identification (plus shift-register recognition off) for Intel Quartus. The clock_mux keep intent is expressed per tool: keep as a string for Vivado, syn_keep for Synplify/LSE, and KEEP ON via altera_attribute for Quartus, restoring the Quartus behavior lost when the boolean keep was converted to a string. Attributes only prevent structural mangling; the crossing paths still need project-level timing constraints, so each module header now carries a ready-to-adapt set_false_path/set_max_delay example per tool and the user guide gains a CDC attributes and timing constraints section.
There was a problem hiding this comment.
Pull request overview
This PR addresses internal RTL audit findings across multiple lm_util utility blocks by fixing functional edge cases, hardening CDC/synchronizer structures for synthesis, and re-enabling/expanding VUnit regression coverage for previously failing configurations.
Changes:
- Fixes functional corner cases in pulse stretching, variable/fixed pulse delaying, counting (incl. down direction), clock measurement, and several
lm_util_pkghelper functions. - Adds vendor-specific synthesis attributes (
async_reg,shreg_extract,syn_preserve,altera_attribute, etc.) to prevent SRL mapping/optimization of synchronizer flops, and documents required project-level timing constraints. - Expands and corrects testbenches plus
sim/scripts/run.pyto cover previously disabled failing parameter sets.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/lm_util_pulse_stretch.vhd | Fixes fixed-length and stretch-mode corner cases; clarifies input/output polarity behavior. |
| src/lm_util_pkg.vhd | Corrects multiple utility functions (min, ceil divisions, power-of-two check, formatting/substr bounds, assert guard). |
| src/lm_util_delay_var.vhd | Fixes reset polarity behavior, drives dv_o in all modes, rejects unsupported architectures, hardens pulse-mode behavior. |
| src/lm_util_delay_pulse.vhd | Triggers delay from leading edge (multi-cycle pulses), fixes reset polarity behavior. |
| src/lm_util_counter.vhd | Makes down-count symmetric to up-count, adds generic validation, adjusts reset/reload behavior. |
| src/lm_util_clock_mux.vhd | Fixes/extends synthesis attributes (keep, synchronizer attributes) and adds constraint guidance. |
| src/lm_util_clock_measure.vhd | Fixes toggle synchronizer staging/edge detect, corrects off-by-one window close, clears ready chain on reset, adds CDC attributes/docs. |
| src/lm_util_ccd_sync_pulse.vhd | Adds CDC attributes and enforces minimum synchronizer depth for metastability reduction. |
| src/lm_util_ccd_sync_bus.vhd | Prevents false edge detect on reset release when request is held asserted. |
| src/lm_util_ccd_resync.vhd | Adds CDC attributes and improves documentation/constraint guidance; strengthens generic check severity. |
| src/lm_util_bitsum.vhd | Widens bitsum_o to avoid wrap on all-ones for power-of-two input widths. |
| src/lm_util_async_reset.vhd | Adds CDC attributes and constraint guidance for async reset resynchronization. |
| sim/tb/tb_vu_lm_util_pulse_stretch.vhd | Updates stimulus and checks for new polarity definition and corrected stretch/fixed-length semantics. |
| sim/tb/tb_vu_lm_util_delay_var.vhd | Fixes TB driving (dv input vs output) and adds dv_o assertions. |
| sim/tb/tb_vu_lm_util_counter.vhd | Adds down-direction and load-mode coverage; updates expectations for new reload behavior. |
| sim/tb/tb_vu_lm_util_clock_measure.vhd | Adjusts TB to check steady-state measurement window (reset-phase independence). |
| sim/tb/tb_vu_lm_util_bitsum.vhd | Updates TB signal width to match widened bitsum_o. |
| sim/scripts/run.py | Re-enables/extends parameter sweeps for previously failing configs; adds new targeted coverage sets. |
| docs/user_guide.md | Updates module descriptions and documents CDC attributes + required timing constraint patterns. |
| CHANGELOG.md | Records breaking interface/behavior changes and summarizes fixes + test coverage improvements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+81
to
+84
| -- check watchdog range against the counter width | ||
| assert (g_wd_timer >= 1) and (g_wd_timer <= 2**g_data_w) | ||
| report "g_wd_timer must be in range 1 to 2**g_data_w!" | ||
| severity failure; |
2**g_data_w overflows the 32-bit VHDL integer at elaboration for g_data_w >= 31, failing valid configurations. Check the equivalent f_ceil_log2(g_wd_timer) <= g_data_w instead; the boolean 'and' short-circuit keeps f_ceil_log2 from being called when g_wd_timer < 1.
Comment on lines
+81
to
+86
| -- check watchdog range against the counter width; the log2 form avoids the | ||
| -- integer overflow of 2**g_data_w for g_data_w >= 31. VHDL boolean 'and' | ||
| -- short-circuits, so f_ceil_log2 is not called when g_wd_timer < 1 | ||
| assert (g_wd_timer >= 1) and (f_ceil_log2(g_wd_timer) <= g_data_w) | ||
| report "g_wd_timer must be in range 1 to 2**g_data_w!" | ||
| severity failure; |
Comment on lines
+77
to
+79
| assert (g_delay_max = 1) or (g_arch_type = C_LM_SRL) or (g_arch_type = C_LM_PULSE) | ||
| report "g_arch_type must be C_LM_SRL or C_LM_PULSE!" | ||
| severity failure; |
The pulse delay trigger was still level-based (input active and counter idle), so an input held active beyond the configured delay retriggered a new delay right after the output pulse was emitted, duplicating the event. Both lm_util_delay_pulse and the lm_util_delay_var pulse architecture now detect the leading edge against the previous input sample. In the delay_var pulse architecture dv_o now pulses together with the delayed output pulse, consistent with the SRL architecture, instead of mirroring dv_i one cycle late. The delay_pulse unit-delay register gains the reset-to-inactive-level behavior the other configurations already had. Testbenches check that the output pulse is one cycle wide and that an input wider than the delay does not retrigger; new configurations cover g_pulse_width greater than g_delay.
Comment on lines
+81
to
+86
| -- check watchdog range against the counter width; the log2 form avoids the | ||
| -- integer overflow of 2**g_data_w for g_data_w >= 31. VHDL boolean 'and' | ||
| -- short-circuits, so f_ceil_log2 is not called when g_wd_timer < 1 | ||
| assert (g_wd_timer >= 1) and (f_ceil_log2(g_wd_timer) <= g_data_w) | ||
| report "g_wd_timer must be in range 1 to 2**g_data_w!" | ||
| severity failure; |
Comment on lines
103
to
109
| if in_rst_n_i = '0' then | ||
| s_in_data_dv <= '0'; | ||
| -- park the edge detector high so a request held asserted across the | ||
| -- reset release is not seen as a new rising edge | ||
| s_req_prim_d1 <= '1'; | ||
| else | ||
| s_req_prim_d1 <= s_req_prim; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a set of functional bugs found during an internal audit of the library, hardens the CDC synchronizers for synthesis, and closes the coverage gaps that were hiding the bugs (all previously disabled failing configurations are re-enabled).
RTL fixes
bitsum_owas one bit too narrow — the all-ones count wrapped to zero for power-of-two input widths. Breaking: the port is nowf_ceil_log2(g_din_w + 1)bits wide.g_pulse_length = 1produced no pulse andg_pulse_overlength = 1killed the output immediately; stretch mode was one cycle short and cut the output on retrigger. Breaking: the input pulse is defined active high, stretch adds exactlyg_pulse_overlengthcycles and is retriggerable.g_wd_timer-1 .. 0symmetric to up.dv_owas undriven in two delay_var configurations and the documented but never-implemented "mem" architecture is now rejected at elaboration.f_smallest(t_natural_arr)always returned 0,f_div_ceil_2pwrreturned non-powers-of-two for large ratios,f_div_ceil(time,time)truncated sub-ns remainders,f_is_power_of_twocrashed on 0,f_string_formatlost the rounding carry,f_string_substrhad an inverted bounds check, and thef_vector_treeguard usedassert true.Synthesis hardening
All hand-rolled synchronizer chains now carry
async_reg/shreg_extractattributes so the flops stay discrete instead of being mapped to SRLs. The clock_mux anti-glitchkeepattribute was boolean-typed (silently ignored by Vivado) and is now a string.Verification
load_i, and the delay_var testbench drove the DUT outputdv_o— both fixed.VUNIT_SIMULATOR=ghdl python sim/scripts/run.py --level fast --clean: 204/204 passed (was 142 tests before this branch).VUNIT_SIMULATOR=ghdl python sim/scripts/run.py --level full: all passed.Known follow-ups (not in this PR)
lm_util_ccd_sync_bushandshake has no deassert acknowledgment (fast-requester reassertion can deadlock) and its output domain has no reset; a redesign needs its own testbench first.lm_util_lfsrseed/width issues and CI workflow improvements (job duplication, tool version pinning, scheduled full-level run).