refactor(subsystems): remove all advantagekit abstractions#18
Open
refactor(subsystems): remove all advantagekit abstractions#18
Conversation
the robot doesnt work in simulation and the abstractions are causing trouble so
the robot doesnt work in simulation and the abstractions are causing trouble so
WalkthroughThis PR removes the IO abstraction pattern across multiple subsystems (Collector, Shooter, Turntable, Drive, Vision), replacing them with direct hardware dependencies. It also removes logging-related utility classes and a Gradle task, and simplifies command initialization in Controls. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/kotlin/org/frc5183/robot/subsystems/drive/SwerveDriveSubsystem.kt (1)
54-58:⚠️ Potential issue | 🔴 CriticalThis callback calls a nonexistent overload.
Line 58 invokes
drive(robotRelativeSpeeds), butSwerveDriveSubsystemhas nodrive(ChassisSpeeds)overload. The available drive methods aredrive(Translation2d, Double, Boolean, Boolean)anddrive(ChassisSpeeds, Array, Array). Usethis::driveRobotOrientedinstead, which accepts a singleChassisSpeedsparameter and delegates to the wrappedSwerveDrive.🛠️ Minimal fix
- { robotRelativeSpeeds: ChassisSpeeds -> drive(robotRelativeSpeeds) }, + this::driveRobotOriented,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/org/frc5183/robot/subsystems/drive/SwerveDriveSubsystem.kt` around lines 54 - 58, The AutoBuilder.configure callback currently references drive(robotRelativeSpeeds), which doesn't exist on SwerveDriveSubsystem; locate the AutoBuilder.configure call and replace that callback with this::driveRobotOriented (a single-parameter ChassisSpeeds-compatible method on SwerveDriveSubsystem). Ensure you do not attempt to call the existing overloads drive(Translation2d, Double, Boolean, Boolean) or drive(ChassisSpeeds, Array, Array); instead wire the single-argument method driveRobotOriented so the AutoBuilder will pass ChassisSpeeds correctly to the subsystem.
🧹 Nitpick comments (2)
src/main/kotlin/org/frc5183/robot/constants/Controls.kt (1)
8-8: Unused import:SwerveInputStream.This import is not referenced anywhere in the file. Remove it to keep the code clean.
-import swervelib.SwerveInputStream🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/org/frc5183/robot/constants/Controls.kt` at line 8, Remove the unused import of SwerveInputStream from the top-level imports in Controls.kt; locate the import line referencing SwerveInputStream and delete it (or let your IDE optimize/remove unused imports) so the file no longer references the unused symbol SwerveInputStream.src/main/kotlin/org/frc5183/robot/Robot.kt (1)
45-54: Vision fusion is implemented but unreachable—verify if disabling it is intentional.
SwerveDriveSubsystemcontains active vision pose fusion logic (lines 80–85) that will never execute becauseRobot.ktpassesnullfor the vision dependency.VisionSubsystemexists and is fully functional with front/back camera support, but is never instantiated anywhere in the codebase. If PhotonVision-based localization is required, instantiateVisionSubsystemand pass it here. If vision is intentionally disabled for now, add a comment explaining why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/org/frc5183/robot/Robot.kt` around lines 45 - 54, SwerveDriveSubsystem's vision fusion code is unreachable because Robot.kt constructs SwerveDriveSubsystem with a null vision dependency; either instantiate and pass the existing VisionSubsystem into SwerveDriveSubsystem (create a VisionSubsystem with the configured front/back cameras and pass that instance instead of null when constructing SwerveDriveSubsystem/SwerveDrive) so PhotonVision-based localization runs, or if vision is intentionally disabled add a clear inline comment where Robot.kt passes null explaining why vision is omitted and when/how to re-enable it; update only the Robot.kt construction site referencing SwerveDriveSubsystem and VisionSubsystem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/kotlin/org/frc5183/robot/constants/Controls.kt`:
- Around line 17-21: The registerControls function references an old constant
name: replace the undefined TELEOP_DRIVE_COMMAND with the renamed DRIVE_COMMAND
in registerControls(drive: SwerveDriveSubsystem) so drive.defaultCommand =
DRIVE_COMMAND, and remove the now-unused TeleopDriveCommand import if it isn’t
used elsewhere in this file to clean up imports.
In `@src/main/kotlin/org/frc5183/robot/subsystems/drive/SwerveDriveSubsystem.kt`:
- Around line 32-35: SwerveDriveSubsystem.periodic() is re-adding identical
vision measurements each tick because frontCamera.estimatedRobotPose and
backCamera.estimatedRobotPose only change when new frames arrive; to fix it add
monotonic timestamp guards (e.g., lastFrontVisionTimestamp and
lastBackVisionTimestamp fields) and only call drive.addVisionMeasurement(...)
when the incoming pose.timestamp is greater than the stored last timestamp, then
update the stored timestamp; reference the symbols
SwerveDriveSubsystem.periodic(), frontCamera.estimatedRobotPose,
backCamera.estimatedRobotPose, and drive.addVisionMeasurement() when making the
change so stale measurements from FixedCamera.updateUnreadResults() are not
repeatedly added.
In
`@src/main/kotlin/org/frc5183/robot/subsystems/turntable/TurntableSubsystem.kt`:
- Around line 13-18: The targets getter currently calls camera.allUnreadResults
each time, consuming unread results multiple times per scheduler loop; change
TurntableSubsystem to snapshot unread results once per loop by adding a private
backing field (e.g., cachedTargets: Array<PhotonTrackedTarget>) and populate it
inside TurntableSubsystem.periodic() by reading camera.allUnreadResults once,
mapping and flattening that result into the cachedTargets, then have the public
targets property simply return cachedTargets so later consumers and the existing
logging use the same snapshot.
---
Outside diff comments:
In `@src/main/kotlin/org/frc5183/robot/subsystems/drive/SwerveDriveSubsystem.kt`:
- Around line 54-58: The AutoBuilder.configure callback currently references
drive(robotRelativeSpeeds), which doesn't exist on SwerveDriveSubsystem; locate
the AutoBuilder.configure call and replace that callback with
this::driveRobotOriented (a single-parameter ChassisSpeeds-compatible method on
SwerveDriveSubsystem). Ensure you do not attempt to call the existing overloads
drive(Translation2d, Double, Boolean, Boolean) or drive(ChassisSpeeds, Array,
Array); instead wire the single-argument method driveRobotOriented so the
AutoBuilder will pass ChassisSpeeds correctly to the subsystem.
---
Nitpick comments:
In `@src/main/kotlin/org/frc5183/robot/constants/Controls.kt`:
- Line 8: Remove the unused import of SwerveInputStream from the top-level
imports in Controls.kt; locate the import line referencing SwerveInputStream and
delete it (or let your IDE optimize/remove unused imports) so the file no longer
references the unused symbol SwerveInputStream.
In `@src/main/kotlin/org/frc5183/robot/Robot.kt`:
- Around line 45-54: SwerveDriveSubsystem's vision fusion code is unreachable
because Robot.kt constructs SwerveDriveSubsystem with a null vision dependency;
either instantiate and pass the existing VisionSubsystem into
SwerveDriveSubsystem (create a VisionSubsystem with the configured front/back
cameras and pass that instance instead of null when constructing
SwerveDriveSubsystem/SwerveDrive) so PhotonVision-based localization runs, or if
vision is intentionally disabled add a clear inline comment where Robot.kt
passes null explaining why vision is omitted and when/how to re-enable it;
update only the Robot.kt construction site referencing SwerveDriveSubsystem and
VisionSubsystem.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df600429-25ab-4d99-8ae3-3f040e9f2ce2
📒 Files selected for processing (27)
build.gradle.ktssrc/main/kotlin/org/frc5183/robot/Robot.ktsrc/main/kotlin/org/frc5183/robot/constants/Controls.ktsrc/main/kotlin/org/frc5183/robot/logging/AutoLogInputs.ktsrc/main/kotlin/org/frc5183/robot/logging/struct/EstimatedRobotPoseStruct.ktsrc/main/kotlin/org/frc5183/robot/logging/struct/PhotonTrackedTargetStruct.ktsrc/main/kotlin/org/frc5183/robot/subsystems/collector/CollectorSubsystem.ktsrc/main/kotlin/org/frc5183/robot/subsystems/collector/io/CollectorIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/collector/io/CollectorIOInputs.ktsrc/main/kotlin/org/frc5183/robot/subsystems/collector/io/RealCollectorIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/drive/SwerveDriveSubsystem.ktsrc/main/kotlin/org/frc5183/robot/subsystems/drive/io/RealSwerveDriveIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/drive/io/SimulatedSwerveDriveIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/drive/io/SwerveDriveIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/drive/io/SwerveDriveIOInputs.ktsrc/main/kotlin/org/frc5183/robot/subsystems/shooter/ShooterSubsystem.ktsrc/main/kotlin/org/frc5183/robot/subsystems/shooter/io/RealShooterIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/shooter/io/ShooterIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/shooter/io/ShooterIOInputs.ktsrc/main/kotlin/org/frc5183/robot/subsystems/turntable/TurntableSubsystem.ktsrc/main/kotlin/org/frc5183/robot/subsystems/turntable/io/RealTurntableIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/turntable/io/TurntableIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/turntable/io/TurntableIOInputs.ktsrc/main/kotlin/org/frc5183/robot/subsystems/vision/VisionSubsystem.ktsrc/main/kotlin/org/frc5183/robot/subsystems/vision/io/RealVisionIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/vision/io/VisionIO.ktsrc/main/kotlin/org/frc5183/robot/subsystems/vision/io/VisionIOInputs.kt
💤 Files with no reviewable changes (20)
- src/main/kotlin/org/frc5183/robot/subsystems/vision/io/RealVisionIO.kt
- src/main/kotlin/org/frc5183/robot/logging/struct/EstimatedRobotPoseStruct.kt
- src/main/kotlin/org/frc5183/robot/subsystems/shooter/io/RealShooterIO.kt
- src/main/kotlin/org/frc5183/robot/subsystems/collector/io/CollectorIO.kt
- src/main/kotlin/org/frc5183/robot/subsystems/drive/io/RealSwerveDriveIO.kt
- src/main/kotlin/org/frc5183/robot/subsystems/turntable/io/TurntableIO.kt
- build.gradle.kts
- src/main/kotlin/org/frc5183/robot/subsystems/shooter/io/ShooterIO.kt
- src/main/kotlin/org/frc5183/robot/logging/AutoLogInputs.kt
- src/main/kotlin/org/frc5183/robot/subsystems/drive/io/SwerveDriveIOInputs.kt
- src/main/kotlin/org/frc5183/robot/subsystems/drive/io/SwerveDriveIO.kt
- src/main/kotlin/org/frc5183/robot/subsystems/vision/io/VisionIOInputs.kt
- src/main/kotlin/org/frc5183/robot/subsystems/vision/io/VisionIO.kt
- src/main/kotlin/org/frc5183/robot/subsystems/collector/io/CollectorIOInputs.kt
- src/main/kotlin/org/frc5183/robot/subsystems/turntable/io/TurntableIOInputs.kt
- src/main/kotlin/org/frc5183/robot/logging/struct/PhotonTrackedTargetStruct.kt
- src/main/kotlin/org/frc5183/robot/subsystems/shooter/io/ShooterIOInputs.kt
- src/main/kotlin/org/frc5183/robot/subsystems/drive/io/SimulatedSwerveDriveIO.kt
- src/main/kotlin/org/frc5183/robot/subsystems/collector/io/RealCollectorIO.kt
- src/main/kotlin/org/frc5183/robot/subsystems/turntable/io/RealTurntableIO.kt
Comment on lines
32
to
35
| class SwerveDriveSubsystem( | ||
| private val io: SwerveDriveIO, | ||
| private val vision: VisionSubsystem? = null, | ||
| val drive: SwerveDrive, | ||
| val vision: VisionSubsystem?, | ||
| ) : SubsystemBase() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "SwerveDriveSubsystem.kt" | head -5Repository: frc5183/cleobotra
Length of output: 137
🏁 Script executed:
find . -type f -name "FixedCamera.kt" | head -5Repository: frc5183/cleobotra
Length of output: 129
🏁 Script executed:
cat -n ./src/main/kotlin/org/frc5183/robot/subsystems/drive/SwerveDriveSubsystem.ktRepository: frc5183/cleobotra
Length of output: 5781
🏁 Script executed:
cat -n ./src/main/kotlin/org/frc5183/robot/subsystems/vision/FixedCamera.ktRepository: frc5183/cleobotra
Length of output: 6666
🏁 Script executed:
find . -type f -name "VisionSubsystem.kt" | head -5Repository: frc5183/cleobotra
Length of output: 133
🏁 Script executed:
cat -n ./src/main/kotlin/org/frc5183/robot/subsystems/vision/VisionSubsystem.ktRepository: frc5183/cleobotra
Length of output: 1545
Prevent re-adding stale vision measurements to the drive estimator.
The SwerveDriveSubsystem.periodic() method forwards every non-null pose to addVisionMeasurement() on every robot tick. However, frontCamera.estimatedRobotPose and backCamera.estimatedRobotPose only update when new frames arrive (see FixedCamera.updateUnreadResults() which feeds only new results to updateEstimatedGlobalPose()). This causes the same measurement timestamp to be re-added repeatedly until the next frame arrives, artificially overweighting stale vision data in the swerve drive's pose estimator.
Add monotonically increasing timestamp guards to each camera before calling addVisionMeasurement:
Suggested guard
class SwerveDriveSubsystem(
val drive: SwerveDrive,
val vision: VisionSubsystem?,
) : SubsystemBase() {
+ private var lastFrontVisionTimestamp = Double.NEGATIVE_INFINITY
+ private var lastBackVisionTimestamp = Double.NEGATIVE_INFINITY
+
val robotPose: Pose2d
get() = drive.pose
@@
override fun periodic() {
if (vision != null) {
- vision.frontRobotPose?.let {
+ vision.frontRobotPose?.takeIf { it.timestampSeconds > lastFrontVisionTimestamp }?.let {
+ lastFrontVisionTimestamp = it.timestampSeconds
addVisionMeasurement(it.estimatedPose.toPose2d(), it.timestampSeconds, vision.frontCamera.currentStandardDeviations)
}
- vision.backRobotPose?.let {
+ vision.backRobotPose?.takeIf { it.timestampSeconds > lastBackVisionTimestamp }?.let {
+ lastBackVisionTimestamp = it.timestampSeconds
addVisionMeasurement(it.estimatedPose.toPose2d(), it.timestampSeconds, vision.backCamera.currentStandardDeviations)
}
drive.updateOdometry()
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/org/frc5183/robot/subsystems/drive/SwerveDriveSubsystem.kt`
around lines 32 - 35, SwerveDriveSubsystem.periodic() is re-adding identical
vision measurements each tick because frontCamera.estimatedRobotPose and
backCamera.estimatedRobotPose only change when new frames arrive; to fix it add
monotonic timestamp guards (e.g., lastFrontVisionTimestamp and
lastBackVisionTimestamp fields) and only call drive.addVisionMeasurement(...)
when the incoming pose.timestamp is greater than the stored last timestamp, then
update the stored timestamp; reference the symbols
SwerveDriveSubsystem.periodic(), frontCamera.estimatedRobotPose,
backCamera.estimatedRobotPose, and drive.addVisionMeasurement() when making the
change so stale measurements from FixedCamera.updateUnreadResults() are not
repeatedly added.
Comment on lines
13
to
+18
| val targets: Array<PhotonTrackedTarget> | ||
| get() = ioInputs.targets | ||
| get() = | ||
| camera.allUnreadResults | ||
| .map { it.targets } | ||
| .flatten() | ||
| .toTypedArray() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "TurntableSubsystem" --type fRepository: frc5183/cleobotra
Length of output: 137
🏁 Script executed:
fd "FixedCamera" --type fRepository: frc5183/cleobotra
Length of output: 127
🏁 Script executed:
git ls-files | grep -E "(Turntable|Vision)" | head -20Repository: frc5183/cleobotra
Length of output: 421
🏁 Script executed:
cat -n src/main/kotlin/org/frc5183/robot/subsystems/turntable/TurntableSubsystem.ktRepository: frc5183/cleobotra
Length of output: 1161
🏁 Script executed:
cat -n src/main/kotlin/org/frc5183/robot/subsystems/vision/FixedCamera.ktRepository: frc5183/cleobotra
Length of output: 6666
🏁 Script executed:
rg "allUnreadResults" --type kotlin -A 2 -B 2Repository: frc5183/cleobotra
Length of output: 1054
🌐 Web query:
PhotonVision PhotonCamera allUnreadResults destructive read semantics
💡 Result:
PhotonCamera.getAllUnreadResults() / GetAllUnreadResults() has destructive-read semantics:
- It returns all
PhotonPipelineResults received since the previous call togetAllUnreadResults(). [1] - Calling it clears the camera’s internal FIFO queue, so subsequent calls return different results (often an empty list if nothing new arrived). [1]
- The FIFO depth is limited to 20 results; if you don’t call it often enough, older results can be dropped. [1]
- PhotonVision’s javadocs explicitly warn to call it exactly once per robot loop. [1]
Related note: getLatestResult() is not destructive (it always returns the most recent NT result), but it’s deprecated in favor of getAllUnreadResults() because it can miss results or return duplicates. [1]
Cache unread camera results once per loop.
targets consumes camera.allUnreadResults on every access, and line 22 already reads it once per scheduler tick for logging. Any later consumer of targets in the same loop will therefore see an empty array. FixedCamera already snapshots unread results in periodic() (lines 74-93); TurntableSubsystem should do the same.
📌 One way to fix it
class TurntableSubsystem(
val motor: SparkMax,
val camera: PhotonCamera,
) : SubsystemBase() {
+ private var cachedTargets: Array<PhotonTrackedTarget> = emptyArray()
+
val targets: Array<PhotonTrackedTarget>
- get() =
- camera.allUnreadResults
- .map { it.targets }
- .flatten()
- .toTypedArray()
+ get() = cachedTargets.copyOf()
override fun periodic() {
+ cachedTargets =
+ camera.allUnreadResults
+ .flatMap { it.targets }
+ .toTypedArray()
+
Logger.recordOutput("Turntable/Speed", motor.get())
Logger.recordOutput("Turntable/Targets", targets.map { it.fiducialId }.toIntArray())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/org/frc5183/robot/subsystems/turntable/TurntableSubsystem.kt`
around lines 13 - 18, The targets getter currently calls camera.allUnreadResults
each time, consuming unread results multiple times per scheduler loop; change
TurntableSubsystem to snapshot unread results once per loop by adding a private
backing field (e.g., cachedTargets: Array<PhotonTrackedTarget>) and populate it
inside TurntableSubsystem.periodic() by reading camera.allUnreadResults once,
mapping and flattening that result into the cachedTargets, then have the public
targets property simply return cachedTargets so later consumers and the existing
logging use the same snapshot.
Trip-kun
approved these changes
Mar 17, 2026
Trip-kun
left a comment
Trip-kun
left a comment
There was a problem hiding this comment.
Looks mostly good to me. See the rabbit's suggestions about missing symbols.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
the robot doesnt work in simulation and the abstractions are causing trouble so
Summary by CodeRabbit
Release Notes
Refactor
Chores