Pivot by barbute · Pull Request #3 · Prosper-FRC/template-talon_standard

barbute · 2024-10-28T23:49:56Z

Pivot code to review

Makes it easier to tune PIDs

Moving subsystem hardware to its own file in order to encourage separation of subsystem logic and hardware interaction Add loggable inputs

NOTE This commit fails build checks, do not use. I am merely committing to track progress

Complies with Anshul formatting

Also add supplementary comments

Forgot to remove CANCoder object before last commit

mduffor · 2024-11-02T02:34:04Z

.vscode/settings.json

Generally one should avoid checking in editor preferences/settings into code repos.

mduffor · 2024-11-02T02:36:30Z

src/main/java/frc/robot/subsystems/pivot/PivotConstants.java

+  // NOTE The configuration only needs to be applied to the leader-motor. The
+  // follower-motor MUST obey the configuration of the leader-motor.
+  public static final KrakenConfiguration kMotorConfiguration =
+      new KrakenConfiguration(true, true, 0.0, 0.0, NeutralModeValue.Brake, false, 0.0, 0.0);


When you have a lot of settings like this (and below with PivotGains) it is useful to line them up vertically, and place an end-of-line comment after each entry telling what the setting is.

mduffor · 2024-11-02T02:37:36Z

src/main/java/frc/robot/RobotContainer.java

    configureBindings();
  }

-  private void configureBindings() {}


In the trunk we'll want to put the code block brackets on their own lines so that the templates can insert their lines between the curly brackets.

mduffor · 2024-11-02T02:39:29Z

src/main/java/frc/robot/RobotContainer.java

+import frc.robot.subsystems.pivot.PivotKrakenHardware;

 public class RobotContainer {
+  private final Pivot kPivot =


We wouldn't use the name "kPivot" because this isn't a constant. We can call it just "pivot", or perhaps "mPivot" if you like the m == instance variable naming scheme.

m - Instance (method) variable
k - Constant (Konstant :-) )
s - Static variable
g - Global variable

This still needs to be addressed

mduffor · 2024-11-02T12:52:57Z

src/main/java/frc/robot/subsystems/pivot/PivotKrakenHardware.java

+    supplyCurrentAmps = List.of(kLeadMotor.getSupplyCurrent(), kFollowMotor.getSupplyCurrent());
+    temperatureCelsius = List.of(kLeadMotor.getDeviceTemp(), kFollowMotor.getDeviceTemp());
+
+    BaseStatusSignal.setUpdateFrequencyForAll(


I think I understand what this is doing, but it's a bit obtuse. We probably need a quick description of Status Signals above, or perhaps a link to the docs. This is also the only place in the code I've seen the use of an ellipsis method signature, and rookie coders will likely be thrown off by it.

mduffor · 2024-11-05T00:19:31Z

src/main/java/frc/robot/subsystems/pivot/Pivot.java

+  }
+
+  // Hardware is where we interact with the motors and sensors directly
+  private final PivotKrakenHardware kPivotHardware;


This would have an 'm' prefix, not 'k', since its state can change. Same with kPivotInputs.
So we'd have mPivotHardware, mPivotInputs, mCurrentPivotGoal, and mCurrentPivotGoalPosition.

mduffor · 2024-11-05T00:27:28Z

src/main/java/frc/robot/subsystems/pivot/Pivot.java

+  private PivotGoal currentPivotGoal = null;
+
+  @AutoLogOutput(key = "Pivot/CurrentGoalPosition")
+  private Rotation2d currentPivotGoalPosition = new Rotation2d();


Would this variable be better named with Rotation instead of Position?

Typically any set point for control over a "position" has the suffix "position" regardless of it being for linear or angular movement (or at least that should be a team standard).

mduffor · 2024-11-05T00:31:01Z

src/main/java/frc/robot/utils/debugging/LoggedTunableNumber.java

@@ -0,0 +1,123 @@
+// Copyright (c) 2024 FRC 6328


When we pull a class in from another repo, we should include a link back to the original file. This will allow us to find the file in case there are bug fixes or we need to track down better documentation.

mduffor · 2024-11-05T00:32:53Z

src/main/java/frc/robot/utils/math/EqualsUtil.java

@@ -0,0 +1,26 @@
+// Copyright (c) FIRST and other WPILib contributors.


Same here, we should give a link to the original file. I've added comments to Anshul's use of this file, and I've rolled it into the BaseCode, so I won't repeat those comments here.

mduffor · 2024-11-05T00:34:37Z

src/main/java/frc/robot/subsystems/pivot/Pivot.java

+      setPosition(currentPivotGoalPosition.getRadians());
+    }
+
+    // Update feedback, feedforward, and motion magic gains if we change them from network tables


This is another place where an ellipsed method signature is used, so the role of this function isn't immediately evident to someone new to the code. A comment to guide readers would help.

mduffor · 2024-11-05T00:40:01Z

src/main/java/frc/robot/subsystems/pivot/PivotKrakenHardware.java

