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 elevator #10

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

Add elevator #10

wants to merge 21 commits into from

Conversation

aidnem
Copy link

@aidnem aidnem commented Dec 13, 2024

Adds:

  • Elevator subsystem (to be renamed later once the subsystem gains more functionality than just an elevator)
  • Elevator mechanism (for control/management of the elevator itself)
  • Elevator IOs for talonFX and then a sim IO that extends TalonFX and uses phoenix's sim
  • Tuning mode for elevator, can tune profile, PID, and FF.

Elevator uses units library in all places possible.

@aidnem aidnem linked an issue Dec 13, 2024 that may be closed by this pull request
5 tasks
@aidnem aidnem requested a review from jkleiber December 13, 2024 17:15
@aidnem
Copy link
Author

aidnem commented Dec 13, 2024

Here's verification of it functioning in sim: I'm setting various setpoints and it's controlling to them (needs slight tuning, as it's off by <0.3 rotations).

Elevator1.mp4

Copy link
Member

@jkleiber jkleiber left a comment

Choose a reason for hiding this comment

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

First pass. Since this is functioning well I want to get the code as clean as possible and leverage coppercore so this can be an example for other subsystems

src/main/java/frc/robot/RobotContainer.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/RobotContainer.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/RobotContainer.java Outdated Show resolved Hide resolved
import edu.wpi.first.units.measure.Mass;
import edu.wpi.first.units.measure.Per;

public final class ElevatorConstants {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the coppercore JSON stuff here? I think it should be ready to try out

@@ -0,0 +1,5 @@
package frc.robot.constants;

public final class FeatureFlags {
Copy link
Member

Choose a reason for hiding this comment

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

We definitely should use coppercore JSON parameter stuff here so we can turn features on/off without recompiling

Comment on lines +210 to +212
} else {
System.out.println("ERROR: Couldn't find solution to seed elevator with CRT");
}
Copy link
Member

Choose a reason for hiding this comment

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

One thing Preston and I discussed after shop on Wednesday is if we should add a small voltage pulse train to push the system out of backlash if this happens. We should create a ticket for handling this case to mitigate the risk of the elevator not functioning correctly. We have the following mitigations available:

  1. LEDs could display pre-match if CRT is seeded
  2. Use the roboRIO user button or some other protected DIO to set that the elevator is at 0 (backup if CRT is always failing pre-match)
  3. send a voltage pulsetrain to wiggle the elevator and get a solution from CRT when unseeded (helpful if code restarts mid match)

Copy link
Author

Choose a reason for hiding this comment

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

Want to gently nudge it down at .5 volts alternating on/off every like 0.5 seconds or something like that?
If so, where would you prefer to handle the scheduling for that? I've never done tiny scale time based control before. I could just check whether ((timestamp % 1) < 0.5) or something and then use that to decide whether or not to apply negative power.

Copy link
Member

Choose a reason for hiding this comment

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

Let's just defer it to a new ticket and discuss there if it becomes needed

*/
public void setGoalHeight(Distance goalHeight) {
// TODO: 2025 coppercore solution for easy value clamping
// https://github.com/team401/coppercore/issues/64
Copy link
Member

Choose a reason for hiding this comment

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

this issue is closed so let's use the coppercore feature


Logger.recordOutput("elevator/goalHeight", goalHeight);

sendGoalHeightToIO();
Copy link
Member

Choose a reason for hiding this comment

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

Why are we doing this here if we are doing it in periodic? I think we should just be doing this in periodic since this function is a simple setter

Comment on lines +257 to +265
public void updateClampedGoalHeight() {
if (goalHeight.lt(minHeight)) {
clampedGoalHeight.mut_replace(minHeight);
} else if (goalHeight.gt(maxHeight)) {
clampedGoalHeight.mut_replace(maxHeight);
} else if (!clampedGoalHeight.equals(goalHeight)) {
clampedGoalHeight.mut_replace(goalHeight);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we be using the value clamping from coppercore here as well?

Copy link
Author

Choose a reason for hiding this comment

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

@jkleiber I need to publish a release for this, should I make it 2025.0.1-beta? You mentioned re-publishing the same release but I wasn't sure in what context that would be the move.


public ElevatorSubsystem(ElevatorMechanism elevatorMechanism) {
this.elevatorMechanism = elevatorMechanism;
// elevatorMechanism.setGoalHeight(Meters.of(0.75));
Copy link
Member

Choose a reason for hiding this comment

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

We can get rid of this - it would be good to add a command that demonstrates going to various preset setpoints though

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.

Add elevator
2 participants