Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (7)
src/main/java/frc/robot/LimelightHelpers.java (5)
639-650: Null-safety for pose conversion and array extractionDefensive null checks avoid NPEs if external sources return null arrays.
Apply:
public static Pose3d toPose3D(double[] inData) { - if (inData.length < 6) { + if (inData == null || inData.length < 6) { return new Pose3d(); }public static Pose2d toPose2D(double[] inData) { - if (inData.length < 6) { + if (inData == null || inData.length < 6) { return new Pose2d(); }private static double extractArrayEntry(double[] inData, int position) { - if (inData.length < position + 1) { + if (inData == null || inData.length <= position) { return 0; } return inData[position]; }Also applies to: 660-668, 707-712
937-947: Improve error detail for malformed URLsIncluding the attempted URL aids debugging.
Apply:
- } catch (MalformedURLException e) { - System.err.println("bad LL URL"); + } catch (MalformedURLException e) { + System.err.println("Bad Limelight URL: " + urlString);
1590-1597: Deprecate misspelled API alias or rename for consistencyMethod name has a typo:
SetFidcuial3DOffset. You already exposesetFiducial3DOffset. Mark the misspelled one deprecated to steer users.Apply:
- public static void SetFidcuial3DOffset(String limelightName, double x, double y, double z) { + @Deprecated + public static void SetFidcuial3DOffset(String limelightName, double x, double y, double z) {
1622-1640: Avoid exact float equality for downscaling selectionDirect float comparisons can be brittle. Prefer a small epsilon or map discrete allowed values.
Example using epsilon:
- public static void SetFiducialDownscalingOverride(String limelightName, float downscale) { - int d = 0; // pipeline - if (downscale == 1.0) { + public static void SetFiducialDownscalingOverride(String limelightName, float downscale) { + int d = 0; // pipeline + final float eps = 1e-3f; + if (Math.abs(downscale - 1.0f) < eps) { d = 1; - } - if (downscale == 1.5) { + } else if (Math.abs(downscale - 1.5f) < eps) { d = 2; - } - if (downscale == 2) { + } else if (Math.abs(downscale - 2.0f) < eps) { d = 3; - } - if (downscale == 3) { + } else if (Math.abs(downscale - 3.0f) < eps) { d = 4; - } - if (downscale == 4) { + } else if (Math.abs(downscale - 4.0f) < eps) { d = 5; } setLimelightNTDouble(limelightName, "fiducial_downscale_set", d); }
872-884: Guard against null RawFiducial entries in printingIf any element is null, printing will NPE.
Apply:
- for (int i = 0; i < pose.rawFiducials.length; i++) { - RawFiducial fiducial = pose.rawFiducials[i]; + for (int i = 0; i < pose.rawFiducials.length; i++) { + RawFiducial fiducial = pose.rawFiducials[i]; + if (fiducial == null) { + System.out.printf(" Fiducial #%d: <no data>%n", i + 1); + continue; + }src/main/java/frc/robot/subsystems/vision/Limelight.java (2)
28-31: Fix constructor parameter typoRename
limelighNametolimelightNamefor clarity and consistency.Apply:
- public Limelight(VisionConsumer consumer, String limelighName) { + public Limelight(VisionConsumer consumer, String limelightName) { m_Consumer = consumer; - m_limelightName = limelighName; + m_limelightName = limelightName; }
49-53: Protect against divide-by-zero; clamp tag countEven if validity checks change, ensure safe math.
Apply:
- double xyStandardDev = 0.5 * Math.pow(poseEstimate.avgTagDist, 2.0) / poseEstimate.tagCount; - double rotationStandardDev = - 5.0 * Math.pow(poseEstimate.avgTagDist, 2.0) / poseEstimate.tagCount; + double count = Math.max(1.0, poseEstimate.tagCount); + double xyStandardDev = 0.5 * Math.pow(poseEstimate.avgTagDist, 2.0) / count; + double rotationStandardDev = 5.0 * Math.pow(poseEstimate.avgTagDist, 2.0) / count;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/frc/robot/LimelightHelpers.java(1 hunks)src/main/java/frc/robot/RobotContainer.java(2 hunks)src/main/java/frc/robot/subsystems/vision/Limelight.java(1 hunks)
🔇 Additional comments (1)
src/main/java/frc/robot/RobotContainer.java (1)
43-44: Vision subsystem wiring looks goodLimelight instantiated with drivetrain vision consumer and name. Field order ensures drivetrain exists first.
| RawFiducial[] rawFiducials = new RawFiducial[tagCount]; | ||
| int valsPerFiducial = 7; | ||
| int expectedTotalVals = 11 + valsPerFiducial * tagCount; | ||
|
|
||
| if (poseArray.length != expectedTotalVals) { | ||
| // Don't populate fiducials | ||
| } else { | ||
| for (int i = 0; i < tagCount; i++) { | ||
| int baseIndex = 11 + (i * valsPerFiducial); | ||
| int id = (int) poseArray[baseIndex]; | ||
| double txnc = poseArray[baseIndex + 1]; | ||
| double tync = poseArray[baseIndex + 2]; | ||
| double ta = poseArray[baseIndex + 3]; | ||
| double distToCamera = poseArray[baseIndex + 4]; | ||
| double distToRobot = poseArray[baseIndex + 5]; | ||
| double ambiguity = poseArray[baseIndex + 6]; | ||
| rawFiducials[i] = new RawFiducial(id, txnc, tync, ta, distToCamera, distToRobot, ambiguity); | ||
| } | ||
| } | ||
|
|
||
| return new PoseEstimate( | ||
| pose, | ||
| adjustedTimestamp, | ||
| latency, | ||
| tagCount, | ||
| tagSpan, | ||
| tagDist, | ||
| tagArea, | ||
| rawFiducials, | ||
| isMegaTag2); | ||
| } |
There was a problem hiding this comment.
Avoid partially-filled RawFiducials; keep counts consistent to prevent NPEs downstream
When poseArray.length != expectedTotalVals, you create an array sized to tagCount but never fill it, leaving null elements. printPoseEstimate will NPE and validPoseEstimate can return true due to non-zero length despite missing data.
Apply:
- RawFiducial[] rawFiducials = new RawFiducial[tagCount];
- int valsPerFiducial = 7;
- int expectedTotalVals = 11 + valsPerFiducial * tagCount;
-
- if (poseArray.length != expectedTotalVals) {
- // Don't populate fiducials
- } else {
- for (int i = 0; i < tagCount; i++) {
+ int valsPerFiducial = 7;
+ int expectedTotalVals = 11 + valsPerFiducial * tagCount;
+ int fidCount = (poseArray.length == expectedTotalVals) ? tagCount : 0;
+ RawFiducial[] rawFiducials = new RawFiducial[fidCount];
+ if (fidCount > 0) {
+ for (int i = 0; i < fidCount; i++) {
int baseIndex = 11 + (i * valsPerFiducial);
int id = (int) poseArray[baseIndex];
double txnc = poseArray[baseIndex + 1];
double tync = poseArray[baseIndex + 2];
double ta = poseArray[baseIndex + 3];
double distToCamera = poseArray[baseIndex + 4];
double distToRobot = poseArray[baseIndex + 5];
double ambiguity = poseArray[baseIndex + 6];
rawFiducials[i] = new RawFiducial(id, txnc, tync, ta, distToCamera, distToRobot, ambiguity);
}
}
return new PoseEstimate(
pose,
adjustedTimestamp,
latency,
- tagCount,
+ fidCount,
tagSpan,
tagDist,
tagArea,
rawFiducials,
isMegaTag2);📝 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.
| RawFiducial[] rawFiducials = new RawFiducial[tagCount]; | |
| int valsPerFiducial = 7; | |
| int expectedTotalVals = 11 + valsPerFiducial * tagCount; | |
| if (poseArray.length != expectedTotalVals) { | |
| // Don't populate fiducials | |
| } else { | |
| for (int i = 0; i < tagCount; i++) { | |
| int baseIndex = 11 + (i * valsPerFiducial); | |
| int id = (int) poseArray[baseIndex]; | |
| double txnc = poseArray[baseIndex + 1]; | |
| double tync = poseArray[baseIndex + 2]; | |
| double ta = poseArray[baseIndex + 3]; | |
| double distToCamera = poseArray[baseIndex + 4]; | |
| double distToRobot = poseArray[baseIndex + 5]; | |
| double ambiguity = poseArray[baseIndex + 6]; | |
| rawFiducials[i] = new RawFiducial(id, txnc, tync, ta, distToCamera, distToRobot, ambiguity); | |
| } | |
| } | |
| return new PoseEstimate( | |
| pose, | |
| adjustedTimestamp, | |
| latency, | |
| tagCount, | |
| tagSpan, | |
| tagDist, | |
| tagArea, | |
| rawFiducials, | |
| isMegaTag2); | |
| } | |
| int valsPerFiducial = 7; | |
| int expectedTotalVals = 11 + valsPerFiducial * tagCount; | |
| int fidCount = (poseArray.length == expectedTotalVals) ? tagCount : 0; | |
| RawFiducial[] rawFiducials = new RawFiducial[fidCount]; | |
| if (fidCount > 0) { | |
| for (int i = 0; i < fidCount; i++) { | |
| int baseIndex = 11 + (i * valsPerFiducial); | |
| int id = (int) poseArray[baseIndex]; | |
| double txnc = poseArray[baseIndex + 1]; | |
| double tync = poseArray[baseIndex + 2]; | |
| double ta = poseArray[baseIndex + 3]; | |
| double distToCamera = poseArray[baseIndex + 4]; | |
| double distToRobot = poseArray[baseIndex + 5]; | |
| double ambiguity = poseArray[baseIndex + 6]; | |
| rawFiducials[i] = new RawFiducial(id, txnc, tync, ta, distToCamera, distToRobot, ambiguity); | |
| } | |
| } | |
| return new PoseEstimate( | |
| pose, | |
| adjustedTimestamp, | |
| latency, | |
| fidCount, | |
| tagSpan, | |
| tagDist, | |
| tagArea, | |
| rawFiducials, | |
| isMegaTag2); | |
| } |
| private static boolean SYNCH_TAKESNAPSHOT(String tableName, String snapshotName) { | ||
| URL url = getLimelightURLString(tableName, "capturesnapshot"); | ||
| try { | ||
| HttpURLConnection connection = (HttpURLConnection) url.openConnection(); | ||
| connection.setRequestMethod("GET"); | ||
| if (snapshotName != null && !"".equals(snapshotName)) { | ||
| connection.setRequestProperty("snapname", snapshotName); | ||
| } | ||
|
|
||
| int responseCode = connection.getResponseCode(); | ||
| if (responseCode == 200) { | ||
| return true; | ||
| } else { | ||
| System.err.println("Bad LL Request"); | ||
| } | ||
| } catch (IOException e) { | ||
| System.err.println(e.getMessage()); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Handle null URL and add HTTP timeouts to prevent hangs
getLimelightURLString can return null. SYNCH_TAKESNAPSHOT dereferences it, causing NPE. Also, no connect/read timeouts: a stalled device can hang the async worker.
Apply:
- private static boolean SYNCH_TAKESNAPSHOT(String tableName, String snapshotName) {
- URL url = getLimelightURLString(tableName, "capturesnapshot");
- try {
- HttpURLConnection connection = (HttpURLConnection) url.openConnection();
- connection.setRequestMethod("GET");
- if (snapshotName != null && !"".equals(snapshotName)) {
- connection.setRequestProperty("snapname", snapshotName);
- }
-
- int responseCode = connection.getResponseCode();
- if (responseCode == 200) {
- return true;
- } else {
- System.err.println("Bad LL Request");
- }
- } catch (IOException e) {
- System.err.println(e.getMessage());
- }
- return false;
- }
+ private static boolean SYNCH_TAKESNAPSHOT(String tableName, String snapshotName) {
+ URL url = getLimelightURLString(tableName, "capturesnapshot");
+ if (url == null) {
+ System.err.println("Invalid Limelight URL for table: " + tableName);
+ return false;
+ }
+ HttpURLConnection connection = null;
+ try {
+ connection = (HttpURLConnection) url.openConnection();
+ connection.setRequestMethod("GET");
+ connection.setConnectTimeout(1000);
+ connection.setReadTimeout(1500);
+ if (snapshotName != null && !"".equals(snapshotName)) {
+ connection.setRequestProperty("snapname", snapshotName);
+ }
+ int responseCode = connection.getResponseCode();
+ if (responseCode == 200) {
+ return true;
+ } else {
+ System.err.println("Bad LL Request, HTTP " + responseCode);
+ }
+ } catch (IOException e) {
+ System.err.println("Snapshot error: " + e.getMessage());
+ } finally {
+ if (connection != null) {
+ connection.disconnect();
+ }
+ }
+ return false;
+ }📝 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 static boolean SYNCH_TAKESNAPSHOT(String tableName, String snapshotName) { | |
| URL url = getLimelightURLString(tableName, "capturesnapshot"); | |
| try { | |
| HttpURLConnection connection = (HttpURLConnection) url.openConnection(); | |
| connection.setRequestMethod("GET"); | |
| if (snapshotName != null && !"".equals(snapshotName)) { | |
| connection.setRequestProperty("snapname", snapshotName); | |
| } | |
| int responseCode = connection.getResponseCode(); | |
| if (responseCode == 200) { | |
| return true; | |
| } else { | |
| System.err.println("Bad LL Request"); | |
| } | |
| } catch (IOException e) { | |
| System.err.println(e.getMessage()); | |
| } | |
| return false; | |
| } | |
| private static boolean SYNCH_TAKESNAPSHOT(String tableName, String snapshotName) { | |
| URL url = getLimelightURLString(tableName, "capturesnapshot"); | |
| if (url == null) { | |
| System.err.println("Invalid Limelight URL for table: " + tableName); | |
| return false; | |
| } | |
| HttpURLConnection connection = null; | |
| try { | |
| connection = (HttpURLConnection) url.openConnection(); | |
| connection.setRequestMethod("GET"); | |
| connection.setConnectTimeout(1000); | |
| connection.setReadTimeout(1500); | |
| if (snapshotName != null && !"".equals(snapshotName)) { | |
| connection.setRequestProperty("snapname", snapshotName); | |
| } | |
| int responseCode = connection.getResponseCode(); | |
| if (responseCode == 200) { | |
| return true; | |
| } else { | |
| System.err.println("Bad LL Request, HTTP " + responseCode); | |
| } | |
| } catch (IOException e) { | |
| System.err.println("Snapshot error: " + e.getMessage()); | |
| } finally { | |
| if (connection != null) { | |
| connection.disconnect(); | |
| } | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/frc/robot/LimelightHelpers.java around lines 1693 to 1712,
SYNCH_TAKESNAPSHOT currently dereferences a URL that can be null and opens an
HttpURLConnection without timeouts which can hang; update the method to first
check if the URL returned from getLimelightURLString is null and immediately log
and return false if so, then after opening the connection set sensible connect
and read timeouts (e.g. a few seconds) before setRequestMethod/headers, and
ensure the connection is properly closed/disconnected in a finally block; also
improve exception handling to log the exception (not just getMessage()) and
always return false on errors or non-200 responses.
| /** Logging: number of tags contributing to the last estimate. */ | ||
| public double getTagCount() { | ||
| return getPoseEstimate().tagCount; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Return type for tag count
PoseEstimate.tagCount is an int. Expose it as int for consistency.
Apply:
- public double getTagCount() {
- return getPoseEstimate().tagCount;
+ public int getTagCount() {
+ return getPoseEstimate().tagCount;
}If other code already consumes double getTagCount(), confirm before changing. Search usages:
🏁 Script executed:
#!/bin/bash
rg -n -C2 --type=java '\bgetTagCount\s*\('Length of output: 443
Use int return type for getTagCount
Replace the return type from double to int to match PoseEstimate.tagCount:
- public double getTagCount() {
- return getPoseEstimate().tagCount;
+ public int getTagCount() {
+ return getPoseEstimate().tagCount;
}📝 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.
| /** Logging: number of tags contributing to the last estimate. */ | |
| public double getTagCount() { | |
| return getPoseEstimate().tagCount; | |
| } | |
| /** Logging: number of tags contributing to the last estimate. */ | |
| public int getTagCount() { | |
| return getPoseEstimate().tagCount; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/frc/robot/subsystems/vision/Limelight.java around lines 90 to
93, the getTagCount method currently returns a double but should return an int
to match PoseEstimate.tagCount; change the method signature to return int and
return getPoseEstimate().tagCount (or cast only if absolutely necessary),
ensuring the return type and any callers are updated to use int.
JacerNition
force-pushed
the
3-Limelight
branch
3 times, most recently
from
72b3736 to
d38179e
Compare
JosephTLockwood
force-pushed
the
3-Limelight
branch
from
d38179e to
ad7d866
Compare
JosephTLockwood
force-pushed
the
2-Logging
branch
from
e1c58c5 to
066dbd5
Compare
JosephTLockwood
force-pushed
the
3-Limelight
branch
from
ad7d866 to
add1203
Compare
1 participant
Summary by CodeRabbit