State Machine

[avidraccoon]

Classes

• StateMachine
• AbstractState
• StateTransition

[avidraccoon]

@jkleiber How does the current implementation look? Code is in the StateMachine Branch

[jkleiber]

@avidraccoon the current implementation looks good - I can leave some more specific comments on a PR when one is ready but this looks directionally correct for now

[avidraccoon]

@aidnem You said you wanted me to look at something, what was it?

[aidnem]

I actually think it wouldn't really work for our use-case with the way you have each state in its own file but I was just saying I liked the syntax of stateless4j. I don't think you can use most of it (also, this state machine would have to be configured in one place and that would make the init of each subsystem describe the entire state machine which would kind of be a nightmare in my opinion). Honestly don't worry about the syntax of this too much because if what you have works we should use it.

[avidraccoon]

What parts of the syntax do you like? Because mine doesn't work, and I am currently rewriting much of it and modifying the syntax.

[avidraccoon]

I think this should probably have a way to interface with monitors right?

[aidnem]

1. I liked having a generic type for the state machine, this way we could define our own state machines similarly to the syntax used below.
2. I could be wrong but I don't think need a way to interface with monitors, and if we do need a way to interface with monitors, it can be handled in the writing of the monitors themselves (each monitor just gets a supplier so it can access whatever it needs to)
   I've written a code example below to demonstrate what I think this would look like:

public class ScoringSubsystem extends MonitoredSubsystem {
  public enum ScoringState {
      IDLE (IdleState),
      PRIME (PrimeState),
      SHOOT (ShootState)
  
      // Attach the AbstractState object to each value of the enum
      private final AbstractState state;
      ScoringState(AbstractState state) {
          this.state = state;
      }
     public AbstractState state() {
          return state;
     }
  }
  
  StateMachine<ScoringState> stateMachine = new StateMachine<ScoringState>();
  public ScoringSubsystem() {
      // Do initialization
      // ....

      // Add monitors
     addMonitor(new Monitor.MonitorBuilder.withName("armEncoderUnplugged")
                                                                       .withStickyness(true)
                                                                       .withIsStateValidSupplier(() -> {/* arm encoder unplugged check ...*/)
                                                                       .withTimeToFault(0.1)
                                                                       .build())
  }

 @override
 public void monitoredPeriodic() {
    /* Do whatever periodic stuff is needed for the state machine, I'm unfamiliar with the API */
  }
}

@avidraccoon @jkleiber please let me know if you have suggestions for this syntax to be improved, this was just what I threw together off the top of my head.
P.S. Sorry for the suspicious indentation I wrote it all in the github comment editor so I didn't even have a monospace font.

[jkleiber]

Don't worry about interfacing monitors with the state machine. As long as we have a way to enable or disable monitors at runtime we can do whatever we need for that

[jkleiber]

Also, having the states separated into their own files is not a requirement from me. Here are my requirements:

1. State machine can be used in a generic way
2. State machine is easy to configure (nice to have is if it can be configured with JSON)
3. Code is very easy to read and trace through

[aidnem]

Would you mind elaborating on what you mean by configuring? What could a simple state machine's JSON potentially look like?

[avidraccoon]

I mean like make it able to get a report of any illegal state transitions attempted by action that way the monitor can access them. Or if we use something like the guarded from stateless4j we will have if about that.

[aidnem]

That's a good point. Although wouldn't it be better to just not code in any illegal state transitions. We're fully in control and if we write code that's broken up enough we might be able to just be really good and not be able to do illegal stuff in the first place.

[avidraccoon]

Yeah but it would mean we could more easily catch mistakes, things we missed when testing, or problems caused by hardware

[aidnem]

Yeah @avidraccoon I thought about it some more and I completely agree, good idea.
Also if we can describe our state machine somewhere (like a JSON file) and then use that specification to verify the behavior of our state machine at runtime using your monitor idea (or using its own separate logging) I think it would add a lot of clarity to the code. For example if we ever wondered how the state machine worked we could just look at the state machine specification JSON and know with full confidence that the code would follow it, otherwise it would be giving us warnings.

[avidraccoon]

How do you think the json should look?

[aidnem]

I'm not sure yet. I'm really tired right now so any ideas I had would probably be a little silly anyway. I'll let it percolate and if I come up with anything I'll let you know tomorrow morning :)

[avidraccoon]

@aidnem do you know how the json should look?

[aidnem]

What if we did:

{
  "name": "ScoringStateMachine",
  "states:" [
     {
       "name": "idle",
       "transitions": ["intake", "prime", "shoot", "amp_prime", "amp_shoot", "pass_prime", "pass_shoot"]
     },
     {
       "name": "intake",
       "transitions": ["idle", "prime", "amp_prime", "pass_prime"],
      },
     {
        "name": "prime",
        "transitions": ["idle", "shoot", "amp_prime", "pass_prime"],
     },
     {
       "name": "shoot",
       "transitions": ["idle", "prime"],
     },
     {
        "name": "amp_prime",
        "transitions": ["idle", "amp_shoot", "prime", "pass_prime"],
     },
     {
       "name": "amp_shoot",
       "transitions": ["idle", "amp_prime"],
     },
     {
        "name": "pass_prime",
        "transitions": ["idle", "pass_shoot", "prime", "amp_prime"],
     },
     {
       "name": "pass_shoot",
       "transitions": ["idle", "pass_prime"],
     }
  ]
}

[avidraccoon]

I just pushed a potential major change to how the state machine works it doesn't have the Json yet, @aidnem could you take a look at it quickly?

[aidnem]

At first glance that looks pretty good, I see you implemented the state machine syntax I was talking about. There's a lot of logic so I'll read it a couple more times since a lot of it went over my head.

Something we could consider: could we an interface like abstract state back in, and maybe call it something like state executor (or keep it named AbstractState like I put in the example)?
Then we could make our state machine types safer:

// StateEnum.java
interface StateEnum {
  public AbstractState state();
}
// AbstractState.java
public interface AbstractState {
  public void onEntry() {}
  public void onExit() {}
  public void periodic() {}
}
// StateMachine.java
public class StateMachine<State implements StateEnum, Trigger> { /* state machine class ... */}
// ScoringSubsystem
public class ScoringSubsystem extends MonitoredSubsystem {
  public enum ScoringState implements StateEnum {
      IDLE (IdleState), // IdleState, PrimeState, and ShootState are all classes that implement AbstractState and are imported from other files.
      PRIME (PrimeState),
      SHOOT (ShootState)
  
      // Attach the AbstractState object to each value of the enum
      private final AbstractState state;
      ScoringState(AbstractState state) {
          this.state = state;
      }
     public AbstractState state() {
          return state;
     }
  }

Let me know if this seems at all sane to you.

[avidraccoon]

Are we wanting to use enums or separate files?

[aidnem]

My solution was to use enums that then referenced states from separate files. That way we can use the enum when we need to use enum values but still split states into separate files.

@jkleiber what's your opinion on this?

[jkleiber]

@aidnem I think using enums to represent the available states is fine. I think defining states in separate files is also fine.

I assume you would be mapping the current state (enum type) to the state definition (some sort of class derived from an abstract State class). I think that pattern seems good

[aidnem]

I assume you would be mapping the current state (enum type) to the state definition (some sort of class derived from an abstract State class). I think that pattern seems good

Yes, this is correct. I've updated the code snippet with a comment indicating this.

[avidraccoon]

@jkleiber Do we prefer functionality or loading from json?

[jkleiber]

@avidraccoon why can't we have both?

But to start out I would take functionality and then we can build in the JSON capability

[avidraccoon]

@jkleiber With the library I used for json I don't know how to map a json value to a java enum value, but I can look into it.

[avidraccoon]

Never mind it is possible, but some part might not be possible do to how java works not sure I'll have to do more research, but almost all of it will be able to be loaded from Json eventually.

[avidraccoon]

@jkleiber do you want me to prioritize merging this branch and then adding more feature or just add them now?

[avidraccoon]

@aidnem could write some example of how you want it to work with configuration so that I can write Tests based off of the example?

[jkleiber]

@avidraccoon since it probably won't be a very high priority to configure the state machine with JSON, let's just try to get the state machine functional and then we can make what I call a "deferred ticket" to make it JSON configurable later.

There are certainly higher priority tasks to work on between now and making state machine JSON configurable

[jkleiber]

Also please make a draft PR to make the branch more visible for review