Add initial content to vga_timing.sv #38
Conversation
Implement VGA timing module with pixel clock generation and coordinate tracking.
rtl/video/vga_timing.sv
Outdated
| @@ -1,0 +1,57 @@ | |||
| `timescale 1ns / 1ps | |||
There was a problem hiding this comment.
Compatibility Fix: Avoid adding "timescale" in commits to ensure all simulators process SV files correctly
Deleted 'timescale' line to ensure file was compatible with all simulators
rtl/video/vga_timing.sv
Outdated
| @@ -1,0 +1,56 @@ | |||
| module VGA_timing( | |||
| input clk, | |||
| input reset, | |||
There was a problem hiding this comment.
Stylistic Error:
"clk" -> "clk_i"
"reset" -> "rst_ni" (Use low reset for consistency with other files)
There was a problem hiding this comment.
@JayR-360 I erased the "timescale" line since not all simulators use that keyword the same. Please add comments to your code (+ add description to PR) and review any comments that I made. After that, the PR (pull request) should be ready to fulfill.
(P.S. Add one-pager if available)
Refactor VGA timing module to add position tracking and improve reset handling. Also handled changes listed in comments
JayR-360
dismissed
Meowcaroni’s stale review
resolved items listed in comments
Added documentation for VGA timing module including parameters, interfaces, behavior, and dependencies.
| initial begin | ||
| counter = 0; | ||
| end | ||
|
|
||
| always@ (posedge clk_i) begin | ||
| if (counter == 2'd2) begin | ||
| pixel_clk <= ~pixel_clk; | ||
| counter <= 0; | ||
| end | ||
| else begin | ||
| counter <= counter + 1; | ||
| end | ||
| end |
There was a problem hiding this comment.
Functionality Issue: For the counter, initial blocks usually are not synthesizable and reset signals are preferred. For the internal clock pixel_clk, you need to have an initial value or simulations won't have a definite signal (1 or 0) to show it as. Please use a reset signal instead as follows:
always @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni) begin
pixel_clk <= 0;
counter <= 0;
end
// Rest of logic goes here
end
| always@ (posedge pixel_clk) begin | ||
| if (rst_ni) begin | ||
| x <= 0; | ||
| y <= 0; | ||
| active_video <= 0; | ||
| end |
There was a problem hiding this comment.
Functionality Issue: To match other subsystems, please treat rst_ni as an asynchronous (independent of clock) reset (see previous comment for example).
Meowcaroni
left a comment
Meowcaroni
left a comment
There was a problem hiding this comment.
Looking good so far, just need to fix some basic functionality issues.
No description provided.