Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for multiple PID slots in MotorController #49

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dejabot
Copy link
Contributor

@dejabot dejabot commented Feb 23, 2024

Description

  • Add support for multiple PID slots in MotorController.
    • NOTE: Slot-specific Output Range only supported on REV. Need to decide how to handle. Currently part of signature but slot ignored on CTRE.
    • Need to add support in LocalMotorController
    • Currently only supporting up to two PID slots for motors. Will add support (tho note that placeholders in config may start getting unwieldy) after proving this direction.
  • Add support to refresh PID values (if specified in config files) when set is called.
  • Fix CAN Spark Max HAL support for follow wrt inverting the follower motor.

How Has This Been Tested?

Needs testing. Sending for early feedback.

  • Unit tests: [Add your description here]
  • Simulator testing: [Add your description here]
  • On-robot bench testing: [Add your description here]
  • On-robot field testing: [Add your description here]

@dejabot dejabot requested a review from rcahoon February 23, 2024 12:13
if PID values are set via config, support live refreshes of these values when set() is called.
@@ -23,16 +30,17 @@ public double getSensorVelocity() {
}

@Override
public void set(final ControlMode mode, final double value) {
public void set(
final ControlMode mode, final double value, int slot, double arbitraryFeedForward) {
switch (mode) {
case PercentOutput:
delegate.set(mode, value);
Copy link
Member

Choose a reason for hiding this comment

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

should these other control modes also pass along the slot and arbFF?


@Override
public void setOutputRange(final double minOutput, final double maxOutput, int slot) {
if (slot != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

the behavior might be confusing to the user - if an output range is set on Slot 0 but not on Slot 1, then the range bound will apply to both slots without warning the user

.logRaw(
Severity.WARNING,
"Ignoring slot for setOutputRange - unsupported on Talon");
}
Copy link
Member

Choose a reason for hiding this comment

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

potentially confusing behavior, as above

}

@Override
public void set(final ControlMode mode, double value) {
public void set(final ControlMode mode, double value, int slot, double arbitraryFeedForward) {
Copy link
Member

Choose a reason for hiding this comment

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

should this have m_device.selectProfileSlot like in CANVictorMotorController?

.logRaw(
Severity.WARNING,
"Ignoring slot for setOutputRange - unsupported on Talon");
}
Copy link
Member

Choose a reason for hiding this comment

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

potentially confusing behavior, as above

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.

2 participants