Multiple timer objects / deleted timers

[jreneew2]

Just a quick question for people who know C++ better than me,

I am writing some code for mechanisms on our robot and am trying to have one command for controlling the shooter in teleop and auto modes. I have in OI.cpp:

buttonA->WhileHeld(new ShooterControl(1)); //forwards

With my undertanding, WhileHeld makes a new instance of the command each time the previous finishes.

and in ShooterControl.cpp

#include "ShooterControl.h"

ShooterControl::ShooterControl(float speed, float target)
{
	// Use Requires() here to declare subsystem dependencies
	// eg. Requires(chassis);
	Requires(Robot::shooterSubsystem.get());
	timer.reset(new Timer());
	timer->Reset();
	timer->Start();
	timeCurrent = 0;
	timeTarget = target;
	inputSpeed = speed;
	isDone = false;

}

// Called just before this Command runs the first time
void ShooterControl::Initialize()
{

}

// Called repeatedly when this Command is scheduled to run
void ShooterControl::Execute()
{
	timeCurrent = timer->Get();
	if(timeTarget == 0) { //if zero use in teleop mode
		Robot::shooterSubsystem->Shoot(1);
		isDone = true;

	}
	else {
		if(timeCurrent >= timeTarget) {
			Robot::shooterSubsystem->Shoot(0);
			isDone = true;
		}
		else {
			Robot::shooterSubsystem->Shoot(inputSpeed);
			isDone = false;
		}
	}
}

// Make this return true when this Command no longer needs to run execute()
bool ShooterControl::IsFinished()
{
	return isDone;
}

// Called once after isFinished returns true
void ShooterControl::End()
{
	Robot::shooterSubsystem->Shoot(0);
}

// Called when another command which requires one or more of the same
// subsystems is scheduled to run
void ShooterControl::Interrupted()
{
	Robot::shooterSubsystem->Shoot(0);
}

Would this code bog down memory because of the multiple instances of the Timer Object? When does a timer get deleted?

Thanks,
Drew

[teslalab2]

If you are worried about weather instances getting deleted I think you should be declaring instances on stack and not the heap. With stack allocation do not have to be explicitly de-allocated. http://gribblelab.org/CBootcamp/7_Me...k_vs_Heap.html

[euhlmann]

The new operator creates objects on the heap, which means C++ will not clean them up automatically, and if you lose all pointers to them before cleaning them up you'll get a memory leak. As @teslalab2 said, you can allocate objects on the stack or associated with another object to have C++ automatically delete them when they go out of scope. If you need a heap object but don't want to worry about manually cleaning it up, you can use a std::shared_pointer. To create a shared Timer pointer, use std::make_shared<Timer>()

[jreneew2]

Thank you for the help! I ended up changing them to shared pointers anyway because WPILib switched to them for most of their code.

- Drew

[Joe Ross]

Originally Posted by jreneew2

With my undertanding, WhileHeld makes a new instance of the command each time the previous finishes.

Just to be clear, a new instance of the command is only created once. It is reused each time the command finishes.

[kylelanman]

In addition to Joe's point the constructor will only be called once. With that in mind you will likely need to move the following out of the constructor and into the initialize method.

timer->Reset();
timer->Start();
...
isDone = false;

This will cause them to execute each time the command is ran instead of just once when the robot is started up.

In this particular case (assuming your comments are correct) your command will only run once because isDone will already be true on subsequent executions of the command. The timer will likely also not behave as you expect if you don't reset and restart it in initialize.