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

Consider being able to access FSM next #205

Closed
RobertBaruch opened this issue Sep 14, 2019 · 12 comments
Closed

Consider being able to access FSM next #205

RobertBaruch opened this issue Sep 14, 2019 · 12 comments
Labels

Comments

@RobertBaruch
Copy link

I've hit a case in my Z80 processor where I have a module with an FSM. Based on that FSM's next, I'd like to assign some signals for some other modules. As an example, the sequencer module that contains the FSM might set the FSM's next to "INITIATE_READ". This should cause the machine cycler module to begin a memory read cycle.

Currently, the only way to do that seems to be to ensure that everywhere I do m.next = "INITIATE_READ" I also do something like self.next_cycle.eq(MCycle.MEMRD). This is clearly error-prone. I could just abstract that into a method and call it something like initiate_read(). But how about the ability to just set up a m.d.comb that assigns self.next_cycle based on fsm.next?

@whitequark
Copy link
Contributor

oMigen had a feature similar to this and it was extremely prone to combinatorial loops, so I never ported it to nMigen. This feature request sounds a bit like an XY problem. Can you give more details about your code? Hopefully there is a more elegant way to solve it (or maybe not, and the motivation would be stronger then).

@emilazy
Copy link
Contributor

emilazy commented Sep 14, 2019

I think that if you can't read m.next, it should at least be m.next(...) or something rather than acting like a write-only property. I found that very confusing.

@RobertBaruch
Copy link
Author

Still a WIP, but here's a bit from the instruction sequencer module, whose purpose is, among other things, to tell the machine cycler module what machine cycle to perform after the current machine cycle is done (machine cycles consist of 3 or more clock cycles):

            with m.FSM(domain="pos", reset="RESET") as fsm:
                # By default...
                self.initiateOpcodeFetch(m)

                with m.State("RESET"):
                    m.d.pos += self.useIX.eq(0)
                    m.d.pos += self.useIY.eq(0)
                    self.dataBuffTo(m, DataBusDestination.INSTR)
                    self.addrBuffFrom(m, AddressBusSource.PC)
                    self.initiateOpcodeFetch(m)

                with m.State("M1_T1"):
                    self.dataBuffTo(m, DataBusDestination.INSTR)
                    self.addrBuffFrom(m, AddressBusSource.PC)
                    m.next = "M1_T2"

                with m.State("M1_T2"):
                    self.dataBuffTo(m, DataBusDestination.INSTR)
                    self.addrBuffFromPostInc(m, AddressBusSource.PC)
                    m.next = "M1_T3"

...

    def initiateOpcodeFetch(self, m):
        m.next = "M1_T1"
        m.d.comb += self.cycle.eq(MCycle.M1)

    def initiateMemRead(self, m):
        m.next = "RDMEM_T1"
        m.d.comb += self.cycle.eq(MCycle.MEMRD)

    def initiateMemWrite(self, m):
        m.next = "WRMEM_T1"
        m.d.comb += self.cycle.eq(MCycle.MEMWR)

So if, during execution of an instruction, the instruction needs to read memory, it can call self.initiateMemRead(m).

The alternate way would be to not have to define that function, and instead just set m.next = "RDMEM_T1" and have some other code do something like:

with m.Switch(m.next):
  with m.Case(fsm.state_for("RDMEM_T1")):
    m.d.comb += self.cycle.eq(MCycle.MEMRD)
  ...
  with m.Default():
    m.d.comb += self.cycle.eq(MCycle.M1)

In my particular case I think the function approach probably is more readable, but maybe there's a use case for reading m.next.

@RobertBaruch
Copy link
Author

Here's another example. Suppose I had an FSM with, say, 30 states. And I'd like to do some combinatorial assignment on 20 of the states, and something different on 10 of the states. I could do this:

  with m.FSM(...) as fsm:
    self.do_default_assignments(m)
    with m.Case("1"):
      ...
    with m.Case("2"):
      ...
    ...
    with m.Case("21"):
      ...
      self.do_other_assignments(m)
    with m.Case("22"):
      ...
      self.do_other_assignments(m)
    ...
    with m.Case("30"):
      ...
      self.do_other_assignments(m)

or, I could do this:

  with m.FSM(...) as fsm:
    with m.If((fsm.next_state == "21") | (fsm.next_state == "22") | ...):
      self.do_other_assignments(m)
    with m.Else():
      self.do_default_assignments(m)

I think the latter is more readable, since it groups all the logic associated with those assignments together, rather than having it scattered throughout the FSM.

@whitequark
Copy link
Contributor

Why shouldn't the last example work more like:

  with m.FSM(...) as fsm:
    self.do_default_assignments(m)
    with m.State("21", "22", ...):
      self.do_other_assignments(m)

@emilazy
Copy link
Contributor

emilazy commented Sep 14, 2019

I think it's kind of non-obvious that nmigen lets you do this Duff's-devicey stuff with cases, as opposed to being more structured like they are in languages like C#.

@whitequark
Copy link
Contributor

Right now it doesn't (#195) and well, maybe it shouldn't. Good question.

@emilazy
Copy link
Contributor

emilazy commented Sep 14, 2019

er, right; I was mixing up Switch and FSM.

@whitequark
Copy link
Contributor

Note that Switch doesn't let you do the Duff's-devicey thing, it's a priority encoder.

@emilazy
Copy link
Contributor

emilazy commented Sep 14, 2019

oh, okay. Apparently I just don't understand nmigen.

@whitequark
Copy link
Contributor

You actually raise a good objection, having FSM and Switch work in radically different ways might not be a good idea. So maybe there's some better solution to #205 (this) and #195 that I don't see.

@whitequark
Copy link
Contributor

Closing in favor of more general discussion in #206.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants