Simple Verilog example for a LED Switch?

Question

I'm trying to build a StateMachine for a 1-hot-encoded simple LED switch button.

Especially I'm trying to understand blocking and nonblocking assignments with my example.

Would you thing the following could be done better, or is completely wrong in any block?

module example (
  input clk,
  input rst,
  input push,

  output reg led_on
);


reg on;
reg off;

reg t_on_off;
reg t_off_on;


always @* begin
  t_on_off = on & (push);
end

always @* begin
  t_off_on = off & (push);
end


always @(posedge clk or posedge rst) begin
  if (rst)              on <= 1'b0;
  else if (t_off_on)    on <= 1'b1;
  else if (t_on_off)    on <= 1'b0;
end

always @(posedge clk or posedge rst) begin
  if (rst)              off <= 1'b1;
  else if (t_off_on)    off <= 1'b0;
  else if (t_on_off)    off <= 1'b1;
end


always @* begin
  led_on = on;
end


endmodule

Especially I'd like to know: could I combine the assignments of the transitions into one single block, like:

always @* begin
  t_on_off = on & (push);
  t_off_on = off & (push);
end

?

Answer 1

If it does not need to be one-hot, then simplify it to:

module example (
  input clk,
  input rst,
  input push,

  output reg led_on
);

always @(posedge clk or posedge rst) begin
  if (rst)        led_on  <= 1'b0;
  else if (push)  led_on  <= !led_on;
end

endmodule

It is functional equivalent to what you have and more readable.

Answer 2

Especially I'd like to know: could I combine the assignments of the transitions into one single block, like...

Yes, you can do that exactly as you have described.

You can also combine the sequential blocks as well if you want:

always @(posedge clk or posedge rst) begin
  if (rst) begin
     on  <= 1'b0;
     off <= 1'b1;
  end else if (t_off_on) begin
     on  <= 1'b1;
     off <= 1'b0;
  end 
  (etc....)
end

Answer 3

Yes, you may combine multiple always blocks into one.

You just need to separate out your synchronous (clocked ones) and asynchronous blocks into separate always blocks.

However, a good style is to have an always block for each individual output. This is easier to read and more real-world like as each always block is concurrent with each other.

Answer 4

Refactoring Suggestion:

output reg led_on;

always @* begin
  led_on = on;
end

to:

output led_on; //wire by default (not declared reg)

assign led_on = on;

You could also do the same with your t_on_off and t_off_on

wire t_on_off;
wire t_off_on;

assign t_on_off = on  & (push);
assign t_off_on = off & (push);

Or if you prefer roll the declare and assign in one line.

wire t_on_off = on  & (push);
wire t_off_on = off & (push);

But if you are rolling the two clocked always blocks into one there is no need for the separate logic, combining @Tim's answer with the t_on_off check:

module example (
  input clk,
  input rst,
  input push,

  output reg led_on
);

reg on;
reg off;

assign  led_on = on;

always @(posedge clk or posedge rst) begin
  if (rst) begin
    on  <= 1'b0;
    off <= 1'b1;
  end
  else if (off & push) begin
    on  <= 1'b1;
    off <= 1'b0;
  end
  else if (on  & push) begin
    on  <= 1'b0;
    off <= 1'b1;
  end
end

endmodule

Answer 5

This has been far in the past already, but the solutions presented are perhaps not exactly what you were expecting. From what I could infer the solutions all consider that the LED will keep toggling as long as the pushbutton is being pressed (i.e. it will toggle at the clock's frequency), making it visually imperceptible if the clock frequency is high. However, I assume you would want something which toggles the LED only once each time the pushbutton is pressed, with the LED state being preserved during this period.

The example below toggles the states of 3 LEDs based on the activity of 3 pushbuttons.

1. led0 is activated whenever pbutton0 is pressed.
2. led1 keeps toggling periodically (based on the size of clk_div) and is reset whenever pbutton1 is pressed.
3. led2 is toggled whenever pbutton2 is pressed.

Note that led0 is combinational, while the other two LEDs are sequential. For toggling led2, the previous state of pbutton2 must be stored; whenever pbutton2(t-1)==0 and pbutton2(t)==1, that means the button has just gone from low to high and, thus, the state of led2 must change.

Lastly, please ignore the clock source, since this was only used to demo the code on the Xilinx SP605 development kit.

////////////////////////////////////////////////////
// This project uses 3 pushbuttons and 3 LEDs.
//  pbutton0 activates led0
//  pbutton1 serves as reset for led1 periodic toggling
//  pbutton2 toggles led2
//
// The clock source (divider+buffer) was created using the clocking IP wizard.
//

module xilinx_sp605_board_leds
(
    input CLK_IN1_P,
    input CLK_IN1_N,

    input pbutton0,
    input pbutton1,
    input pbutton2,

    output led0,
    output reg led1,
    output reg led2
);

// declarations
wire clk;
wire reset = pbutton1;
reg [19:0] clk_div;
reg pbutton2_reg;

// led0 = state of pbutton0
assign led0 = pbutton0;

// differential clock divider+buffer
clk_source CLK_SOURCE (
    .CLK_IN1_P(CLK_IN1_P),
    .CLK_IN1_N(CLK_IN1_N),
    .CLK_OUT1(clk),
    .RESET(reset));

// led1, led2 toggling
always @(posedge reset or posedge clk)
begin
    if(reset)begin
        clk_div <= 0;
        led1 <= 0;

        pbutton2_reg <= 0;
        led2 <= 0;
    end else begin
        clk_div <= clk_div + 1;
        if(clk_div==0) 
            led1 <= ~led1;

        pbutton2_reg <= pbutton2;
        if(~pbutton2_reg && pbutton2)
            led2 <= ~led2;
    end
end

endmodule