Various VPI examples #19

bellaz89 · 2020-04-28T11:31:05Z

In this PR four VPI usage examples are added.

VPI examples

The directory contains some VPI usage examples:

helloworld: minimal VPI code example that runs a simulation, prints a message and exits.
access: signal read/write example on an adder component using vpi_put_value and vpi_get_value.
list: example on signal hierarchy iteration using vpi_iterateand vpi_scan.
timestep: shows how to run a simulation for an arbitrary number of timesteps.

to run a test, just go in an example directory and execute

sh run.sh

umarcor

Overall, I think that 6 examples can be extracted from this content, instead of 4. I would propose the following structure:

quickstart
- hello
- helloworld
- access
- access_hierarchy
- timestep
list

Please, add these additional examples/tests to CI. To do so, edit .github/workflows/test.yml and add new items to the task lists in lines 19 and 66.

See further comments below.

.gitignore

umarcor · 2020-04-30T02:02:04Z

doc/vpi/examples/index.rst

@@ -1,4 +1,5 @@
 Examples
 ########

-TBC
+See :cosimtree:`VPI examples <vpi>` directory 


As commented in #1, TBC at the end of doc/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 in doc/vpi/index.rst and included in the toctree.

Anyway, this file should contain a toctree (as done in doc/vhpidirect/examples/index.rst), so that a nice TOC is shown in the sidebar.

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

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:

Each of them can be written in a separte RST file with a header.

All of them can be written in the same file, using different headers for each of them. See https://raw.githubusercontent.com/ghdl/ghdl-cosim/master/doc/vhpidirect/examples/quickstart.rst.

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.

Please see the changes I did yesterday......

It is really annoying when GitHub shows a subset of changes or an outdated set of changes...

umarcor · 2020-04-30T02:07:45Z

vpi/README.md

@@ -0,0 +1,14 @@
+## VPI examples


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.

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/vpi

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.

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.

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.~~

This file should contain one or multiple links to the docs, where these very brief descriptions are extended.

Do we want a single vpi/README.md? Two (vpi/quickstart/README.md and vpi/list/README.md)? Or all of them?

umarcor · 2020-04-30T02:11:56Z

vpi/README.md

+
+The directory contains some VPI usage examples:
+
+1. [helloworld](./helloworld/) minimal VPI code example that runs a simulation, prints a message and exits. 


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.

See above: this is repository documentation. The full documentation in rst format will come with successive PRs

Ref #19 (comment).

umarcor · 2020-04-30T02:13:42Z

vpi/README.md

+3. [list](./list/) example on signal hierarchy iteration using `vpi_iterate`and `vpi_scan`.
+4. [timestep](./timestep/) shows how to run a simulation for an arbitrary number of timesteps. 
+
+to run a test, just go in the directory and execute 


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.

