Skip to content

Add RFC for structured VCD output #74

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

Merged
merged 8 commits into from
May 13, 2025
Merged

Conversation

eigenform
Copy link
Contributor

@eigenform eigenform commented Jan 22, 2025

@whitequark whitequark added meta:nominated Nominated for discussion on the next relevant meeting area:core RFC affecting APIs in amaranth-lang/amaranth labels Jan 22, 2025
@whitequark
Copy link
Member

whitequark commented Jan 22, 2025

Thank you, this is very well written! My comments:

  • Some examples of how the UI of GTKWave and Surfer will look like afterwards would be very helpful. (I think there are already RFCs which include media to use as an example.)
  • An alternative is to keep the ability to use both formats, via the use of an environment variable (which is useful if you want to upgrade a third party codebase without modifying it, in possibly many places) and/or a parameter to write_vcd().

(Edit: I now noticed the "Does this feature need to be gated/opt-in by default?" unresolved question, which would cover my second comment.)

@@ -204,9 +207,13 @@ $upscope $end
- Should we continue to include the "flattened" [pure bit-vector] representation of aggregate signals in the VCD?
- Does this feature need to be gated/opt-in by default?

- The simulator currently depends on [westerndigitalcorporation/pyvcd](https://github.com/westerndigitalcorporation/pyvcd) when writing VCD files.
However, `pyvcd` only emits scope types defined by the VCD specification (and does not include the `vhdl_record`/`vhdl_array` scopes).
When implementing this RFC, should we continue relying on `pyvcd`, or does this warrant the addition of our own code in the simulator for writing VCD files?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason to use pyvcd (VCD itself is not hard to emit) is its support for .gtkw files. I really don't want to have more GTKWave specific code in Amaranth...

@whitequark
Copy link
Member

Thinking about it, I feel that a prototype for this functionality would be not too difficult to build (you could depend on a git repo with a fork of pyvcd) and it would allow everyone impacted by the change to evaluate how it'll affect their workflow.

One consumer with little support for advanced VCD features that we need to care about is the Amaranth playground.

@whitequark whitequark closed this Jan 23, 2025
@whitequark whitequark reopened this Jan 23, 2025
@whitequark
Copy link
Member

I closed the PR accidentally (Shift+Enter), apologies!

@whitequark
Copy link
Member

We have discussed this RFC on the 2025-05-12 meeting. The dispositon was to merge. The unresolved questions were approached as follows:

  1. We should continue to have a "flattened" representation because, if nothing else, data.Layout permits having bits that are not a part of any field. Such bits would be completely invisible otherwise.
  2. We should make this feature opt-out, both with a write_vcd argument and an environment variable. We will not port it to 0.5.x series.
  3. We should work with pyvcd upstream to add these types; it is not reasonably expected to be a problem. (Pyvcd is a non-exposed implementation detail that isn't, in principle, a part of the RFC in first place.)
  4. The Amaranth playground already has issues with structured data. It uses a library d3-wave that has "custom row renderers" that can be used to extend it in an application using it. We should update the playground code to emit custom row rendereds for structures (actually, we should've done it a while ago since right now strings don't display properly; Waveform renderer disappears for FSM signals and string reprs playground#2).

The next steps would be:

  1. To update the RFC text with the information above so it can be merged. (this is on @eigenform)
  2. To merge it and open a tracking issue for it in the main repo. (this is on me)
  3. To implement and ship it. (this is on @eigenform or anyone else interested)

@whitequark whitequark removed the meta:nominated Nominated for discussion on the next relevant meeting label May 12, 2025
@whitequark
Copy link
Member

whitequark commented May 13, 2025

Tracking issue opened: amaranth-lang/amaranth#1599. Can you add that to the body please? Along with start date.

@whitequark whitequark merged commit a838a3b into amaranth-lang:main May 13, 2025
@whitequark
Copy link
Member

Thank you for writing and updating the RFC!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core RFC affecting APIs in amaranth-lang/amaranth
Development

Successfully merging this pull request may close these issues.

2 participants