[MrRoboSteve]

Originally Posted by DjScribbles

void Collector::MoveCollector(bool extend)
{
	if (extend == true)
	{
		if (collectorLifter->Get() != DoubleSolenoid::kForward)
		{
			timeTravel.Reset();
			timeTravel.Start();
		}
		collectorLifter->Set(DoubleSolenoid::kForward);
	}
...

A couple things we do to make code like this easier to read:

1. Don't test Booleans against true, just say "if (extend)"
2. Up your abstraction in your subsystem a bit -- for example, instead of saying

collectorLifter->Get() != DoubleSolenoid::kForward

make a method so that you can say

collectorLifter->IsForward()

3. Similarly, create methods like

collectorLifter->MoveForward()

4. Once you do this, some of the logic that is in your subsystem becomes easier to move into a command.

We agree that state is best maintained in the subsystem, not a command. I think it's easier to treat commands as transients.

A final note -- we would use some sort of sensor (limit switches, encoders, or magnetic reed switches on the air cylinder) to detect the position of the actuator, rather than using timing, particularly in cases where the actuator's position can interfere with another part of the robot.

If you look at my other post in this thread, there's an example of the supervisor command pattern that I really like for subsystems that require regular observation to work -- your subsystem qualifies in that you have the transient states where it is moving from one goal state to another.