Remove (almost) all verible warnings#17
Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on reducing Verible lint warnings across the RTL by tightening signal/parameter typing, adding explicit casts, and cleaning up some formatting to make intent clearer.
Changes:
- Refactors
hvsync_generatorport declarations and timing constants (move to typedlocalparams; renames constants). - Narrows/annotates widths for several parameters/localparams and intermediate signals to reduce width-mismatch warnings.
- Updates sprite/rendering math and gating in cannon/character/HUD-related modules.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/rtl/hvsync_generator.sv |
Reworks module ports and VGA timing constants using typed localparams and updated naming. |
src/rtl/hud.sv |
Adjusts HUD lives scaling constant width to better match downstream module port widths. |
src/rtl/character.sv |
Narrows char_code and intermediate arithmetic/index signals; adds explicit size casts for ROM indexing. |
src/rtl/cannon_laser.sv |
Adds explicit parameter and localparam types/widths for scaling-related constants. |
src/rtl/cannon_display.sv |
Tweaks sprite-relative coordinate math and adds a “scale=0 invisible” gating expression. |
src/rtl/cannon.sv |
Similar rendering math/gating changes as cannon_display.sv for the main cannon sprite. |
src/rtl/alien_projectile.sv |
Adds explicit parameter/localparam widths for scaling and speed. |
src/rtl/alien.sv |
Minor whitespace-only formatting cleanup. |
src/chipinvaders.sv |
Adds explicit logic typing to sprite/scaling localparams used by the top-level game wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rel_x = 3'((hpos - X_POS) / SCALING); | ||
| rel_y = 3'((vpos - Y_POS) / SCALING); | ||
|
|
||
| if (rel_y < SpriteHeight && rel_x < SpriteWidth) begin | ||
| if (is_digit) begin |
There was a problem hiding this comment.
rel_x/rel_y are truncated to 3 bits before the bounds check. This can cause pixels far outside the 5x5 sprite to wrap (mod 8) and incorrectly satisfy rel_x < SpriteWidth / rel_y < SpriteHeight, effectively repeating the sprite pattern across the screen. Keep rel_x/rel_y wide for the arithmetic and bounds check, and only truncate/cast when indexing the ROM.
| always_comb begin | ||
| rel_x = (10'(pix_x) - x_reg) / scale; | ||
| rel_y = (10'(pix_y) - CANNON_Y) / scale; | ||
| rel_x = (pix_x - x_reg) / scale; | ||
| rel_y = (pix_y - CANNON_Y) / scale; | ||
|
|
There was a problem hiding this comment.
scale is used as a divisor unconditionally. When scale == 0 (which the comment says should mean invisible), this will result in division-by-zero behavior (X in simulation / tool-dependent synthesis). Guard the division and in_sprite_bounds calculation with if (scale > 0) (or clamp scale to at least 1) so the invisible case is safe.
| always_comb begin | ||
| rel_x = (10'(pix_x) - x_reg) / scale; | ||
| rel_y = (10'(pix_y) - SHIP_Y) / scale; | ||
| rel_x = (pix_x - x_reg) / scale; | ||
| rel_y = (pix_y - SHIP_Y) / scale; | ||
|
|
There was a problem hiding this comment.
scale is used as a divisor unconditionally. Even though you later mask output with scale > 0, rel_x/rel_y are still computed via / scale, so scale == 0 causes division-by-zero behavior. Handle scale == 0 explicitly (e.g., set cannon_graphics = 0 and skip the division / bounds logic).
| localparam logic [3:0] LiveScale = SCALE * 2; | ||
| localparam logic [15:0] LiveStep = (LiveW + LiveGap) * LiveScale; |
There was a problem hiding this comment.
LiveScale is narrowed to 4 bits, but it is computed as SCALE * 2 where SCALE is also 4 bits. For SCALE >= 8, this truncates and will produce an incorrect LiveStep and HUD layout. Consider keeping LiveScale wide enough for the multiplication (e.g., 5+ bits) and casting only at the port connection, or add a parameter constraint/assertion that limits SCALE to a safe range.
| localparam logic [15:0] HBack = 48; // horizontal left border (back porch) | ||
| localparam logic [15:0] HFront = 16; // horizontal right border (front porch) | ||
| localparam logic [15:0] HSync = 96; // horizontal sync width | ||
| // verticallogic constants |
There was a problem hiding this comment.
Typo in comment: verticallogic constants looks like it should be vertical constants.
| // verticallogic constants | |
| // vertical constants |
No description provided.