I think this should go in the main README, along with an explanation about "groups of examples" (see #19 (comment)).

Now included in #21.

umarcor · 2020-04-30T02:43:31Z

vpi/access/vpi.c

+
+    val.format = vpiBinStrVal;
+    val.value.str = "0101";
+    printf("set %s in tb.nibble1 \n", val.value.str);


It seems that there are two main differences between the vpi.c sources of helloworld and access:

In access the values of some signals are written/read.

In 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.

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??

See #19 (comment).

Or are you referring to the top module ports??

I mean that the same C source can be used to first inspect ent directly (no testbench, no hierarchy) and then inspect tb/ent (with testbench, with hierarchy).

In the first case, ent.vhd and vpi_access.c are required, and -Dhdl_prefix="ent".

For the second case, ent.vhd, tb.vhd and vpi_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.

umarcor · 2020-04-30T02:44:09Z

vpi/list/run.sh

@@ -0,0 +1,20 @@
+#!/usr/bin/env sh


See comment about duplicated files above.

umarcor · 2020-04-30T02:44:24Z

vpi/list/tb.vhd

@@ -0,0 +1,39 @@
+library ieee;


See comment about duplicated files above.

umarcor · 2020-04-30T02:47:35Z

vpi/list/vpi.c

@@ -0,0 +1,71 @@
+#include <vpi_user.h>


It seems that all VHDL sources are the same for all the examples. Do you think that it can be a common pattern for all VPI examples? Or do you expect other features to require different VHDL sources?

I ask it because it seems that the 4-7 examples that are part of this PR differ on the vpi.c file and the compilation commands only.

The thing is that to demonstrate very basic usage of VPI, the VHDL component under test do not matter so much. So, for helloworld I can, for example, just put a complete void top module without any signals, because at the end the VPI code is interacting with the simulator but not with the simulation. If you think it is better, I can try to make the ent.vhd and tb.vhd even more concise in each test.

This. In VHPIDIRECT examples, many C sources are common but VHDL sources need to be different. Here, as you said, VHDL sources are almost irrelevant (as long as they contain the minimum features that we want to test). Hence, instead of making ent.vhd and tb.vhd more concise in each test, I would suggest to make them general enough so that they can used in any VPI example (last structure in #19 (comment)). If some example in the future requires some weird feature in VHDL, we will consider adding different VHDL sources to the corresponding subdir. But, for now, it seems unnecessary.

vpi/list/vpi.c

bellaz89 · 2020-04-30T09:53:26Z

@umarcor
Thank you for the feedback! I am waiting for your response :)

In the meanwhile, I tried to implement a part of your suggestions.

…ment variable

umarcor · 2020-04-30T23:37:03Z

I don't understand why I cannot reply to some of the previous (outdated) comments, but I can reply to some other. Hence, I think that the review and this comment might be a little messy. Hope we can understand each other...

So, do you suggest to put every common vhd file in the same directory?

The structure that we are following is that each new file that is added to the repo, is required for a reason. If two examples use a source which is almost the same except for a line or a few lines, we consider whether it can be shared by adding some parameter. Should doing so involve too much complexity, it is ok to duplicate very similar files.

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).

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.

We need to balance complexity and verbosity. On the one hand, we want intermediate and advanced users to pick any isolated example and run/use it directly. On the other hand, we want unexperienced users which are reading the docs in order (as a tutorial), not to be continuously diffing many files that look similar and which might or might not be the same.

We are currently handling this as subsections (groups of examples). So, in this PR the "atomic unit" is vpi/quickstart. The script that is guaranteed to work alone is vpi/quickstart/run.sh. Then, vpi/quickstart/access/run.sh might exist, and it might be completely independent, but this is not guaranteed. In some cases, you need to call the parent script first. In other cases, the parent script does execute the child explicitly.

umarcor · 2020-05-01T00:23:32Z

vpi/common_vpi.h

+                 PLI_INT32 reason,
+                 int64_t cycles){
+
+    s_cb_data cbData;


I think that this deserves a reference/cite, either here or in the docs. Where should users search for the definition of s_cb_data? In vpi_user.h? The standard? Some book?

Now explained in #21.

umarcor · 2020-05-01T00:29:45Z

vpi/list/tb.vhd

+
+    begin
+
+        ent_0: entity work.ent 


This feels not to be valid VHDL. Is it on purpose?

Anyway, correct or purposely incorrect, I'd suggest to use a for generate instead:

i_ent: for I in 0 to 3 generate w_ent: entity work.ent port map (nibble1 => nibble1, nibble2 => nibble2, sum => sum, carry_out => carry_out); end generate;

The script is running, the CI completes without any error, and GHDL doesn't pop out even a Warning. Therefore I think it is valid VHDL.

Surprising... I would expect sum and carry_out to collide, since four different drivers are connected to the same signal. I simulated it and, when one of the instances drives a different value, sum and/or carry_out are X. @tgingold, should this produce a warning?

Anyway, I think we should build a proper adder chain to sum two 4*4=16bit numbers.

Ok, now I see that you were talking about a multiple-driving issue. I agree that it is strange that GHDL doesn't catch it and I will fix that. Please could you be a bit more descriptive and giving the code lines where the problem is next time?

Please could you be a bit more descriptive and giving the code lines where the problem is next time?

Sure! Sorry about it. I was convinced that you had done it on purpose, because you only wanted to explore the hierarchy without caring about the content.

I will take care of fixing this source if you want; so you can focus on VPI-specific issues which I'm less comfortable with.

Equivalent source included in #21.

umarcor · 2020-05-01T00:41:14Z

vpi/quickstart/helloworld/vpi.c

+    cbData.user_data = 0;
+    cbData.value = 0;
+
+    // the callback is executed after a delay of 0


From my ignorance, it seems that these lines (35-37) are optional. That is, the first example would also work with cbData.time = NULL (as per the common header above). However, I'm afraid that cbData.time = NULL and cbData.reason = cbAfterDelay might produce either a crash or weird behaviour.

Hence, I think that we should have three "hello" examples:

begin/hello: same as begin_end but without common_vpi.h, or same as helloworld but without simuTime.

helloworld

begin_end

Anyway, we can move forward and I will take care of adding begin/hello after this PR is merged.

Example hello is now included in #21.

umarcor · 2020-05-01T00:46:20Z

vpi/quickstart/timestep/vpi.c

+ * VPI. This is accomplished registering a chain of callbacks.
+ */
+
+// register a cbAfterDelay callback that executes delay_ro_cb after 1 timestep


I think that some discussion/explanation is required about why four different callbacks are used. Why do we need to register cbReadOnlySynch if cbReadWriteSynch is going to be executed in the same chain anyway?

bellaz89 · 2020-05-01T20:20:43Z

Hi,

I think that I won't work anymore on this PR. Either because I don't believe that, given the scope of the PR, implementing more changes will lead to any quality improvement, and (more important) because I don't have more time to spend on this. So, take it as it is and do what you want with the PR. You can merge it and do the changes you prefer later, you can reject it or you can make it as a pending PR indefinitely. I am fine with whatever decision you will take.

SomeParticles added 5 commits April 28, 2020 12:25

added soome VPI examples

97e51da

now the VPI examples follows the ghdl-cosim rules

1d6b84c

added dirname /bin/bash

52975d3

removed binary files

0d5789d

removed binary files

f478d8f

bellaz89 mentioned this pull request Apr 28, 2020

Previous work and future ideas #1

Open

15 tasks

umarcor reviewed Apr 30, 2020

View reviewed changes

excluded all tb* and ent* in .gitignore

3297c11

SomeParticles added 6 commits April 30, 2020 12:28

added corrections on PR feedbacks

f30f585

fixed error on test.yml

7fd3508

removed Windows VPI tests due to the necessity to change PATH environ…

f09c924

…ment variable

more modification and comments added

9efed06

added PATH to vpi/list/run.sh

a684de7

disabled VPI tests on windows

93ba510

umarcor reviewed May 1, 2020

View reviewed changes

corrected multiple drive issue in vpi/list/tb.vhd

976dcf5

This was referenced May 3, 2020

Reorganize, update README, add refs, and add vpi/quickstart #21

Merged

doc: add hint about passing multiple '--vpi=' ghdl/ghdl#1276

Merged

eine added the Interface: VPI label May 10, 2020


		The directory contains some VPI usage examples:

		1. [helloworld](./helloworld/) minimal VPI code example that runs a simulation, prints a message and exits.

Various VPI examples #19

Are you sure you want to change the base?

Various VPI examples #19

Conversation

bellaz89 commented Apr 28, 2020 • edited Loading

VPI examples

umarcor left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellaz89 Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellaz89 Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellaz89 Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

umarcor Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellaz89 Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellaz89 Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

umarcor Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

bellaz89 commented Apr 30, 2020 • edited Loading

umarcor commented Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umarcor May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellaz89 commented May 1, 2020

bellaz89 commented Apr 28, 2020 •

edited

Loading

umarcor left a comment •

edited

Loading

bellaz89 Apr 30, 2020 •

edited

Loading

bellaz89 Apr 30, 2020 •

edited

Loading

bellaz89 Apr 30, 2020 •

edited

Loading

umarcor Apr 30, 2020 •

edited

Loading

bellaz89 Apr 30, 2020 •

edited

Loading

bellaz89 Apr 30, 2020 •

edited

Loading

umarcor Apr 30, 2020 •

edited

Loading

bellaz89 commented Apr 30, 2020 •

edited

Loading

umarcor commented Apr 30, 2020 •

edited

Loading

umarcor May 1, 2020 •

edited

Loading