Miscellaneous Changes by cuttestkittensrule · Pull Request #5 · Prospect-Robotics/2026-Robot-Code

cuttestkittensrule · 2026-01-23T04:24:56Z

Add a handful of improvements. The commit names pretty much explain what they do. Here is a short list of changes:

Make the gradlew script executable
Use a try-finally block to ensure the unlocking of the Drive's lock
Remove a placeholder file (as it was no loger serving a purpose)
Cancel the autonomous command in autonomousExit
Have the time shown be in our timezone
Fix a potential TOCTOU bug
Add one test
- Just makes sure that creating RobotContainer and running a single periodic run don't throw an exception.

Generally, the commits are very small and removed from each other, so if we don't want to implement one of these changes, we can just revert it :)

…r commands depend on the hopper

vdikov

Thanks @cuttestkittensrule ,

Really nice touches here. I have added a few comments.

I like how the PR is organized in meaningful, chunk-size, self-explanatory commits. What I was wondering is how you can address my comments without corrupting this commits organization. Could you just "amend" the commits when addressing my comments, so these commits keep the same logical chunk-size structure and are addressing my comments? I believe you could just git commit --amend against the individual commits, but haven't used it myself yet.

vdikov · 2026-01-23T16:28:07Z

+        module.periodic();
+      }
+    } finally {
+      odometryLock.unlock();


It would be a good idea to propose this change upstream (as well):

https://github.com/Mechanical-Advantage/AdvantageKit/blob/709689949038543b0510538c5b819a08b633d051/template_projects/sources/talonfx_swerve/src/main/java/frc/robot/subsystems/drive/Drive.java#L160

I'd be curious to see if the Mechanical Advantage team engages in an useful discussion when you do so. I'm wondering if they might have some reasons to believe this might not be needed.

vdikov · 2026-01-23T16:48:01Z

          boolean isFlipped =
-              DriverStation.getAlliance().isPresent()
-                  && DriverStation.getAlliance().get() == Alliance.Red;
+              DriverStation.getAlliance().map((alliance) -> alliance == Alliance.Red).orElse(false);


Oh, interesting catch.

Please add a comment in the code as well that calling DriverStation.getAlliance() more than once should be avoided (I.e., add the commit comment as a code comment so that code readers can see it, even when they don't review commits history).

Also, this could be even simpler (and avoid the Optional.map(...) syntax, which could be a bit confusing for novice Java readers):

// Note: we must avoid expressions that call DriverStation.getAlliance() more than once // since its return value can change any time, including mid-expression evaluation, and // that could lead to run-time errors. boolean isFlipped = DriverStation.getAlliance().orElse(Alliance.Blue) == Alliance.Red;

This is likely not necessary, as if the robot is not connected to FMS, it will use the alliance from the driverstation.

vdikov · 2026-01-23T16:53:26Z

+    // See https://www.chiefdelphi.com/t/driverstation-getalliance-in-gradle-test/
+    if (!HAL.initialize(500, 0)) {
+      throw new IllegalStateException("Could not initialize Hardware Abstraction Layer");
+    }
+    DriverStationSim.setEnabled(true);
+    DriverStationSim.notifyNewData();
+    SimHooks.setHALRuntimeType(RuntimeType.kSimulation.value);
+
+    CommandScheduler commandScheduler = CommandScheduler.getInstance();
+    commandScheduler.enable();
+    commandScheduler.cancelAll();
+    commandScheduler.unregisterAllSubsystems();


This set of operations looks non-trivial to me. How did we come up with it?
Were we referring to a unit-test guide for WPILib code somewhere?
I see bits and pieces of this routine might be coming from https://www.chiefdelphi.com/t/driverstation-getalliance-in-gradle-test/ . If that's the case, elaborate a bit more in the comment.

Ie, instead of just // See https://www.chie..., you can say something like // Initialization based on https://www.chiefdelphi.com/t/driverstation-getalliance-in-gradle-test/

This is copied from lib2813. I would have used it straight from lib2813, but we are going to use so little of it anyways that it isn't really worth it. We can probably just use the lib2813 version when we put it on maven central

Gotcha. So leave a comment like "Code forked from testing/src/main/java/com/team2813/lib2813/testing/junit/jupiter/WPILibExtension.java")

Otherwise, this code appears miraculously in the current repo with no prior logs or reference on how it came to be, and what was it designed to solve in the first place.

Is this where the TOCTOU vulnerability is fixed?

Also please add some comments to these.

@spderman3333 The TOCTOU error is fixed in the commit "Prevent TOCTOU error". This set of changes is part of some backend stuff from lib2813 for testing, which I opted to copy over instead of adding the lib2813 submodule just for two files related to testing. I could add some comments, but I don't think it will be super useful since we are likely going to delete these files soon.

@vdikov updated this comment in the lib2813 version of this file in Prospect-Robotics/lib2813#121. Requested a review from you.

cuttestkittensrule · 2026-01-24T03:53:00Z

@vdikov You can only git commit --amend the most recent commit, but I can do a rebase to change a commit in the middle, so I can change any individual commit in this PR.

Makes sure it is canceled if something other than teleop is started, so nothing weird happens

TOCTOU -> time of check/time of use. The Driver station alliance could theorhetically change from having an alliance to not having an alliance in between the DriverStation#getAlliance() call, especially with JNI being involved, and could throw an exception. To prevent this, it is called only once, and then converted to the information on if it should be flipped. You could change this to be the same as before, but with storing the driver station alliance in a variable, and it would work.

…a periodic cycle without crashing

spderman3333 · 2026-01-26T17:50:31Z

          boolean isFlipped =
-              DriverStation.getAlliance().isPresent()
-                  && DriverStation.getAlliance().get() == Alliance.Red;
+              DriverStation.getAlliance().map((alliance) -> alliance == Alliance.Red).orElse(false);


This is likely not necessary, as if the robot is not connected to FMS, it will use the alliance from the driverstation.

spderman3333 · 2026-01-26T19:22:38Z

This is an unnecessary change, and just makes the code slightly harder to read.
I'd argue that lambdas are more readable than method references.

I can change the method references back to lambdas if you want me to, but we definitely need to make the InstantCommands depend on the Intake subsystem, since they didn't do that before

spderman3333 · 2026-01-26T19:23:22Z

  @Override
  public void autonomousPeriodic() {}

-  /** This function is called once when teleop is enabled. */


Is it necessary to remove the comment here? It doesn't really harm the readability of the file.

This comment wasn't removed, it still exists where the teleopInit functon is. As for why this comment isn't in front of autonomousExit(), it is because it would be inaccurate for autonomousExit().

kcooney · 2026-01-27T14:55:02Z

What I was wondering is how you can address my comments without corrupting this commits organization. Could you just "amend" the commits when addressing my comments, so these commits keep the same logical chunk-size structure and are addressing my comments? I believe you could just git commit --amend against the individual commits, but haven't used it myself yet.

If you enable commit.autosquash, you can do:

git commit --fixup=[sha1 of commit to amend]
git rebase -i main

Details here (well worth a read; I didn't know about the :/ notation)

Rebasing now might make it harder for reviewers to see the changes that were made in response to comments, so it might be better to do the git commit --fixup now, and rebase right before merging.

side-note: rebasing is usually fine on branches you own, but rebasing shared branches can cause problems.

vdikov · 2026-01-31T03:06:02Z

What I was wondering is how you can address my comments without corrupting this commits organization. Could you just "amend" the commits when addressing my comments, so these commits keep the same logical chunk-size structure and are addressing my comments? I believe you could just git commit --amend against the individual commits, but haven't used it myself yet.

If you enable commit.autosquash, you can do:
git commit --fixup=[sha1 of commit to amend]
git rebase -i main
Details here (well worth a read; I didn't know about the :/ notation)

Rebasing now might make it harder for reviewers to see the changes that were made in response to comments, so it might be better to do the git commit --fixup now, and rebase right before merging.

side-note: rebasing is usually fine on branches you own, but rebasing shared branches can cause problems.

Got it. Probably not worth it to overcomplicate it then. I was hoping it would be much more like hg amend and hg evolve.

If we could somehow switch to Jujutsu which I imagine handles this better, and end up with a simpler playbook for the students, that might be the path forward. I have personally not tried Jujutsu yet, so I'm just speculating this would looks simpler with it.

vdikov · 2026-01-31T03:13:06Z

@cuttestkittensrule , what's the status of this PR? Are there any blocking / fundamentally unresolvable comments left?

If it looks like there's just little / trivial amount of work left, please consider pushing through and merging it.

FWIW, I see that this PR is improving things and it would be a shame to let it go to waste.

Also, I just want to point out that by now we have multiple people spending time on it - you putting it together, in the first place, but also three reviewers who went through review it. Reviewing a PR is non-trivial work.

It would be gratifying to all of us to see it merge :) And conversely, if there's something unsurmountable preventing us from landing it - please bring it up. Understanding what it is, learning from it - that's going to be also a win and a good return of everyone's invested time.

Just let's not leave it rot or go to waste.

spderman3333

lgtm

spderman3333 · 2026-01-31T23:30:47Z

I think @vdikov should re-review this too, but it could be merged.

cuttestkittensrule added 4 commits January 21, 2026 23:13

Make gradlew executable

3d0fb5a

Ensure that the odometry lock is unlocked if an exception is thrown

5fc25b6

Use method references instead of lambdas, and actually make the hoppe…

ccfc295

…r commands depend on the hopper

Remove hopper placeholder file

f6c7fd0

cuttestkittensrule requested review from codingduck106 and spderman3333 January 23, 2026 04:24

vdikov reviewed Jan 23, 2026

View reviewed changes

cuttestkittensrule added 4 commits January 23, 2026 19:58

Move autonomous command canceling to teleopExit

4138c30

Makes sure it is canceled if something other than teleop is started, so nothing weird happens

Use our time zone for the build constants

5272a68

Add a test to make sure that we can create a robot container and run …

54991b8

…a periodic cycle without crashing

cuttestkittensrule force-pushed the kyle/misc-improvements branch from f9863a7 to 54991b8 Compare January 25, 2026 03:33

spderman3333 reviewed Jan 26, 2026

View reviewed changes

kcooney changed the title ~~Miscelaneous Changes~~ Miscellaneous Changes Jan 27, 2026

spderman3333 approved these changes Jan 31, 2026

View reviewed changes

spderman3333 merged commit af0d75f into main Feb 1, 2026
2 checks passed

spderman3333 deleted the kyle/misc-improvements branch February 1, 2026 02:27

Uh oh!

Conversation

cuttestkittensrule commented Jan 23, 2026

Uh oh!

vdikov left a comment

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!

spderman3333 Jan 26, 2026 • 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!

cuttestkittensrule commented Jan 24, 2026

Uh oh!

spderman3333 Jan 26, 2026 • 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!

kcooney commented Jan 27, 2026

Uh oh!

vdikov commented Jan 31, 2026

Uh oh!

vdikov commented Jan 31, 2026

Uh oh!

spderman3333 left a comment

Choose a reason for hiding this comment

Uh oh!

spderman3333 commented Jan 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

spderman3333 Jan 26, 2026 •

edited

Loading

spderman3333 Jan 26, 2026 •

edited

Loading