Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Various VPI examples #19
base: master
Are you sure you want to change the base?
Various VPI examples #19
Changes from 5 commits
97e51da
1d6b84c
52975d3
0d5789d
f478d8f
3297c11
f30f585
7fd3508
f09c924
9efed06
a684de7
93ba510
976dcf5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 commented in #1,
TBC
at the end ofdoc/index.rst
should be replaced with one or a few notes/tips/hints about minimum knowledge to use VPI. Optionally, that content might be written indoc/vpi/index.rst
and included in the toctree.Anyway, this file should contain a
toctree
(as done indoc/vhpidirect/examples/index.rst
), so that a nice TOC is shown in the sidebar.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.
I agree that proper documentation in RST format is necessary. For now, what I can do, is to add a list of examples in rst format. The rest will come later :P
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.
I wrote a couple of paragraphs in #1 (comment), which you can copy and paste at the end of
doc/index.rst
.The examples cannot be a list. There are two options:
As long as all the files (one or multiple) are added to the toctree, the TOC in the sidebar will have the same structure regardless of the number of files.
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.
Please see the changes I did yesterday......
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.
It is really annoying when GitHub shows a subset of changes or an outdated set of changes...
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 file should be written in RST and located in subdir
doc
. The motivation to use RST is that we might want to add labels for cross-referencing these examples. The location is because we are keeping docs separate from code sources.Moreover, each example should have a header of the appropriate level. This is necessary for the TOC.
In VHPIDIRECT, examples are grouped (Quick start, Arrays, Other projects...). You might want to add all your examples to a section named "Quick Start", to use/create some other section, or to not create any section yet.
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.
I am ok with this. However, I just did a similar thing as in
./systemc/README.md
. If you think it is unnecessary I will remove that, but having a small overview of the folder is helpful for people that just want to explore the repo without directly using the documentation. See https://github.com/bellaz89/ghdl-cosim/tree/master/vpiThere 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.
Oh, the existence of
./systemc/README.md
is a kind of "artifact". Repo https://github.com/ghdl/ghdl-systemc-fosdem15 was created some weeks ago, due to a user's demand. The name was not correct, and it was renamed to https://github.com/ghdl/ghdl-systemc-fosdem16 a few days later. Then, we discussed about widening the scope to "co-simulation", and this repo was forked from there. We considered to rename the other once again, but we thought it would be too much trouble. Nonetheless, the content of the "old" repo was moved to subdir./systemc
and the README remains. The plan is to add it to the docs (as other examples), once we guess how to make it fit.Honestly, I had not thought about it, but it sounds as a good idea! From this point of view:
The docs need not to link this file, but headers should link to the corresponding subdirs.vpi/README.md
? Two (vpi/quickstart/README.md
andvpi/list/README.md
)? Or all of them?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.
Explanations need to be a little more verbose. As a rule of thumb, the first paragraph in each example explains why the example is relevant (why we decided to add it). Second and further paragraphs explain what is implemented in the example and which files/functions/features are used.
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.
See above: this is repository documentation. The full documentation in rst format will come with successive PRs
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.
Ref #19 (comment).
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 comment applies to all the examples in this repo. It would better fit either as an admonition in the main page, or in the README.
It can be a separate PR.
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.
I think this should go in the main README, along with an explanation about "groups of examples" (see #19 (comment)).
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.
Now included in #21.
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.
Please, make it parametric (add a generic to define the word-length).
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.
Sorry, but is this really helpful to make the example easy to understand from people with basic knowledge? I can change it but adding a generic just adds unnecessary information and details for the purpose of the example.
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 commented in #1 (comment), we target people with basic knowledge about using GHDL and VPI at the same time. But we expect users to know VHDL and VPI, or we provide them the references to go and learn.
Apart from that, the purpose of this examples is to learn. An important part of learning a new technology is being able to play with it. Hence, we are ok with using features which add very little complexity from VHDL's perspective, and which make it easier for users to play with this, our toy. Think e.g. of a user that wants to test what would happen with signals which are larger than 32 or 64 bits.
The question might or might not make sense, but in any case we don't want him/her to stop doing it because of how cumbersome it is to find and change hardcoded values.
From this point of view, a generic or a constant can be used, but it is general bad practice (in any language) to hardcode sizes. Of course, I don't mean to criticize your coding; just to draw the limit for oversimplification.
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 it possible to pass multiple
*.vpi
arguments? Or do all VPI features need to be built into a single module?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 answer is I don't know. Probably @tgingold or @eine knows XD.
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.
I will ask.
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.
Currently you can pass only one
--vpi=
option. Well, only the last one is kept. But handling more than one shouldn't be a difficult change.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.
ghdl/ghdl#1276
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.
Please, make the testbench parametric or define the word-length as a constant.
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.
See above.
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.
It seems that there are two main differences between the
vpi.c
sources ofhelloworld
andaccess
:access
the values of some signals are written/read.access
the signals are not in the top-level entity, but in an instance.Hence, I think that two
access
examples should exist. Each showing one of the new features. Of course, both can share sources. It seems that#define hdl_prefix tb.
can be added and overwritten when calling GHDL/GCC.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.
I am ok in splitting the tests, or if you prefer, I can just make a simple tb.vhd that do not use ent.vhd to just demonstrate how to read/write signals. Or are you referring to the top module ports??
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.
See #19 (comment).
I mean that the same C source can be used to first inspect
ent
directly (no testbench, no hierarchy) and then inspecttb/ent
(with testbench, with hierarchy).ent.vhd
andvpi_access.c
are required, and-Dhdl_prefix="ent"
.ent.vhd
,tb.vhd
andvpi_access.c
are required, and-Dhdl_prefix="tb.ent"
.The purpose of this pair of examples is to show that the access logic is the same, regardless of using hierarchical names or not.
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 this is the only common file for now, I think that subdir
common
is not required. I'd move it tovpi/common.h
. How common is it? Is it expected to be used by examples other than yours? Would it make sense to have it upstreamed tovpi_user.h
(or added to GHDL codebase)?Anyway, please name it
*.c
and add a matching^.h
.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.
common_vpi.h contains a single function that helps the user to register VPI callbacks and it is used in all the VPI examples. I separated it so the VPI plugin sources are not bloated with callback registering code.
I can put it out of common. I don't think that it can be added to vpi_user.h since vpi_user.h should contain only VPI calls/definitions that are defined by the IEEE PLI 2.0 standard. I don't know what is the GHDL policy about including such kind of code. So I cannot give any opinion on that :P Sorry.
It can be put in a different compilation unit, but since it contains only a very simple function, I do not see any benefit in doing it from a clarity point of view.
Moreover, since this repo uses shell scripts and not makefiles to generate and execute the examples, the function gets recompiled anyways in each example.
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.
Let's keep it as
vpi/common.c
andvpi/common.h
for now, and I'll ask Tristan about providing avpi_user_ghdl.h
.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 this source is exactly the same as
vpi/access/ent.vhd
, do not add both of them. Makeaccess
be a case ofhelloworld
and reuse all the common sources.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.
So, do you suggest to put every common vhd file in the same directory?
Even if some examples contain similar or same code, I would not recommend moving them out of the particular example folder to limit the necessity to look around to the minimum (and IMHO it is very important in a documentation project). I know that
common_vpi.h
in this sense is an (the only) exception. We can discuss that :) .I didn't get what you say when you write
Since they are two distinct examples I wouldn't recommend mixing them together.
BTW now VHDL sources are a bit different from example to example, so the need to put everything in the same places doesn't apply so much anymore.
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.
Both are subexamples of "quickstart":
vpi/quickstart/
ent.vhd
tb.vhd
vpi_hello.c
vpi_helloworld.c
vpi_access.c
vpi_access_hierarchy.c
vpi_timestep.c
Or, to make the distinction more clear:
vpi/quickstart/
ent.vhd
vpi_hello.c
vpi_helloworld.c
vpi_timestep.c
access/
tb.vhd
vpi_access.c
vpi_access_hierarchy.c
Moreover, if the entity and testbench will be shared with other examples:
vpi/
ent.vhd
tb.vhd
quickstart/
vpi_hello.c
vpi_helloworld.c
vpi_timestep.c
access/
vpi_access.c
vpi_access_hierarchy.c
The point is that 5 examples exist in the docs, independently of which of these structures is used for the sources.
It is easier to understand for new users that they need to focus on different
*.c
files only, because the "already know" the content of others. Intermediate/advanced users can quickly cherry-pick the subset of files they need. They are not going to reuse the providedrun.sh
anyway.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 multiple examples share most of the sources and some steps in this script can be shared, please build and execute all the examples in the same script.
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.
Please, give an example to do this in a proper way.
Also, in this way the user loses the ability to compile/run a single test, so I discourage this solution. For code examples IMHO it is more important self-containment rather than code reuse.
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.
See, for example, https://github.com/ghdl/ghdl-cosim/tree/master/vhpidirect/quickstart/sharedvar or https://github.com/ghdl/ghdl-cosim/tree/master/vhpidirect/arrays/intvector.
Overall, the docs define which examples exist and how they are named. Sources might or might not match the structure.
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 the future, we would like to use some other mechanism to allow running single tests, while keeping shared steps in a common source (not duplicated). For now, we prioritize avoiding duplication over execution granularity.
Some options to handle testing better are makefiles or VUnit, but we have not given a thought to it yet. Whatever solution we use, we should be able to export nice and short bash scripts, so that users don't need to understand the complexity of the test runner.
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.
I believe that this example fits as "how to do nothing other than building and running a simulation with an VPI module" (#1 (comment)). Thanks a lot for adding it!
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.
You are welcome :D
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.
Ideally, this very first example would not require any header other than
vpi_user.h
. Is it possible to add a simpler version ofregister_cb
here?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.
Yep! Are you fine if I just register the
cbAfterDelay
callback?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.
It's up to you. I feel that registering a printf at start or finish is easier to understand as the very first contact than
AfterDelay
. From my ignorance, I would ask myself "a delay after what?", which would deviate me from the purpose of the example. However, if the explanation/introduction of the example clarifies it. For example:VPI allows to register callbacks at multiple events. In this example, a callback is set with a delay of N cycles/ms after whatever, but the same procedure can be applied to
cbStartOfSimulation
orcbEndOfSimulation
. See REFERENCE for a complete list of available callback names/identifiers.delay_ro_cb
signifficantly different fromstart_cb
?register_cb
required only to pass the delay?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.
I think that this callback, which seems to be the one that requires a non-trivial
register_cb
, should not be part of the very first example. Hence, I believe that helloworld can be split in two:hello
andhelloworld
.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.
Ok I changed helloworld and added the example beginend which should be equivalent to what you said and shows how to register callbacks that runs at the beginning/end of a simulation.
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.
See comment about duplicated files above.
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.
See comment about duplicated files above.
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.
See comment about duplicated files above.