-
Notifications
You must be signed in to change notification settings - Fork 16
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
vhdl support and configuration page #72
Conversation
Hi, Thank you for sending the pull request! It's awesome to see you contributing to this project. However, before I can look at this request you need to add DCO sign-off to commits in this repository. We require DCO sign-off for all commits to this repository (including all authors). Please see the following URL to see more information and BeeWare also have a Beginners guide to DCOs. You can use Thanks! |
Signed-off-by: Michael Riegert <[email protected]>
bd68305
to
f2e00cd
Compare
So the readthedocs build is failing with;
I think we just need to install the VHDL plugins using conda? |
There is already a conda package for
|
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.
Generally looks really good!
Just a few small comments.
I think we can just add
|
I tried adding it to the environment.yml locally but conda failed to find it, probably because it's not in the repositories. Otherwise, I think that can work. The conda version of yosys doesn't give an error about being unable to load plugins at runtime as well.
|
f2e00cd
to
1095a38
Compare
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 really nice! Just left with fixing the docs before we merge.
FTR, Yosys on Windows does not support loading modules dynamically. They need to be built-in. Therefore, the Conda package is suitable for Linux hosts only. However, GHDL and Yosys (with built-in ghdl-yosys-plugin) are available on MSYS2. Therefore, Windows users should be able to use this extension together with Sphinx installed on MSYS2. As a matter of fact, that's how/where I build/test the docs of open source projects every day. I will give this a try. |
README.rst
Outdated
Which will pass ``-m ghdl`` to Yosys when calling it. Similarly, setting this to the path of a | ||
ghdl-yosys-plugin shared library will also work. | ||
|
||
Unfortunately, at this time GHDL and ghdl-yosys-plugin aren't supported by YoWASP. |
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 welcome help from others explicitly!
Unfortunately, at this time GHDL and ghdl-yosys-plugin aren't supported by YoWASP. However, we'd love to have it available. Are you aware of some proof-of-concept linking WASM compiled from both C++ and Ada? Do you want to give it a try? Let us know!
@@ -0,0 +1,23 @@ | |||
library IEEE; |
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.
Maybe add some VHDL 2008 example too? That's one relevant feature of GHDL compared to almost any other available VHDL parsing tool.
docs/conf.py
Outdated
@@ -58,6 +58,10 @@ | |||
'sphinxcontrib_hdl_diagrams', | |||
] | |||
|
|||
# Uncomment for YosysHQ/fpga-toolchain |
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 wonder if this can be passed as a CLI or envvar. In #73, I use make -C docs html
for building the docs. That's something I do in many other projects with Docs based on Sphinx. Hence, I'd love to do HDL_DIAGRAM=system:prebuilt make -C docs html
or anything similar, instead of manually modifying the conf script.
Nonetheless, I think we can handle this a separated PR, after this is merged.
docs/configuration/index.rst
Outdated
GHDL | ||
++++ | ||
|
||
GHDL can either be prebuilt into Yosys or loaded at runtime. If it is loaded at |
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.
There is some duplication between the info in the README and the info here, isn't it? I would suggest keeping the GHDL details in one place (here). In fact, I'd suggest moving Usage
from the readme to the docs.
print("Running YoWASP yosys: {}".format(ycmd)) | ||
yowasp_yosys.run_yosys(ycmd) | ||
elif yosys == 'system': | ||
ycmd = "yosys -p '{cmd}' {src}".format(src=src, cmd=cmd) | ||
ycmd = "yosys {options} -p '{cmd}' {src}".format(options=options, src=src, cmd=cmd) |
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.
There is a commit in #73 for cleaning this. You might want to pick it.
yosys_opt = "-m ghdl " | ||
elif ghdl == "prebuilt": | ||
yosys_opt = "" | ||
elif os.path.exists(ghdl): |
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 would use Path(ghdl).exists()
. This is a Python language preference.
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.
Are you sure about this? I tried this as an example and got an error:
>>> from os import path
>>> path('conf.py').exists()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: 'module' object is not callable
On a similar note, os.path
maybe should be replaced with just path
throughout the code as it's imported independently from os (or the latter should be removed and use os.path
everywhere). Seems redundant to have both.
import os # line 26
.
.
.
from os import path # line 33
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. I should have specified:
from pathlib import Path
See https://docs.python.org/3/library/pathlib.html, https://docs.python.org/3/library/pathlib.html#pathlib.Path.exists and https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module.
raise HDLDiagramError("Cannot use YoWASP for VHDL (yet)") | ||
module = options['module'] | ||
ilfn = path.join(self.builder.outdir, self.builder.imagedir, options['outname'] + '.v') | ||
vhdl_to_verilog(source_path, ilfn, module, self.builder.config.hdl_diagram_ghdl, yosys) |
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'm not sure to understand this approach. My perception is that you are supporting Verilog sources only or VHDL sources only. Then, you convert VHDL to Verilog as a preprocess step, and then use exactly the same functions you'd use for "regular" Verilog sources. Therefore, generating drawings of mixed-language designs is not supported by this PR. Is this correct? If so, I would explain it somewhere, together with inviting people to contribute for making mixed-language diagrams possible.
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 100% right. After going back through the fomu workshop mixed-hdl example, I want to implement this differently than what I have now. I'm going to continue this discussion back on #65.
Signed-off-by: Michael Riegert <[email protected]>
Signed-off-by: Michael Riegert <[email protected]>
6c7e615
to
2c20f5b
Compare
So it looks like to get ghdl-yosys-plugin and ghdl added to symbiflow conda we'll have to get gcc-ada added as well (I'd recommend adding it to conda-forge instead of symbiflow). There's already one for MinGW but that doesn't help for Linux/MacOS. I've opened this issue to keep track of it. I've also started work here on updating the ghdl-yosys-plugin package recipe. I don't think the missing package should block this from being merged, it would just mean that VHDL support wouldn't be able to be used until it does (if people are using conda for generating their docs). |
Currently GHDL / ghdl-yosys-plugin aren't in the conda file / on the conda repository, so that will have to be done before / after this (or added to this) to make sure VHDL can work for anyone. Advice on what to do about this is welcome.
Unfortunately ghdl-yosys-plugin isn't compatible with YoWASP (to the best of my knowledge) so full-fledged Yosys will have to be used for VHDL support. I could be wrong in this though, I haven't looked too much into it.
I tested this with YosysHQ/fpga-toolchain.