Skip to content

removed vision - #11

Open
Orcasphynx wants to merge 1 commit into
developmentfrom
chore/cleanup
Open

removed vision#11
Orcasphynx wants to merge 1 commit into
developmentfrom
chore/cleanup

Conversation

@Orcasphynx

Copy link
Copy Markdown
Collaborator

No description provided.

@github-actions

Copy link
Copy Markdown

AI Code Review

Hey team!

This pull request looks great for streamlining our robot's codebase by removing the vision components. It's always good to keep our code focused on what's actively being used, and updating dependencies is a solid practice for keeping us on the latest and greatest!

Positive Highlights

  • Clear Intent: The PR title "removed vision" perfectly describes the changes, which makes reviewing and understanding the purpose straightforward.
  • Clean Removal: You've done a good job of completely removing the VisionSubsystem and its associated logic from RobotContainer and DrivetrainSubsystem. This prevents dead code from lingering.
  • Dependency Updates: Updating Phoenix6 and PhotonLib versions is excellent! Staying current with libraries helps us take advantage of bug fixes, new features, and performance improvements.

Suggestions

Code Quality & FRC/WPILib Best Practices

  • Unused Import in RobotContainer: It looks like the import for frc.robot.subsystems.vision.VisionSubsystem; is still present in RobotContainer.java even after the vision instance was removed.
    • Suggestion: Consider removing this import to keep the file tidy and prevent potential confusion. An IDE like VS Code or IntelliJ usually highlights these as unused.
  • DriveState Class Re-evaluation: In DrivetrainSubsystem.java, the removal of driveState = DriveState.getInstance(); and its associated periodic() logic means that anything DriveState was coordinating (beyond vision) is now gone.
    • Suggestion: If DriveState was intended to manage other aspects of the drivetrain's state or coordinate with other subsystems (as per our Superstructure Pattern guideline), it might be helpful to either re-introduce a simplified DriveState or integrate its essential non-vision logic directly into the DrivetrainSubsystem or a new superstructure. If its sole purpose was vision interaction, then its removal is perfectly fine.
    • Why it matters: A dedicated state management class helps prevent conflicts and ensures predictable robot behavior, especially for complex actions.
  • Logging Consistency: You're using a custom Telemetry class and registering it with the drivetrain. RobotContainer is also annotated with @Logged, which implies Epilogue is being used.
    • Suggestion: It might be helpful to consolidate our logging strategy. Consider using Epilogue (or AdvantageKit if that's the team's direction) exclusively for all logging, including drivetrain telemetry, to ensure consistent data collection, easier analysis, and potentially deterministic replay. If Telemetry has unique features, perhaps integrate them into an Epilogue-compatible format.
    • Why it matters: Consistent logging makes debugging and analysis much more efficient, especially when trying to understand robot behavior during matches or simulations.

Questions

  • Could you share a bit more about the original design intent for the DriveState class? Was it solely for vision integration, or did it have other responsibilities that might need to be re-implemented or moved elsewhere now that vision is removed?
  • What are your thoughts on using Epilogue for all logging, including the telemetry currently handled by your Telemetry class, given that RobotContainer is already set up for it?

Great work on this PR! Keep up the excellent coding!


This review was automatically generated by AI. Please use your judgment and feel free to discuss any suggestions!

@github-actions

Copy link
Copy Markdown

✓ Build successful and code formatting check passed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant