MiSTer-devel / SMS_MiSTer

Sega Master System for MiSTer
45 stars 43 forks source link

Update edge detection method from VHDL 87 to VHDL 93 #131

Closed birdybro closed 2 years ago

birdybro commented 2 years ago

Changes the edge detection method from VHDL 1987's standard to VHDL 1993's standard in order to secure the core from potential undefined behavior.

birdybro commented 2 years ago

Technically, from what I've been reading, clk'event and clk = '1' can result in non-0 triggers (e.g. 1, U, H, X, Z, W, etc...) incorrectly initiating the if-then statement, whereas rising_edge(clk) does not initiate unless the trigger is explicitly 1 (https://vhdlwhiz.com/clkevent-vs-rising_edge/). This is because all of the non-0 triggers are contained within clk'event. The potential for undefined behavior is probably hypothetical but exists nonetheless. This change will also make it so simulators/testbenches hypothetically behave the same as the synthesized implementation (all else being equal).

If this method was used on purpose for some reason, feel free to close the PR. I didn't see anything that relied upon this behavior, but I do lack the significant experience to analyze the code completely.

sorgelig commented 2 years ago

both writings are exactly the same and don't cause any misinterpretations. VHDL supports 3 different ways for clock waiting: if (clk'event and clk = '1') if rising_edge(clk) wait until rising_edge(clk)

birdybro commented 2 years ago

Even though it's probably only relevant in simulation, rising_edge(clk) is equivalent to clk'event and clk'last_value = ‘0’ and clk = ‘1’ and not the equivalent of clk'event and clk = ‘1’.

This is because rising_edge's function is described as:

FUNCTION rising_edge  (SIGNAL s : std_ulogic) RETURN BOOLEAN IS
BEGIN
    RETURN (s'EVENT AND (To_X01(s) = '1') AND
                        (To_X01(s'LAST_VALUE) = '0'));
END;

FUNCTION To_X01  ( s : std_ulogic ) RETURN  X01 IS
BEGIN
    RETURN (cvt_to_x01(s));
END;

CONSTANT cvt_to_x01 : logic_x01_table := (
                     'X',  -- 'U'
                     'X',  -- 'X'
                     '0',  -- '0'
                     '1',  -- '1'
                     'X',  -- 'Z'
                     'X',  -- 'W'
                     '0',  -- 'L'
                     '1',  -- 'H'
                     'X'   -- '-'
                    );

clk'event and clk = 1 doesn't check for the last value the way rising_edge does (and falling_edge is the inverse). At least from what I'm reading this seems to be the case. Not trying to argue though, just excited to learn and sharing what I'm learning.

sorgelig commented 2 years ago

It's not relevant to clock network. Clock doesn't have other than 0 and 1 values. But generally speaking i like rising_edge() version. Easier to read.

birdybro commented 2 years ago

Me too :D

It makes more sense too.