Re: [myhdl-list] Some comments on the VHDL code that MyHDL generates

[Angel E. <ang...@gm...>]

Actually I already fixed problems #1 (type definition indentation), #2
(signal declaration indentation) and #4.1 (location of the entity
level documentation). They were surprisingly easy to fix! :-)

I got 3 patches ready which I'll send in a minute to the list.

Cheers,

Angel


On Wed, Oct 10, 2012 at 9:04 AM, Angel Ezquerra
<ang...@gm...> wrote:
> In my quest to somehow start using MyHDL for my work, I've been
> playing around with it, trying to create a small module in MyHDL from
> scratch.
>
> Since Christopher Felton suggested that I may be able to generate VHDL
> in a way that my colleagues would not even notice, I created a small,
> simple module and looked into the VHDL code that MyHDL generates.
>
> My first impression is that the code is in fact quite similar to what
> I would have written, which is nice. However I have a few comments:
>
> 1. "type" definitions are not properly indented. For example:
>     The following MyHDL:
>
> ~~~
> t_State = enum('INIT', 'WORKING')
> ~~~
>
> becomes the following VHDL:
>
> ~~~
> package pck_sync_module is
>
>     type t_enum_t_State_1 is (
>     INIT,
>     WORKING
> );
>
> end package pck_sync_module;
> ~~~
>
> Here you can see that the lines below the type keyword itself
> (starting with "INIT") are not properly indented.
> Instead it should be (IMHO):
>
> ~~~
> package pck_sync_module is
>
>     type t_enum_t_State_1 is (
>         INIT,
>         WORKING
>     );
> ~~~
>
> 2. I prefer to have the declarations between the "architecture" header
> and the begin keyword to be indented. I also prefer to indent the
> architecture contents (everything between begin/end). This is nothing
> major, and obviously open to debate, but I think it looks clearer. For
> example, I'd prefer:
>
> ~~~
> architecture MyHDL of sync_module is
>
>     signal s_2pps_counter: unsigned(2 downto 0);
>     signal pulse_sync: std_logic;
>
> begin
> ~~~
>
> rather than the current:
>
> ~~~
> architecture MyHDL of sync_module is
>
> signal s_2pps_counter: unsigned(2 downto 0);
> signal pulse_sync: std_logic;
>
> begin
> ~~~
>
> 3. For some reason MyHDL added 3 empty lines between the architecture
> "begin" and the first line of code in it:
>
> ~~~
> architecture MyHDL of sync_module is
>
> signal s_2pps_counter: unsigned(2 downto 0);
> signal pulse_sync: std_logic;
> signal first_pulse: std_logic;
> signal pulse_state: t_enum_t_State_1;
>
> begin
>
>
>
> -- Main process
> SYNC_MODULE_P_PULSE: process (clk, rst) is
>     variable pulse_state3: t_enum_t_State_1;
> begin
>     if (rst = '1') then
> ~~~
>
> See the 3 blank lines betwen begin and the first comment. I would
> think that one single blank would be enough.
>
> 4. I tried to use docstrings to add comments to the generated VHDL, so
> that if someone where to inspect it it would be easier to understand
> without needing to go to my source MyHDL code. This works in most
> cases but there are a few things that could be improved (IMHO) and
> some things that I was just not able to do:
>
>     4.1. If I add a docstring to a function that contains an @always
> block, MyHDL generates a corresponding "top entity". I expected to see
> the docstring above the entity declaration. Instead MyHDL places the
> comment between the entity declaration and the architecture
> declaration, which I think it is a bit weird. I'd rather see the
> documentation as close to the top of the file as possible (i.e. right
> above the entity keyword, so that I can document it).
>
>     4.2. I am unable to use a docstring to comment an enum declaration
> (or any other signal declaration for that matter). This seems to be
> due to the fact that this is done outside of the instance generators.
> I think this is important, particularly for enums which are used for
> state machines. Maybe all MyHDL objects (signals, intbv, etc) could
> have a "comment" or "doc" field that you could set to include some
> info about them on the generated VHDL / Verilog code?
>
>     4.3. Is does not seem possible to add a "top level" file comment
> via a docstring. I guess this is not very important right now since
> there is not a one to one mapping between MyHDL .py files and the
> generated .vhd file.
>
> Please note that this is in no way a criticism of MyHDL. I know that
> the main goal of MyHDL is not to generate human readable VHDL code.
> However I think these small details could improve the generated code
> substantially. Items 1 to 3 could be fixed on the user's end by
> running the generated code through an automated VHDL indenter tool but
> unfortunately I don't know of any good, free indenter that is also
> easy to automate (perhaps emacs in batch mode?).
>
> I have some more comments which are not related to the actual code
> that MyHDL generates but I'll leave that for another email.
>
> Cheers,
>
> Angel