Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Naming Methodology #10

Open
JimLewis opened this issue Jul 4, 2020 · 35 comments
Open

Naming Methodology #10

JimLewis opened this issue Jul 4, 2020 · 35 comments
Labels
Discussion Enhancement New feature or request

Comments

@JimLewis
Copy link

JimLewis commented Jul 4, 2020

Hi Patrick,
I see lots of code. However, I think we might want to agree on naming methodology as it would be nice if all were the same.

I would like something that is obvious - even to people who don't have the secret decoder ring for the naming convention. The conventions that use T_ or _T are a reflection of a generation of people who could not type.

Going further mode views are new. When people see V_ they are going to thing variable.

Best Regards,
Jim

@Paebbels
Copy link
Member

Paebbels commented Jul 4, 2020

The proposal is using

  • Uses UpperCammelCase
  • T_... for types
  • V_... for views
  • ..._Vector for arrays
  • ..._In if the main data is flowing inwards
  • ..._Out if the main data is flowing outwards
  • Abbreviated letters are in full-uppercase
  • PCB related interfaces are called ..._PCB, so types without this extension are fabric internal signals
    (the majority of types will be of this class)
  • All records are declared with their matching array type.
  • Aliases are defined, if a type has a second very popular name.

Unfortunately, VHDL is not very clever when it comes to names. Everything resides in a single scope, therefore we need to distinguish types from objects and views.

@JimLewis
Copy link
Author

JimLewis commented Jul 4, 2020

I propose that instead of

  • T_ ... that we use _Interface or Interface for interface types
  • T_... that we use something that is understood naturally without a secret decoder ring. I have used Type as a suffix, but am open.
  • V_... that we use _View or View
  • For ..._In also consider In as suffix (w/o _). "In" follows main data flow
    • Similar thoughts for ...Out
    • Like: I2C_InView vs I2CViewIn vs I2CView_In
    • No Like: I2CInView vs I2C_In_View vs I2C_View_In
  • For each interface there shall be a vector of that interface also declared of the form: _Interface_Vector
  • _PCB vs FPGA Internals
  • View naming for AddressBus which historically was Master/Initiator and Slave/Target
    • AxiAddress should be unsigned.
    • _SlaveView is not going to fly in the US (and it will make my wife angry with me)
    • Master is probably ok since it is used in so many different contexts.

Paebbels added a commit that referenced this issue Jul 4, 2020
@Paebbels
Copy link
Member

Paebbels commented Jul 4, 2020

@JimLewis please review #12 and accept if you agree.

@Paebbels Paebbels added the Enhancement New feature or request label Jul 4, 2020
@Paebbels Paebbels mentioned this issue Jul 4, 2020
65 tasks
@Paebbels Paebbels added this to the Prepare first release milestone Jul 5, 2020
@LarsAsplund
Copy link

@Paebbels @JimLewis I think simplicity is important considering that there are many people that will have to adapt to a style they are not used to. Looking at the code we have identifiers like DifferentialLane_Interface, DifferentialLane_TransmitterView and DifferentialLane_Interface_Vector. Where to use camel case and where to use underscore becomes complex. Is there a significant value in that complexity? Why not just underscores or just camel case?

@JimLewis
Copy link
Author

JimLewis commented Jul 5, 2020

@LarsAsplund I understand your concern. Here the '_' provides visual separation of the name vs. the modifier(s). DifferentialLate is the base name. _Interface designates that it is an interface. _TransmitterView designates that it is a view for the transmitter. _Interface_Vector designates that it is an array of interfaces.

OTOH, if we were to write, Differential_Lane_Transmitter_View then one does not readily see the name DiferentialLane from the modifiers. One does not see that it is a single modifier TransmitterView vs multiple separate modifiers like _Interface_Vector.

Likewise the same could be said if we were to write it as, DifferentialLaneTransmitterView.

@JimLewis
Copy link
Author

JimLewis commented Jul 5, 2020

@Paebbels
Looks like all of the files have tabs in them. The tabs render in GitHub as 8 spaces.

Can we propose to not use tabs and have the editors convert tabs on entry to
spaces instead? This is particularly important when copying entities and
making them into components as suddenly the tab stops are not aligned
anymore. In addition to make a component instance, I like to copy the
component or entity and edit from there. It really helps with column
cuts to have only spaces as the alignment then never shifts.

@JimLewis
Copy link
Author

JimLewis commented Jul 5, 2020

I propose that for packages, we use some sort of naming that differentiates their names,

  • Such as Axi4LitePkg or Axi4Lite_Pkg . I prefer it without the _ as that is what OSVVM does.

Can we alias design unit names - such as packages to have a new name?
This would help transition from an older naming style to a newer one.
If not, we need to add it as part of the language revision.

@LarsAsplund
Copy link

@JimLewis I understand the idea but is the value of being clear about the modifiers really worth the extra complexity? If there is a significance in terms of understanding what is meant it would mean that the identifier cannot be understood if you read it for me since they all sound alike. Doesn't that suggest that there is a problem with the fundamental naming?

I think the fact that you're suggesting that it should be _Interface but not _Pkg is a good example of the type of confusion complex rules leads to ;-)

@LarsAsplund
Copy link

Considering that interface is part of the base name for AXI, SPI and others I think it should be that for the differential interface as well. The record should have a type suffix like any other type identifier.

@Paebbels
Copy link
Member

Paebbels commented Jul 5, 2020

@JimLewis using

  • tabs for indentation (one per level), and
  • space for alignment

is intended and correct.

It's problem of GitHub not applying the rules of .editorconfig. The repository has a matching editor configuration files that is nowadays understood by almost all editors either directly or via plugin.

See: https://github.com/VHDL/Interfaces/blob/dev/.editorconfig#L3-L10

[*]
charset = utf-8
# end_of_line = lf
insert_final_newline = true
trim_trailing_whitespace = true
indent_style = tab
indent_size = 2
tab_width = 2

@JimLewis
Copy link
Author

JimLewis commented Jul 5, 2020

@Paebbels
And then when if you print - even to pdf it is very bad.

You always have tools that do not understand your edit rules.

@JimLewis
Copy link
Author

JimLewis commented Jul 5, 2020

File naming?
Can I propose that we have the file name match the design unit name. IE: no - characters.

For the file extension, are we doing .vhd or .vhdl?

@LarsAsplund
Copy link

Another simplification is to not treat abbreviations differently. Then there is no discussion about why it's AXI and not PKG.

@Paebbels
Copy link
Member

Paebbels commented Jul 5, 2020

To answer some of the questions above:

Modifiers

Yes, I agree with Jim, we need a more clear naming and readability then a SingleLongCamelCaseWord or a chained_long_snake_case_word.

This collection of interfaces will grow and get lots of interface variations. Of cause we will try the best to minimize it by using partially constrained/unconstrained types, but we'll have variation. For each interface we'll then have even more views. So we need a naming scheme that can handle it. The naming scheme must also allo users to create names in a similar style, ever if this repository is not providing these names.

Package names

Package names are not in the same namespace as types and views. I see no need to apply such a suffix to design units too.

File extension

  • *.vhd = virtual hard disk / virtual hard drive
  • *.vhdl = VHDL

Abbreviations

There are two possible styles for abbreviations:

  • all uppercase: XML, I2C, AXI, OSVVM
  • First upper, rest lower case: Xml, I2c, Axi, Osvvm

Dashes in filenames

The official name is AXI4-Lite. Hence the file name in my proposed upload.

Suggestions: We can remove dashes and merge words.

@LarsAsplund
Copy link

