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

Signal decoder inference for Enums doesn't work for Layout members #393

Closed
shawnanastasio opened this issue May 29, 2020 · 3 comments
Closed
Milestone

Comments

@shawnanastasio
Copy link
Contributor

@shawnanastasio shawnanastasio commented May 29, 2020

This is a duplicate of m-labs/nmigen#333 that I've moved here since this seems to be the correct upstream repository.

When creating a signal with shape set to an Enum-derived type, the decoder function is automatically inferred to a built-in function that turns the Enum value and name into a nice human-readable string.

When defining a layout with an field's shape set to an Enum-derived type though, the decoder inference doesn't happen. As far as I can see, this is because the Layout constructor will cast all fields' shapes with Shape.cast, resulting in the original Enum type being lost.

https://github.com/nmigen/nmigen/blob/26a15b31f7fc278783cf9286d5af1877edc8290f/nmigen/hdl/rec.py#L48-L50

This results in an actual Shape object being passed to the Signal constructor instead of an Enum, and thus the built in Enum decoder can't be used.

I can think of a few ways to fix this, but wanted to hear the maintainers' thoughts to see if they have a preferred way of going about it.

My first idea is to not cast shapes in the Layout constructor, but instead check whether the user-provided type is a valid shape, probably by calling Shape.cast, discarding the value, and checking for an exception.

The other idea is to add a field to Shape which stores the original type that a given object was casted from. This would also require some extra logic in the Signal constructor to not only check the shape's type when checking for default decoders, but also checking the original type of casted Shape objects.

Curious to hear any thoughts on this.

Update: To test out the first idea, I simply removed the shape = from line 50 of the above code and it seems to behave as expected. If this is deemed an acceptable solution I can create a PR.

@whitequark
Copy link
Member

@whitequark whitequark commented May 30, 2020

As far as I can see, this is because the Layout constructor will cast all fields' shapes with Shape.cast, resulting in the original Enum type being lost.

Yep.

My first idea is to not cast shapes in the Layout constructor, but instead check whether the user-provided type is a valid shape, probably by calling Shape.cast, discarding the value, and checking for an exception.

That seems like a reasonable solution which is otherwise inline with the current implementation.

@shawnanastasio
Copy link
Contributor Author

@shawnanastasio shawnanastasio commented May 30, 2020

Though it seems to fix the original issue, removing the shape cast assignment causes a few test cases to fail:

======================================================================
FAIL: test_slice_tuple (nmigen.test.test_hdl_rec.RecordTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/shawnanastasio/opt/fpga/nmigen/nmigen/test/test_hdl_rec.py", line 199, in test_slice_tuple
    r2 = r1["a", "c"]
  File "/home/shawnanastasio/opt/fpga/nmigen/nmigen/hdl/rec.py", line 164, in __getitem__
    for field_name, field_value in self.fields.items()
  File "/home/shawnanastasio/opt/fpga/nmigen/nmigen/hdl/rec.py", line 137, in __init__
    assert isinstance(field, Signal) and field_shape == field.shape()
AssertionError

Should these tests be changed to check if the field's shape matches the Signal's shape after calling Shape.cast, rather than a direct comparison?

@whitequark
Copy link
Member

@whitequark whitequark commented May 30, 2020

Yep

shawnanastasio added a commit to shawnanastasio/nmigen that referenced this issue May 31, 2020
Don't save the result of Shape.cast in the Layout constructor.
This allows the original user-provided shape to be preserved,
while still checking its validity.

This fixes Enum decoder inference for Signals (amaranth-lang#393).
shawnanastasio added a commit to shawnanastasio/nmigen that referenced this issue May 31, 2020
Don't save the result of Shape.cast in the Layout constructor.
This allows the original user-provided shape to be preserved,
while still checking its validity.

This fixes Enum decoder inference for Signals (amaranth-lang#393).
shawnanastasio added a commit to shawnanastasio/nmigen that referenced this issue Jun 2, 2020
Don't save the result of Shape.cast in the Layout constructor.
This allows the original user-provided shape to be preserved,
while still checking its validity.

This fixes Enum decoder inference for Signals (amaranth-lang#393).
shawnanastasio added a commit to shawnanastasio/nmigen that referenced this issue Jun 3, 2020
Don't save the result of Shape.cast in the Layout constructor.
This allows the original user-provided shape to be preserved,
while still checking its validity.

This fixes Enum decoder inference for Signals (amaranth-lang#393).
shawnanastasio added a commit to shawnanastasio/nmigen that referenced this issue Jun 3, 2020
Don't save the result of Shape.cast in the Layout constructor.
This allows the original user-provided shape to be preserved,
while still checking its validity.

This fixes Enum decoder inference for Signals (amaranth-lang#393).
@whitequark whitequark added this to the 0.3 milestone Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants