Refactoring #17

Open
Irishumanoid wants to merge 43 commits from refactoring into main
Irishumanoid commented 2023-09-27 13:07:30 -05:00 (Migrated from github.com)

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.

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.
garrettwitowski (Migrated from github.com) requested changes 2023-10-04 15:06:13 -05:00
garrettwitowski (Migrated from github.com) left a comment

Please remove unnecessary/unrelated code from the repo, including custom PID.

Please remove unnecessary/unrelated code from the repo, including custom PID.
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:06:39 -05:00

Please re-add this plugin and ensure you run it on the code

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'
}
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:06:52 -05:00

Please re-add spotless

Please re-add spotless
@ -0,0 +3,4 @@
import edu.wpi.first.math.interpolation.Interpolatable;
/**Implement using TreeMapInterpolator*/
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:07:24 -05:00

What is this?

What is this?
@ -0,0 +4,4 @@
/**Implement using TreeMapInterpolator*/
public class ExponentialInterpolator implements Interpolatable<ExponentialInterpolator>, InverseInterpolator<ExponentialInterpolator> {
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:07:52 -05:00

What's the purpose of this class? Can you add a brief description please?

What's the purpose of this class? Can you add a brief description please?
@ -0,0 +1,103 @@
package frc.Lib.MotionProfiles;
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:42:23 -05:00

We should chat about all of this profiling -- I'd like to better understand your line of thinking here.

We should chat about all of this profiling -- I'd like to better understand your line of thinking here.
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:09:32 -05:00

Please remove custom PID

Please remove custom PID
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:11:08 -05:00

nit: consider making magic numbers class constants (you don't need to do this now, just something to keep in mind for the future)

nit: consider making magic numbers class constants (you don't need to do this now, just something to keep in mind for the future)
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:40:16 -05:00

Do we need this in the 2023 season code base?

Do we need this in the 2023 season code base?
@ -176,6 +183,22 @@ public final class Constants {
kMaxAngularSpeedRadiansPerSecond,
kMaxAngularSpeedRadiansPerSecondSquared
);
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:42:58 -05:00

We don't have a shooter, right? Let's remove anything related to shooter from this year's codebase.

We don't have a shooter, right? Let's remove anything related to shooter from this year's codebase.
garrettwitowski (Migrated from github.com) commented 2023-09-28 11:43:42 -05:00

What is this for? It'd help to have a little bit of documentation on the class. :)

What is this for? It'd help to have a little bit of documentation on the class. :)
garrettwitowski (Migrated from github.com) commented 2023-10-04 14:56:55 -05:00

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?

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 possible
String logPath = LogFileUtil.findReplayLog(); // Pull the replay log from AdvantageScope (or prompt the user)
Logger.setReplaySource(new WPILOGReader(logPath)); // Read replay log
garrettwitowski (Migrated from github.com) commented 2023-10-03 12:02:46 -05:00

Let's remove this instead of commenting it out

Let's remove this instead of commenting it out
garrettwitowski (Migrated from github.com) commented 2023-10-03 12:06:53 -05:00

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.

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.
garrettwitowski (Migrated from github.com) commented 2023-10-03 12:15:54 -05:00

What does getGamePieceHighSetpoint do here? Consider separating the logic for setpoints/positions out from the command logic.

What does getGamePieceHighSetpoint do here? Consider separating the logic for setpoints/positions out from the command logic.
garrettwitowski (Migrated from github.com) commented 2023-10-03 12:17:36 -05:00

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).

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();
}
garrettwitowski (Migrated from github.com) commented 2023-10-03 12:11:22 -05:00

super nit: this could potentially return null (if the lookup fails) -- consider guarding against that

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 SmartDashboard
double p = SmartDashboard.getNumber("P Gain", 1);
garrettwitowski (Migrated from github.com) commented 2023-10-04 15:04:06 -05:00

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?

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?
garrettwitowski (Migrated from github.com) commented 2023-10-04 14:45:47 -05:00

Please remove commented out code

Please remove commented out code
garrettwitowski (Migrated from github.com) commented 2023-10-04 14:46:49 -05:00

This should be unused right?

This should be unused right?
garrettwitowski (Migrated from github.com) commented 2023-10-04 15:04:47 -05:00

nit: unless we need to operate on the encoder directly, we should simplify this to just return the position of the encoder.

nit: unless we need to operate on the encoder directly, we should simplify this to just return the position of the encoder.
garrettwitowski (Migrated from github.com) commented 2023-10-04 14:48:16 -05:00

Why is this included here? We don't have a shooter on the 2023 robot ;)

Why is this included here? We don't have a shooter on the 2023 robot ;)
@ -42,7 +42,7 @@ public class ChargeStationStates {
}
garrettwitowski (Migrated from github.com) commented 2023-10-04 14:50:12 -05:00

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 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
garrettwitowski (Migrated from github.com) commented 2023-10-04 14:50:23 -05:00

Why?

Why?
garrettwitowski (Migrated from github.com) commented 2023-10-04 14:50:44 -05:00

Do we need this?

Do we need this?
garrettwitowski (Migrated from github.com) commented 2023-10-04 14:51:17 -05:00

Why do we have this class? If you want this, it'd be better to include it in a separate package/project.

Why do we have this class? If you want this, it'd be better to include it in a separate package/project.
garrettwitowski (Migrated from github.com) reviewed 2023-10-04 15:11:12 -05:00
@ -0,0 +37,4 @@
public double position; //rotations
public double velocity; //rpm
garrettwitowski (Migrated from github.com) commented 2023-10-04 15:11:12 -05:00

Should just include this as a requirement to calculate then ;)

Should just include this as a requirement to calculate then ;)
garrettwitowski (Migrated from github.com) requested changes 2023-10-04 15:13:43 -05:00
garrettwitowski (Migrated from github.com) left a comment

Let's try to keep experimental changes out of these CRs -- reduces noise in the commit history and makes us less error prone :)

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 testing
public class ElevatorDirectProfileCommand extends CommandBase {
garrettwitowski (Migrated from github.com) commented 2023-10-04 15:12:40 -05:00

This appears fine after just a quick spot check, but aren't we missing a pre-requisite here for the velocity control?

This appears fine after just a quick spot check, but aren't we missing a pre-requisite here for the velocity control?
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin refactoring:refactoring
git switch refactoring

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.

git switch main
git merge --no-ff refactoring
git switch refactoring
git rebase main
git switch main
git merge --ff-only refactoring
git switch refactoring
git rebase main
git switch main
git merge --no-ff refactoring
git switch main
git merge --squash refactoring
git switch main
git merge --ff-only refactoring
git switch main
git merge refactoring
git push origin main
Sign in to join this conversation.
No description provided.