REV Motor IO Layer by Spybh66 · Pull Request #190 · WHS-FRC-3467/W8-Library

Spybh66 · 2025-10-22T15:54:09Z

@garrett928 is working on the rev io implementation

Changes

implement a rev motorIO layer
- For now, support a sparkmax
- For now, support sim but not full physics sim
add example rotary subsytem using the REV IO layer
Add testing for rev rotary subsytem
add more tests for the rotary subsytem
add a default.instructions.md for copilot usage
default the motorio and mechanism.java to throw a notimplementedexception error.
- because these interface methods are default you dont have to implemented them. They have a default empty {} implementation. But this means you could call it in a subsytem and not know its empty. This change should allow the flexibility of some motors to have some but not all methods and still warn the user of an obvious error of using a non implemented function.

Known issues

Rev spark controllers not closing properly

Bugs Found

waituntilgoalcommand bug
I tried writing a test for this command and it fail for both rev and talonfx. below was the fixture

    @Test
    void testWaitUntilGoalCommand()
    {
        // Set a goal and wait for it to complete
        TestUtil.runTest(
            rotary.setSetpoint(Rotary.Setpoint.RAISED)
                .andThen(rotary.waitUntilGoalCommand(Rotary.Setpoint.RAISED.getSetpoint())),
            5,
            rotary);

        try {
            assertTrue(rotary.nearGoal(Rotary.Setpoint.RAISED.getSetpoint()));
        } catch (Exception e) {
            fail("Failed wait until goal command test: " + e.getMessage());
        }
    }

homeCommand
I tried writing a test for the homecommand in the rotary.java but it also failed

    @Test
    void testHomeCommand()
    {
        // Test the homing sequence
        TestUtil.runTest(rotary.homeCommand(), 5, rotary);

        try {
            // After homing, should be near the HOME position (0 degrees)
            assertTrue(rotary.nearGoal(Rotary.Setpoint.HOME.getSetpoint()));
        } catch (Exception e) {
            fail("Failed home command test: " + e.getMessage());
        }
    }

Copilot

Pull request overview

This pull request implements a REV motor IO layer to support REV Robotics motor controllers (SparkMax/SparkFlex) alongside the existing CTRE TalonFX implementation. The changes follow the hardware abstraction pattern established in the codebase and include an example rotary subsystem implementation with comprehensive testing.

Key Changes

