Refactoring #17
No reviewers
Labels
No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
chainlynx/2023_Season!17
Loading…
Reference in a new issue
No description provided.
Delete branch "refactoring"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Removed auto-elevator, auto-intake, and auto-retract commands by setting game piece state in separate instant command. Condensed triggers in RobotContainer. Added AutoRoutine builder.
Please remove unnecessary/unrelated code from the repo, including custom PID.
Please re-add this plugin and ensure you run it on the code
@ -99,6 +99,8 @@ tasks.withType(JavaCompile) {options.compilerArgs.add '-XDstringConcat=inline'}Please re-add spotless
@ -0,0 +3,4 @@import edu.wpi.first.math.interpolation.Interpolatable;/**Implement using TreeMapInterpolator*/What is this?
@ -0,0 +4,4 @@/**Implement using TreeMapInterpolator*/public class ExponentialInterpolator implements Interpolatable<ExponentialInterpolator>, InverseInterpolator<ExponentialInterpolator> {What's the purpose of this class? Can you add a brief description please?
@ -0,0 +1,103 @@package frc.Lib.MotionProfiles;We should chat about all of this profiling -- I'd like to better understand your line of thinking here.
Please remove custom PID
nit: consider making magic numbers class constants (you don't need to do this now, just something to keep in mind for the future)
Do we need this in the 2023 season code base?
@ -176,6 +183,22 @@ public final class Constants {kMaxAngularSpeedRadiansPerSecond,kMaxAngularSpeedRadiansPerSecondSquared);We don't have a shooter, right? Let's remove anything related to shooter from this year's codebase.
What is this for? It'd help to have a little bit of documentation on the class. :)
Topic for future discussion (i.e., no action needed now): this is a good thought -- might we want to consider other states/pre-conditions in other autos? How can we design our code such that it accounts for these in a more implicit way?
@ -35,0 +58,4 @@} else {setUseTiming(false); // Run as fast as possibleString logPath = LogFileUtil.findReplayLog(); // Pull the replay log from AdvantageScope (or prompt the user)Logger.setReplaySource(new WPILOGReader(logPath)); // Read replay logLet's remove this instead of commenting it out
nit: These binding names are a bit confusing at first glance -- I'd advise renaming them for clarity if possible.
To clarify: the confusion comes from iterating through this array of Bindings -- perhaps renaming your loop-control variable something like "position binding" or "elevatorPositionBinding" or something else more illustrative would help address this confusion.
What does getGamePieceHighSetpoint do here? Consider separating the logic for setpoints/positions out from the command logic.
This... does nothing right? If this does do something, I strongly recommend against it -- you're modifying an input parameter, which is usually not a great idea (you can get into weird situations where this is being used in unexpected ways).
@ -245,0 +256,4 @@Map.of("init retract", new IntakeCommand(m_intake, 0.5).withTimeout(0.8))).build();}super nit: this could potentially return null (if the lookup fails) -- consider guarding against that
@ -90,3 +105,3 @@public void moveElevator(Double setpoint) {public void moveElevator(double setpoint) {// read PID coefficients from SmartDashboarddouble p = SmartDashboard.getNumber("P Gain", 1);Can you help me understand why we want repeated (periodic) calls to this? We're not adjusting this.elevatorSetpoint in this method, so why do we need the periodic calls?
Please remove commented out code
This should be unused right?
nit: unless we need to operate on the encoder directly, we should simplify this to just return the position of the encoder.
Why is this included here? We don't have a shooter on the 2023 robot ;)
@ -42,7 +42,7 @@ public class ChargeStationStates {}Why this change? Also, this comment just implies that this function didn't used to do this -- a better comment would be to explain why we set the state to "climbing" in this function
Why?
Do we need this?
Why do we have this class? If you want this, it'd be better to include it in a separate package/project.
@ -0,0 +37,4 @@public double position; //rotationspublic double velocity; //rpmShould just include this as a requirement to calculate then ;)
Let's try to keep experimental changes out of these CRs -- reduces noise in the commit history and makes us less error prone :)
@ -0,0 +10,4 @@import frc.robot.Subsystems.ElevatorSubsystem;//bind this command to a trigger in RobotContainer for testingpublic class ElevatorDirectProfileCommand extends CommandBase {This appears fine after just a quick spot check, but aren't we missing a pre-requisite here for the velocity control?
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.