Add Object Detection Support #13
Conversation
…ctDetectionTelemetry
…tection telemetry
…ved maintainability
…ggle to object detection telemetry
…ctDetectionTelemetry
📝 WalkthroughWalkthroughThe pull request introduces substantial modifications to the vision subsystem of the robotics framework. It replaces the existing Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (2)
🔇 Additional comments (11)src/main/java/frc/alotobots/library/vision/photonvision/apriltag/PhotonvisionAprilTagSubsystemConstants.java (3)
The use of Line range hint The camera configuration shows:
Please:
Consider adding a comment explaining the camera selection strategy and future plans for the commented-out cameras. Line range hint The ✅ Verification successful
The
The code follows the established patterns for vision subsystems in FRC, where cameras are initialized once and remain active until robot shutdown. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for PhotonCamera cleanup patterns in the codebase
rg -A 5 "close\(\)|shutdown\(\)" | grep -C 5 "PhotonCamera"
# Check if there are any existing patterns for managing PhotonCamera lifecycle
ast-grep --pattern 'try ($camera = new PhotonCamera($_)) { $$$ }'
Length of output: 127 Script: #!/bin/bash
# Let's try a broader search for PhotonCamera usage patterns
rg -l "PhotonCamera"
# Check PhotonCamera instantiation patterns
ast-grep --pattern 'new PhotonCamera($$$)'
# Look for any dispose/close patterns in subsystem classes
rg -A 5 "extends SubsystemBase" | grep -C 10 "PhotonCamera"
# Search for any lifecycle management in test files
fd -e java -E "build" | xargs rg "PhotonCamera.*close|PhotonCamera.*dispose"
Length of output: 3184 src/main/java/frc/alotobots/RobotContainer.java (3)
The new imports follow the logical package structure for vision components.
While the AprilTag subsystem is properly configured with the drivetrain, there's no setup for the object detection subsystem. Please verify if:
Let's check for any other configuration points: #!/bin/bash
# Search for object detection subsystem configuration
echo "Checking for object detection configuration:"
rg "photonvisionObjectDetectionSubsystem\." --type java -A 3
# Check if there are similar setup methods in other files
echo "Checking for other setup methods:"
ast-grep --pattern 'setupVision($$$) {
$$$
}'
There's an architectural inconsistency in how the vision subsystems are initialized:
This difference in initialization patterns might indicate:
Let's verify the dependency requirements: ✅ Verification successfulLet me try another verification approach to understand the dependency patterns better. Let me check the implementation of the DetectedObject class to understand why the drivetrain dependency is needed. Based on the DetectedObject implementation, I can now provide a final response about the architectural pattern: Drivetrain dependency is correctly used in vision subsystems The dependency injection pattern is actually consistent with the system's requirements:
Key evidence from the code:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check how these subsystems are used in the codebase
# Check AprilTag subsystem usage patterns
echo "Checking AprilTag subsystem usage:"
ast-grep --pattern 'new PhotonvisionAprilTagSubsystem($$$)'
# Check ObjectDetection subsystem usage patterns
echo "Checking ObjectDetection subsystem usage:"
ast-grep --pattern 'new PhotonVisionObjectDetectionSubsystem($$$)'
# Check method calls that might explain the dependency
echo "Checking drivetrain-related method calls:"
rg -l "drivetrain" --type java | xargs ast-grep --pattern 'photonvisionObjectDetectionSubsystem.$$$'
Length of output: 604 Script: #!/bin/bash
# Check implementation of both subsystems
echo "=== PhotonvisionAprilTagSubsystem Implementation ==="
fd "PhotonvisionAprilTagSubsystem.java" --type f --exec cat {}
echo -e "\n=== PhotonVisionObjectDetectionSubsystem Implementation ==="
fd "PhotonVisionObjectDetectionSubsystem.java" --type f --exec cat {}
# Check where drivetrain is used in object detection
echo -e "\n=== Drivetrain Usage in Object Detection ==="
rg "drivetrain" -B2 -A2 --type java
Length of output: 38297 Script: #!/bin/bash
# Check DetectedObject implementation to understand drivetrain usage
echo "=== DetectedObject Implementation ==="
fd "DetectedObject.java" --type f --exec cat {}
# Check if there are any methods in ObjectDetection subsystem that use drivetrain
echo -e "\n=== Methods using drivetrain in ObjectDetection ==="
rg -p "drivetrain" PhotonVisionObjectDetectionSubsystem.java
Length of output: 11287 src/main/java/frc/alotobots/library/vision/photonvision/objectdetection/DetectedObject.java (1)
The static ✅ Verification successfulLet me gather more context about the usage patterns of DetectedObject class. Based on the analysis of the code, I can now generate the final response: Static drive field is thread-safe in this context The static
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for potential concurrent access to the static drive field
# Search for concurrent access patterns
rg -l "ExecutorService|Runnable|Thread|CompletableFuture|parallel|async" | while read -r file; do
echo "=== $file ==="
rg "DetectedObject\.(?:get)?drive" "$file" -A 5 -B 5
done
# Search for multiple initialization points
ast-grep --pattern 'DetectedObject.setDrive($_)'
Length of output: 1069 Script: #!/bin/bash
# Check how DetectedObject is used and accessed
rg "DetectedObject" -A 5 -B 5
# Check for any periodic/scheduled tasks that might access drive field
rg "periodic|schedule|interval|timer" -l | while read -r file; do
echo "=== $file ==="
rg "DetectedObject|drive" "$file" -A 5 -B 5
done
# Look for the full DetectedObject class implementation
ast-grep --pattern 'class DetectedObject {
$$$
}'
Length of output: 59204 src/main/java/frc/alotobots/library/drivetrains/swerve/ctre/SwerveDriveSubsystem.java (2)Line range hint While the PR title is "Add Object Detection Support", this class has been modified to work specifically with AprilTags but shows no integration with general object detection functionality. Please clarify:
#!/bin/bash
# Search for files that might contain the object detection implementation
echo "Files potentially related to object detection:"
fd -e java -e kt | rg -l "ObjectDetection|Detection|Vision"
# Search for references to the new subsystems mentioned in the AI summary
echo "\nReferences to new vision subsystems:"
rg "PhotonVisionObjectDetectionSubsystem|PhotonvisionAprilTagSubsystem"
While the changes transition to AprilTag-based vision, the imports and usage suggest this class only handles AprilTag detection. Given that the PR's objective is to "Add Object Detection Support", we should verify if object detection functionality needs to be integrated here as well. Also applies to: 22-22 src/main/java/frc/alotobots/game/HMIStation.java (1)
The code was previously using JoystickUtilities without an explicit import. Let's verify this doesn't break any existing functionality. src/main/java/frc/alotobots/library/vision/photonvision/objectdetection/PhotonVisionObjectDetectionSubsystem.java (1)
Ensure Accessing Run the following script to confirm that both arrays have the same length: ✅ Verification successfulArrays are properly synchronized - no issues found The
The arrays are properly aligned in both length and order, eliminating any risk of 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that CAMERAS and CAMERA_OFFSETS arrays have the same length.
# Expected: Both arrays should be declared and have the same length.
# Using ast-grep to extract the array lengths.
ast-grep --pattern $'public static final PhotonCamera[] CAMERAS = new PhotonCamera[$_];' PhotonVisionObjectDetectionSubsystemConstants.java
ast-grep --pattern $'public static final Transform3d[] CAMERA_OFFSETS = new Transform3d[$_];' PhotonVisionObjectDetectionSubsystemConstants.java
Length of output: 482 Script: #!/bin/bash
# First, let's find the correct path to the constants file
fd PhotonVisionObjectDetectionSubsystemConstants.java
# Then, let's search for array declarations and initializations
rg -A 5 "CAMERAS|CAMERA_OFFSETS" --type java
Length of output: 18684 Script: #!/bin/bash
# Let's get the full content of the constants file to see the array declarations and their sizes
cat src/main/java/frc/alotobots/library/vision/photonvision/objectdetection/PhotonVisionObjectDetectionSubsystemConstants.java
Length of output: 1269 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (26)
src/main/java/frc/alotobots/library/vision/photonvision/objectdetection/GameElement.java (4)
5-9: Consider adding input validation for the fieldsSince these fields represent physical measurements and identifiers:
- Length, width, and height should be positive numbers
- Name should not be null or empty
- ClassId might need range validation based on PhotonVision's specifications
private String name; private double length; private double width; private double height; private int classId; + +private void validateInputs(String name, double length, double width, double height, int classId) { + if (name == null || name.trim().isEmpty()) { + throw new IllegalArgumentException("Name cannot be null or empty"); + } + if (length <= 0 || width <= 0 || height <= 0) { + throw new IllegalArgumentException("Dimensions must be positive"); + } + // Add classId validation if there are known constraints from PhotonVision +}
11-19: Enhance constructor documentationThe documentation should:
- Include @throws tags if input validation is added
- Consider making the units more prominent in parameter documentation
/** * Represents a game element (measurements are in meters) * - * @param name The name of the object - * @param length The length of the object - * @param width The width of the object - * @param height The height of the object (floor to top) + * @param name The name of the object + * @param length The length of the object in meters + * @param width The width of the object in meters + * @param height The height of the object in meters (floor to top) * @param classId The class ID reported by PhotonVision + * @throws IllegalArgumentException if any dimension is not positive or name is empty */
20-26: Add input validation to constructorThe constructor should validate inputs before assignment.
public GameElement(String name, double length, double width, double height, int classId) { + validateInputs(name, length, width, height, classId); this.name = name; this.length = length; this.width = width; this.height = height; this.classId = classId; }
28-46: LGTM! Consider making the class immutableThe getter implementations are clean and follow best practices. Since this class represents physical game elements with fixed properties, consider making it immutable by:
- Making all fields final
- Ensuring deep immutability if you add any object fields in the future
- private String name; - private double length; - private double width; - private double height; - private int classId; + private final String name; + private final double length; + private final double width; + private final double height; + private final int classId;src/main/java/frc/alotobots/library/vision/photonvision/apriltag/PhotonvisionAprilTagSubsystemConstants.java (3)
Line range hint
45-46: Document vision pose estimation control flagsThe boolean flags control critical vision functionality but lack documentation explaining:
- Impact of enabling/disabling vision pose estimation
- Reasons for potentially limiting pose estimation to teleop
Add documentation explaining the purpose and implications of these flags:
+ /** Controls whether vision-based pose estimation is used at all */ public static final boolean USE_VISION_POSE_ESTIMATION = true; + /** When true, vision pose estimation only runs during teleop, potentially saving processing power during auto */ public static final boolean ONLY_USE_POSE_ESTIMATION_IN_TELEOP = false;
Line range hint
48-59: Review vision standard deviation values and update processSeveral concerns with the current configuration:
- The comment "NEEDS TO BE COPIED FROM PHOTONVISION ON COMPETITION DAY" suggests a manual process that could be error-prone
- The heading standard deviation (1000) effectively disables vision-based heading correction
Consider:
- Automating the process of obtaining these values
- Documenting why the heading correction is disabled
- Adding validation to ensure values are updated before competition
Would you like help creating a utility to automatically fetch these values from PhotonVision?
Line range hint
61-71: Document constant tuning methodologyThe pose estimation and smoothing constants lack documentation explaining:
- How these values were determined
- What effects adjusting these values has on performance
- When these values should be adjusted
Consider adding detailed comments explaining:
- The tuning process for these constants
- The impact of different values
- Guidelines for when and how to adjust them
src/main/java/frc/alotobots/RobotContainer.java (1)
27-28: Consider adding Javadoc for new subsystem variables.While the variable declarations are well-structured, adding Javadoc comments would improve code documentation and maintainability.
+ /** Subsystem handling AprilTag detection and processing */ private final PhotonvisionAprilTagSubsystem photonvisionAprilTagSubsystem; + /** Subsystem managing object detection capabilities */ private final PhotonVisionObjectDetectionSubsystem photonvisionObjectDetectionSubsystem;src/main/java/frc/alotobots/library/vision/photonvision/objectdetection/DetectedObject.java (2)
102-121: Improve error handling consistency and documentation.The
getDistanceandgetAnglemethods have inconsistent null checking and could benefit from better error handling:
getDistancechecks for null target and robotToCamera, butgetAngleonly checks drive- Both methods silently return 0 on error, which could mask issues
- Javadoc doesn't document error cases
Consider these improvements:
/** * Gets the distance from the center of the robot to the object. * * @return The distance in meters. + * @throws IllegalStateException if drive is not set or required fields are null */ public double getDistance() { - if (target == null || robotToCamera == null) { - return 0; + if (drive == null || target == null || robotToCamera == null) { + throw new IllegalStateException("Required fields not initialized"); } return PhotonUtils.getDistanceToPose(drive.getPose(), pose.toPose2d()); } /** * Gets the field-relative angle from the robot to the object. (Rotational) * * @return The angle in radians. + * @throws IllegalStateException if drive is not set */ public double getAngle() { - if (drive != null) { - return Math.atan2( - pose.getY() - drive.getState().Pose.getY(), - pose.getX() - drive.getState().Pose.getX()); + if (drive == null) { + throw new IllegalStateException("Drive not initialized"); } - return 0; + return Math.atan2( + pose.getY() - drive.getState().Pose.getY(), + pose.getX() - drive.getState().Pose.getX()); }
123-126: Enhance toString output with more useful information.The current implementation could be more helpful for debugging by including additional information about the target and transformations.
Consider enhancing the output:
@Override public String toString() { - return String.format("Target at (%.2f, %.2f, %.2f)", pose.getX(), pose.getY(), pose.getZ()); + return String.format("Target[id=%d] at (%.2f, %.2f, %.2f), distance=%.2f, angle=%.2f°", + target != null ? target.getFiducialId() : -1, + pose.getX(), pose.getY(), pose.getZ(), + getDistance(), + Units.radiansToDegrees(getAngle())); }src/main/java/frc/alotobots/library/drivetrains/swerve/ctre/SwerveDriveSubsystem.java (2)
38-38: Consider a more specific variable nameThe variable name
subSysPhotonvisionis now less accurate since it specifically handles AprilTag functionality. Consider renaming it tosubSysAprilTagoraprilTagSubsystemto better reflect its specific purpose.- private PhotonvisionAprilTagSubsystem subSysPhotonvision; + private PhotonvisionAprilTagSubsystem aprilTagSubsystem;
146-148: Update method name and documentationThe method name and documentation should be updated to reflect that it specifically sets an AprilTag subsystem:
- The method name could be more specific
- The Javadoc comment refers to "PhotonVision subsystem" which is outdated
- /** - * Sets the SubSys_PhotonVision object to use for vision-based pose estimation. - * - * @param subSysPhotonvision The PhotonVision subsystem to use. - */ - public void setPhotonVisionSubSys(PhotonvisionAprilTagSubsystem subSysPhotonvision) { - this.subSysPhotonvision = subSysPhotonvision; + /** + * Sets the AprilTag subsystem to use for vision-based pose estimation. + * + * @param aprilTagSubsystem The AprilTag subsystem to use. + */ + public void setAprilTagSubsystem(PhotonvisionAprilTagSubsystem aprilTagSubsystem) { + this.aprilTagSubsystem = aprilTagSubsystem;src/main/java/frc/alotobots/game/HMIStation.java (3)
Line range hint
41-47: Consider implementing the suggested button naming improvements.The TODO comment suggests renaming buttons based on function rather than number. This would improve code readability and maintainability. For example:
- public final JoystickButton driverButton1 = new JoystickButton(driverController, 1); - public final JoystickButton driverButton2 = new JoystickButton(driverController, 2); - public final JoystickButton driverButton3 = new JoystickButton(driverController, 3); + public final JoystickButton primaryActionButton = new JoystickButton(driverController, 1); + public final JoystickButton secondaryActionButton = new JoystickButton(driverController, 2); + public final JoystickButton tertiaryActionButton = new JoystickButton(driverController, 3);
Line range hint
173-179: Clean up commented-out auxiliary button declarations.These commented-out button declarations should either be implemented or removed to maintain clean code.
- // public final JoystickButton auxButton3 = new JoystickButton(auxdriverController, 3); - // public final JoystickButton auxButton4 = new JoystickButton(auxdriverController, 4); - // public final JoystickButton auxButton5 = new JoystickButton(auxdriverController, 5); - // public final JoystickButton auxButton6 = new JoystickButton(auxdriverController, 6); - // Button Box
Line range hint
227-229: Consider enhancing alliance position handling.The
alliancePosition()method retrieves but doesn't use the alliance color. Consider enhancing this to support the new vision system's potential alliance-specific behaviors.This could be particularly relevant for object detection where alliance color might influence target identification or tracking behavior.
src/main/java/frc/alotobots/library/vision/photonvision/apriltag/PhotonvisionAprilTagSubsystem.java (3)
Line range hint
52-74: Enhance initialization logging and validationWhile the initialization logic is solid, it would benefit from more detailed logging and validation.
Consider adding more detailed logging and validation:
private void initializePoseEstimators() { poseEstimators = new ArrayList<>(); if (CAMERAS.length != CAMERA_OFFSETS.length) { - throw new RuntimeException( - "PhotonCamera object is missing offset! Did you add an offset in Photonvision_Constants?"); + throw new IllegalStateException( + String.format("Camera count (%d) does not match offset count (%d). Check PhotonVision_Constants.", + CAMERAS.length, CAMERA_OFFSETS.length)); } for (int i = 0; i < CAMERAS.length; i++) { if (CAMERAS[i] != null) { + System.out.printf("Initializing camera %d (%s)...%n", i, CAMERAS[i].getName()); PhotonPoseEstimator estimator = new PhotonPoseEstimator( fieldLayout, PhotonPoseEstimator.PoseStrategy.MULTI_TAG_PNP_ON_COPROCESSOR, CAMERAS[i], CAMERA_OFFSETS[i]); estimator.setMultiTagFallbackStrategy(PhotonPoseEstimator.PoseStrategy.LOWEST_AMBIGUITY); poseEstimators.add(estimator); camerasEnabled[i] = CAMERAS[i].isConnected(); + System.out.printf("Camera %d initialized. Connected: %b%n", i, camerasEnabled[i]); } else { + System.out.printf("Warning: Camera %d is null, skipping initialization%n", i); poseEstimators.add(null); camerasEnabled[i] = false; } } }
Line range hint
77-119: Add error handling and timestamp validation for tag detectionThe tag detection logic should handle potential camera failures and validate result timestamps.
Consider adding error handling and timestamp validation:
private List<PhotonTrackedTarget> getDetectedTags() { List<PhotonTrackedTarget> allDetectedTags = new ArrayList<>(); - for (PhotonCamera camera : CAMERAS) { - var result = camera.getLatestResult(); - if (result.hasTargets()) { - allDetectedTags.addAll(result.getTargets()); + for (int i = 0; i < CAMERAS.length; i++) { + PhotonCamera camera = CAMERAS[i]; + if (camera == null || !camerasEnabled[i]) { + continue; + } + try { + var result = camera.getLatestResult(); + // Validate result timestamp (e.g., not too old) + double latency = result.getLatencyMillis() / 1000.0; + if (latency > MAX_ACCEPTABLE_LATENCY) { + System.out.printf("Warning: High latency (%.2fs) from camera %d%n", latency, i); + continue; + } + if (result.hasTargets()) { + allDetectedTags.addAll(result.getTargets()); + } + } catch (Exception e) { + System.err.printf("Error getting results from camera %d: %s%n", i, e.getMessage()); } } return allDetectedTags; }
Line range hint
380-399: Optimize camera pose estimation and add null checksThe per-camera pose estimation could be optimized by checking camera connection status first and adding proper null checks.
Consider this optimization:
public List<Pair<Integer, Pair<Pose3d, Double>>> getPerCameraEstimatedPoses() { List<Pair<Integer, Pair<Pose3d, Double>>> perCameraPoses = new ArrayList<>(); for (int i = 0; i < poseEstimators.size(); i++) { + // Check camera validity first to avoid unnecessary operations + PhotonCamera camera = CAMERAS[i]; + if (camera == null || !camerasEnabled[i] || !camera.isConnected()) { + continue; + } + PhotonPoseEstimator estimator = poseEstimators.get(i); - PhotonCamera camera = CAMERAS[i]; + if (estimator == null) { + continue; + } - if (estimator != null && camerasEnabled[i] && camera != null && camera.isConnected()) { - // Get the latest result directly from the camera first + try { var result = camera.getLatestResult(); if (result.hasTargets()) { - // Only update the estimator if we actually have targets var estimate = estimator.update(); if (estimate.isPresent()) { EstimatedRobotPose pose = estimate.get(); perCameraPoses.add( new Pair<>(i, new Pair<>(pose.estimatedPose, pose.timestampSeconds))); } } + } catch (Exception e) { + System.err.printf("Error estimating pose for camera %d: %s%n", i, e.getMessage()); } } return perCameraPoses; }src/main/java/frc/alotobots/library/vision/photonvision/objectdetection/PhotonVisionObjectDetectionSubsystem.java (3)
33-35: Return an unmodifiable list to prevent external modificationCurrently,
getDetectedObjects()returns a newArrayList, which creates a copy but doesn't prevent external code from modifying theDetectedObjectinstances within. To ensure encapsulation and prevent unintended modifications, consider returning an unmodifiable view.You can modify the method as follows:
-public List<DetectedObject> getDetectedObjects() { - return new ArrayList<>(detectedObjects); +public List<DetectedObject> getDetectedObjects() { + return Collections.unmodifiableList(detectedObjects); }Don't forget to import
java.util.Collections.
77-77: Consider logging when no targets are foundCurrently, if a camera doesn't have targets, there's no feedback. For debugging and monitoring purposes, consider logging when no targets are detected by an enabled camera.
Example:
} else { // Optionally log that no targets were found for this camera logger.fine("No targets found for camera: " + camera.getName()); }Ensure the appropriate logging level is used to avoid cluttering logs.
68-75: Extract target processing into a separate method for clarityTo improve readability and maintainability, consider extracting the target processing logic inside the loop into a separate method.
Example:
private void processTargets(PhotonTrackedTarget target, int cameraIndex) { DetectedObject object = DetectedObject.fromPhotonTarget( target, PhotonVisionObjectDetectionSubsystemConstants.CAMERA_OFFSETS[cameraIndex]); detectedObjects.add(object); }Update the loop:
for (PhotonTrackedTarget target : result.getTargets()) { - // Create DetectedObject using camera transform for each target - DetectedObject object = - DetectedObject.fromPhotonTarget( - target, PhotonVisionObjectDetectionSubsystemConstants.CAMERA_OFFSETS[i]); - detectedObjects.add(object); + processTargets(target, i); }src/main/java/frc/alotobots/library/vision/photonvision/apriltag/PhotonvisionAprilTagTelemetry.java (5)
90-94: Consolidate duplicate Javadoc comments ininitializeFieldmethodThe
initializeFieldmethod contains duplicate Javadoc comments. Merging them will improve readability and avoid redundancy.Apply this diff to combine the comments:
/** - * Initializes the field widget in Shuffleboard. - */ -/** * Initializes the field widget in Shuffleboard. Creates and configures a Field2d widget to * display robot position and AprilTag locations. */
99-104: Merge duplicate Javadoc comments ininitializeCameraWidgetsmethodThere are duplicate Javadoc comments in the
initializeCameraWidgetsmethod. Consolidating them will enhance clarity.Apply this diff to merge the comments:
/** * Initializes camera-specific widgets in Shuffleboard. - */ -/** * Initializes camera-specific widgets in Shuffleboard. Creates a widget for each camera in the * CAMERAS array, displaying its pose and status information. */ + * Creates a widget for each camera in the + * CAMERAS array, displaying its pose and status information. */
Line range hint
125-132: Eliminate duplicate Javadoc comments inupdateCameraWidgetsmethodThe
updateCameraWidgetsmethod has two Javadoc comments that can be combined for better readability.Apply this diff to consolidate the comments:
/** * Updates the camera-specific widgets with the latest data. * * @param cameras The array of PhotonCameras to get data from - */ -/** * Updates the camera-specific widgets with the latest data from each camera. Displays connection * status, target detection results, and pose information. * * @param cameras Array of PhotonCameras to get data from */ + * Displays connection status, target detection results, and pose information. */
241-243: Correct reference to 'photonvisionTab' in the Javadoc commentThe comment incorrectly refers to
photonvisionTab. It should be updated toaprilTagTabto reflect the refactoring.Apply this diff to correct the comment:
/** - * photonvisionTab Draws tracer lines from each camera's detected pose to its detected AprilTags. + * Draws tracer lines from each camera's detected pose to its detected AprilTags. * * @param cameraPoses List of camera poses and their indices */
Line range hint
244-273: Consider refactoringdrawTracerLinesmethod for improved readabilityThe
drawTracerLinesmethod contains nested loops and complex logic. Refactoring it into smaller helper methods could enhance readability and maintainability.For example, you could extract the logic for creating trajectories and drawing tag boxes into separate methods.
🛑 Comments failed to post (8)
src/main/java/frc/alotobots/library/vision/photonvision/objectdetection/DetectedObject.java (2)
52-95:
⚠️ Potential issueAdd input validation and improve error handling in factory method.
The
fromPhotonTargetmethod has several areas that could be improved:
- No validation of input parameters
- Hardcoded array index
GAME_ELEMENTS[0]is fragile- Complex transformation logic could use more detailed comments
Consider these improvements:
public static DetectedObject fromPhotonTarget( PhotonTrackedTarget target, Transform3d robotToCamera) { + if (target == null || robotToCamera == null) { + throw new IllegalArgumentException("Target and robotToCamera must not be null"); + } + + // Validate game element exists + if (GAME_ELEMENTS.length == 0) { + throw new IllegalStateException("No game elements configured"); + } // Calculate distance using the first game element's height double targetToCameraDistance = PhotonUtils.calculateDistanceToTargetMeters( robotToCamera.getZ(), GAME_ELEMENTS[0].getHeight(), robotToCamera.getRotation().getY(), Units.degreesToRadians(target.getPitch())); // Rest of the method...Committable suggestion skipped: line range outside the PR's diff.
14-14: 🛠️ Refactor suggestion
Consider removing static state for better testability and maintainability.
The static
drivefield creates a global state that can make the code harder to test and maintain. Consider injecting the drive subsystem through the constructor instead.Here's a suggested refactoring approach:
- @Getter private static SwerveDriveSubsystem drive; + @Getter private final SwerveDriveSubsystem drive; - public DetectedObject(Pose3d pose, PhotonTrackedTarget target, Transform3d robotToCamera) { + public DetectedObject(Pose3d pose, PhotonTrackedTarget target, Transform3d robotToCamera, SwerveDriveSubsystem drive) { this.pose = pose; this.target = target; this.robotToCamera = robotToCamera; + this.drive = drive; }Committable suggestion skipped: line range outside the PR's diff.
src/main/java/frc/alotobots/library/vision/photonvision/apriltag/PhotonvisionAprilTagSubsystem.java (2)
247-274: 🛠️ Refactor suggestion
Validate smoothing parameters and add bounds checking
The smoothing logic should validate alpha parameters and ensure they're within valid bounds (0-1).
Add parameter validation:
private double exponentialSmooth(double oldValue, double newValue, double alpha) { + if (alpha < 0.0 || alpha > 1.0) { + System.err.printf("Warning: Invalid smoothing alpha (%.2f), clamping to [0,1]%n", alpha); + alpha = Math.max(0.0, Math.min(1.0, alpha)); + } return alpha * newValue + (1.0 - alpha) * oldValue; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private double exponentialSmooth(double oldValue, double newValue, double alpha) { if (alpha < 0.0 || alpha > 1.0) { System.err.printf("Warning: Invalid smoothing alpha (%.2f), clamping to [0,1]%n", alpha); alpha = Math.max(0.0, Math.min(1.0, alpha)); } return alpha * newValue + (1.0 - alpha) * oldValue; }
36-43: 🛠️ Refactor suggestion
Add error handling for field layout initialization
The constructor should handle potential failures when loading the AprilTag field layout, as this is a critical component for pose estimation.
Consider wrapping the field layout initialization with try-catch:
public PhotonvisionAprilTagSubsystem() { - this.fieldLayout = AprilTagFields.k2024Crescendo.loadAprilTagLayoutField(); + try { + this.fieldLayout = AprilTagFields.k2024Crescendo.loadAprilTagLayoutField(); + } catch (Exception e) { + throw new RuntimeException("Failed to load AprilTag field layout", e); + } this.poseEstimators = new ArrayList<>(); this.camerasEnabled = new boolean[CAMERAS.length]; Arrays.fill(camerasEnabled, true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public PhotonvisionAprilTagSubsystem() { try { this.fieldLayout = AprilTagFields.k2024Crescendo.loadAprilTagLayoutField(); } catch (Exception e) { throw new RuntimeException("Failed to load AprilTag field layout", e); } this.poseEstimators = new ArrayList<>(); this.camerasEnabled = new boolean[CAMERAS.length]; Arrays.fill(camerasEnabled, true); initializePoseEstimators(); telemetry = new PhotonvisionAprilTagTelemetry(); System.out.println("PhotonVision AprilTag subsystem initialized");src/main/java/frc/alotobots/library/vision/photonvision/objectdetection/PhotonVisionObjectDetectionSubsystem.java (2)
64-66:
⚠️ Potential issueHandle potential null results from
camera.getLatestResult()
camera.getLatestResult()might returnnullif no result is available. Accessing methods on anullresult will cause aNullPointerException. Add a null check before using the result.Apply this change to handle null results:
var result = camera.getLatestResult(); +if (result != null && result.hasTargets()) { - if (result.hasTargets()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.var result = camera.getLatestResult(); if (result != null && result.hasTargets()) {
23-23: 🛠️ Refactor suggestion
Consider using a logging framework instead of
System.out.printlnUsing a logging framework (e.g.,
java.util.logging,Log4j, orSLF4J) provides better control over log levels and output destinations, which is more suitable for production code.Apply this change to use a logger:
- System.out.println("PhotonVisionObjectDetection Subsystem Initialized"); + private static final Logger logger = Logger.getLogger(PhotonVisionObjectDetectionSubsystem.class.getName()); + logger.info("PhotonVisionObjectDetection Subsystem Initialized");Don't forget to import the appropriate logging classes.
Committable suggestion skipped: line range outside the PR's diff.
src/main/java/frc/alotobots/library/vision/photonvision/objectdetection/PhotonVisionObjectDetectionTelemetry.java (2)
140-145:
⚠️ Potential issueFix potential index misalignment between
cameraWidgetsandCAMERASIn the constructor,
cameraWidgetsis populated only when a camera is notnull. This can cause misalignment between the indices ofCAMERASandcameraWidgets, leading to potentialIndexOutOfBoundsExceptionor incorrect widget updates in theupdateObjectsmethod.Apply this diff to ensure that
cameraWidgetsaligns with theCAMERASarray by adding a placeholder when a camera isnull:// Initialize camera widgets - position them below the field for (int i = 0; i < PhotonVisionObjectDetectionSubsystemConstants.CAMERAS.length; i++) { PhotonCamera camera = PhotonVisionObjectDetectionSubsystemConstants.CAMERAS[i]; - if (camera != null) { - cameraWidgets.add(new CameraWidget(tab, camera.getName(), i * 2)); - } + CameraWidget widget = (camera != null) + ? new CameraWidget(tab, camera.getName(), i * 2) + : null; + cameraWidgets.add(widget); }This ensures that
cameraWidgetshas the same size asCAMERAS, maintaining index alignment.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for (int i = 0; i < PhotonVisionObjectDetectionSubsystemConstants.CAMERAS.length; i++) { PhotonCamera camera = PhotonVisionObjectDetectionSubsystemConstants.CAMERAS[i]; CameraWidget widget = (camera != null) ? new CameraWidget(tab, camera.getName(), i * 2) : null; cameraWidgets.add(widget); }
185-212:
⚠️ Potential issueAdd null checks for
CameraWidgetinstances inupdateObjectsAfter ensuring index alignment, you need to handle potential
nullentries incameraWidgetswhen a camera isnull. Without this check, attempting to access anullwidget will result in aNullPointerException.Apply this diff to add a null check before using the
CameraWidget:for (int i = 0; i < PhotonVisionObjectDetectionSubsystemConstants.CAMERAS.length; i++) { PhotonCamera camera = PhotonVisionObjectDetectionSubsystemConstants.CAMERAS[i]; - if (camera != null && i < cameraWidgets.size()) { + if (camera != null && i < cameraWidgets.size() && cameraWidgets.get(i) != null) { CameraWidget widget = cameraWidgets.get(i); + if (widget != null) { // Update connection status widget.connectionStatus.setBoolean(camera.isConnected()); // Existing code... + } } }This ensures that you only interact with non-null
CameraWidgetinstances.Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
PhotonvisionAprilTagSubsystemandPhotonVisionObjectDetectionSubsystemfor enhanced object detection capabilities.DetectedObjectandGameElementto represent detected objects and game elements, respectively.Bug Fixes
Documentation
These updates aim to improve the overall functionality and user experience in vision-based tasks within the robotics framework.