Add support for VHDL packages #3899
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3899 +/- ##
==========================================
+ Coverage 76.79% 76.81% +0.01%
==========================================
Files 56 56
Lines 8429 8461 +32
Branches 2057 2071 +14
==========================================
+ Hits 6473 6499 +26
- Misses 1479 1486 +7
+ Partials 477 476 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
56a0893 to
226e54a
Compare
|
Doesn't seem to work with any of the commercial simulators but I don't have a license at the moment to debug. |
|
Questa FLI passes Questa VHPI fails I have no idea why the discovery tests are different now in Riviera VHPI. |
44b50d0 to
d09dc7f
Compare
|
Our CI only tests released versions of simulators, so we will have to wait until the version of NVC with the fix is released before integrating this PR. But I have manually confirmed that it does work on NVC master branch. I fixed the issue in VCS. It wasn't finding a file, so I added The Riviera issue was due to calling |
src/cocotb/_init.py
Outdated
| handle._discover_all() | ||
| # All VHPI sims have issue with discovering sub-objects, | ||
| # so we just always iterate to discover all children. | ||
| handle._discover_all() |
There was a problem hiding this comment.
What's the problem here? vhpi_handle_by_name doesn't work in packages? I would rather fix this in NVC than have this workaround recursively discover all objects as I build VHPI objects lazily and this would cause a lot of redundant allocations.
There was a problem hiding this comment.
I am not entirely sure. I know for certain this is required for Icarus and Riviera. I need to poke into it more, but ran out of time before passing out last night.
There was a problem hiding this comment.
Looks like only NVC supports getting child handles by name from package objects. Weird...
I'd hate to add all those simulators to the list that needs to _discover_all first, and there's probably a better way to do that by delegating to discover_all if a lookup fails, but that's work I don't want to have to do before 2.0. So I guess we will just pessimize those simulators for the 2.0 release and try to fix it before 2.1.
312021b to
a722e7d
Compare
src/cocotb_tools/runner.py
Outdated
| @@ -1053,6 +1053,7 @@ def _build_command(self) -> List[_Command]: | |||
| + ["-a"] | |||
| + [str(source) for source in self.sources if is_vhdl_source(source)] | |||
| + [str(source) for source in self.vhdl_sources] | |||
| + ["--preserve-case"] | |||
There was a problem hiding this comment.
Also add this flag to the documentation at https://github.com/cocotb/cocotb/blob/master/docs/source/custom_flows.rst.
|
@nickg Would you mind splitting out the change that makes cocotb use |
Co-authored-by: Kaleb Barrett <dev.ktbarrett@gmail.com>
See issue #1833 and also nickg/nvc#891.
I have only been able to test this with latest NVC master branch.