Impressions: Command Based Robot

[DjScribbles]

As an example of what I mean by business logic, here is the bulk of code from our collector:

We need to know the state of the collector when we control our arm (the two can collide while the collector is up). So having the collector subsystem know it's state seems like the appropriate way to manage this.

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

Collector::CollectorState Collector::GetState()
{
	if ((timeTravel.Get() >= TIME_TRAVELING_UP) && (collectorLifter->Get() == DoubleSolenoid::kReverse))
	{
		return UP;
	}
	if ((timeTravel.Get() < TIME_TRAVELING_UP) && (collectorLifter->Get() == DoubleSolenoid::kReverse))
	{
		return TRAVELING_UP;
	}
	if ((timeTravel.Get() >= TIME_TRAVELING_DOWN) && (collectorLifter->Get() == DoubleSolenoid::kForward))
	{
		return DOWN;
	}
	if ((timeTravel.Get() < TIME_TRAVELING_DOWN) && (collectorLifter->Get() == DoubleSolenoid::kForward))
	{
		return TRAVELING_DOWN;
	}
	return IDLE;
}

It doesn't seem appropriate to tuck the state into a command somewhere. I could see setting the currentState on the End of command though; however the logic of how long it takes to actuate still seems more appropriate in the subsystem, because it is an attribute of that subsystem, not of the command that triggered the action.