VUnit / vunit

VUnit is a unit testing framework for VHDL/SystemVerilog
http://vunit.github.io/
Other
723 stars 258 forks source link

Logging improvements #168

Closed suoto closed 8 years ago

suoto commented 8 years ago

I would like to check with you some improvements to the HDL logger system:

I can help with the VHDL implementation of both items but I don't have as much experience with SystemVerilog as I have with VHDL. I can work on the SystemVerilog implementation as well if required, only it will take more time.

Anyway, let me know what you think.

LarsAsplund commented 8 years ago

@suoto Haven't had time to look into this yet. Does PCK_FIO contain functions to convert a format string into a normal string or is it all integrated into their own logging functionality? If the formatting can be used in isolation it should be possible to use the tools together. Not implementing another flavor of the same functionality may be better use of open source resources.

suoto commented 8 years ago

@LarsAsplund I looked at PCK_FIO's code and it writes the formatted string to a file. It seems doable but I'll have to take a closer look to tell (mainly about differences across compilers and VHDL standards).

suoto commented 8 years ago

@LarsAsplund I (finally!) got the time to take a better look into PCK_FIO package. The library expects a file to be passed as parameter but the way I see to use its functionality without changing its code is to make it return the formatted string instead. I created a small wrapper function for this, see below:

(please not that I kept the function signature and overall style similar to PCK_FIO's fprint.)

impure function sprintf (
    Format:  in    string;
    A1 , A2 , A3 , A4 , A5 , A6 , A7 , A8 : in string := FIO_NIL;
    A9 , A10, A11, A12, A13, A14, A15, A16: in string := FIO_NIL;
    A17, A18, A19, A20, A21, A22, A23, A24: in string := FIO_NIL;
    A25, A26, A27, A28, A29, A30, A31, A32: in string := FIO_NIL
    ) return string is

    file F     : text;
    variable L : line;
begin
    -- Call pck_fio main function with no actual file
    fprint (F, L,
    Format,
    A1 , A2 , A3 , A4 , A5 , A6 , A7 , A8 ,
    A9 , A10, A11, A12, A13, A14, A15, A16,
    A17, A18, A19, A20, A21, A22, A23, A24,
    A25, A26, A27, A28, A29, A30, A31, A32);

    -- Return the formatted output as a string
    return L.all;
end function;

The examples below shows some formatted outputs.

variable foo : std_logic_vector(15 downto 0) := x"1234";
...
info(sprintf("Reasonable representation           %r", fo(foo)));
# 0 ps: INFO: Reasonable representation           0x1234
info(sprintf("Binary representation               %b", fo(foo)));
# 0 ps: INFO: Binary representation               0001001000110100
info(sprintf("Decimal representation:             %d", fo(foo)));
# 0 ps: INFO: Decimal representation:             4660
info(sprintf("String representation               %s", fo(foo)));
# 0 ps: INFO: String representation               V:0001001000110100
info(sprintf("qualified (internal) representation %q", fo(foo)));
# 0 ps: INFO: qualified (internal) representation V:0001001000110100

I have tested with GHDL 0.33 and ModelSim 10.2c. Adapting to VHDL 2008 is a matter of commenting function overloads that use std_ulogic.

I haven't found a less verbose way of using it.

Do you consider this way would be an useful improvement?

LarsAsplund commented 8 years ago

@suoto Yes, that would make PCK_FIO more open such that it can be used with our logging as well as all other logging solutions out there. I would suggest that you propose this addition to the maintainers of the project, close this issue, and then open a new one just addressing the output coloring you also suggested.

suoto commented 8 years ago

Ok, since there's no solution for PCK_FIO right now, I'll follow your suggestion and close this issue.

I have contacted PCK_FIO's maintainers to check on this but since it's GPL'ed, one could just make the changes and release under GPL while keeping copyright to the original owner. That's actually my personal choice because it would allow deeper changes. In the implementation shown in my previous comment for instance, I'm not comfortable having a file variable when there is no file to be written at all.

Either way, whenever there's an alternative I can open a new issue to see how/if VUnit could use it.

Thanks for your feedback!

nfrancque commented 5 years ago

Was any further progress made on this issue? I'd also like to use the PCK_FIO library for logging.

LarsAsplund commented 5 years ago

@nanduhumedo No, there are no news in this area. Have there been any updates to that package? I still think it would be best if we could reuse rather than reimplement. If that is not possible I guess we could consider making our own formatting package

nfrancque commented 5 years ago

I don't believe there have been any updates, but I've used a wrapper around PCK_FIO that implemented info(), debug(), and error() . Will need to get the creator's permission, but could most likely share this if you have any interest in adapting it. Tested today, it is still backwards compatible with the current log functions since by default it takes only strings.

But you bring up a good point that it would obviously be better to modify the original package.

@suoto Was there ever a reply from PCK_FIO's maintainers?

suoto commented 5 years ago

@nanduhumedo No reply, unfortunately. The original source was under GPL v1, I cloned it to https://github.com/suoto/hdl_string_format and made it GPL v2, I'm happy to maintain it. (I'm not a license expert but afaik GPL code can be forked if it remains open source, so it should be fine really).

nfrancque commented 4 years ago

Not sure why I didn't reply to this originally, found it while thinking about it again.

If it was GPL then yes I believe we can make whatever modifications we want as long as source is published. Which it has to be, because this is open source.

Usage is always fine.

Also looks like your use of GPL v2 is okay, and auto-upgrades PCK_FIO to v2.

Maybe we reopen this or start a new one for discussion?