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

Emit signal groups in GTKW files #1245

Merged
merged 1 commit into from
May 22, 2024
Merged

Conversation

tilk
Copy link
Contributor

@tilk tilk commented Mar 25, 2024

Fixes #764.

Things to look at in the review:

  • Group names for views and interfaces. The current code uses "view" and "interface". Maybe the name should reference a layout/signature in some way?
  • Treating single-field view as in GTKW files with signal groups #764 (comment) is not implemented. It could be implemented by adding a special case.
  • I've added tests that check if the code doesn't raise an exception, the resulting gtkw files are not checked. I'm not sure how to best check them; I could compare them with a reference, but this would freeze the signal naming. Is signal naming for views and interfaces specified?
  • Had to add another change related to the recent private names commit. Now that there are three separate recursive functions in this PR, maybe the recursion pattern should be abstracted away?

@tilk tilk requested a review from whitequark as a code owner March 25, 2024 20:59
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 88.70968% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 91.05%. Comparing base (dde8334) to head (35380ec).
Report is 27 commits behind head on main.

Files Patch % Lines
amaranth/sim/pysim.py 86.00% 3 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1245      +/-   ##
==========================================
+ Coverage   90.48%   91.05%   +0.57%     
==========================================
  Files          43       45       +2     
  Lines       10864    11256     +392     
  Branches     2660     2449     -211     
==========================================
+ Hits         9830    10249     +419     
+ Misses        855      828      -27     
  Partials      179      179              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@whitequark
Copy link
Member

@tilk Could you rebase the PR please? We've finally landed MemoryData, a proper replacement for MemoryIdentity that also improves tracing experience, but this caused a conflict here.

amaranth/sim/core.py Outdated Show resolved Hide resolved
@whitequark whitequark added this to the 0.5 milestone Apr 9, 2024
@whitequark whitequark changed the title Gtkw groups Emit signal groups in GTKW files Apr 9, 2024
@whitequark
Copy link
Member

@tilk, the rework of the simulator necessary for the RFC 36 has introduced some merge conflicts here--could you fix them please?

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@whitequark
Copy link
Member

Could you squash the PR please? The commit message could be something like

sim: group signal traces according to their function.

@tilk
Copy link
Contributor Author

tilk commented May 22, 2024

Could you squash the PR please?

Done.

@whitequark whitequark added this pull request to the merge queue May 22, 2024
@whitequark
Copy link
Member

Thanks!

Merged via the queue into amaranth-lang:main with commit 51e0262 May 22, 2024
19 of 21 checks passed
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.

GTKW files with signal groups
2 participants