@Paebbels Using tabs for indentation and space for alignment is also something I consider more complex than necessary. As you know I've done some statistical analysis of GitHub and a majority, about 75% if I recall correctly, use spaces instead of tabs and a majority uses 2 spaces for every indentation level. For VHDL files that is. Regardless of the style chosen the majority of users will have to adapt to some extent. We want to keep that to a minimum, that is go with the majority. There can of course be cases where the majority is wrong about something which has significant value. The value in naming conventions is primarily readability and the only study I read on indentation (let's see if I can find it) concludes that there is no difference in readability if your indentation steps are in the 2 - 4 range but it's worse outside of that range. Interestingly people in the study felt that 5 was good although the data showed it was not. The idea with tabs is that people can set it to whatever they prefer. The study suggests that that preference may be factually wrong. Any step in the 2 - 4 range should be equally readable and if the majority uses 2 I think we should fix it to that by using spaces.

@LarsAsplund
Copy link

@Paebbels

  • Can you give some examples when naming gets less readable because of not using underscores (or just using underscores).
  • I use .vhd but either one is fine.
  • I usually use _pkg to avoid name collision with an entity. This case is different so I'm ok with excluding it. Nevertheless, I don't think it should be _Interface since its part of the base name if present at all. JTAG for example doesn't need "interface" to know that it is an interface.
  • There are more variants of abbreviations. The most common would be all lower case clk. Then we have those mixing upper of lower case like FoV (field of view). What I suggest is that abbreviations are treated as words and use the same style as other words. So axi4_lite or Axi4Lite

@LarsAsplund
Copy link

