Update Icarus Verilog build to support WAVES environment variable, 2023 version

[ddribin]

Details:

• Update Makefile.icarus to support WAVES by creating an iverilog_dump.v file that is automatically added to the build.
• Remove VCD support from examples/simple_dff/dff.sv

This is basically an updated version of PR #2239 against current top-of-tree. It generates a VCD instead of an FST. And it puts the waveform file in sim_build/.

[imphil]

Are we sure we want "It generates a VCD instead of an FST."? FST can be read just fine by GtkWave, and whoever is using icarus is most likely also using a free waveform viewer. And of course it's the significantly better format, with more information being preserved and files being much smaller.

[imphil]

LGTM! I'd go for FST traces unless there's a strong reason not to. And this will need a newsfragment, but I can do that after this PR goes in.

[imphil]

To reduce the risk of accidentally clashing with a user module of that name can we prefix this module with something rare? A random number, cocotb_internal_implementation_of_iverilog_dump, something of that sort?

[ddribin]

Yeah, good point. I think a cocotb_ prefix ought to be good enough? So how about cocotb_iverilog_dump.v ?

[imphil]

There might be collisions no matter what name we're choosing, so feel free to use the one you like best.

[ktbarrett]

There is more tlak about adding support for FST vs VCD into WAVES at some point more globally. For now just getting the feature in there in any way is a step forward.

[ddribin]

Are we sure we want "It generates a VCD instead of an FST."? FST can be read just fine by GtkWave, and whoever is using icarus is most likely also using a free waveform viewer. And of course it's the significantly better format, with more information being preserved and files being much smaller.

Happy to do FST. I just went with VCD because it is more "standard" ? But, true, I use GTKWave, so FST would work just fine.

[ddribin]

I also should update the docs, which currently mention the need to add a code fragment to your .v file for Icarus.

[imphil]

On FST vs VCD: We have other simulators writing waveforms in their proprietary format when WAVES is set, so doing the same for Icarus seems consistent. So I'd personally go for that, but I'll leave it up to you @ddribin, it's your PR after all! Thanks for working on it!

Let me know when you're ready for a merge.

[imphil]

Rerunning CI against a master merge.

[codecov]

Codecov Report

Merging #3324 (9617ed9) into master (efe8d3a) will not change coverage.
The diff coverage is n/a.

❗ Current head 9617ed9 differs from pull request most recent head 5938f56. Consider uploading reports for the commit 5938f56 to get more accurate results

@@           Coverage Diff           @@
##           master    #3324   +/-   ##
=======================================
  Coverage   48.85%   48.85%           
=======================================
  Files          49       49           
  Lines        8816     8816           
  Branches     2446     2446           
=======================================
  Hits         4307     4307           
  Misses       3932     3932           
  Partials      577      577           

[ddribin]

On FST vs VCD: We have other simulators writing waveforms in their proprietary format when WAVES is set, so doing the same for Icarus seems consistent. So I'd personally go for that, but I'll leave it up to you @ddribin, it's your PR after all! Thanks for working on it!

Let me know when you're ready for a merge.

I was going to ask if there's precedent for WAVES producing formats other than VCD. Given other simulators do produce something other than VCD, I'll switch to FST, by default. We can add an override option for VCD later.

I just pushed changes for this and the documentation. Please take a look at the doc changes, but if there are no further comments, this is ready to merge.