started field awareness#68
Open
astrobot-coder wants to merge 5 commits intomainfrom
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds trench coordinate constants/translations, exposes current chassis speeds on RobotState (populated from Drive), and introduces a FieldAwareness singleton that computes offsets/distances from the robot pose to the four trench translations. Changes
Sequence DiagramsequenceDiagram
participant Drive as Drive
participant RobotState as RobotState
participant FieldAwareness as FieldAwareness
participant Constants as Constants
Drive->>RobotState: update odometry (pose)
Drive->>RobotState: setRobotChassisSpeeds(chassisSpeeds)
RobotState->>RobotState: store pose & robotChassisSpeeds
FieldAwareness->>RobotState: getInstance()
FieldAwareness->>RobotState: getOdometryTranslation()
RobotState-->>FieldAwareness: current Translation2d (pose)
FieldAwareness->>RobotState: getRobotChassisSpeeds()
RobotState-->>FieldAwareness: ChassisSpeeds
FieldAwareness->>Constants: read TRENCH_TRANSLATIONS
Constants-->>FieldAwareness: Translation2d[]
FieldAwareness->>FieldAwareness: compute offsets/distances to trenches
FieldAwareness-->>Caller: return double[] distances
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/org/team5924/frc2026/subsystems/drive/Drive.java (1)
210-210: Commented code references non-existent method.The commented-out code now references
getSpeeds(), but this method doesn't exist in theDriveclass (onlygetChassisSpeeds()exists at line 428). If this setpoint generator code is intended for future use, consider keeping the method name consistent or removing stale commented code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/drive/Drive.java` at line 210, The commented-out line referencing previousSetpoint = new SwerveSetpoint(getSpeeds(), getModuleStates()) points to a non-existent method getSpeeds(); update or remove this stale comment: either change getSpeeds() to the existing getChassisSpeeds() if the intent was to construct SwerveSetpoint from the current chassis speeds (referencing previousSetpoint and SwerveSetpoint), or delete the commented line entirely to avoid confusion—make the change near the previousSetpoint / SwerveSetpoint comment so future readers see the corrected method name or the comment removed.src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java (2)
18-27: Method is private and never called; hardcodes array size.
getDistanceToTrenches()is private but has no callers, making it dead code. If this is intended for future use, consider making it package-private or public. Also, derive the array size from the constant to avoid maintenance issues:♻️ Proposed improvements
- private double[] getDistanceToTrenches(){ - double[] distance = new double[4]; + public double[] getDistanceToTrenches() { + double[] distance = new double[Constants.Field.TRENCH_TRANSLATIONS.length]; Translation2d robotPosition = RobotState.getInstance().getOdometryPose().getTranslation(); - for (int i = 0; i < 4; i++) { + for (int i = 0; i < Constants.Field.TRENCH_TRANSLATIONS.length; i++) { distance[i] = Constants.Field.TRENCH_TRANSLATIONS[i].getDistance(robotPosition); } return distance; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java` around lines 18 - 27, The private method getDistanceToTrenches() is dead code and also hardcodes the array size; either remove it or make it accessible if intended for use (change visibility to package-private or public on getDistanceToTrenches), and compute the array length from Constants.Field.TRENCH_TRANSLATIONS.length rather than 4; keep the existing logic that uses RobotState.getInstance().getOdometryPose().getTranslation() and Constants.Field.TRENCH_TRANSLATIONS[i].getDistance(robotPosition) but replace the fixed-size new double[4] with a new double[Constants.Field.TRENCH_TRANSLATIONS.length] (or delete the whole method if unused).
9-16: Singleton is not thread-safe.The lazy initialization pattern used here can create multiple instances if
getInstance()is called concurrently from multiple threads. While FRC robot code is largely single-threaded, consider using a static initializer for safety:🛡️ Thread-safe alternative
public class FieldAwareness { - private static FieldAwareness instance; - - - public static FieldAwareness getInstance() { - if (instance == null) instance = new FieldAwareness(); - return instance; - } + private static final FieldAwareness instance = new FieldAwareness(); + + public static FieldAwareness getInstance() { + return instance; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java` around lines 9 - 16, The FieldAwareness singleton uses lazy initialization in getInstance() which is not thread-safe; change it to a thread-safe pattern by initializing the single instance eagerly or using a thread-safe holder (for example, assign instance at declaration or replace getInstance() with a synchronized accessor or an inner static holder); update the FieldAwareness class so the private static FieldAwareness instance is created in a static initializer (or via the holder pattern) and simplify getInstance() to return that single guaranteed thread-safe instance.
🤖 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/java/org/team5924/frc2026/Constants.java`:
- Line 82: TRENCH_TRANSLATIONS is defined with large numeric values (likely
inches) but WPILib geometry and RobotState.getOdometryPose() use meters, causing
wrong distance calculations in FieldAwareness.getDistanceToTrenches(); convert
the trench coordinates to meters (e.g., replace raw inch literals with
Units.inchesToMeters(...) or pre-convert values) so Translation2d instances are
constructed in meters, and ensure any other uses assume meters consistently.
In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java`:
- Around line 29-34: The private method tdfyguijoi contains unused local
variables (ChassisSpeeds, Translation2d), a keyboard-mash name, and a typo in
the comment; either delete this placeholder entirely if it's not referenced, or
rename it to a meaningful name (e.g., computeFieldVelocity or
updateFieldAwareness) and replace the comment with a proper TODO (// TODO:
describe intended behavior), then either implement the intended logic using
RobotState.getInstance().getRobotChassisSpeeds(), ChassisSpeeds, and
Translation2d or leave a clear TODO and ensure the method is referenced/used;
also remove the stray variables if you choose to keep a stub.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java`:
- Around line 18-27: The private method getDistanceToTrenches() is dead code and
also hardcodes the array size; either remove it or make it accessible if
intended for use (change visibility to package-private or public on
getDistanceToTrenches), and compute the array length from
Constants.Field.TRENCH_TRANSLATIONS.length rather than 4; keep the existing
logic that uses RobotState.getInstance().getOdometryPose().getTranslation() and
Constants.Field.TRENCH_TRANSLATIONS[i].getDistance(robotPosition) but replace
the fixed-size new double[4] with a new
double[Constants.Field.TRENCH_TRANSLATIONS.length] (or delete the whole method
if unused).
- Around line 9-16: The FieldAwareness singleton uses lazy initialization in
getInstance() which is not thread-safe; change it to a thread-safe pattern by
initializing the single instance eagerly or using a thread-safe holder (for
example, assign instance at declaration or replace getInstance() with a
synchronized accessor or an inner static holder); update the FieldAwareness
class so the private static FieldAwareness instance is created in a static
initializer (or via the holder pattern) and simplify getInstance() to return
that single guaranteed thread-safe instance.
In `@src/main/java/org/team5924/frc2026/subsystems/drive/Drive.java`:
- Line 210: The commented-out line referencing previousSetpoint = new
SwerveSetpoint(getSpeeds(), getModuleStates()) points to a non-existent method
getSpeeds(); update or remove this stale comment: either change getSpeeds() to
the existing getChassisSpeeds() if the intent was to construct SwerveSetpoint
from the current chassis speeds (referencing previousSetpoint and
SwerveSetpoint), or delete the commented line entirely to avoid confusion—make
the change near the previousSetpoint / SwerveSetpoint comment so future readers
see the corrected method name or the comment removed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/RobotState.javasrc/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.javasrc/main/java/org/team5924/frc2026/subsystems/drive/Drive.java
Comment on lines
+29
to
+34
| private void tdfyguijoi() { // TOD: name later | ||
| ChassisSpeeds speeds = RobotState.getInstance().getRobotChassisSpeeds(); | ||
| Translation2d velocity = new Translation2d(speeds.vxMetersPerSecond, speeds.vyMetersPerSecond); | ||
|
|
||
|
|
||
| } |
Contributor
There was a problem hiding this comment.
Placeholder method with nonsensical name and no implementation.
This method has a random keyboard-mash name (tdfyguijoi), is private and unused, contains incomplete logic (creates variables but doesn't use them), and has a typo in the comment ("TOD" instead of "TODO"). Consider either:
- Removing this placeholder until the implementation is ready, or
- Renaming with a meaningful name and adding a proper
// TODO:comment explaining the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java`
around lines 29 - 34, The private method tdfyguijoi contains unused local
variables (ChassisSpeeds, Translation2d), a keyboard-mash name, and a typo in
the comment; either delete this placeholder entirely if it's not referenced, or
rename it to a meaningful name (e.g., computeFieldVelocity or
updateFieldAwareness) and replace the comment with a proper TODO (// TODO:
describe intended behavior), then either implement the intended logic using
RobotState.getInstance().getRobotChassisSpeeds(), ChassisSpeeds, and
Translation2d or leave a clear TODO and ensure the method is referenced/used;
also remove the stray variables if you choose to keep a stub.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java (1)
18-27: UseTRENCH_TRANSLATIONS.lengthinstead of hardcoded4.This makes the code more maintainable if the number of trenches changes.
♻️ Suggested refactor
private double[] getDistanceToTrenches(){ - double[] distance = new double[4]; + double[] distance = new double[Constants.Field.TRENCH_TRANSLATIONS.length]; Translation2d robotPosition = RobotState.getInstance().getOdometryPose().getTranslation(); - for (int i = 0; i < 4; i++) { + for (int i = 0; i < Constants.Field.TRENCH_TRANSLATIONS.length; i++) { distance[i] = Constants.Field.TRENCH_TRANSLATIONS[i].getDistance(robotPosition); } return distance; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java` around lines 18 - 27, Replace the hardcoded trench count in getDistanceToTrenches by using Constants.Field.TRENCH_TRANSLATIONS.length: allocate the distance array with new double[Constants.Field.TRENCH_TRANSLATIONS.length], loop from 0 to that length (e.g., for (int i = 0; i < Constants.Field.TRENCH_TRANSLATIONS.length; i++)) and index Constants.Field.TRENCH_TRANSLATIONS[i] to compute distances; this ensures getDistanceToTrenches adapts if TRENCH_TRANSLATIONS changes size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java`:
- Around line 18-27: Replace the hardcoded trench count in getDistanceToTrenches
by using Constants.Field.TRENCH_TRANSLATIONS.length: allocate the distance array
with new double[Constants.Field.TRENCH_TRANSLATIONS.length], loop from 0 to that
length (e.g., for (int i = 0; i < Constants.Field.TRENCH_TRANSLATIONS.length;
i++)) and index Constants.Field.TRENCH_TRANSLATIONS[i] to compute distances;
this ensures getDistanceToTrenches adapts if TRENCH_TRANSLATIONS changes size.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java (1)
29-36:⚠️ Potential issue | 🟡 MinorRemove or implement placeholder method before merge.
At Line 29-36,
tdfyguijoi()still has a non-descriptive name, empty loop, and computed-but-unused values. This leaves dead/incomplete behavior in the class.Proposed cleanup (if deferring implementation)
- private void tdfyguijoi() { // TODO: name later - ChassisSpeeds speeds = RobotState.getInstance().getRobotChassisSpeeds(); - Translation2d velocity = new Translation2d(speeds.vxMetersPerSecond, speeds.vyMetersPerSecond); - - for (int i = 0; i < 4; i++) { - - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java` around lines 29 - 36, The method tdfyguijoi() in FieldAwareness contains a placeholder name, unused computed values (ChassisSpeeds speeds and Translation2d velocity) and an empty for-loop; either implement its intended behavior or remove/replace it: if implementing, rename tdfyguijoi to a descriptive name (e.g., updateFieldAwareness or computeRobotVelocity), use RobotState.getInstance().getRobotChassisSpeeds() and the Translation2d velocity in the logic (populate the loop with the actual per-wheel or per-sensor computations), and remove the empty loop; if deferring, delete the method entirely (or replace it with a TODO stub with a clear name and no dead variables) so no unused variables or empty control structures remain.
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java (1)
18-24: Avoid hardcoding trench count at Line 19 and Line 22.Using literal
4duplicates source-of-truth and can drift fromConstants.Field.TRENCH_TRANSLATIONS.Proposed refactor
private Translation2d[] getOffsetsToTrenches(){ - Translation2d[] offsets = new Translation2d[4]; + Translation2d[] trenches = Constants.Field.TRENCH_TRANSLATIONS; + Translation2d[] offsets = new Translation2d[trenches.length]; Translation2d robotPosition = RobotState.getInstance().getOdometryPose().getTranslation(); - for (int i = 0; i < 4; i++) { - offsets[i] = Constants.Field.TRENCH_TRANSLATIONS[i].minus(robotPosition); + for (int i = 0; i < trenches.length; i++) { + offsets[i] = trenches[i].minus(robotPosition); } return offsets; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java` around lines 18 - 24, In getOffsetsToTrenches(), avoid hardcoding the trench count by sizing and iterating based on Constants.Field.TRENCH_TRANSLATIONS.length rather than literal 4; change the offsets array initialization and the for-loop bound to use Constants.Field.TRENCH_TRANSLATIONS.length so the method adapts if TRENCH_TRANSLATIONS changes; keep RobotState.getInstance().getOdometryPose().getTranslation() as the reference and assign offsets[i] = Constants.Field.TRENCH_TRANSLATIONS[i].minus(robotPosition) inside the loop.
🤖 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/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java`:
- Around line 13-16: The FieldAwareness class exposes getInstance() but the
constructor is not private so callers can bypass the singleton; make the
FieldAwareness constructor private (or package-private with controlled factory)
to enforce the singleton, keep the existing public static FieldAwareness
getInstance() as the sole accessor, and if desired add lazy-init thread-safety
(e.g., synchronized block or use an initialization-on-demand holder) to the
singleton pattern.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java`:
- Around line 29-36: The method tdfyguijoi() in FieldAwareness contains a
placeholder name, unused computed values (ChassisSpeeds speeds and Translation2d
velocity) and an empty for-loop; either implement its intended behavior or
remove/replace it: if implementing, rename tdfyguijoi to a descriptive name
(e.g., updateFieldAwareness or computeRobotVelocity), use
RobotState.getInstance().getRobotChassisSpeeds() and the Translation2d velocity
in the logic (populate the loop with the actual per-wheel or per-sensor
computations), and remove the empty loop; if deferring, delete the method
entirely (or replace it with a TODO stub with a clear name and no dead
variables) so no unused variables or empty control structures remain.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java`:
- Around line 18-24: In getOffsetsToTrenches(), avoid hardcoding the trench
count by sizing and iterating based on
Constants.Field.TRENCH_TRANSLATIONS.length rather than literal 4; change the
offsets array initialization and the for-loop bound to use
Constants.Field.TRENCH_TRANSLATIONS.length so the method adapts if
TRENCH_TRANSLATIONS changes; keep
RobotState.getInstance().getOdometryPose().getTranslation() as the reference and
assign offsets[i] = Constants.Field.TRENCH_TRANSLATIONS[i].minus(robotPosition)
inside the loop.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9c97253-0d4a-43b5-b0bd-7af8276a2083
📒 Files selected for processing (1)
src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java
Comment on lines
+13
to
+16
| public static FieldAwareness getInstance() { | ||
| if (instance == null) instance = new FieldAwareness(); | ||
| return instance; | ||
| } |
Contributor
There was a problem hiding this comment.
Singleton can still be instantiated directly.
getInstance() implies singleton usage, but without a private constructor, other code can still call new FieldAwareness() and bypass this contract.
Proposed fix
public class FieldAwareness {
private static FieldAwareness instance;
+ private FieldAwareness() {}
+
public static FieldAwareness getInstance() {
if (instance == null) instance = new FieldAwareness();
return instance;
}📝 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.
Suggested change
| public static FieldAwareness getInstance() { | |
| if (instance == null) instance = new FieldAwareness(); | |
| return instance; | |
| } | |
| public class FieldAwareness { | |
| private static FieldAwareness instance; | |
| private FieldAwareness() {} | |
| public static FieldAwareness getInstance() { | |
| if (instance == null) instance = new FieldAwareness(); | |
| return instance; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/team5924/frc2026/subsystems/awareness/FieldAwareness.java`
around lines 13 - 16, The FieldAwareness class exposes getInstance() but the
constructor is not private so callers can bypass the singleton; make the
FieldAwareness constructor private (or package-private with controlled factory)
to enforce the singleton, keep the existing public static FieldAwareness
getInstance() as the sole accessor, and if desired add lazy-init thread-safety
(e.g., synchronized block or use an initialization-on-demand holder) to the
singleton pattern.
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.
No description provided.