Search code examples
verilogfpgasystem-verilogxilinxvga

My verilog VGA driver causes the screen to flicker (Basys2)


I'm trying to recreate Adventure(1979) in Verilog and so far I have character movement, collision and map generation done. It didn't flicker that much before I separated the maps into modules now it flickers constantly. When I was looking up this issue, I found out that the clock on the Basys2 board is pretty noisy and could be the culprit. However, putting the maps into modules shouldn't have made it worse unless I messed something up. Any idea what happened?

Here's my map generator:

module map_generator(clk_vga, reset, CurrentX, CurrentY, HBlank, VBlank, playerPosX, playerPosY, mapData
);

  input clk_vga;
  input reset;
  input [9:0]CurrentX;
  input [8:0]CurrentY;
  input HBlank;
  input VBlank;
  input [9:0]playerPosX;
  input [8:0]playerPosY;

  output [7:0]mapData;

  reg [7:0]mColor;
  reg [5:0]currentMap = 0;

  wire [7:0]startCastle;

  StartCastle StartCastle(
    .clk_vga(clk_vga),
    .CurrentX(CurrentX),
    .CurrentY(CurrentY),
    .mapData(startCastle)
  );

  always @(posedge clk_vga) begin
    if(reset)begin
      currentMap <= 0;
    end
  end

  always @(posedge clk_vga) begin
    if(HBlank || VBlank) begin
      mColor <= 0;
    end
    else begin
      if(currentMap == 4'b0000) begin
        mColor[7:0] <= startCastle[7:0];
      end
      //Add more maps later
    end
  end

  assign mapData[7:0] = mColor[7:0];

endmodule

Here's the startCastle:

module StartCastle(clk_vga, CurrentX, CurrentY, active, mapData);

  input clk_vga;
  input [9:0]CurrentX;
  input [8:0]CurrentY;
  input active;

  output [7:0]mapData;

  reg [7:0]mColor;

  always @(posedge clk_vga) begin

    if(CurrentY < 40) begin
      mColor[7:0] <= 8'b11100000;
    end
    else if(CurrentX < 40) begin
      mColor[7:0] <= 8'b11100000;
    end
    else if(~(CurrentX < 600)) begin
      mColor[7:0] <= 8'b11100000;
    end
    else if((~(CurrentY < 440) && (CurrentX < 260)) || (~(CurrentY < 440) && ~(CurrentX < 380))) begin
      mColor[7:0] <= 8'b11100000;
    end else
      mColor[7:0] <= 8'b00011100;       
  end

  assign mapData = mColor;
endmodule

Here's the VGA driver which is connected to my top module:

module vga_driver(clk_50MHz, vs_vga, hs_vga, RED, GREEN, BLUE, HBLANK, VBLANK, CURX, CURY, COLOR, CLK_DATA, RESET);

  input clk_50MHz;
  output vs_vga;
  output hs_vga;
  output [2:0] RED;
  output [2:0] GREEN;
  output [1:0] BLUE;
  output HBLANK;
  output VBLANK;

  reg VS = 0;
  reg HS = 0;

  input RESET;

  //current client data
  input [7:0] COLOR;
  output CLK_DATA;
  output [9:0] CURX;
  output [8:0] CURY;

  //##### Module constants (http://tinyvga.com/vga-timing/640x480@60Hz)
  parameter HDisplayArea = 640;  // horizontal display area
  parameter HLimit = 800;        // maximum horizontal amount (limit)
  parameter HFrontPorch = 16;    // h. front porch
  parameter HBackPorch = 48;         // h. back porch
  parameter HSyncWidth = 96;         // h. pulse width

  parameter VDisplayArea = 480;  // vertical display area
  parameter VLimit = 525;        // maximum vertical amount (limit)
  parameter VFrontPorch = 10;    // v. front porch
  parameter VBackPorch = 33;         // v. back porch
  parameter VSyncWidth = 2;      // v. pulse width 

  //##### Local variables
  wire clk_25MHz;

  reg [9:0] CurHPos = 0; //maximum of HLimit (2^10 - 1 = 1023)
  reg [9:0] CurVPos = 0; //maximum of VLimit
  reg HBlank_reg, VBlank_reg, Blank = 0;

  reg [9:0] CurrentX = 0;    //maximum of HDisplayArea
  reg [8:0] CurrentY = 0;    //maximum of VDisplayArea (2^9 - 1 = 511)

  //##### Submodule declaration
  clock_divider clk_div(.clk_in(clk_50MHz), .clk_out(clk_25MHz));

  //shifts the clock by half a period (negates it)
  //see timing diagrams for a better understanding of the reason for this
  clock_shift clk_shift(.clk_in(clk_25MHz), .clk_out(CLK_DATA));

  //simulate the vertical and horizontal positions
  always @(posedge clk_25MHz) begin
    if(CurHPos < HLimit-1) begin
      CurHPos <= CurHPos + 1;
    end
    else begin
      CurHPos <= 0;

      if(CurVPos < VLimit-1)
        CurVPos <= CurVPos + 1;
      else
        CurVPos <= 0;
    end
    if(RESET) begin
      CurHPos <= 0;
      CurVPos <= 0;
    end
  end

  //##### VGA Logic (http://tinyvga.com/vga-timing/640x480@60Hz)

  //HSync logic
  always @(posedge clk_25MHz)
    if((CurHPos < HSyncWidth) && ~RESET)
      HS <= 1;
    else
      HS <= 0;

  //VSync logic     
  always @(posedge clk_25MHz)
    if((CurVPos < VSyncWidth) && ~RESET)
      VS <= 1;
    else
      VS <= 0;

//Horizontal logic      
  always @(posedge clk_25MHz) 
    if((CurHPos >= HSyncWidth + HFrontPorch) && (CurHPos < HSyncWidth + HFrontPorch + HDisplayArea) || RESET)
      HBlank_reg <= 0;
    else
      HBlank_reg <= 1;

  //Vertical logic
  always @(posedge clk_25MHz)
    if((CurVPos >= VSyncWidth + VFrontPorch) && (CurVPos < VSyncWidth + VFrontPorch + VDisplayArea) || RESET)
      VBlank_reg <= 0;
    else
      VBlank_reg <= 1;

  //Do not output any color information when we are in the vertical
  //or horizontal blanking areas. Set a boolean to keep track of this.
  always @(posedge clk_25MHz)
    if((HBlank_reg || VBlank_reg) && ~RESET)
      Blank <= 1;
    else
      Blank <= 0;

  //Keep track of the current "real" X position. This is the actual current X
  //pixel location abstracted away from all the timing details
  always @(posedge clk_25MHz)
    if(HBlank_reg && ~RESET)
      CurrentX <= 0;
    else
      CurrentX <= CurHPos - HSyncWidth - HFrontPorch;

  //Keep track of the current "real" Y position. This is the actual current Y
  //pixel location abstracted away from all the timing details
  always @(posedge clk_25MHz) 
    if(VBlank_reg && ~RESET)
      CurrentY <= 0;
    else
      CurrentY <= CurVPos - VSyncWidth - VFrontPorch;

  assign CURX = CurrentX;
  assign CURY = CurrentY;
  assign VBLANK = VBlank_reg;
  assign HBLANK = HBlank_reg;
  assign hs_vga = HS;
  assign vs_vga = VS;

  //Respects VGA Blanking areas
  assign RED = (Blank) ? 3'b000 : COLOR[7:5];
  assign GREEN = (Blank) ? 3'b000 : COLOR[4:2];
  assign BLUE = (Blank) ? 2'b00 : COLOR[1:0];
endmodule

clk_div:

module clock_divider(clk_in, clk_out);
  input clk_in;
  output clk_out;

  reg clk_out = 0;

  always @(posedge clk_in)
    clk_out <= ~clk_out;

endmodule

clk_shift:

module clock_shift(clk_in, clk_out);
  input clk_in;
  output clk_out;

  assign clk_out = ~clk_in;
endmodule

Solution

  • I'm posting this as an answer because I cannot put a photo in a comment.

    Is this what your design looks like? VGA output from the OP design

    My only guess ATM is that you might have misplaced some ports during instantiation of vga_driver and/or map_generator (if you used the old style instantiation). Nevertheless, I'm going to check VGA timmings, as I can see a strange vertical line at the left of the screen, as if the hblank interval was visible.

    By the way: I've changed the way you generate the display. You use regs for HS, VS, etc, which get updated the next clock cycle. I treat display generation as a FSM, so outputs come from combinational blocks triggered by certain values (or range of values) from the counters. Besides, I start horizontal and vertical counters so position (0,0) measured in pixel coordinates in the screen actually maps to values (0,0) from horizontal and vertical counters, so no arithmetic needed.

    This is my version of VGA display generation:

    module videosyncs (
       input wire clk,
    
       input wire [2:0] rin,
       input wire [2:0] gin,
       input wire [1:0] bin,
    
       output reg [2:0] rout,
       output reg [2:0] gout,
       output reg [1:0] bout,
    
       output reg hs,
       output reg vs,
    
       output wire [10:0] hc,
       output wire [10:0] vc
       );
    
       /* http://www.abramovbenjamin.net/calc.html */
    
       // VGA 640x480@60Hz,25MHz
       parameter htotal = 800;
       parameter vtotal = 524;
       parameter hactive = 640;
       parameter vactive = 480;
       parameter hfrontporch = 16;
       parameter hsyncpulse = 96;
       parameter vfrontporch = 11;
       parameter vsyncpulse = 2;
       parameter hsyncpolarity = 0;
       parameter vsyncpolarity = 0;
    
       reg [10:0] hcont = 0;
       reg [10:0] vcont = 0;
       reg active_area;
    
        assign hc = hcont;
        assign vc = vcont;
    
       always @(posedge clk) begin
          if (hcont == htotal-1) begin
             hcont <= 0;
             if (vcont == vtotal-1) begin
                vcont <= 0;
             end
             else begin
                vcont <= vcont + 1;
             end
          end
          else begin
             hcont <= hcont + 1;
          end
       end
    
       always @* begin
          if (hcont>=0 && hcont<hactive && vcont>=0 && vcont<vactive)
             active_area = 1'b1;
          else
             active_area = 1'b0;
          if (hcont>=(hactive+hfrontporch) && hcont<(hactive+hfrontporch+hsyncpulse))
             hs = hsyncpolarity;
          else
             hs = ~hsyncpolarity;
          if (vcont>=(vactive+vfrontporch) && vcont<(vactive+vfrontporch+vsyncpulse))
             vs = vsyncpolarity;
          else
             vs = ~vsyncpolarity;
        end
    
       always @* begin
          if (active_area) begin
             gout = gin;
             rout = rin;
             bout = bin;
          end
          else begin
             gout = 3'h00;
             rout = 3'h00;
             bout = 2'h00;
          end
       end
    endmodule   
    

    Which is instantiated by your vga_driver module, which becomes nothing but a wrapper for this module:

    module vga_driver (
      input wire clk_25MHz,
      output wire vs_vga,
      output wire hs_vga,
      output wire [2:0] RED,
      output wire [2:0] GREEN,
      output wire [1:0] BLUE,
      output wire HBLANK,
      output wire VBLANK,
      output [9:0] CURX,
      output [8:0] CURY,
      input [7:0] COLOR,
      input wire RESET
      );
    
      assign HBLANK = 0;
      assign VBLANK = 0;
    
      videosyncs syncgen (
         .clk(clk_25MHz),
         .rin(COLOR[7:5]),
         .gin(COLOR[4:2]),
         .bin(COLOR[1:0]),
    
         .rout(RED),
         .gout(GREEN),
         .bout(BLUE),
    
         .hs(hs_vga),
         .vs(vs_vga),
    
         .hc(CURX),
         .vc(CURY)
       );
    endmodule
    

    Note that in map_generator, the first if statement in this always block will never be true. We can forget about it, as the VGA display module will blank RGB outputs when needed.

      always @(posedge clk_vga) begin
        if(HBlank || VBlank) begin //
          mColor <= 0;             // Never reached
        end                        //
        else begin                 //
          if(currentMap == 4'b0000) begin
            mColor[7:0] <= startCastle[7:0];
          end
          //Add more maps later
        end
      end
    

    Using the same approach, I've converted the map generator module to be a combinational module. For example, for map 0 (the castle -without the castle, I see-) it is like this:

    module StartCastle(
      input wire [9:0] CurrentX,
      input wire [8:0] CurrentY,
      output wire [7:0] mapData
      );
    
      reg [7:0] mColor;
      assign mapData = mColor;
    
      always @* begin
        if(CurrentY < 40) begin
          mColor[7:0] <= 8'b11100000;
        end
        else if(CurrentX < 40) begin
          mColor[7:0] <= 8'b11100000;
        end
        else if(~(CurrentX < 600)) begin
          mColor[7:0] <= 8'b11100000;
        end
        else if((~(CurrentY < 440) && (CurrentX < 260)) || (~(CurrentY < 440) && ~(CurrentX < 380))) begin
          mColor[7:0] <= 8'b11100000;
        end else
          mColor[7:0] <= 8'b00011100;       
      end
    endmodule
    

    Just a FSM whose output is the colour that goes in a pixel. The input being the coordinates of the current pixel.

    So when it is time to display map 0, map_generator simply switches to it based upon the current value of currentMap

    module map_generator (
      input wire clk,
      input wire reset,
      input wire [9:0]CurrentX,
      input wire [8:0]CurrentY,
      input wire HBlank,
      input wire VBlank,
      input wire [9:0]playerPosX,
      input wire [8:0]playerPosY,
      output wire [7:0]mapData
      );
    
      reg [7:0] mColor;
      assign mapData = mColor;
    
      reg [5:0]currentMap = 0;
    
      wire [7:0] castle_map;
      StartCastle StartCastle(
        .CurrentX(CurrentX),
        .CurrentY(CurrentY),
        .mapData(castle_map)
      );
    
      always @(posedge clk) begin
        if(reset) begin
          currentMap <= 0;
        end
      end
    
      always @* begin
        if(currentMap == 6'b000000) begin
          mColor = castle_map;
        end
          //Add more maps later
      end
    endmodule
    

    This may look like a lot of comb logic is generated and so glitches may happen. It's actually very fast, no noticeable glitches on screen, and you can use the actual current x and y coordinates to choose what to display on screen. Thus, no need for an inverted clock. My final version of your design has only one 25MHz clock.

    By the way, you want to keep device dependent constructions away from your design, placing things like clock generators in separate modules that will be connected to your design in the top module, which should be the only device dependent module.

    So, I've written a device-agnostic adventure module, which will contain the entire game:

    module adventure (
      input clk_vga,
      input reset,
      output vs_vga,
      output hs_vga,
      output [2:0] RED,
      output [2:0] GREEN,
      output [1:0] BLUE
      );
    
      wire HBLANK, VBLANK;
      wire [7:0] COLOR;
      wire [9:0] CURX;
      wire [8:0] CURY;
      wire [9:0] playerPosX = 10'd320;  // no actually used in the design yet
      wire [8:0] playerPosY = 9'd240;   // no actually used in the design yet
    
      vga_driver the_screen (.clk_25MHz(clk_vga), 
                             .vs_vga(vs_vga), 
                             .hs_vga(hs_vga), 
                             .RED(RED), 
                             .GREEN(GREEN), 
                             .BLUE(BLUE), 
                             .HBLANK(HBLANK), 
                             .VBLANK(VBLANK), 
                             .CURX(CURX), 
                             .CURY(CURY), 
                             .COLOR(COLOR)
                             );
      map_generator the_mapper (.clk(clk_vga), 
                                .reset(reset), 
                                .CurrentX(CURX), 
                                .CurrentY(CURY), 
                                .HBlank(HBLANK), 
                                .VBlank(VBLANK), 
                                .playerPosX(playerPosX), 
                                .playerPosY(playerPosY), 
                                .mapData(COLOR)
                                );
    endmodule
    

    This module is not complete: it lacks inputs from joystick or any other input device to update player current position. For now, player current position is fixed.

    The top level design (TLD) is written exclusively for the FPGA trainer you have. It is here where you need to generate proper clocks using your device's available resources, such as the DCM in Spartan 3/3E devices.

    module tld_basys(
      input wire clk_50MHz,
      input wire RESET,
      output wire vs_vga,
      output wire hs_vga,
      output wire [2:0] RED,
      output wire [2:0] GREEN,
      output wire [1:0] BLUE
      );
    
      wire clk_25MHz;  
    
      dcm_clocks gen_vga_clock (
                                .CLKIN_IN(clk_50MHz), 
                                .CLKDV_OUT(clk_25MHz)
                               );
    
      adventure the_game (.clk_vga(clk_25MHz), 
                          .reset(RESET), 
                          .vs_vga(vs_vga), 
                          .hs_vga(hs_vga), 
                          .RED(RED), 
                          .GREEN(GREEN), 
                          .BLUE(BLUE)
                          );
    endmodule
    

    The DCM generated clocks goes in this module (generated by the Xilinx Core Generator)

    module dcm_clocks (CLKIN_IN, 
                 CLKDV_OUT
                 );
    
       input CLKIN_IN;
       output CLKDV_OUT;
    
       wire CLKFB_IN;
       wire CLKFX_BUF;
       wire CLKDV_BUF;
       wire CLKIN_IBUFG;
       wire CLK0_BUF;
       wire GND_BIT;
    
       assign GND_BIT = 0;
       BUFG  CLKDV_BUFG_INST (.I(CLKDV_BUF), 
                             .O(CLKDV_OUT));                         
       IBUFG  CLKIN_IBUFG_INST (.I(CLKIN_IN), 
                               .O(CLKIN_IBUFG));
       BUFG  CLK0_BUFG_INST (.I(CLK0_BUF), 
                            .O(CLKFB_IN));
       DCM_SP #(.CLKDV_DIVIDE(2.0), .CLKIN_DIVIDE_BY_2("FALSE"), 
             .CLKIN_PERIOD(20.000), .CLKOUT_PHASE_SHIFT("NONE"), 
             .DESKEW_ADJUST("SYSTEM_SYNCHRONOUS"), .DFS_FREQUENCY_MODE("LOW"), 
             .DLL_FREQUENCY_MODE("LOW"), .DUTY_CYCLE_CORRECTION("TRUE"), 
             .FACTORY_JF(16'hC080), .PHASE_SHIFT(0), .STARTUP_WAIT("FALSE") ) 
             DCM_SP_INST (.CLKFB(CLKFB_IN), 
                           .CLKIN(CLKIN_IBUFG), 
                           .DSSEN(GND_BIT), 
                           .PSCLK(GND_BIT), 
                           .PSEN(GND_BIT), 
                           .PSINCDEC(GND_BIT), 
                           .RST(GND_BIT), 
                           .CLKDV(CLKDV_BUF), 
                           .CLKFX(), 
                           .CLKFX180(), 
                           .CLK0(CLK0_BUF), 
                           .CLK2X(), 
                           .CLK2X180(), 
                           .CLK90(), 
                           .CLK180(), 
                           .CLK270(), 
                           .LOCKED(), 
                           .PSDONE(), 
                           .STATUS());
    endmodule
    

    Although it is safe (for Xilinx devices) to use a simple clock divider as you did. If you fear that the synthesizer won't treat your divided clock as an actual clock, add a BUFG primitive to route the output from the divider to a global buffer so it can be used as a clock with no problems (see the module above for an example on how to do this).

    As a final note, you may want to add more independency from the final device, by using 24-bit colours for your graphics. At the TLD, you will use the actual number of bits per colour component you really have, but if you move from the Basys2 with 8-bit colour trainer board to the, say, Nexys4 board, with 12-bit colour, you will automatically enjoy a richer output display.

    Now, it looks like this (no vertical bars at the left, and colours seem to be more vibrant)

    Another output from Basys using a different VGA display module