It means somebody hasn't thought much about how to write their state machines.
This isn't a loop, it is one state in a state machine. There are presumably many states that do similar things, with minor variations, and it is too easy to lose track of the details.
In every clock cycle, a state machine executes the default actions (things you do in every cycle) and the code for the specific state it is in now. So it has to keep track of its state, and, if necessary, switch to another state.
So a typical single-process state machine looks like
process(reset,clk) is -- and nothing else in the sensitivity list!
begin
if reset = '1' then
state <= Idle;
elsif rising_edge(clk) then
-- default actions
ID_START <= '0';
if Counter /= 0 then
Counter <= Counter - 1;
end if;
-- state machine proper
case state is
when WAIT_ST =>
-- state actions
...
when others =>
state <= Idle;
end case;
end if;
end process;
Hopefully you can find this basic structure and each of its components in yours. (There are also 2-process and 3-process state machines which bring additional problems and no real advantage, I'll ignore them).
Several tools to improve a state machine:
(1) Use default actions.
Here, I'm assuming your ID_START
signal is set to '0' in every state except one, probably labelled START_ST
. If that's the case, you can add a default action, ID_START <= '0';
as I have done above. Then you can override this action in the one (or few) state(s) that need any other value, as
when START_ST =>
ID_START <= '1';
and you can delete all the other ID_START <= '0';
assignments in other states. Slightly less clutter...
However, if ID_START
is set in one state, cleared in one other, and is unassigned (holds its value) in all other states, then you can't do this. So use judgment, as always, when refactoring.
(2) Use named constants rather than magic numbers.
What register is address X"01", and what does X"00" do to it?
CPU_CAN_DATA <= NOP;
CPU_CAN_ADDR <= Control_Reg;
(3) Use records to group associated signals.
(You can put them in a package and re-use it anywhere you need it)
type CPU_CAN is record
WE : std_logic;
DATA : std_logic_vector(7 downto 0);
ADDR : unsigned(7 downto 0)
end record;
signal CPU_CAN_BUS : CPU_CAN;
NB addresses are usually natural numbers, hence unsigned. Data can be anything, not necessarily a number, hence std_logic_vector.
And now you can assign related signals in a single statement:
CPU_CAN_BUS <= ( WE=> '0', ADDR => Ctrl_Reg, DATA => NOP);
You can probably do the same for CAN_CS, can_init_start, can_read_start.
(4) Use procedures and functions to streamline repetitive operations.
(5) Use a programmable delay counter.
If WAIT_ST is the only state that needs a delay counter, the current approach is fine. However if you need several delays, they can share a counter - which counts as one of the default actions above. Because it counts down, you only need to compare it to 0, (saving a little hardware) and it's programmed when you jump INTO a state like WAIT_ST
if something then
Counter <= WAIT_FOR;
state_n <= WAIT_ST;
end if;
(6) stop before you make things more complicated! i.e. if mux_sel
doesn't logically fit any of the above, leave it alone...
The end result of these changes is something like
when WAIT_ST =>
-- CAN_CS, can_init_start, can_read_start, ID_START are defaults
mux_sel <= "00";
CPU_CAN_BUS <= ( WE=> '0', ADDR => Ctrl_Reg, DATA => NOP);
INST <= Frame_Type & Sub_Frame_Type;
if Counter = 0 then then
state_n <= IDLE_ST;
end if; -- else we stay in WAIT_ST
Better?
case
andprocess
statement. Could you please post these single lines too. \$\endgroup\$