REV Motor IO Implementation: Added MotorIORev and MotorIORevSim classes to support REV motor controllers with real hardware and simulation
Interface Enhancements: Modified MotorIO and Mechanism interfaces to throw NotImplementedException for unimplemented default methods and added simplified runPosition/runVelocity overloads
Example RevRotary Subsystem: Created a new example rotary subsystem using the REV IO layer with constants, commands, and comprehensive test coverage

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
vendordeps/REVLib.json	Added REV Robotics library dependency v2025.0.3
vendordeps/photonlib.json	Updated PhotonVision library from v2025.3.1 to v2025.3.2
vendordeps/libgrapplefrc2025.json	Updated Grapple library from 2025.0.8 to 2025.1.3
src/main/java/frc/lib/io/motor/MotorIORev.java	New real hardware implementation for REV motor controllers
src/main/java/frc/lib/io/motor/MotorIORevSim.java	New simulation implementation for REV motor controllers
src/main/java/frc/lib/io/motor/MotorIO.java	Added simplified position/velocity methods and NotImplementedException to defaults
src/main/java/frc/lib/io/motor/MotorIOSim.java	Enhanced documentation for simulation interface methods
src/main/java/frc/lib/io/motor/MotorIOTalonFX.java	Added comprehensive documentation for existing TalonFX methods
src/main/java/frc/lib/mechanisms/Mechanism.java	Changed default methods to throw NotImplementedException
src/main/java/frc/lib/mechanisms/rotary/RotaryMechanism.java	Added position logging in periodic method
src/main/java/frc/lib/mechanisms/rotary/RotaryMechanismReal.java	Implemented new simplified position/velocity methods
src/main/java/frc/lib/mechanisms/rotary/RotaryMechanismSim.java	Implemented new simplified methods and close()
src/main/java/frc/robot/subsystems/revRotary/RevRotary.java	New example rotary subsystem using REV motors
src/main/java/frc/robot/subsystems/revRotary/RevRotaryConstants.java	Configuration and factory methods for RevRotary subsystem
src/main/java/frc/robot/subsystems/rotary/Rotary.java	Refactored enum pattern to use LoggedTunableNumber, removed close()
src/main/java/frc/robot/subsystems/rotary/RotaryConstants.java	Added comprehensive documentation
src/main/java/frc/robot/RobotContainer.java	Integrated RevRotary subsystem and added cleanup helper
src/main/java/frc/robot/Ports.java	Added CAN ID for REV rotary subsystem motor
src/main/java/frc/lib/util/LoggedTunableNumber.java	Modified initialization timing (with debug statements)
src/main/java/frc/lib/io/objectDetection/*.java	Package rename from objectdetection to objectDetection (casing fix)
src/test/java/frc/robot/subsystems/revRotary/RevRotaryTest.java	Comprehensive test suite for RevRotary subsystem
src/test/java/frc/robot/subsystems/rotary/RotaryTest.java	Added additional test cases
src/test/java/frc/robot/RobotContainerTest.java	Enhanced setup and added REV subsystem cleanup
.github/instructions/default.instructions.md	New AI assistant instructions documenting library architecture

…s.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…s.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

garrett928 · 2025-11-25T20:26:08Z

@copilot open a new pull request to apply changes based on the comments in this thread. Do not implement the following though:

The simulation implementation directly sets position and velocity instead of properly controlling the motor. This bypasses the closed-loop control and physics simulation. The methods should use the controller to set references similar to MotorIORev, and let the simulation iterate to produce realistic behavior. Current implementation makes the simulation unrealistic.

Copilot · 2025-11-25T20:26:19Z

@garrett928 I've opened a new pull request, #218, to work on those changes. Once the pull request is ready, I'll request review from you.

…ng typo, and unused field Co-authored-by: garrett928 <22306616+garrett928@users.noreply.github.com>

Clean up review feedback from PR #190

jw00541 · 2025-11-25T22:05:47Z

This is my favorite PR ever

CJendantix · 2025-11-25T22:07:03Z

Slopbot at its finest ✨

…67/W8-Library into implement-rev-motorio

Spybh66 · 2025-11-26T22:59:18Z

I think this is good to go, I can't approve since I opened the PR

CJendantix · 2025-11-27T18:02:17Z

-        return "";
+        throw new NotImplementedException("This method has not been implemented");


This method needs to be removed for replay support

CJendantix · 2025-11-27T18:02:56Z

-    {}
+    {
+        throw new NotImplementedException("This method has not been implemented");
+    }


This breaks replay

CJendantix · 2025-11-27T18:02:59Z

-    {}
+    {
+        throw new NotImplementedException("This method has not been implemented");
+    }


This breaks replay

CJendantix · 2025-11-27T18:03:12Z

-    {}
+    {
+        throw new NotImplementedException("This method has not been implemented");
+    }


This breaks replay

CJendantix · 2025-11-27T18:03:25Z

+    {
+        throw new NotImplementedException("This method has not been implemented");
+    }



This breaks replay

CJendantix · 2025-11-27T18:50:38Z

+        new LoggedTunableNumber("Rotary RAISED", -90);

    @RequiredArgsConstructor
-    @SuppressWarnings("Immutable")


Why has this been removed?

didn't mean to. i'll add it back

CJendantix · 2025-11-27T18:51:20Z

-        RAISED(Degrees.of(RAISED_SETPOINT.get()));
-
-        private final Angle setpoint;
+        HOME(null),


HOME is not a setpoint, it shouldn't act as one. Also, for future reference, avoid setting anything to null unless it is a small, local variable.

CJendantix · 2025-11-27T18:54:18Z

+
+    /**
+     * Helper method for unit testing.
+     * 
+     * If we have any REV subsystems, close their resources here. IF we don't do this then we will
+     * get duplicate CAN ID errors when running multiple tests.
+     */
+    public void closeREVSubsystems()
+    {
+        revRotary.close();
+    }


Make RobotContainer implement AutoClosable and close all necessary subsystems.

CJendantix · 2025-11-27T18:54:29Z