+        temperatureCelsius.get(1));
+
+    // Optimize the CANBus utilization by explicitly telling all CAN signals we
+    // are not using to simply not be sent over the CANBus


You should describe why you are passing 0 and 1, or at least use comments to give an idea of what is being passed. I assume this is the call?
https://api.ctr-electronics.com/phoenix6/release/java/com/ctre/phoenix6/hardware/ParentDevice.html#optimizeBusUtilization()

Since this code is meant to work with an arm design similar to Redux (where there are two motors working together to move a mass) some of the telemetry data will be the same. Status signals are how we get that telemetry data from a TalonFX motor controller - and it's more convenient to store them in objects that can be accessed throughout the class.

Knowing this, the internal position and velocity of the two motors must be the same (since they are mechanically linked to the arm) however the applied volts, supply current draw, and temperature would be different for the motors. So to log them more conveniently, I store them in a list where index 0 is the leader motor and index 1 is the follower motor. When setting the update frequency at which telemetry to be set, every status signal that I wish to modify must be passed into the method so I call both the leader's (0) and the follower's (1) status signals. I can add documentation if it's not clear in the code that this was just a way of conveniently storing the status signals.

mduffor · 2024-11-05T00:47:55Z

src/main/java/frc/robot/subsystems/pivot/PivotKrakenHardware.java

+   * @param v The new voltage gain
+   * @param a The new acceleration gain
+   * @param g The new gravity gain
+   */


Eventually we'll need docs or links on how to solve for kS, kV, kA, and kG.

mduffor · 2024-11-24T23:50:16Z

@barbute , have you looked over and addressed all the comments in this review yet?

mduffor · 2024-11-24T23:51:11Z

src/main/java/frc/robot/RobotContainer.java

+import frc.robot.subsystems.pivot.PivotKrakenHardware;

 public class RobotContainer {
+  private final Pivot kPivot =


This still needs to be addressed

Add custom setpoint Fully tuned arm Add soft limits for voltage control

Merge tuned pivot to pivot base

… formatting

barbute and others added 19 commits October 17, 2024 17:49

Add Pivot subsystem

c06c9b0

Add default constants for PivotConstants.java

544a3b3

Apply configurations to motors

36d2dbf

Add and log telemetry from motors

eb919a4

Add list of goals for the pivot to go to

d10c67e

Add LoggedTunableNumber.java

99430c8

Makes it easier to tune PIDs

Add control methods

e259305

Edit periodic

dcbea8b

Add pivot bindings

b8ee98c

Update README.md

3094e28

Update README.md

5661a51

Create PivotKrakenHardware.java

7de13fa

Moving subsystem hardware to its own file in order to encourage separation of subsystem logic and hardware interaction Add loggable inputs

Initialize hardware in PivotKrakenHardware.java

47aa0a5

NOTE This commit fails build checks, do not use. I am merely committing to track progress

Move hardware to PivotKrakenHardware.java

2d707ac

Add update gains methods

6616259

Make names accurate

231c4b9

Add supplementary comments

a8b0b22

Initialize LoggedTunableNumbers outsie of constructor

4d5ed91

Complies with Anshul formatting

Log position of the goal for debugging

a90f5ea

barbute requested a review from mduffor October 28, 2024 23:49

barbute added 4 commits October 28, 2024 19:08

Use motion magic instead of pure position control

df16a7e

Also add supplementary comments

More supplemental comments

c987a2b

Use REV ThroughBore as absolute encoder instead of CANCoder

6a15cd5

Cleanup PivotKrakenHardware.java

7ae78ff

Forgot to remove CANCoder object before last commit

mduffor reviewed Nov 2, 2024

View reviewed changes

.vscode/settings.json

Copy link
Copy Markdown

Contributor

mduffor Nov 2, 2024

Choose a reason for hiding this comment

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

Generally one should avoid checking in editor preferences/settings into code repos.

mduffor reviewed Nov 2, 2024

View reviewed changes

mduffor reviewed Nov 5, 2024

View reviewed changes

mduffor self-requested a review November 5, 2024 00:48

mduffor requested changes Nov 24, 2024

View reviewed changes

barbute added 3 commits December 12, 2024 18:21

Update .gitignore

0411a26

Update PivotConstants.java

44adffb

Update control constants

76bcfd1

Add custom setpoint Fully tuned arm Add soft limits for voltage control

barbute mentioned this pull request Dec 13, 2024

Merge tuned pivot to pivot base #12

Merged

mduffor added 2 commits January 3, 2025 10:40

Merge pull request #12 from Prosper-FRC/pivot-redux

2af80ff

Merge tuned pivot to pivot base

[MD] Update to production with WPILib 2025. Fixed 2025 changes. Fixed…

10315dd

… formatting

		@@ -0,0 +1,26 @@
		// Copyright (c) FIRST and other WPILib contributors.

Conversation

barbute commented Oct 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mduffor Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mduffor Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mduffor commented Nov 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mduffor Nov 2, 2024 •

edited

Loading

mduffor Nov 5, 2024 •

edited

Loading