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

Support arbitrary waveform viewer #1002

Merged
merged 2 commits into from
Aug 18, 2024
Merged

Support arbitrary waveform viewer #1002

merged 2 commits into from
Aug 18, 2024

Conversation

oscargus
Copy link
Contributor

@oscargus oscargus commented Mar 12, 2024

Closes #970

Haven't really had time to test this fully, so putting in draft. But since it seems like 5.0 is upcoming, this may be something to consider.

Basically, there is a --viewer argument that lets you select a wave form viewer. Also, all --gtkwave-* arguments are renamed --viewer-*, but with aliases.

(At least that is the idea once fully working.)

Motivated by https://surfer-project.org/ that is beginning to gain some traction. (Although possible to run in the browser there are also native versions.)

@oscargus oscargus force-pushed the viewer branch 6 times, most recently from 829e97b to 7a925f8 Compare March 12, 2024 10:26
@umarcor
Copy link
Member

umarcor commented Mar 12, 2024

But since it seems like 5.0 is upcoming, this may be something to consider.

We cannot deprecate gtkwave arguments in the next release. We need to first release a version with the new feature, announcing that "old" aliases will be removed in a future version. Then, in a future release, we can remove the deprecated keywords/arguments.
Therefore, this PR will not introduce breaking changes, and we can have it merged as soon as it is ready (no matter if it's 5.0 or 5.1).

Other than that, this looks very interesting. So, if it's ready by the time we want to tag 5.0, we can sure have it merged before.

@oscargus
Copy link
Contributor Author

The thing I am unsure how to handle are the option strings. Is there some previous example of aliasing those? But if the purpose is to deprecate in the long run, maybe they need to be handled explicitly anyway, so duplicated arguments and then check which has information.

@LarsAsplund
Copy link
Collaborator

LarsAsplund commented May 31, 2024

@oscargus Previously, we've handled them explicitly. Here is an example:

vunit/vunit/sim_if/ghdl.py

Lines 255 to 256 in 2c0b75e

if source_file.compile_options.get("ghdl.flags", []) != []:
raise RuntimeError("'ghdl.flags was removed in v5.0.0; use 'ghdl.a_flags' instead")

In your case it would be a warning rather than an error

@oscargus oscargus force-pushed the viewer branch 8 times, most recently from ecafe6b to 6acff41 Compare July 11, 2024 10:26
@oscargus
Copy link
Contributor Author

Now I finally got around to finish this.

The news-item summarizes the changes quite good, but brief.

It seems like the emitted warnings are filtered, but I cannot really figure out how and where...

As a missing waveform viewer now raises after the simulation (which is not ideal, but I couldn't figure out another way to define the waveform viewer in the run-file), I had to remove one test. Let me know if this should be reverted or if there is another way to specify waveform viewer in the run-file.

It is of course possible to just limit the choice to surfer or gtkwave. That would simplify things a bit.

@oscargus oscargus marked this pull request as ready for review July 11, 2024 11:02
vunit/sim_if/nvc.py Outdated Show resolved Hide resolved
vunit/sim_if/ghdl.py Outdated Show resolved Hide resolved
vunit/sim_if/nvc.py Outdated Show resolved Hide resolved
vunit/sim_if/ghdl.py Outdated Show resolved Hide resolved
@oscargus
Copy link
Contributor Author

Now I have sorted things out (and passed the lint, wasn't easy with the long warning message...). I ended up factoring out the common viewer code to an OSSMixin-class that is used for both GHDL and NVC.

I'm a bit tempted to create a third class of options, maybe viewer_options as the viewer will be the same for/independent of ghdl and nvc it will also make sense to have common viewer, viewer_fmt, etc. This will also allow checking if it exists before actually trying to open the viewer (hence, keeping the previous behavior).

Taking the idea of being able to generate waves without opening the viewer, #1003 and #1042, one may also consider other naming.

docs/news.d/1002.feature.rst Outdated Show resolved Hide resolved
vunit/sim_if/_ossmixin.py Outdated Show resolved Hide resolved
vunit/sim_if/_ossmixin.py Outdated Show resolved Hide resolved
docs/news.d/1002.feature.rst Outdated Show resolved Hide resolved
vunit/sim_if/ghdl.py Outdated Show resolved Hide resolved
@oscargus
Copy link
Contributor Author

All comments should be fixed. Sorry about that!

They are in a separate commit to make it easier to check, but I suggest to squash (I can squash if you do not have it enabled at GitHub).

@LarsAsplund LarsAsplund merged commit 2a276be into VUnit:master Aug 18, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameters for alternative wave form viewer
4 participants