Incrementing an internal signal

Question

VHDL coding problem :( Hello! I've been working on this problem for awhile. I have a feeling it's a beginning problem that I don't quite understand.

--I'm accessing internal memory, 4 rows of 2 bit numbers each. I've been able to read and write to memory just fine, my problem is incrementing the address at which I will store the next data set.

--My unit is controlled by an FSM with three states. Idle, reed and rite. I have three internal signals, addressin: a pointer to the address to be read next, addressout: a pointer to the address to be written next and addressall, the address which will go into the actual memory stage.

    PROCESS (y)
    BEGIN
    CASE y IS
    WHEN I=>
    enable<='0';

    WHEN reed=>
    enable<='0';
    IF (addressin="00" OR addressin="01" OR addressin="10") THEN

        addressin<=addressin+"01";
    ELSE
        addressin<="00";
    END IF;
    addressall<=addressin;

    WHEN rite=>
    enable<='1';
    IF (addressout="00" OR addressout="01" OR addressout="10") THEN
        addressout<=addressout+1;
    ELSE
        addressout<="00";
    END IF;
    addressall<=addressout;

    END CASE;
END PROCESS;    

memorystage: memory PORT MAP (clck, NOT reset, NOT enable, addressall, datain, dataout);    

The process will activate when y changes (the states changing is in code not seen.) My problem is, the addressin will change, the address out will change, and so will the addressall... But never by one, and never in any sequential logic... (I have this in hex display to see) For example I'll get for addressin : 3 3 1 0 3 0 3 0 2 0 2 1 0.... same with the other two address signals. I have no idea what I'm doing wrong. This is only part of a larger project, so I pulled this out to work on it by itself. :( what am I doing wrong? Thanks. -Jenn

Answer 1

I think you've fallen prey to VHDL's (very well defined, but often confusing to novices) signal update rules. See also this question.. When do signals get assigned in VHDL?

A signal is only updated at the end of a process (or when a wait happens, but we'll leave that option for now!)

---

IF (addressin="00" OR addressin="01" OR addressin="10") THEN
    addressin<=addressin+"01";
ELSE
    addressin<="00";
END IF;
addressall<=addressin;

addressall will get the previous value of addressin, not the value you've just set it to as that hasn't yet been updated.

To get the behaviour you want, change addressin to a variable within the process (and you'll have to change all the <= assignments to := assignments). Variables work the way you want them to - updates are applied immediately.

I almost always use variables for things within a process and leave signals for communication with other processes.

Answer 2

Not sure if you can use a synchronous design but I threw one together. This design will increment through your address pointer (addressin, addressout) as quickly as the clock updates if you are in the reed or rite state.

PROCESS (y, clk) BEGIN
    IF rising_edge(clk) then
        enable<='0';
        CASE y IS
            WHEN I=>

            WHEN reed=>
                IF(addressin ="11") then
                    addressin <= "00";
                ELSE
                    addressin <= addressin + 1;
                END IF;

                addressall<=addressin;

            WHEN rite=>
                enable<='1';

                IF(addressout ="11") then
                    addressout <= "00";
                ELSE
                    addressout <= addressin + 1;
                END IF;

                addressall<=addressout;
        END CASE;
    END IF;
END PROCESS;    

memorystage: memory PORT MAP (clck, NOT reset, NOT enable, addressall, datain, dataout); 

If you only want it to update once per state change you could do something like

prev_state : PROCESS (y,clk) BEGIN
    IF rising_edge(clk) then
        ybuff <= y;
    END IF;
END PROCESS;

PROCESS (y, clk) BEGIN
    IF rising_edge(clk) then
        IF (ybuff /= y) then
            enable <= '0';
            CASE y IS 
                ....
                ....

If you want something else you need to give us more of your code. This is a bit hacked (as I dont have your state logic or update signals)

I agree with Martin that your current updating of addressall is not the correct method. Like he said you could use variables to fix this or if you really want to keep it, you should put it in a separate process block that updates addressall to make the oddity more explicit and controlled. Right now (and with the single updater I posted second) you will always be one address behind your pointers (and one full cycle behind your state machine)

Answer 3

A few additional points to consider:

1) There is an inconsistency in how the in/out addresses are incremented: addressin<=addressin+"01"; and addressout<=addressout+1;. The behavior of these assignments will depend on the signal type of the operands, which is why that information is helpful, though this may not be a real problem.

2) It should be relatively safe to assume that any changes on the address signals are directly related to switching on y, since that is the only signal in this process' sensitivity list. Have you inspected the value of y alongside the addresses in a waveform viewer?

3) Paul suggested this but I just want to clarify why you may want this circuit to be "clocked" (synchronous). Without a clock in the sensitivity list, this process is only sensitive to changes on y. Therefore, any memory elements implied by this process will be completely ignorant of the clock and will only be affected by changes on input y. In other words, to advance the read pointer, y would have to toggle between the idle and reed states. You will have to decide if this is intended or if the logic should advance the read pointer every clock cycle that reed state is present on y. It may be non-traditional for a memory element to not receive a designated clock because clocks receive special treatment to simplify static timing analysis, but it's not prohibited by the language (can't promise the same for downstream tools).

In general, it sounds like you have a good idea what you're trying to accomplish and grasp some of VHDL's nuances, so the next step is to further isolate the unexpected behavior. More code would help rule out some potential problems, but perhaps the best way to simultaneously test your assumptions and provide a reproducible example is to create a testbench that illustrates the unexpected behavior.

For example, here is a testbench to exercise a clocked copy of your process.

Library ieee;
Use ieee.std_logic_1164.all;
Use ieee.numeric_std.all;

Entity test Is
End Entity;

Architecture main of test Is
    Signal enable, clock : std_logic := '0';
    Signal addressin, addressout, addressall : unsigned(0 to 1) := "00";
    Type op_t is (reed, idle, rite);
    Signal y : op_t; 
    Signal done : boolean;
Begin
    clocks: process
    begin
        if not done then
            clock <= not clock;
            wait for 0.5 ns;
        else
            wait;
        end if;
    end process;

    PROCESS (clock, y)
    BEGIN
    if rising_edge(clock) then
    CASE y IS
        WHEN idle=>
            enable<='0';

        WHEN reed=>
            enable<='0';
            IF (addressin="00" OR addressin="01" OR addressin="10") THEN
                addressin<=addressin+1;
            ELSE
                addressin<="00";
            END IF;
            addressall<=addressin;

        WHEN rite=>
            enable<='1';
            IF (addressout="00" OR addressout="01" OR addressout="10") THEN
                addressout<=addressout+1;
            ELSE
                addressout<="00";
            END IF;
            addressall<=addressout;

    END CASE;
    end if;
    END PROCESS;

    main: Process
    Begin
        y <= idle;
        wait for 1 ns;
        y <= reed;
        wait for 1 ns;
        assert addressall = "00";
        wait for 1 ns;
        assert addressall = "01";
        wait for 1 ns;
        assert addressall = "10";
        wait for 1 ns;
        assert addressall = "11";
        y <= idle;
        wait for 1 ns;
        done <= true;
        wait;
    End Process;

End Architecture;