Is this a bad style of coding?

[Alexium]

Greetings.
I have suddenly found myself writing such a code:

---

Code VHDL - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
RXLoop: process (clk, rst)
begin
    if clk'event and clk = '1' then
       RXNewPacket <= '0';                                      
        if RXDone = '1' then
            if some_condition then
                --some code
            else
                RXNewPacket <= '1';
            end if;         
        end if;
    end if;
end process;

---

So, I have this signal, RXNewPacket, that should be implemented as a flip-flop. And it looks like I'm doing two asignments to it when some_condition is false. On the one hand, by the rules of VHDL, only the last asignment will take effect (and this is what I need). But somehow it doesn't look right to me, and I tried to avoid this coding technique in the past.
Will this code work correctly with any abstract synthesizer, and are there any reasons to avoid this?
Thanks.

 

[permute]

this is generally accepted. It is generally bad practice if RXNewPacket is defined in multiple different if-else blocks. The above code is a compromise most designers will accept. it really has a strong role in combinatorial circuits where leaving the signal out of one path would result in a latch. It is common whenever there are a lot of conditions that result in the signal being set to '0', and only a few that set it to '1'.

 

[TrickyDicky]

I use it regularly in state machines, and other cases where assigning something is done rarely (so give it a default value like you have and override it later). Ive not had a problem.

From a readability point of view be careful not to embed too many nested if-else trees. It can become annoying and difficult to read, but the synthesisor should just flatten it into a single logic circuit.

 

[Alexium]

Thanks for clarifying that for me.

 

[TA37]

That's fine for a flag/pulse type signal that has a 'default' value most of the time. Particularly if it is only sparsely set to a non-default in the if/then/else tree. Which is most likely the case for a signal with a name like that.

Of course if this was being used as a state variable/bit, you don't want to assign a value by default since the whole point of state variables is to hold on to what they had last cycle. And a default "A <= A;" is unnecessary since A already keeps its old value unless there's an "A <= B;" somewhere in the if/then/else.

also, rising_edge(clk) is a little more modern than clk'event and clk='1'.

Otherwise, fairly good style. and kudos for actually labeling the clocked process.