-
Notifications
You must be signed in to change notification settings - Fork 0
Miscellaneous Changes #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3d0fb5a
5fc25b6
ccfc295
f6c7fd0
4138c30
5272a68
bd760f5
54991b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,9 +83,16 @@ public static Command joystickDrive( | |
| linearVelocity.getX() * drive.getMaxLinearSpeedMetersPerSec(), | ||
| linearVelocity.getY() * drive.getMaxLinearSpeedMetersPerSec(), | ||
| omega * drive.getMaxAngularSpeedRadPerSec()); | ||
|
|
||
| // We have to call DriverStation#getAlliance() only once, since the return value could | ||
| // change in between calls to it. So, we turn the Optional<Alliance> from | ||
| // DriverStation#getAlliance() to an Optional<Boolean> by mapping a Red alliance to | ||
| // `true`, and defaulting to `false` if the DriverStation#getAlliance() returned an empty | ||
| // optional. This achieves the intended behavior of having `true` if, and only if | ||
| // DriverStation#getAlliance() returned a non-empty option with Alliance.Red, but avoiding | ||
| // accidentally unwrapping an empty optional | ||
| boolean isFlipped = | ||
| DriverStation.getAlliance().isPresent() | ||
| && DriverStation.getAlliance().get() == Alliance.Red; | ||
| DriverStation.getAlliance().map((alliance) -> alliance == Alliance.Red).orElse(false); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is likely not necessary, as if the robot is not connected to FMS, it will use the alliance from the driverstation. |
||
| drive.runVelocity( | ||
| ChassisSpeeds.fromFieldRelativeSpeeds( | ||
| speeds, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,13 +151,16 @@ public Drive( | |
|
|
||
| @Override | ||
| public void periodic() { | ||
| odometryLock.lock(); // Prevents odometry updates while reading data | ||
| gyroIO.updateInputs(gyroInputs); | ||
| Logger.processInputs("Drive/Gyro", gyroInputs); | ||
| for (var module : modules) { | ||
| module.periodic(); | ||
| try { | ||
| odometryLock.lock(); // Prevents odometry updates while reading data | ||
| gyroIO.updateInputs(gyroInputs); | ||
| Logger.processInputs("Drive/Gyro", gyroInputs); | ||
| for (var module : modules) { | ||
| module.periodic(); | ||
| } | ||
| } finally { | ||
| odometryLock.unlock(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be a good idea to propose this change upstream (as well): 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. |
||
| } | ||
| odometryLock.unlock(); | ||
|
|
||
| // Stop moving when disabled | ||
| if (DriverStation.isDisabled()) { | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an unnecessary change, and just makes the code slightly harder to read.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can change the method references back to lambdas if you want me to, but we definitely need to make the |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| /* | ||
| Copyright 2025-2026 Prospect Robotics SWENext Club | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
| // This is taken from [lib2813](https://github.com/Prospect-Robotics/lib2813/blob/main/testing/src/main/java/com/team2813/lib2813/testing/junit/jupiter/CommandTester.java). | ||
| // When lib2813 is on maven central, this file can be deleted in favor of adding a dependency on lib2813. | ||
| package com.team2813; | ||
|
|
||
| import edu.wpi.first.wpilibj2.command.Command; | ||
|
|
||
| /** | ||
| * Allows tests to run commands. | ||
| * | ||
| * <p>Tests can get an instance by using {@link WPILibExtension}. | ||
| * | ||
| * @since 2.0.0 | ||
| */ | ||
| public interface CommandTester { | ||
|
|
||
| /** Schedules the provided command and runs it until it completes. */ | ||
| void runUntilComplete(Command command); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| package com.team2813; | ||
|
|
||
| import edu.wpi.first.wpilibj2.command.Commands; | ||
| import org.junit.jupiter.api.Assumptions; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
||
| @ExtendWith(WPILibExtension.class) | ||
| public class RobotContainerTest { | ||
| @Test | ||
| public void canCreateRobotAndRunPeriodic(CommandTester tester) { | ||
| Assumptions.assumeTrue( | ||
| Constants.simMode == Constants.Mode.SIM, "The sim mode must be sim to run tests!"); | ||
| // create a robot container | ||
| RobotContainer robotContainer = new RobotContainer(); | ||
| // Run one periodic cycle | ||
| tester.runUntilComplete(Commands.none()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| /* | ||
| Copyright 2025-2026 Prospect Robotics SWENext Club | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
| package com.team2813; | ||
|
|
||
| import edu.wpi.first.hal.HAL; | ||
| import edu.wpi.first.wpilibj.RuntimeType; | ||
| import edu.wpi.first.wpilibj.TimedRobot; | ||
| import edu.wpi.first.wpilibj.simulation.DriverStationSim; | ||
| import edu.wpi.first.wpilibj.simulation.SimHooks; | ||
| import edu.wpi.first.wpilibj2.command.CommandScheduler; | ||
| import org.junit.jupiter.api.extension.*; | ||
|
|
||
| /** | ||
| * JUnit Jupiter extension for testing code that depends on WPILib. | ||
| * | ||
| * <p>Also provides a {@link CommandTester} for tests. | ||
| * | ||
| * <p>Example use: | ||
| * | ||
| * <pre>{@code | ||
| * @ExtendWith(WPILibExtension.class) | ||
| * public final class FlightSubsystemTest { | ||
| * | ||
| * @Test | ||
| * public void initiallyNotInAir() { | ||
| * var flight = new FlightSubsystem(); | ||
| * | ||
| * assertThat(flight.inAir()).isFalse(); | ||
| * } | ||
| * | ||
| * @Test | ||
| * public void takesFlight(CommandTester commandTester) { | ||
| * var flight = new FlightSubsystem(); | ||
| * Command takeOff = flight.createTakeOffCommandCommand(); | ||
| * | ||
| * commandTester.runUntilComplete(takeOff); | ||
| * | ||
| * assertThat(flight.inAir()).isTrue(); | ||
| * } | ||
| * } | ||
| * }</pre> | ||
| * | ||
| * @since 2.0.0 | ||
| */ | ||
| // This is taken from [lib2813](https://github.com/Prospect-Robotics/lib2813/blob/main/testing/src/main/java/com/team2813/lib2813/testing/junit/jupiter/WPILibExtension.java). | ||
| // When lib2813 is on maven central, this file can be deleted in favor of adding a dependency on lib2813. | ||
| public final class WPILibExtension | ||
| implements Extension, | ||
| AfterAllCallback, | ||
| AfterEachCallback, | ||
| BeforeAllCallback, | ||
| ParameterResolver { | ||
| private static final double NANOS_PER_SECOND = 1_000_000_000d; | ||
|
|
||
| @Override | ||
| public void beforeAll(ExtensionContext context) { | ||
| // 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(); | ||
|
Comment on lines
+70
to
+81
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This set of operations looks non-trivial to me. How did we come up with it? Ie, instead of just
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this where the TOCTOU vulnerability is fixed?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also please add some comments to these.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vdikov updated this comment in the lib2813 version of this file in Prospect-Robotics/lib2813#121. Requested a review from you. |
||
| } | ||
|
|
||
| @Override | ||
| public void afterEach(ExtensionContext context) { | ||
| CommandScheduler commandScheduler = CommandScheduler.getInstance(); | ||
| commandScheduler.cancelAll(); | ||
| commandScheduler.unregisterAllSubsystems(); | ||
| } | ||
|
|
||
| @Override | ||
| public void afterAll(ExtensionContext context) { | ||
| CommandScheduler commandScheduler = CommandScheduler.getInstance(); | ||
| commandScheduler.cancelAll(); | ||
| commandScheduler.unregisterAllSubsystems(); | ||
| commandScheduler.disable(); | ||
| DriverStationSim.setEnabled(false); | ||
| DriverStationSim.notifyNewData(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean supportsParameter( | ||
| ParameterContext parameterContext, ExtensionContext extensionContext) | ||
| throws ParameterResolutionException { | ||
| return CommandTester.class.equals(parameterContext.getParameter().getType()); | ||
| } | ||
|
|
||
| @Override | ||
| public CommandTester resolveParameter( | ||
| ParameterContext parameterContext, ExtensionContext extensionContext) { | ||
| CommandScheduler scheduler = CommandScheduler.getInstance(); | ||
|
|
||
| return command -> { | ||
| SimHooks.pauseTiming(); | ||
| try { | ||
| scheduler.schedule(command); | ||
| do { | ||
| scheduler.run(); | ||
| SimHooks.stepTiming(TimedRobot.kDefaultPeriod); | ||
| } while (scheduler.isScheduled(command)); | ||
| } finally { | ||
| SimHooks.resumeTiming(); | ||
| } | ||
| }; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to remove the comment here? It doesn't really harm the readability of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forautonomousExit().