amaranth icon indicating copy to clipboard operation
amaranth copied to clipboard

Using Enum as FSM state does not respect the Enum value

Open andresmanelli opened this issue 3 years ago • 3 comments

Maybe this is an expected behavior, but if you declare an enum like

class myEnum(enum.Enum):
    STATE_A = 0
    STATE_B = 1
    STATE_C = 2

and then create a FSM like

with m.FSM() as fsm:
    with m.State(myEnum.STATE_B):
        [...]
        m.next = myEnum.STATE_C
    with m.State(myEnum.STATE_A):
        [...]
        m.next = myEnum.STATE_B
    with m.State(myEnum.STATE_C):
        [...]
        m.next = myEnum.STATE_A

Then the states will be mapped as

myEnum.STATE_B/0
myEnum.STATE_C/1
myEnum.STATE_A/2

which breaks the comparison of signals declared with the enum, like

a = Signal(myEnum)
# Let's say state is STATE_B
a = fsm.state
# Then this fails
assert(yield a == myEnum.STATE_B)

I understand from dsl.py that the states are created in a first-come first-serve basis (If I'm not wrong), so there is any way of making the assertion hold ?

Thanks !

andresmanelli avatar Feb 16 '22 21:02 andresmanelli

That's completely unintentional--the FSM states are supposed to be strings only at the moment.

I agree that having enum variants be usable as FSM states would be a nice feature.

whitequark avatar Feb 16 '22 22:02 whitequark

Adding to the 0.4 milestone, with the change being to deprecate anything but strings being used as FSM state names.

whitequark avatar Jan 31 '23 14:01 whitequark

That's completely unintentional--the FSM states are supposed to be strings only at the moment.

Now that I'm looking at the code again, I think this might have been intentional (I don't recall)--it's clearly written such that any Python object can be used as the state name, not just a string.

I don't think we should touch the existing FSM interface at this point. It works but it's crude and limited. I'll write an RFC for a better FSM interface and then we can deprecate the existing one wholesale.

whitequark avatar Feb 16 '23 22:02 whitequark