vhdl manual clock hour set

Question

I am trying to make an alarm clock for a final project in one of my classes. I am using push buttons on a DE1 Altera board to manually increment hours and mins. The mins work but I can not get the hours to increment manually. All pin assignments are correct.

library ieee;
use ieee.std_logic_1164.all;
use ieee.std_logic_arith.all;
use ieee.std_logic_unsigned.all;

entity ClkMain is port (
    clk,pb_hr,pb_min,clk_set,almr_enbl: in std_logic;
    almr_hr: in integer range 0 to 23;
    almr_min: in integer range 0 to 59;
    clk_min : out integer range 0 to 59;
    clk_hr : out integer range 0 to 23;
    almr_indct : out bit
);
end ClkMain;

architecture Behavioral of ClkMain is
    signal sec, min: integer range 0 to 60 :=0;
    signal hr: integer range 0 to 24 := 0;

begin
    clk_min <= min;
    clk_hr <= hr;

    process(clk)   --normal clock operation
    begin

        if(clk'event and clk='1') then
            sec <= sec + 1;

            if(sec + 1 = 60 or (pb_min = '1' and clk_set = '1') ) then
                sec <= 0;
                min <= min + 1;

                if (min + 1 = almr_min and hr = almr_hr and almr_enbl = '1') then
                    almr_indct <= '1';
                else
                    almr_indct <= '0';
                end if;

                if(min + 1 = 60 ) then
                    hr <= hr + 1;
                    min <= 0;

                    if(hr + 1 = 24) then
                        hr <= 0;

                        if (clk'event and clk='1' and pb_hr = '1' and clk_set = '1')then
                            hr <= hr + 1;
                        end if;

                    end if;
                end if;
            end if;
        end if;

    end process;
end Behavioral;

Answer 1

You can see where the error is by indenting properly:

library ieee;
use ieee.std_logic_1164.all;
-- use ieee.std_logic_arith.all;
-- use ieee.std_logic_unsigned.all;

entity ClkMain is
    port (
        clk,pb_hr,pb_min,clk_set,almr_enbl: in std_logic;
        almr_hr: in integer range 0 to 23;
        almr_min: in integer range 0 to 59;
        clk_min: out integer range 0 to 59;
        clk_hr: out integer range 0 to 23;
        almr_indct : out bit
     );
end ClkMain;

architecture Behavioral of ClkMain is
    signal sec, min: integer range 0 to 60 :=0;
    signal hr: integer range 0 to 24 := 0;

begin
    clk_min <= min;
    clk_hr <= hr;

    process(clk)   --normal clock operation
    begin

        if clk'event and clk = '1'  then
            sec <= sec + 1;

            if sec + 1 = 60 or (pb_min = '1' and clk_set = '1') then
                sec <= 0;
                min <= min + 1;

                if min + 1 = almr_min and hr = almr_hr and almr_enbl = '1' then
                        almr_indct <= '1';
                    else
                        almr_indct <= '0';
                end if;

                if min + 1 = 60  then
                    hr <= hr + 1;
                    min <= 0;

                    if hr + 1 = 24 then
                        hr <= 0;

                        if clk'event and clk = '1' and pb_hr = '1' and clk_set = '1' then
                            hr <= hr + 1;

                        end if; 
                    end if; 
                end if; 
            end if; 
        end if;
    end process;
end Behavioral;

The clk condition is enclosed in the outermost if statement and isn't necessary:

if clk'event and clk = '1' and pb_hr = '1' and clk_set = '1' then

Should be

if pb_hr = '1' and clk_set = '1' then

And that brings us to what's wrong. pb_hr is only evaluated at 11 PM:

                if hr + 1 = 24 then
                    hr <= 0;

                    if pb_hr = '1' and clk_set = '1' then
                        hr <= hr + 1;

                    end if; 
                end if; 

At a minimum these two if statements need to be at the same nesting level.

Unfortunately it also makes you take a look up the if statement nesting levels where you notice you can only set hours at 23:59:59, or you're also holding down pb_min and clk_set is true.

Also notice you almr_indct is true for a minute no matter what you do. I'd suggest moving the sets and alarm detection outside the enclosing if statement with the clock condition (keep them in the same process). It should also be invalidated when clk_set is true.

Looking even further back:

        if sec + 1 = 60 or (pb_min = '1' and clk_set = '1') then
            sec <= 0;
            min <= min + 1;

We see you could reach 60 for a button push. That all needs to be fixed. It's also possible to move the alarm comparison outside of the counters and disable during clock set.

So you could manipulate the process statement:

architecture foo of clkmain is
    signal sec, min: integer range 0 to 59 := 0;
    signal hr: integer range 0 to 23 := 0;
    signal sec_neq_59:       std_logic;
    signal min_neq_59:       std_logic;
    signal hr_neq_23:        std_logic;

begin

    clk_min <= min;
    clk_hr <= hr;

    sec_neq_59 <= '0' when sec = 59 else '1';
    min_neq_59 <= '0' when min = 59 else '1';
    hr_neq_23  <= '0' when  hr = 23 else '1';

CLOCK_PROCESS:
    process(clk)
    begin
        if clk'event and clk = '1'  then
ALARM_INDICATON:
            if min = almr_min and hr = almr_hr and almr_enbl = '1' then
                almr_indct <= to_bit(not clk_set);
            else
                almr_indct <= '0';
            end if;
SET_MINUTES:
            if pb_min = '1' and clk_set = '1' then
                if min_neq_59 = '1' then
                    min <= min + 1;
                else
                    min <= 0;
                end if;
SET_HOURS:
            elsif pb_hr = '1' and clk_set = '1' then
                if hr_neq_23 = '1' then
                    hr <= hr + 1;
                else 
                    hr <= 0;
                end if;
INCREMENT_SECONDS:
            elsif sec_neq_59 = '1' then
                sec <= sec + 1;
            else                -- sec = 59
                sec <= 0;
INCREMENT_MINUTES:
                if min_neq_59 = '1' then
                    min <= min + 1;
                else               -- :59:59
                    min <= 0;
INCREMENT_HOURS:
                    if hr_neq_23 = '1' then 
                        hr <= hr + 1;
                    else           -- 23:59:59
                        hr <= 0;
                    end if;
                end if;
            end if; 
        end if;
    end process;
end architecture foo;

And with the opportunity I fixed the range for the sec, min and hr counters. The secret is evaluating before incrementing, you intercept a terminal count with a synchronous load.

Also switched to equality comparisons to specific values, separated them to reduce hardware by having one set and prioritized the push buttons over the clock operation by using elsif.

So now push buttons can't cause range errors in minutes and hours, and are independent of actual clock time.

I don't think it's valid to reset seconds when incrementing minutes with the push button. It might be valid to keep seconds at 0 while clock_set is true, which would stop the clock from running when being set. That doesn't work if you're only fixing daylight savings time or changing time zones, though.

I haven't simulated this. It analyzes and elaborates. Range errors in assignment would show up during simulation.

I left almr_indct as type bit, but did use clk_set as a condition for the alarm indication.