-
Notifications
You must be signed in to change notification settings - Fork 2
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
Prepare first release candidate #5
base: release
Are you sure you want to change the base?
Conversation
…d tristate interfaces.
This file has been successfully checked by Riviera-PRO 2020.04.
This file has been successfully checked by Riviera-PRO 2020.04.
This file has been successfully checked by Riviera-PRO 2020.04.
This file has been successfully checked by Riviera-PRO 2020.04.
This will add some of the interfaces listed in #6. |
This file has been successfully checked by Riviera-PRO 2020.04.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again some very high-level comments without diving into the actual standards
alias SFP_CageView is SFP_ICView'converse; | ||
|
||
type SFP_Interface_Vector is | ||
array(natural range <>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think we should use integer ranges. I find the natural ranges of some of the standard vector types limiting at times. Can't come up with a good example for negative indices here but I don't think we should add such a restriction unless we have very good reasons for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As natural
is a subtype of integer
, the range could be extended later if a usecase requires it. Such a change is backward compatible.
|
||
-- Tristate (3-state) interface | ||
type Tristate_Interface is record | ||
I : std_logic; -- Input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a need to comment the meaning of an identifier the identifier should probably be renamed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question for the whole repository should be if we:
- follow original naming of standards, because many developers might be used to such naming.
- minimally cleaning up names to unify naming across interfaces and packages in that repository
- fully cleaning up all names
use IEEE.std_logic_1164.all; | ||
|
||
-- Signal name mappings | ||
-- SCK -> Serial Clock; alternatives: BCLK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another example when I think we can use the full name rather than the abbreviation. Assuming we accept the interface to standar mismatch (I do)
-- RX -> Receive Data (RxD) | ||
package UART is | ||
type UART_Interface is record | ||
RX : std_logic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example where I think the abbreviation is very well established/understood.
PoC/CSE.vhdl
Outdated
-- ============================================================================= | ||
-- Authors: Patrick Lehmann | ||
-- | ||
-- Generic Package: VHDL-2019 Command-Status-Error interface description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this according to some standard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a commonly used interface in the PoC library - hence the path to the package. It's used in layered designs in combination with hierarchical state machines.
(cherry picked from commit 506bb3c25320137028aebc19f141362620878842)
@LarsAsplund, @JimLewis, @lievenlemiengre, @cmarqu, @umarcor, @nickg, @BAHayhoe, @pblecua, @martinjthompson, @brimdavis, @Nic30, @NJDFan, @tgingold, @M-KraftKugler, @MortenZdk, @tmeissner, @yyde, @marlonjames, @bradleyharden There is now support for "interfaces" in Xilinx Vivado 2023.1 - so a first synthesis tool can use this stuff. I tested the code and except for some minor syntax updates, it was accepted by the tool.
|
At many places, you create an alias of the converse attribute of a mode view. I don't think this is allowed by the LRM, as you can only alias a named entity. Probably the LRM needs to be fixed. |
From LRM 6.6 Alias:
So, alias to views fall under nonobject alias. From LRM 7.2 Attribute Specification, we know that named entities can be decorated with attributes. That' allowed for:
In LRM 16.2.5 Predefined attributes of named entities, we see attribute In Glossary we find:
What would be missing to use an alias with a view returned from an attribute? |
Oh, it's not listed in 6.1 Declarations → General. |
Yes. I think mode view declarations must be added in the 6.1 list, and probably there must be a special case for 'converse.
|
I created https://gitlab.com/IEEE-P1076/VHDL-Issues/-/issues/293 to adjust the LRM. |
So what should a conforming VHDL-2019 implementation do here: error on an alias of |
@nickg I think an error is not needed, as views where added in many places like LRM 7.2 to allow such an alias. Also by writing examples and adding a Also the discussion in http://www.eda-twiki.org/cgi-bin/view.cgi/P1076/LCS2016_045c shows clearly the intentions by the the LCS authors to allow aliases. So we should consider this an LRM bug / oversight and fix it in the next revision. That's why I created an issue in the LRM repository. If your tool has a special mode / flag to allow non LRM (fully) compliant features, you could activate aliases in such a feature and keep your tool "pedantic" (that's how commercial tools call there inverse flag) until the LRM is fixed. @nickg does this answer help you and your tool? |
There is no benefit to requiring a "pedantic" flag for this. Based on the examples and referenced discussions, it is clear that the LRM committee intended that this feature be a named entity. Hence, I recommend that the LRM is interpreted as if views were included in the list of declarations in 6.1. The problem with a pedantic off flag for this is that other things may also be allowed by the pedantic off flag that are not desired. |
Isn't it a problem in the LRM that we have that bullet list in 6.1 and the bnf for entity_class. Aren't these redundant. It seems the LRM would benefit of the definition of entity_class were to replace the bullet list of 6.1 for the definition of named entity. Redundant things are hard to maintain. |
As I specified the change to |
The master branch of nickg/nvc has some preliminary support for interfaces. I'd welcome any testing/feedback. |
|
||
package Common is | ||
-- Differential signaling (DS or LVDS) | ||
type Differatial_Interface is record |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type Differatial_Interface is record | |
type Differential_Interface is record |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I'll search where I used that type elsewhere.
This will add: