amaranth-lang / amaranth-soc

System on Chip toolkit for Amaranth HDL
BSD 2-Clause "Simplified" License
84 stars 31 forks source link

Migrate bus primitives to `amaranth.lib.wiring` #49

Closed jfng closed 1 year ago

jfng commented 1 year ago

Supersedes #48, as the changes to CSR and Wishbone are similar in nature, and would benefit from a common review.

jfng commented 1 year ago

Should we make path= a part of our vocabulary? path is kind of a Catherine-ism, just what I used for years to refer to this sort of tuple. It's entirely possible another name is better.

In the latest changes, I have kept path= in CSR and Wishbone Interface.__init__(), as they are wrappers of lib.wiring.Interface with additional metadata.

However, I have removed path= from Decoder and Arbiter.__init__(). The name of their bus interfaces is an implementation detail. This is consistent with how things were before this PR.

whitequark commented 1 year ago

@jfng

In the latest changes, I have kept path= in CSR and Wishbone Interface.__init__(), as they are wrappers of lib.wiring.Interface with additional metadata.

Thanks. My question is actually broader in scope than just SoC; I wonder if "path" is really a good term for this in general, including Amaranth itself.

jfng commented 1 year ago

Thanks. My question is actually broader in scope than just SoC; I wonder if "path" is really a good term for this in general, including Amaranth itself.

I prefer "path" over alternatives like "name", as it conveys the idea that it is a sequence of strings, rather that a string itself.

Amaranth SoC uses "name" for a similar situation (resource names are prefixed by names of parent windows when iterating MemoryMap.all_resources()), and I think "path" would be appropriate here too.

jfng commented 1 year ago

The following test fails with Python 3.8:

FAIL: test_trigger_wrong (tests.test_event.SourceSignatureTestCase)
----------------------------------------------------------------------
ValueError: 'foo' is not a valid Trigger

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/runner/work/amaranth-soc/amaranth-soc/tests/test_event.py", line 54, in test_trigger_wrong
    src = event.Source.Signature(trigger="foo")
AssertionError: "'foo' is not a valid Source.Trigger" does not match "'foo' is not a valid Trigger"

But passes with Python 3.11.5

jfng commented 1 year ago

The following test fails with Python 3.8:

FAIL: test_trigger_wrong (tests.test_event.SourceSignatureTestCase)
----------------------------------------------------------------------
ValueError: 'foo' is not a valid Trigger

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/runner/work/amaranth-soc/amaranth-soc/tests/test_event.py", line 54, in test_trigger_wrong
    src = event.Source.Signature(trigger="foo")
AssertionError: "'foo' is not a valid Source.Trigger" does not match "'foo' is not a valid Trigger"

But passes with Python 3.11.5

This error message format was changed in Python 3.9.

For consistency across supported Python versions, we can raise our own ValueError instead.

jfng commented 1 year ago

In the latest changes, Interface constructors take the same parameters as their corresponding Signatures (in addition to path=)`.

jfng commented 1 year ago

Merged. Thanks @whitequark for the review!