vunit icon indicating copy to clipboard operation
vunit copied to clipboard

Rework codec

Open dalex78 opened this issue 3 years ago • 4 comments

Linked with issue #818

The proposed modifications on the codec package are retro-compatible. The tb_codec.vhd has not been modified to underline this. The tb of the com package tb_com_codec.vhd has not been touched either.

The tests compile and pass for data_type and com package with Modelsim 2020.1. I will try with GHDL soon.

Here is a list of changes (non-exhaustive):

  • Add comments into the code which explains the purpose of this package and how each type is encoded.
  • Removed hard coded value in the code
  • Replace the type 'string' used to encode by an alias named 'code_t' to ease readability and maintainbility.
  • Inverse the function and alias names to ease readability, maintainbility and ease ctrl+F:
    function encode(constant data : std_ulogic_vector) return string;
    function decode(constant code : string) return std_ulogic_vector;
    alias encode_std_ulogic_vector is encode[std_ulogic_vector return string];
    alias decode_std_ulogic_vector is decode[string return std_ulogic_vector];
    
    becomes
    function encode_std_ulogic_vector(data : std_ulogic_vector) return string;
    function decode_std_ulogic_vector(code : string) return std_ulogic_vector;
    alias encode is encode_std_ulogic_vector[std_ulogic_vector return string];
    alias decode is decode_std_ulogic_vector[string return std_ulogic_vector];
    
  • The code length for integer automatically adapts to the width of the integer (1993/2002/2008: 32bits, 2019: 64bits). (Not tested with VHDL-2019).
  • Added clearly which function should be used by a VUnit user vs a VUnit advanced user vs a VUnit developer.
  • Functions encode_array_header and get_range are replaced (these names are not consistent with the other functions):
    • function encode_range(range_left : integer; range_right : integer; is_ascending : boolean) return code_vec_t;
    • function decode_range(code : code_vec_t) return range_t;
    • encode_array_header and get_range are deprecated but preserved for backward compatibility.
    • encode_array_header and get_range take the encoded string of range_left, range_right and is_ascending while encode_range and decode_range directly take integers and a boolean.
    • Note that encode_array_header can encode two ranges but it was never used for that purpose (anywhere in the VUnit repo). The encode_range can only encode a single range.
  • Replace the ieee.numeric_std.unsigned by ieee.numeric_std.unresolved_unsigned
  • Replace the ieee.numeric_std.signed by ieee.numeric_std.unresolved_signed
  • Simplify the local log2 function used to encode the real type by using the ieee.math_real package. The same idea could be applied to string, but I did not see the use case so it was not implemented.
  • For each encoded type which is a scalar, an enumerated type or a record type, there is a function named function code_length_**type_name** return natural which indicate how many characters are needed to encode a value of the selected type.
  • For each encoded type which is an array type, there is a function named function code_length_**type_name**(length : natural) return natural; which indicate how many character are needed to encode a value of the selected type.
  • The type std_ulogic_array is similar to the std_ulogic_vector but with an integer range. However, it was not the case for bit_vector. There is now a type bit_array for that purpose.
  • The function to_byte_array and from_byte_array are replaced (these names are not consistent with the other functions and the introduction of bit_array required a change of bit_vector to bit_array):
    • function encode_raw_bit_array(data : bit_array) return code_t;
    • function decode_raw_bit_array(code : code_t) return bit_array;
    • to_byte_array and from_byte_array are deprecated but preserved for backward compatibility.
  • Create a new package common_pkg which contains constants/fonction used by the codec package but not specific to it like:
    -- Retrieve the integer width of the simulator
    function get_simulator_integer_width return positive;
    
  • Added a page in the online documentation which shows the declaration package of the codec.vhd and codec-2008p.vhd files.
  • Updated the python code to generate the encode/decode functions for user-specified type.

dalex78 avatar Apr 28 '22 22:04 dalex78

@std-max pointed that unresolved_unsigned does not exists in VHDL-93. I lauched test only with VHDL 2008. I will run some more tests in VHDL 93 and correct this (and eventually other mistakes of the same nature)

dalex78 avatar Apr 29 '22 10:04 dalex78

I have lauched all tests inside the vunit/vhdl sub-directories with Modelsim 2020.1 and GHDL 3.0.0-dev (v2.0.0-101-g791ff0c1). The tests have been perfomed with VHDL-93 (when possible) and VHDL-2008. All tests pass in all configurations (GHDL vs Modelsim and 93 vs 2008).

Note: I made slight modifications because GHDL raises an error when casting a null range of type bit_array(-1 downto 0) to bit_vector. Error: 'bound check failure' Example:

  procedure decode_bit_vector(constant code : in code_t; variable index : inout code_index_t; variable result : out bit_vector) is
    variable ret_val : bit_array(result'range);
  begin
    decode_bit_array(code, index, ret_val);
    result := bit_vector(ret_val); -- ERROR 'bound check failure' with GDHL but not with modelsim
  end procedure;

This code has been transformed into:

  procedure decode_bit_vector(constant code : in code_t; variable index : inout code_index_t; variable result : out bit_vector) is
    variable ret_val : bit_array(result'range);
  begin
    if ret_val'length /= 0 then -- GHDL work-around
      decode_bit_array(code, index, ret_val);
      result := bit_vector(ret_val);
    else
      result := "";
    end if;
  end procedure;

LRM: VHDL-2008: '5.2.1 General'

A constraint is compatible with a subtype if each bound of the range belongs to the subtype or if the range constraint defines a null range. Otherwise, the range constraint is not compatible with the subtype.

So I guess this is a GHDL mistake. I will link the corresponding issue from the GHDL repo when created.

dalex78 avatar Apr 30 '22 18:04 dalex78

@LarsAsplund ? @umarcor ?

dalex78 avatar May 07 '22 19:05 dalex78

After 8b8c2db I can run all tests in ModelSim but GHDL fails compilation of codec_builder without error message. Riviera-PRO and Active-HDL compile (after changing the non-positive code_t range and adding the in mode) but they fail simulation with an internal error which I can't decrypt. I will have to do some bisecting of the changes to find the root cause. I guess you can't get hold of Riviera-PRO and Active-HDL but I recommend testing with free GHDL

The commits 4e7fd60bd76cf3255aec8417c5649ffb61298f9a and 7515a1f52f1ed4577a385b2bc07cf720d7dd7ee5 update the sources so that it compiles with GHDL (GHDL 3.0.0-dev (v2.0.0-101-g791ff0c1)). There is a comment about that earlier: https://github.com/VUnit/vunit/pull/822#issuecomment-1114030144.

I tested with Modelsim the commit 8b8c2db0ad43a33f4113063a1ab0278543f88787 but no other tool.

And I tested with Modelsim and GHDL starting commit e2de513df289ab8c79a7271a48b6a55bae893525 (see comment https://github.com/VUnit/vunit/pull/822#issuecomment-1114030144).

dalex78 avatar Jul 20 '22 06:07 dalex78