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

sim: Implemented signal notability for use in generating actually useful GTKW files #624

Closed
wants to merge 1 commit into from

Conversation

lethalbit
Copy link

@lethalbit lethalbit commented Aug 7, 2021

This adds a notable param to the Signal object which is then flattened into a dictionary of groups and signal names in the pysim engine for use in writing a GTKW file that contains only what you're interested in, rather than all of the signals in the design, or what has been specified when calling write_vcd.

You can specify the group for the signal, or just mark it as notable and it will be included in the global group by default when written out to the GTKW file.

A small example would be as follows:

m = Module()
nya = Signal(notable=True)
awoo = Signal()

m.d.sync += [
    nya.eq(~nya),
    awoo.eq(nya),
]

sim = Simulator(m)

sim.add_clock(1 / 12e6, domain = "sync")

with sim.write_vcd("nya.vcd", "nya.gtkw"):
    sim.run()

There is also a functioning test case in the tests/test_sim.py file, and as of this commit all the tests still pass.

@codecov
Copy link

codecov bot commented Aug 7, 2021

Codecov Report

Merging #624 (bd32c56) into master (abb2642) will decrease coverage by 0.00%.
The diff coverage is 84.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #624      +/-   ##
==========================================
- Coverage   81.46%   81.45%   -0.01%     
==========================================
  Files          49       49              
  Lines        6446     6472      +26     
  Branches     1287     1299      +12     
==========================================
+ Hits         5251     5272      +21     
- Misses       1007     1010       +3     
- Partials      188      190       +2     
Impacted Files Coverage Δ
nmigen/hdl/ast.py 89.74% <57.14%> (-0.25%) ⬇️
nmigen/sim/pysim.py 90.70% <92.30%> (-0.10%) ⬇️
nmigen/build/run.py 22.05% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abb2642...bd32c56. Read the comment docs.

@whitequark
Copy link
Member

whitequark commented Aug 7, 2021

The ability to write GTKW files itself is auxiliary; it was added for parity with Migen. I don't think we should add any core language features purely to improve convenience of using GTKW files.

@lethalbit
Copy link
Author

lethalbit commented Aug 7, 2021

Yeah that's entirely fair, figured it was worth a shot as manually creating GTKW's and organizing the signals is kinda a pain.

Wasn't that much work anyway so no harm no foul~

@anuejn
Copy link
Contributor

anuejn commented Aug 7, 2021

I think there could be a nice way to implement that in downstream code if there was a way to attach something like tags to signals and later on collect all signals with a certain tag. This mechanism could also be handy for something like an ila or very ad-hoc debug CSRs.

@mithro
Copy link

mithro commented Aug 7, 2021

I do like the idea of being able to add arbitrary tags (metadata?) to signals.

@whitequark
Copy link
Member

whitequark commented Aug 8, 2021

I'm not sure how I feel about signal metadata. It's certainly useful, but it's also liable to be the place where all the miscellaneous stuff gets put in, without structure or namespacing, leading to conflicts between different users of metadata.

@anuejn
Copy link
Contributor

anuejn commented Aug 8, 2021

Wouldn't it be possible to design the API in a way that avoids such problems?
One thing I could imagine would be that all metadata one can attach are objects of downstream classes. Then, one can query all metadata that is instance of a certain class.

@whitequark
Copy link
Member

whitequark commented Aug 8, 2021

One thing I could imagine would be that all metadata one can attach are objects of downstream classes. Then, one can query all metadata that is instance of a certain class.

That's the approach I picked for nmigen-soc, yeah. It could be applicable here, too; I like the idea!

@modwizcode
Copy link
Contributor

modwizcode commented Sep 1, 2021

That solution probably solves avoiding the mess. But it doesn't solve the potential for conflicting needs for similar metadata. Like a general "export this trace to gtkw" or "add to ILA group x" metadata class might be worth having as a built-in given just how common such features would be generally helpful (maybe not the ILA stuff if we're not going to include anything like that, but auto-add to gtkw/listing of interesting sim signals is probably worth it)

@lethalbit lethalbit closed this Dec 10, 2021
@lethalbit lethalbit deleted the signal_notability branch Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants