amaranth icon indicating copy to clipboard operation
amaranth copied to clipboard

Emit signal groups in GTKW files

Open tilk opened this issue 1 year ago • 3 comments

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 https://github.com/amaranth-lang/amaranth/issues/764#issuecomment-2014220740 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 avatar Mar 25 '24 20:03 tilk

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 :warning:
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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 25 '24 21:03 codecov[bot]

@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.

whitequark avatar Apr 09 '24 17:04 whitequark

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

whitequark avatar May 09 '24 03:05 whitequark

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

sim: group signal traces according to their function.

whitequark avatar May 22 '24 00:05 whitequark

Could you squash the PR please?

Done.

tilk avatar May 22 '24 08:05 tilk

Thanks!

whitequark avatar May 22 '24 08:05 whitequark