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

Add method 'set_objects' #481

Closed
wants to merge 3 commits into from
Closed

Conversation

umarcor
Copy link
Member

@umarcor umarcor commented Apr 20, 2019

As commented in #476, this PR allows to provide some pre-built object (typically from C sources) to have them built with the VHDL design. The primary use case for this is to provide VHPI implementations, but it can also be used to just wrap a GHDL simulation.

The objects are saved in a new sim option named objects. Currently, these are only used in ghdl_interface. However, in the future it is possible to extend it to ModelSim/QuestaSim and/or Riviera-Pro.

Moreover, method simulator_check is added to test/common.py and supports_vhpi is added to simulator_interface. These allow to evaluate if a simulator supports VHPI, before trying to run an example that requires it.

Note that neither tests nor examples are provided in this PR, because those are implemented in #465 and #476.

@kraigher
Copy link
Collaborator

Maybe the name is to general? Is it assumed to be a VHPI object or just any object? Isnt there Verilog foreign interfaces also? In modelsim and riviera pro there is a pli sim_option. Would those objects be compatible with the objects you propose? Maybe this should be called "vhpi_objects" if it is assumed that it is VHPI.

@umarcor
Copy link
Member Author

umarcor commented Apr 23, 2019

Maybe the name is to general? Is it assumed to be a VHPI object or just any object?

"Any" object, which is to be linked with GHDL.

Isnt there Verilog foreign interfaces also? In modelsim and riviera pro there is a pli sim_option.

Sim option 'pli' should be equivalent. Certainly, 'pli' can be renamed to 'objects', since the expected functionality and UX is the same. Or, the other way, I can rewrite this PR so no new methods/field are added, and 'pli' is renamed and reused instead.

VPI is different. GHDL provides specific CLI options to handle it.

Would those objects be compatible with the objects you propose?

I don't think so. C sources are slightly different. The user would need to select which object to add to set_objects, depending on the simulator which is to be used.

Maybe this should be called "vhpi_objects" if it is assumed that it is VHPI.

It is not needed. Please, see run.py, main.c and stubs.c in https://github.com/VUnit/vunit/pull/465/files.

@kraigher
Copy link
Collaborator

I am worried that there is no general purpose definition of what an "object" is in all simulators. If there is no common definition then this just becomes a poor abstraction that will only fit GHDL. As a first rule I try to avoid having wrappers for simulator settings that can just as well be directly done by the user in a simulator flag. Both because often simulators do not have a common standard for concepts but also to avoid a large surface area of potential support questions when the abstraction does not work. If we find some very common setting that has a common concept for all simulators we could consider adding an abstraction for it in VUnit. I am not sure this is the case here and also this PR does not implement the abstraction for all simulators which becomes a risk/technical debt.

@umarcor
Copy link
Member Author

umarcor commented Apr 25, 2019

Certainly, after better thinking about this, set_objects is not required at all. It was needed when I implemented 'external arrays' as being automatically loaded when set_objects was used. But now, it is done through from_args (see https://github.com/VUnit/vunit/pull/476/files#diff-de57c7bf93948bb862ec34662b2425e9). I got confused at some point while splitting the original contribution into multiple PRs.

However, I think that the first two commits in this PR are useful. I will close this and open a new PR for that.

Thanks for your insightful comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants