UCSBarchlab / PyRTL

A collection of classes providing simple hardware specification, simulation, tracing, and testing suitable for teaching and research. Simplicity, usability, clarity, and extensibility are the overarching goals, rather than performance or optimization.
http://ucsbarchlab.github.io/PyRTL
BSD 3-Clause "New" or "Revised" License
253 stars 76 forks source link

Assorted WaveRenderer UI improvements #428

Closed fdxmw closed 1 year ago

fdxmw commented 1 year ago

See the updated screenshots and tests for a preview:

Also:

mdko commented 1 year ago

I think all of this looks great! I'm torn between the old characters used for up/down in Utf8WaveRenderer (link) (which match the transitions on binary signals) and the new ones you've chosen (which I find do make it easier to read). Just putting my two cents out there.

fdxmw commented 1 year ago

I'm torn between the old characters used for up/down in Utf8WaveRenderer (link) (which match the transitions on binary signals)

Thanks for taking a look! Can you help me understand how the old up/down characters matched the transitions on binary signals? For me the old characters didn't quite align, see the little bumps and gaps on screenshot, while the new characters seem to align for various fonts I've experimented with (screenshot)

fdxmw commented 1 year ago

Also this is all very subjective, I think these are improvements but I'm happy to make changes if people see things differently :)

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 96.03% and project coverage change: -0.04 :warning:

Comparison is base (bec4c37) 90.51% compared to head (a9277d8) 90.47%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## development #428 +/- ## =============================================== - Coverage 90.51% 90.47% -0.04% =============================================== Files 24 24 Lines 6071 6142 +71 =============================================== + Hits 5495 5557 +62 - Misses 576 585 +9 ``` | [Impacted Files](https://codecov.io/gh/UCSBarchlab/PyRTL/pull/428?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UCSBarchlab) | Coverage Δ | | |---|---|---| | [pyrtl/passes.py](https://codecov.io/gh/UCSBarchlab/PyRTL/pull/428?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UCSBarchlab#diff-cHlydGwvcGFzc2VzLnB5) | `94.70% <ø> (+0.04%)` | :arrow_up: | | [pyrtl/simulation.py](https://codecov.io/gh/UCSBarchlab/PyRTL/pull/428?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UCSBarchlab#diff-cHlydGwvc2ltdWxhdGlvbi5weQ==) | `93.00% <95.96%> (-0.44%)` | :arrow_down: | | [pyrtl/compilesim.py](https://codecov.io/gh/UCSBarchlab/PyRTL/pull/428?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UCSBarchlab#diff-cHlydGwvY29tcGlsZXNpbS5weQ==) | `91.03% <100.00%> (-0.47%)` | :arrow_down: | ... and [6 files with indirect coverage changes](https://codecov.io/gh/UCSBarchlab/PyRTL/pull/428/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UCSBarchlab) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UCSBarchlab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UCSBarchlab)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mdko commented 1 year ago

I'm torn between the old characters used for up/down in Utf8WaveRenderer (link) (which match the transitions on binary signals)

Thanks for taking a look! Can you help me understand how the old up/down characters matched the transitions on binary signals? For me the old characters didn't quite align, see the little bumps and gaps on screenshot, while the new characters seem to align for various fonts I've experimented with (screenshot)

Oh, sorry, I think I was looking at the wrong picture! Your new version looks much better, including those transitions. I’m 100% pro your changes. 😎

fdxmw commented 1 year ago

A number of updates to this pull request:

  1. Rising and falling edges are now vertical. This is done by rendering binary wires across two lines in the terminal.
  2. Added an environment variable option PYRTL_RENDERER to select a particular renderer. There are currently 5:
    1. 'powerline' : Render value transitions with powerline triangle glyphs. This produces the familiar hexagon shapes that are common in logic analyzers. Binary wires are rendered with vertical rising and falling edges. Unfortunately powerline glyphs are not standard, and there doesn't seem to be a way to detect their availability, so users must manually enable this option.
    2. 'utf-8' : Default renderer on all platforms except Windows. Binary wires are rendered with vertical rising and falling edges, and values are rendered in rectangles.
    3. 'utf-8-alt' : Binary wires are rendered with diagonal rising and falling edges, and values are rendered in rectangles.
    4. 'cp437' : Default renderer on Windows. Binary wires are rendered with vertical rising and falling edges, using only the box drawing characters in 8-bit ASCII.
    5. 'ascii' : Basic renderer, limited to 7-bit ASCII.
  3. Removed render_trace()'s segment_delim and extra_line options. extra_line is now mandatory with two-line rendering for binary wires, and segment_delim complicates the code and doesn't seem to be used.
  4. Decreased render_trace()'s default segment_size to 1. I often find myself counting cycles between tick marks, and I find it's much easier to just read the cycle number from the ruler.