Nh command loop by noahheller · Pull Request #178 · FRC4048/FRC2024_Java

noahheller · 2024-03-22T14:37:21Z

contains a couple of optimizations that might increase performance. If we dont like any of them they are separated by commits, so we can cherrypick

…eduler loop overrun)

# Conflicts: # src/main/java/frc/robot/subsystems/Deployer.java # src/main/java/frc/robot/subsystems/Feeder.java # src/main/java/frc/robot/subsystems/Ramp.java

# Conflicts: # src/main/java/frc/robot/subsystems/Ramp.java # src/main/java/frc/robot/utils/BlinkinPattern.java

armadilloz · 2024-03-24T17:57:52Z

src/main/java/frc/robot/subsystems/Deployer.java

-            .withSize(2, 2);
-        */
+
        Logger.logBoolean(baseLogName + "FWD LMT", isDeployerForwardLimitSwitchClosed(), Constants.ENABLE_LOGGING);


Move to the if (debug)?

I feel like the benefit of leaving it (what if the switch gets bent and the robot is not hitting the switch?) is greater than the performance gain.

I don't think anyone is looking at these values during the competition (we have diags for that), and even if they are important, they can be placed behind another LOGGING constant that can default to TRUE, so we can control it.

src/main/java/frc/robot/subsystems/Feeder.java

armadilloz · 2024-03-24T18:01:21Z

src/main/java/frc/robot/subsystems/Ramp.java

            SmartShuffleboard.put("Ramp", "Desired pos", rampPos);
            SmartShuffleboard.put("Ramp", "Reverse Switch Tripped", getReversedSwitchState());
            SmartShuffleboard.put("Ramp", "Forward Switch Tripped", getForwardSwitchState());
-//            double pidP = SmartShuffleboard.getDouble("Ramp", "PID P", neoPidMotor.getPidController().getP());


The code above (setting thePID_FF) can probably be be part of a command rather then run every time in periodic

Since the ramp is a PID and the MoveRamp command finishes instantly, what do you recommend? Reading the document it does say to use the method "Infrequently" because it takes time. I could store the last PID Value and compare against it before setting it?

Is there a functional reason that the command be done immediately? I would think that it would make sense for a command to terminate once the ramp it at the position.
So you can have a sequential group of two commands: One to get the ramp "close enough" with FF and another to get the ramp closer (without the FF), and the second one to end once the ramp error is within range.

I checked and I don't think it will break anything if I make the ramp move not end immediately. I added the logic to the ramp commands

armadilloz · 2024-03-24T18:03:12Z

src/main/java/frc/robot/subsystems/Ramp.java

-//            if (pidI != neoPidMotor.getPidController().getI()) neoPidMotor.getPidController().setI(pidI);
-//            if (pidD != neoPidMotor.getPidController().getD()) neoPidMotor.getPidController().setD(pidD);
-//            if (pidFF != neoPidMotor.getPidController().getFF()) neoPidMotor.getPidController().setFF(pidFF);
-            SmartShuffleboard.put("Ramp", "Forward Switch Tripped", getForwardSwitchState());


why deleting the debug shuffleboard values?

delete selection was to big when removing pid stuff

armadilloz · 2024-03-24T18:04:06Z

src/main/java/frc/robot/subsystems/Ramp.java

+                    .withSize(1, 1);
        }

        Logger.logDouble(baseLogName + "EncoderValue", getRampPos(), Constants.ENABLE_LOGGING);


Move to if(debug)?

Still some logs left outside the condition

src/main/java/frc/robot/subsystems/SwerveDrivetrain.java

src/main/java/frc/robot/subsystems/LightStrip.java

src/main/java/frc/robot/commands/MoveToGamepiece.java

…nneeded logging variable

armadilloz · 2024-03-25T02:34:23Z

src/main/java/frc/robot/subsystems/LightStrip.java

-    public synchronized void setPattern(BlinkinPattern pattern) {
-        this.pattern = pattern;
+    public void setPattern(BlinkinPattern pattern) {
+        this.pattern.set(pattern);


Seems like there is no real need to protect the pattern and the running since they are primitives (boolean and a reference), though using an atomic reference here does not hurt. The one thing that does require protecting as it is modified from both threads is the map, which is currently vulnerable.

changed to concurrent hashmap

armadilloz · 2024-03-25T02:39:31Z

src/main/java/frc/robot/subsystems/Vision.java

  public double getPieceOffestAngleY() {
    return ty.getDouble(0);
  }
+  public double getTv(){


A better name would be isPieceSeen. This one sounds like it porvides you with an appliance.

armadilloz · 2024-03-25T02:42:39Z

src/main/java/frc/robot/subsystems/SwerveDrivetrain.java

    @Override
    public void periodic() {
+        gyroValue = getGyro();
        frontLeftDriveCurrent = ((CANSparkMax)frontLeft.getSwerveMotor().getDriveMotor()).getOutputCurrent();


All the Current stuff can be placed inside if(CURRENT_DEBUG). It can be set to true is we want it in the competition, but this way we at least have some control over it

armadilloz

Mostly fixed, a few more comments.
Plus, I don't see anything dangerous here that should be kept away, with the potential exception of the Limelight piece command that seems to have functional changes.

# Conflicts: # src/main/java/frc/robot/commands/MoveToGamepiece.java # src/main/java/frc/robot/commands/ramp/RampFollow.java # src/main/java/frc/robot/constants/GameConstants.java # src/main/java/frc/robot/subsystems/Ramp.java # src/main/java/frc/robot/subsystems/SwerveDrivetrain.java

noahheller added 4 commits March 21, 2024 11:09

multi-thread led strip (needs to be tested. Might improve command sch…

d1453f7

…eduler loop overrun)

moved shuffleboard stuff into debug

326bc8a

made limit only check for piece while running MoveToGamepiece command

43cd542

Merge branch 'main' into nh-command-loop

edd4828

# Conflicts: # src/main/java/frc/robot/subsystems/Deployer.java # src/main/java/frc/robot/subsystems/Feeder.java # src/main/java/frc/robot/subsystems/Ramp.java

noahheller self-assigned this Mar 22, 2024

noahheller added 3 commits March 22, 2024 11:03

stop logging joystick

3e4b61a

Merge branch 'main' into nh-command-loop

2d7a250

# Conflicts: # src/main/java/frc/robot/subsystems/Ramp.java # src/main/java/frc/robot/utils/BlinkinPattern.java

deleted empty file

e421a7b

armadilloz requested changes Mar 24, 2024

View reviewed changes

noahheller added 4 commits March 24, 2024 18:44

use atomic reference instead of synchronized block. Also got rid of u…

ccc1dd7

…nneeded logging variable

Merge branch 'main' into nh-command-loop

c1ea130

comment fixes

1b25122

I think I fixed the logic

b830a9c

noahheller requested a review from armadilloz March 24, 2024 23:38

armadilloz reviewed Mar 25, 2024

View reviewed changes

armadilloz requested changes Mar 25, 2024

View reviewed changes

comment fixes

4754c03

noahheller marked this pull request as ready for review March 25, 2024 03:33

noahheller requested a review from armadilloz March 25, 2024 03:33

noahheller added 5 commits March 25, 2024 17:09

piece seen is now boolean

8b93d87

Merge branch 'main' into nh-command-loop

7509596

merge conflict

1210a5d

merge conflict

296fd66

Comments

Conversation

noahheller commented Mar 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armadilloz Mar 25, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

armadilloz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

armadilloz Mar 25, 2024 •

edited

Loading