I think I found the indentation study: https://www.cs.umd.edu/~ben/papers/Miara1983Program.pdf
However, a later study trying to replicate the first but with modern eye-tracking devices (https://www.infosun.fim.uni-passau.de/publications/docs/Bauer19.pdf) didn't reach the same conclusion. Instead they concluded that the indentation depth has no significance which suggest that we can use anything as long as we're consistent (or we will have a lot of file diffs only related to spacing). If it doesn't matter I would still suggest taking the route of least resistance, that is use what most people use. 2 spaces.

@tmeissner
Copy link
Member

tmeissner commented Jul 5, 2020

I have to ask what the intention of these interfaces is. Is it for testing the VHDL-19 features in the various simulators / tools? Or should they serve as examples for using interfaces in RTL designs for all users of VHDL-19?

Honestly, with these naming conventions and other style enforcements, I wouldn't never use these interfaces as they are in projects in our company. But this would be almost the same with other rules, so maybe that doesn't matter.

@LarsAsplund
Copy link

@tmeissner The intention is that you would use them in your company code rather than write them yourself. Just like you would use types in ieee, vunit_lib, OSVVM and other external libs which may or may not share your company style. If the style presented here is a show stopper for that then tell us more.

@tmeissner
Copy link
Member

tmeissner commented Jul 5, 2020

No show stopper, but sometimes difficult. We use suffixes and prefixes for example.
An excerpt of our style:

  • 2 spaces everywhere instead of tabs
  • Suffixes for port directions (_i, _o, _io)
  • Prefix for type definitions (t_)
  • Prefix for (shared variables) (v_, sv_)
  • Signals all lowercase with _ as only extra char
  • Entities, packages in camel case with suffix (P for package, E for entity)
  • *.vhd as file extensions - we prefer 3-char file extensions for reasons, honestly, nobody I know uses *.vhdl

We also use code with other styles, no real problem.

I heard the first time of .editorconfig, and I assume that only few people know that feature either. We use sublime text and have user defined configs in that editor. There are even people which use the editors of the vendor tools and I don't know if you can use that settings in these editors either ...

@eine
Copy link

eine commented Jul 5, 2020

Regarding vhdl vs vhd, see: https://ghdl.readthedocs.io/en/latest/quick_start/hello/README.html?highlight=extension#hello-world-program

Both .vhdl and .vhd extensions are used for VHDL source files, while .v is used for Verilog.

Since, extension .vhd is also interpreted as a Virtual Hard Disk file format, some users prefer .vhdl, to avoid ambiguity. This is the case with GHDL’s codebase. However, in order to maintain backward-compatibility with legacy DOS systems, other users prefer .vhd.

@LarsAsplund
Copy link

@tmeissner The more rules the more painful to adapt but even with few rules most people would probably have to adapt to some extent. I like the thinking of new languages where the coding style is part of the LRM. Kill the discussion from day one. The VHDL standard is a mix of all possible coding styles so I think that ship has sailed. I think the best we can do from the standard side is to make type casting between types where only naming differs very simple. @Paebbels Didn't you just started a discussion on that?

The proposed interfaces are really just a bunch of named wires so if most people were to rename those wires to match their own style or use some clever type casting mechanism then much of the value in this repo is lost.

@eine
Copy link

eine commented Jul 6, 2020

I think the best we can do from the standard side is to make type casting between types where only naming differs very simple. @Paebbels Didn't you just started a discussion on that?

http://grouper.ieee.org/groups/1076/email/msg01278.html

The proposed interfaces are really just a bunch of named wires so if most people were to rename those wires to match their own style or use some clever type casting mechanism then much of the value in this repo is lost.

I believe that many people are going to rename those wires, and it is still ok. I think that the main purpose of this repository is to showcase Interfaces, which is an unknown feature yet, but which is a game changer for the traditional "VHDL is so verbose" mantra. I'm quite sure that we all want tools to support interfaces ASAP.

Apart from that, it serves as a common definition for user A and user B to know how their customized interfaces match each other. This is quite stupid for simple interfaces, but it can be complex with composed interfaces, such as AXI. Following Patrick and Jim's discussion about matching/aliasing/spaceshipping types, if each user maps/binds custom interfaces to the common definitions here, any number of different styles can be mixed, without mapping each pair explicitly.

@LarsAsplund
Copy link

I think that the main purpose of this repository is to showcase Interfaces, which is an unknown feature yet, but which is a game changer for the traditional "VHDL is so verbose" mantra. I'm quite sure that we all want tools to support interfaces ASAP.

If that's the purpose we should rethink what we're doing and develop just a few examples

Apart from that, it serves as a common definition for user A and user B to know how their customized interfaces match each other. This is quite stupid for simple interfaces, but it can be complex with composed interfaces, such as AXI. Following Patrick and Jim's discussion about matching/aliasing/spaceshipping types, if each user maps/binds custom interfaces to the common definitions here, any number of different styles can be mixed, without mapping each pair explicitly.

Matching/aliasing/spaceshipping would save the day. Could it be that there is no point in publishing all these interfaces before we have this in place?

That brings me to another question. What are the limits on how quickly a new LRM can be released? Are there limits on how small the updates can be. Basically, what is the agility of IEEE if we push it to its extreme?

@LarsAsplund
Copy link

This is roughly how I think about code style development

code style

Typical problems are:

  • No scientific studies exists or the results are inconclusive. The indentation depth rule is an example of that
  • People can have strong opposite preferences about rules without value which means that there is a value in creating a rule to avoid the discussion when code is reviewed

@JimLewis
Copy link
Author

JimLewis commented Jul 6, 2020

That brings me to another question. What are the limits on how quickly a new LRM can be released?
Are there limits on how small the updates can be. Basically, what is the agility of IEEE if we push it to its extreme?

You have to get a PAR. DASC has to approve the PAR (2 weeks). Then NESCOM needs to approve the PAR. NESCOM meets the quarterly IEEE SA meetings. PARS must be submitted well before that date (I think 30 days. When the standards revision work is done we solicit for the the ballot pool - which is 30 to 45 days. Ballot pool cannot be open for more than a maximum time or it automatically dissolves. Then ballot, which I think is another 30 days. Potential reballoting because someone did not like your terminology. Submit for RevCom approval which synchronizes with the IEEE SA quarterly meetings and must be submitted well in advance of the date.

NESCOM and REVCOM reviews start before the meeting date and the meeting is just a formal approval. They also let you correct small things as part of the process. So I look at it as a coaching session to make sure things are right - or are right by the time the process is done.

This time when I asked about getting a PAR, DASC asked what we wanted to work on. So for each revision we would need to justify what we are doing.

@JimLewis
Copy link
Author

JimLewis commented Jul 6, 2020

@LarsAsplund

I think the best we can do from the standard side is to make type casting between types where only naming differs very simple. @Paebbels Didn't you just started a discussion on that?

Deja. Already done in VHDL-2019. See: http://www.eda-twiki.org/cgi-bin/view.cgi/P1076/LCS2016_075

@JimLewis
Copy link
Author

JimLewis commented Jul 6, 2020

@eine @LarsAsplund
That said, to do the VHDL-2019 supported type conversion of records, the order of the elements in the records must be identical. There is no mapping between one and the other. Hence, if someone does not want to use the same naming style, if they use this as a template and don't change the order, then they can use type conversion (aka type casting).

@eine
Copy link

eine commented Jul 6, 2020

If that's the purpose we should rethink what we're doing and develop just a few examples

Apart from that, whatever exists in this repo will be really valuable for new users (those who don't have other styles they need to match/reuse) to use as-is. This applies to examples of OSVVM or VUnit verification components which use these definitions. Although those are not part of this repo per se, they are part of the showcase. This builds on your concerns in the reflector about what should be LRM, what IEEE libs and what community libs.

@JimLewis what about mapping, for example, the following?

	type SimpleVGA_Interface is record
		Red            : unresolved_unsigned;
		Green          : unresolved_unsigned;
		Blue           : unresolved_unsigned;
		HorizontalSync : std_ulogic;
		VerticalSync   : std_ulogic;
	end record;
	type VGARGB_Interface is record
		RGB            : std_ulogic_vector(2 downto 0);
		HorizontalSync : std_ulogic;
		VerticalSync   : std_ulogic;
	end record;

@JimLewis
Copy link
Author

JimLewis commented Jul 6, 2020

@eine The language cannot help you there - even if Red, Green, and Blue are only 1 bit each (IE match in size of the RGB element). We would need something else to help here.

@JimLewis
Copy link
Author

JimLewis commented Jul 6, 2020

@eine I added your example (slightly modified) to http://www.eda-twiki.org/cgi-bin/view.cgi/P1076/MapFunctions

Feel free to add to it and/or take it over. There may be other stuff from the previous revision.

@Paebbels
Copy link
Member

The longer this discussion gets about naming style as part of coding style, we also have #27. We need so kind of easy mapping or aliasing of simple and complex wire structures in record structures.

With this repository, I would like to establish a common sense interface description as de facto standards. At best we can add official second and third names like we have with I²C, PMBus, SMBus and TWI (two wire interface). We also should correct misleading naming confusions/mistakes in standardized interfaces.

With such a mapping / aliasing, we can offer alternatives and moreover users can change an interface to there style and feelings.

@JimLewis
Copy link
Author

Some of Lars comments about mixing _ with camel case are sinking in. If we do the suffixes right, I don't think we need the _. Ie: if we define major names - like View and then allow View to have a modifier - in, out, Super, Minion

We need some more complete examples of naming in use.

I am concerned with Interface. What do we name the signal that connects to the interface? Lets say we have UartInterface, then what is the signal named. For AXI, I named my AxiBus, but for Uart that does not work. Hence that leads me to thinking we need a UartInterfaceType (the type) and UartInterface (the signal).

@LarsAsplund
Copy link

I am concerned with Interface. What do we name the signal that connects to the interface? Lets say we have UartInterface, then what is the signal named.

In my own code I called the record the interface type (foo_if_t) and the signal is the interface (foo_if). It follows the same logic I use for state types (foo_state_t) and the signal/variable holding the state (foo_state).

I would also suggest that we define the closely related transaction type that can be used to represent transactions at a higher abstraction layer. For example, the AXI stream interface type is a record containing an AXI stream transaction together with ready and valid

@JimLewis
Copy link
Author

JimLewis commented Jul 19, 2020

How close do we want to match the name that is part of a standards (or defacto standards) document?

I bring this up as I see the AXI abbreviation of Addr being lengthened to Address, Strb to Strobe, and Prot to Protect.
Whether it is Strobe or Strb, or Prot or Protect, one still has to look them up in the standard document to determine what they actually mean.

If this is not going to be part of a standards effort, I think if we deviate too far from the established names, like them or not, people will not use this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants