Skip to content

Conversation

@SusanC3
Copy link
Contributor

@SusanC3 SusanC3 commented Mar 30, 2022

No description provided.

@SusanC3 SusanC3 requested review from AndrewKim1S and masoug March 30, 2022 05:59
@masoug
Copy link

masoug commented Mar 30, 2022

Unfortunately github doesn't let me comment lines that didn't change so I'm putting it here as a reminder:

Add break statements to shooter state machine: https://github.com/Eaglestrike/2022-RobotCode/blob/hoodRaisedZero/src/main/cpp/Shooter.cpp#L126-L131

Comment on lines 232 to 248
if(m_hood.GetSupplyCurrent() >= ShooterConstants::zeroingcurrent){
m_hood.Set(ControlMode::PercentOutput, 0.0);
return;
}
else if(m_hood.GetSelectedSensorPosition() > ShooterConstants::hoodMax &&
m_angle >= ShooterConstants::hoodMax){
m_hood.Set(ControlMode::PercentOutput, 0.0);
return;
}
else if(m_hood.GetSelectedSensorPosition() < ShooterConstants::hoodMin &&
m_angle <= ShooterConstants::hoodMin){
m_hood.Set(ControlMode::PercentOutput, 0.0);
return;
}
else {
m_hood.Set(ControlMode::Position, m_angle);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest refactoring this into:

Suggested change
if(m_hood.GetSupplyCurrent() >= ShooterConstants::zeroingcurrent){
m_hood.Set(ControlMode::PercentOutput, 0.0);
return;
}
else if(m_hood.GetSelectedSensorPosition() > ShooterConstants::hoodMax &&
m_angle >= ShooterConstants::hoodMax){
m_hood.Set(ControlMode::PercentOutput, 0.0);
return;
}
else if(m_hood.GetSelectedSensorPosition() < ShooterConstants::hoodMin &&
m_angle <= ShooterConstants::hoodMin){
m_hood.Set(ControlMode::PercentOutput, 0.0);
return;
}
else {
m_hood.Set(ControlMode::Position, m_angle);
}
const double curr_hood_angle = m_hood.GetSelectedSensorPosition();
const bool hood_overcurrent = m_hood.GetSupplyCurrent() >= ShooterConstants::zeroingcurrent;
const bool hood_too_high = curr_hood_angle > ShooterConstants::hoodMax &&
m_angle >= ShooterConstants::hoodMax;
const bool hood_too_low = curr_hood_angle < ShooterConstants::hoodMin &&
m_angle <= ShooterConstants::hoodMin;
if (hood_overcurrent || hood_too_high || hood_too_low) {
m_hood.Set(ControlMode::PercentOutput, 0.0);
} else {
m_hood.Set(ControlMode::Position, m_angle);
}
  • make one call to GetSelectedSensorPosition() and cache the value in a local variable so you're not repeatedly hammering the CAN bus for the same data.
  • remove the return statements in each branch since the cases are already mutually-exclusive. leaving them in might cause bugs later on if stuff is added after this code block (such as the smartdashboard call).

@masoug
Copy link

masoug commented Mar 30, 2022

I'd personally avoid mixing and matching between m_hood.Set(ControlMode::PercentOutput, ...) and m_hood.Set(ControlMode::Position, ...); since it's not super clear what the motor's behavior is when rapidly switching between closed-loop position control versus open-loop power commands. Could this cause the motor to behave weird if the hood angle is close to the upper/lower limits? Perhaps it's better to do m_hood.Set(ControlMode::Position, ShooterConstants::hoodMax (or min) ); when m_angle is out of bounds?

I think it's easier to debug hood issues if the motor is always commanded as PercentOutput and we just use the WPIlib PID implementation. That simplifies our understanding of the motor's behavior, and we can more closely examine (based on what comes out of pid.Calculate()) why the hood shoots at the wrong position. I wouldn't worry about the control loop performance at this stage yet, even if the Talon implementation is better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants