Problem with PIPO Register - Verilog

Question

I'm building a PIPO register to implement on a fpga. I have written the code below, but there is something wrong with my register module I believe.

The value of Q won't save. My intention was that Q would save the value of D until enable is changed. But Q is constantly changing itself to match the value of D.

I've posted a link to what the project should look like. Thanks for the help.

https://www.youtube.com/watch?list=PLGwEmhvkv6WYhkw0QkGyyJKj_dFdhe3Jk&v=vzGEKGQRNio&feature=emb_title&ab_channel=RealDigital.org

`timescale 1ns / 1ps

module register (
input clk,
input en,
input  [7:0] D,
output reg  [7:0] Q
);
//always @ (posedge(clk),posedge(en))
always @ (en)
begin
   if (en == 1)
     Q <= 8'b00000000; 
      
    
   else
     Q <= D;
    
   
   end    
   endmodule



module mux (
input Sel,
input  [7:0] I0,
input  [7:0] I1,
output reg  [7:0] led
);

always @ (Sel,I0,I1)
begin     
    if (Sel ==  1)
     led <= I1;   
   
    else   
      led <= I0;
end
endmodule




module pipo(
//input clk,
input en,
input  [7:0] sw,
input Sel,
input  [7:0] I0,
output  [7:0] led


);

wire [7:0] sData;
register piporeg(

//.clk(clk),
.en(en),
.D(sw),
.Q(sData)
);

mux pipomux(
.Sel(Sel),
.I0(sw),
.I1(sData),
.led(led)
);



endmodule
```

Answer 1

I don't know what a PIPO is but it doesn't matter. I see a few problems.

First, a common beginner mistake: you're doing a bad job defining modules. A multiplexer is one line. If you put it in a module you need many more lines. A register is 2 lines. Also no need for it be a module. There may be times when these do need to be in their own modules but those are advanced edge cases. This can easily be one module.

Then there's your functional problem. You say the register doesn't store anything. You defined it so that if enable is high, you output 0, and if enable is low, you output the data. That's not a register, it's an AND gate (true if data and ~enable are true).

Put back the posedge statement that you commented out. But leave the enable out of it, you can't give a register two clocks.

So what's the enable for? I'm not sure. Is it a load enable for a register? Right now it looks like you're trying to use it as a reset. If you want the register to hold its value when enable is true then say so: use q <= q;. You probably want it when enable is low, not high.

Third mistake, stylistically: if (en==1'b1) is redundant. You can just say if (en). It's a bit, so it has a true or false state naturally.