-
Notifications
You must be signed in to change notification settings - Fork 0
Vision #2
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
base: main
Are you sure you want to change the base?
Conversation
jovapo
left a comment
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.
Comments in line. The main items are:
- isMinimumDistanceTooSmall naming confusing: the name says "too small" but the logic rejects targets that are too far (>= 2.5m). Either rename or fix the logic.
- FieldCentricFacingAngle allocated every loop cycle: creates a new object + resets the heading PID every 20ms. Pull it out to a field like the other swerve requests.
- Try to get pose from lib not hard coded
| return false; | ||
| } | ||
|
|
||
| public boolean isMinimumDistanceTooSmall(EstimatedRobotPose pose) { |
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.
isMinimumDistanceTooSmall could have a better name — the logic rejects poses where distance is large (>= 2.5m), not small. Consider renaming to isClosestTagTooFar or similar.
|
|
||
| private List<CamAndEstimator> cameras = new ArrayList<>(); | ||
|
|
||
| public Consumer<VisionPose> updateDrivetrain; |
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.
These fields (updateDrivetrain, getSimPose, visionSim, cameraProp) should be private. Being public + @Logged may cause issues with non-serializable types like VisionSystemSim.
| } | ||
| } | ||
|
|
||
| public boolean usingTwoTags(EstimatedRobotPose pose) { |
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.
usingTwoTags is defined but never called — remove it or wire it up.
| && (pose.estimatedPose.getY() > 0); | ||
| } | ||
|
|
||
| public Pose3d khubLoc() { |
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.
khubLoc() is unused and doesn't follow Java naming conventions. Remove or rename to getHubLocation().
|
|
||
| public static final double kAmbiguityTolerance = 0.25; | ||
| public static final double kDistanceTolerance = 2.5; | ||
| public static final double kMaxAngleTolerance = 70; |
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.
kMaxAngleTolerance is defined but never used anywhere. Remove or add TODO for future use.
| public static final Distance kFieldHeight = Meters.of(8.07); // y | ||
| public static final Distance kFieldWidth = Meters.of(16.54); // x | ||
|
|
||
| public static List<Transform3d> kCameraOffsets = |
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.
kCameraOffsets should be final and use List.of(...) directly instead of wrapping in new ArrayList<>(). Currently it's a mutable list — anyone could accidentally .add() or .clear() it.
| /*new Transform3d(0, -0.4347972, 0.54864, new Rotation3d(0, 0, Math.PI / 2))*/ )); | ||
|
|
||
| public static final Pose3d kHubLocation = | ||
| new Pose3d(Inches.of(469.11), Inches.of(158.84), Inches.of(72), new Rotation3d()); // ); |
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 there some constant we can reference in the frc lib? That would be better than something hard coded here. If you need to hardcode it make sure this pose3D constructor actually takes Distance vs doubles.
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.
There's not much of a point doing calculations on the april-tag maps when you can just reference the field drawings
|
|
||
| public static final Pose3d kHubLocation = | ||
| new Pose3d(Inches.of(469.11), Inches.of(158.84), Inches.of(72), new Rotation3d()); // ); | ||
| public static Matrix<N3, N1> kBaseDeviation = VecBuilder.fill(1.5, 1.5, 10); |
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.
kBaseDeviation is missing final.
| cameraProp = new SimCameraProperties(); | ||
| cameraProp.setCalibration(1280, 800, Rotation2d.fromDegrees(70)); | ||
| cameraProp.setCalibError(0.25, 0.08); | ||
| cameraProp.setFPS(20); |
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 should be 59 or ~60
| } | ||
| } | ||
|
|
||
| if (minDistance >= kDistanceTolerance) { |
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.
minDistance can at most be 1 but kDistanceTolerance is set to 2.5 so this will never trigger
…he logic to make sense
jovapo
left a comment
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.
a few minor things still to fix but giving a stamp to unblock.
| drivetrain.applyRequest( | ||
| () -> | ||
| // on y button press rotate robot to angle from getAngleToHub() | ||
| new SwerveRequest.FieldCentricFacingAngle() |
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.
we can cover best practices here at a later meeting. This will add some latency but might not be the worst. The real question is how many times will this be called in a round. If over 100 (could be) then likely we should fix it.
Tested irl on commit 224fb89 with f424890 reverted because the orange pi was still running 2025 photon vision
only formatting changes made since then and it works in sim.