+    @BeforeEach
+    void setup()
+    {
+        assert HAL.initialize(500, 0); // initialize the HAL, crash if failed


Wrong assert

this is what your tests do:

public class RotaryTest implements AutoCloseable { Rotary rotary; @BeforeEach // this method will run before each test void setup() { assert HAL.initialize(500, 0); // initialize the HAL, crash if failed rotary = RotaryConstants.get(); /* enable the robot */ DriverStationSim.setEnabled(true); DriverStationSim.notifyNewData(); /* delay ~100ms so the devices can start up and enable */ Timer.delay(0.100); }

Yes, it's wrong. I didn't write that. You can either
a. Change just yours
b. Change all of them
or
c. Do nothing

As to what you choose? I don't really care.

CJendantix · 2025-11-27T18:56:43Z

    public void close()
    {
-        rotary.close();
+        if (rotary != null) {


When would rotary == null?

this was a left over from some debugging. i'll remove it

garrett928 · 2025-12-11T01:58:35Z

@CJendantix thanks for the review. Sorry for the delay, I've been busy with work and the holidays.

I'm fine with not merging the PR until its feature parity with CTRE. I'll make the changes you suggested and add the rest of the sim support

garrett928 added 12 commits October 21, 2025 23:08

first pass at rev IO

4560855

Merge branch 'dev' into implement-rev-motorio

f70cae7

add seperate rev subsytem and update docs

35fc34f

refactor MotorIORev: clean up imports and improve code formatting

6b067e8

Merge branch 'dev' into implement-rev-motorio

cd9d261

merge dev

e152663

merge rotary

cbb9d84

merge dev

631cc53

merge dev

1e90100

partially working

b9de937

rev sim mostly working

6bdb24f

working rev sim

ee5d549

garrett928 self-assigned this Nov 25, 2025

garrett928 added 3 commits November 25, 2025 13:35

Refactor rotary subsystem

53a3377

test working

cb649e1

update sim

ecf6f8f

garrett928 marked this pull request as ready for review November 25, 2025 20:08

garrett928 requested a review from Copilot November 25, 2025 20:08

Copilot started reviewing on behalf of garrett928 November 25, 2025 20:08 View session

Copilot finished reviewing on behalf of garrett928 November 25, 2025 20:11

Copilot AI reviewed Nov 25, 2025

View reviewed changes

garrett928 and others added 9 commits November 25, 2025 15:21

Update src/main/java/frc/robot/subsystems/revRotary/RevRotaryConstant…

e285b3e

…s.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

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

f29b6e6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/main/java/frc/lib/io/motor/MotorIORev.java

b125589

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update .github/instructions/default.instructions.md

80e922b

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/main/java/frc/robot/subsystems/revRotary/RevRotary.java

c9cc5b5

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/main/java/frc/robot/subsystems/revRotary/RevRotaryConstant…

afd56fd

…s.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/test/java/frc/robot/subsystems/rotary/RotaryTest.java

97d8fc8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/main/java/frc/lib/io/motor/MotorIORevSim.java

21602a8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/main/java/frc/lib/io/motor/MotorIOSim.java

81f0df1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Initial plan

078adda

Copilot AI mentioned this pull request Nov 25, 2025

Clean up review feedback from PR #190 #218

Merged

garrett928 and others added 4 commits November 25, 2025 15:28

add close method

2448e41

Address review comments: fix debug prints, initialization logic, nami…

01e8bb3

…ng typo, and unused field Co-authored-by: garrett928 <22306616+garrett928@users.noreply.github.com>

Merge branch 'implement-rev-motorio' into copilot/sub-pr-190

2b5bef9

Merge pull request #218 from WHS-FRC-3467/copilot/sub-pr-190

9952272

Clean up review feedback from PR #190

garrett928 enabled auto-merge November 26, 2025 01:54

Merge branch 'dev' into implement-rev-motorio

3e7c325

Spybh66 commented Nov 26, 2025

View reviewed changes

garrett928 added 5 commits November 26, 2025 15:58

rename folder

475d964

Merge branch 'implement-rev-motorio' of https://github.com/WHS-FRC-34…

66dc14c

…67/W8-Library into implement-rev-motorio

add comment

5eed051

fix import

c0dd131

remove debug logging

6743832

CJendantix requested changes Nov 27, 2025

View reviewed changes

garrett928 marked this pull request as draft December 11, 2025 01:58

auto-merge was automatically disabled December 11, 2025 01:58
Pull request was converted to draft

		return "";
		throw new NotImplementedException("This method has not been implemented");

Conversation

Spybh66 commented Oct 22, 2025 • edited by garrett928 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Known issues

Bugs Found

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garrett928 commented Nov 25, 2025

Uh oh!

Copilot AI commented Nov 25, 2025

Uh oh!

jw00541 commented Nov 25, 2025

Uh oh!

CJendantix commented Nov 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Spybh66 commented Nov 26, 2025

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!

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!

garrett928 commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Spybh66 commented Oct 22, 2025 •

edited by garrett928

Loading