feat(hardware/gyro): implement gyroscopes #9

Baconing · 2024-10-02T17:53:24Z

Implements IMUs and Single Axis gyroscopes.

Summary by CodeRabbit

New Features
- Introduced the ADIS16448IMU, ADIS16470IMU, and NavXIMU classes for enhanced inertial measurement capabilities.
- Integrated the wpiunits-java library to improve unit handling in robotics programming.
- Added comprehensive unit tests for ADIS16448IMU, ADIS16470IMU, IMU, and NavXIMU classes to ensure functionality and reliability.
Bug Fixes
- Removed the outdated SingleAxisGyroscope class, streamlining the codebase.

These updates enhance the functionality and performance of the robotics applications by providing more robust IMU options, improved unit management, and thorough testing coverage.

coderabbitai · 2024-10-02T17:53:32Z

Walkthrough

The changes involve the introduction of two new classes, ADIS16448IMU and ADIS16470IMU, which extend an abstract IMU class, providing specific implementations for the ADIS16448 and ADIS16470 Inertial Measurement Units. The IMU class serves as a framework for handling various IMU functionalities, including calibration, resetting, and accessing raw data. Additionally, a new class NavXIMU has been added to interface with the NavX IMU sensor. A dependency for the wpiunits-java library has also been added to the build configuration. Unit test classes for the new IMU implementations and existing classes have been created to ensure functionality.

Changes

File Path	Change Summary
`src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java`	Class added: `ADIS16448IMU` with multiple constructors and methods for IMU functionality.
`src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16470IMU.java`	Class added: `ADIS16470IMU` with multiple constructors and methods for IMU functionality.
`src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java`	Class added: `IMU` with methods defining IMU behavior, including angle retrieval and acceleration methods.
`src/main/java/net/frc5183/librobot/hardware/gyro/NavXIMU.java`	Class added: `NavXIMU` with constructors and overridden methods for NavX IMU functionality.
`build.gradle`	Dependency added: `compileOnly "edu.wpi.first.wpiunits:wpiunits-java:2024.3.2"`
`src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16448Tests.java`	Class added: `ADIS16448Tests` with unit tests for `ADIS16448IMU`.
`src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16470Tests.java`	Class added: `ADIS16470Tests` with unit tests for `ADIS16470IMU`.
`src/test/java/net/frc5183/librobot/hardware/gyro/IMUTests.java`	Class added: `IMUTests` with tests for `IMU.CartesianAxis`.
`src/test/java/net/frc5183/librobot/hardware/gyro/NavXTests.java`	Class added: `NavXTests` with unit tests for `NavXIMU`.

Poem

In the world of gyros, we spin and we sway,
With ADIS16448 and 70 at play.
The old class has hopped away,
New paths for our data, bright as the day!
Calibration and rotation, oh what a thrill,
In our robotic dance, we’ll master the skill! 🐇✨

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?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 15

🧹 Outside diff range and nitpick comments (1)

src/main/java/net/frc5183/librobot/hardware/gyro/single/SingleAxisGyroscope.java (1)
6-6: Improve TODO comment

The current TODO comment "// todo" is too vague. It's best practice to provide more context in TODO comments to guide future development. Consider expanding the comment to describe what specifically needs to be implemented or addressed.

For example:
// TODO: Implement methods for getting and setting gyroscope angle, and for calibration

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 40ab712 and a699ec7.

📒 Files selected for processing (5)

src/main/java/net/frc5183/librobot/hardware/gyro/SingleAxisGyroscope.java (0 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/single/SingleAxisGyroscope.java (1 hunks)

💤 Files with no reviewable changes (1)

src/main/java/net/frc5183/librobot/hardware/gyro/SingleAxisGyroscope.java

🧰 Additional context used

🪛 GitHub Check: pmd

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

[warning] 8-8: Unused import 'java.util.Optional'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

[warning] 23-23: Avoid unused private fields such as 'yaw'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 28-28: Avoid unused private fields such as 'pitch'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 33-33: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

[warning] 4-4: Unused import 'edu.wpi.first.wpilibj.ADIS16448_IMU'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

src/main/java/net/frc5183/librobot/hardware/gyro/single/SingleAxisGyroscope.java

[warning] 3-3: Unused import 'edu.wpi.first.math.geometry.Rotation2d'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

[failure] 5-5: No abstract method which means that the keyword is most likely used to prevent instantiation. Use a private or protected constructor instead.
If an abstract class does not provide any methods, it may be acting as a simple data container
that is not meant to be instantiated. In this case, it is probably better to use a private or
protected constructor in order to prevent instantiation than make the class misleadingly abstract.

AbstractClassWithoutAnyMethod (Priority: 1, Ruleset: Design)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_design.html#abstractclasswithoutanymethod

🔇 Additional comments (2)

src/main/java/net/frc5183/librobot/hardware/gyro/single/SingleAxisGyroscope.java (1)

7-9: Enum definition looks good

The Axis enum is well-defined and correctly represents the three primary axes of rotation for a gyroscope (YAW, PITCH, and ROLL). The naming is clear and follows standard conventions.
src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1)
42-44: Consider Thread Safety for the offset Variable

If the IMU class is accessed from multiple threads, consider making the offset variable volatile or using synchronization to ensure thread safety when setting and getting the offset.

Run the following script to check for concurrent usage of the IMU class:

✅ Verification successful

Thread Safety for the offset Variable Verified

No concurrent usages of the IMU class were detected based on automated analysis.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Search for usages of IMU in a concurrent context.

# Test: Find instances where IMU is accessed from multiple threads.
# Expect: Identify any concurrent usages.
rg --type java --pcre2 '(new Thread|ExecutorService|CompletableFuture).*IMU'
Length of output: 78

Script:
#!/bin/bash
# Description: Comprehensive search for IMU usage in various concurrent contexts.

# Test: Find instances where IMU is accessed from multiple threads using different concurrency utilities.
# Expect: Identify any concurrent usages.

rg --type java --pcre2 '(new Thread|ExecutorService|CompletableFuture|ForkJoinPool|Callable|Runnable).*IMU'
Length of output: 109

src/main/java/net/frc5183/librobot/hardware/gyro/single/SingleAxisGyroscope.java

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

… methods

…for acceleration.

…f FQDN in switch statement

coderabbitai

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a699ec7 and d180bee.

📒 Files selected for processing (3)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1 hunks)

🧰 Additional context used

🪛 GitHub Check: pmd

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

[warning] 9-9: Unused import 'java.util.Optional'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

[warning] 36-36: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

[warning] 5-5: Unused import 'edu.wpi.first.wpilibj.ADIS16448_IMU'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

[warning] 23-23: Avoid unused private fields such as 'yaw'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 28-28: Avoid unused private fields such as 'pitch'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 33-33: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

🔇 Additional comments (25)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (6)

9-13: LGTM: Class declaration and offset field

The abstract class declaration and the private offset field are well-implemented. The use of Rotation3d for the offset is appropriate for representing 3D rotation.

27-27: LGTM: Abstract method declaration

The getRawRotation3d() method is correctly declared as abstract. The absence of the @NotNull annotation aligns with the previous review suggestion.

33-33: LGTM: Correct return type for acceleration

The getAccelerationMetersPerSecondSquared() method now correctly returns a Translation3d object, which is appropriate for representing acceleration. This change addresses the previous review comment and improves the accuracy of the API.

39-49: LGTM: Offset getter and setter methods

The setOffset() and getOffset() methods are well-implemented with clear and accurate documentation. They provide appropriate access to the offset field while maintaining encapsulation.

74-76: LGTM: IMUAxis enum

The IMUAxis enum is well-defined, providing a clear representation of the three axes (X, Y, Z) of a gyroscope. This enum will be useful for operations that require specifying a particular axis.

1-77: Summary of IMU abstract class review

The IMU abstract class provides a solid foundation for implementing various Inertial Measurement Units. The review has identified a few areas for improvement:

Clarifying the units in the getRotation3d() method name and documentation.

Implementing generics for the getRawIMU() method to improve type safety.

Overall, the class is well-structured and documented. Addressing these suggestions will further enhance its usability and maintainability.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (6)

111-118: LGTM: Correct implementation of getRawRotation3d().

The method correctly converts gyro angles from degrees to radians before creating the Rotation3d object. This implementation addresses the previous concern about unit conversion.

120-123: LGTM: Correct implementation of getAccelerationMetersPerSecondSquared().

The method now correctly returns a Translation3d object to represent acceleration, addressing the previous concern about using the appropriate class for acceleration representation.

145-167: LGTM: Well-implemented axis conversion methods.

The fromIMUAxis() and toIMUAxis() methods are correctly implemented using switch expressions, which is a clean and modern approach. These methods provide a clear way to convert between different axis representations.

9-9: ⚠️ Potential issue

Remove unused import.

The import java.util.Optional is not used in this class and can be removed to keep the code clean.

🧰 Tools

🪛 GitHub Check: pmd

[warning] 9-9: Unused import 'java.util.Optional'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

1-168: Overall, well-implemented ADIS16448IMU class with minor improvements needed.

The ADIS16448IMU class provides a comprehensive wrapper for the ADIS16448 IMU, with good attention to detail in constructors, utility methods, and documentation. However, there are a few areas that need attention:

Remove the unused import of java.util.Optional.

Address the unused roll field.

Verify the axis assignment logic in the constructors and remove the TODO comments once confirmed.

Consider adding unit tests to ensure correct behavior, especially for the axis assignment logic.

Once these minor issues are addressed, the implementation will be more robust and maintainable.

🧰 Tools

🪛 GitHub Check: pmd

[warning] 9-9: Unused import 'java.util.Optional'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

[warning] 36-36: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

53-63: Verify axis assignment logic and remove TODO comments.

The axis assignment logic in the constructors has TODO comments indicating that it needs confirmation. This is a critical part of the IMU initialization and should be verified. Once verified, remove the TODO comments.

Consider adding unit tests to ensure the correct axis assignment for different configurations.

Also applies to: 96-106

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (13)

5-5: Remove unused import 'edu.wpi.first.wpilibj.ADIS16448_IMU'

The import statement for edu.wpi.first.wpilibj.ADIS16448_IMU is not used in the code and can be removed to clean up the imports.

🧰 Tools

🪛 GitHub Check: pmd

[warning] 5-5: Unused import 'edu.wpi.first.wpilibj.ADIS16448_IMU'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

39-41: LGTM!

The constructor logic is correct, and the implementation is accurate.

47-53: LGTM!

The constructor logic is correct, and the implementation is accurate.

62-64: LGTM!

The constructor logic is correct, and the implementation is accurate.

74-76: LGTM!

The constructor logic is correct, and the implementation is accurate.

86-92: LGTM!

The constructor logic is correct, and the implementation is accurate.

95-101: LGTM!

The method correctly retrieves the raw rotation data as a Rotation3d object by converting the angles from degrees to radians.

104-106: LGTM!

The method correctly retrieves the acceleration data as a Translation3d object.

109-111: LGTM!

The method correctly overrides the calibrate() method to call the corresponding method on the underlying ADIS16470_IMU instance.

114-116: LGTM!

The method correctly overrides the reset() method to call the corresponding method on the underlying ADIS16470_IMU instance.

119-121: LGTM!

The method correctly returns the underlying ADIS16470_IMU instance.

128-134: LGTM!

The method correctly converts an IMUAxis to the corresponding ADIS16470_IMU.IMUAxis.

141-148: LGTM!

The method correctly converts an ADIS16470_IMU.IMUAxis to the corresponding IMUAxis.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java

coderabbitai

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (2)

build.gradle (1)

51-51: LGTM! Consider grouping WPILib dependencies.

The addition of the WPIUnits dependency is appropriate and aligns with the PR objectives. It will provide useful unit conversion utilities for working with gyroscopes and IMUs.

The version (2024.3.2) is consistent with other WPILib dependencies, which is good for compatibility.

Consider grouping all WPILib dependencies together for better organization. You could move this line up to be with the other WPILib dependencies (around line 44-46).
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1)
127-133: Ensure all possible axes are handled in fromIMUAxis method

In the fromIMUAxis method, all possible values of IMUAxis are mapped, but it's good practice to handle unexpected values explicitly, even if they are not currently expected.

Consider adding a default case to handle any unforeseen IMUAxis values:
 public static ADIS16470_IMU.IMUAxis fromIMUAxis(IMUAxis axis) {
     return switch (axis) {
         case X -> ADIS16470_IMU.IMUAxis.kX;
         case Y -> ADIS16470_IMU.IMUAxis.kY;
         case Z -> ADIS16470_IMU.IMUAxis.kZ;
+        default -> throw new IllegalArgumentException("Unknown IMUAxis: " + axis);
     };
 }
This ensures that the method fails with a clear exception if an unexpected axis is encountered.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a699ec7 and 2e5b57d.

📒 Files selected for processing (4)

build.gradle (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java

🧰 Additional context used

🪛 GitHub Check: pmd

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

[warning] 9-9: Unused import 'java.util.Optional'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

[warning] 36-36: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

[warning] 22-22: Avoid unused private fields such as 'yaw'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 27-27: Avoid unused private fields such as 'pitch'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 32-32: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

🔇 Additional comments (1)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1)

103-105: Verify the units of acceleration data

The imu.getAccelX(), imu.getAccelY(), and imu.getAccelZ() methods may return acceleration values in units of Gs (gravitational acceleration), not meters per second squared. To ensure accurate calculations, confirm the units and convert to meters per second squared if necessary.

Run the following script to check the documentation comments for getAccelX(), getAccelY(), and getAccelZ() methods:

This script searches for the ADIS16470_IMU.java file and prints the documentation for the acceleration methods to verify the units returned.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

…ion3d() and IMU.getRawRotation3d()

coderabbitai

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (2)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (2)
34-91: Consider using consistent default calibration time across constructors

The constructors are well-implemented and provide good flexibility. However, there's a minor inconsistency in the default calibration time:

The no-arg constructor uses the default ADIS16470_IMU constructor, which likely uses a 500ms calibration time.

Other constructors default to ADIS16470_IMU.CalibrationTime._4s.

For consistency, consider using the same default calibration time across all constructors.

135-147: Consider clarifying the error message in toIMUAxis method

The toIMUAxis method is well-implemented using a switch expression. However, the error message in the default case could be more informative.

Consider updating the error message to:
-            default -> throw new IllegalArgumentException("IMU axis must be one of  " + axis);
+            default -> throw new IllegalArgumentException("Unexpected IMU axis: " + axis);
This change makes it clearer that the error occurs when an unexpected axis value is encountered.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2e5b57d and 3881104.

📒 Files selected for processing (3)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1 hunks)

🧰 Additional context used

📓 Learnings (1)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1)

Learnt from: Baconing
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java:121-123
Timestamp: 2024-10-03T15:11:52.897Z
Learning: The methods `imu.getAccelX()`, `imu.getAccelY()`, and `imu.getAccelZ()` return values in meters per second squared, not in Gs.

🪛 GitHub Check: pmd

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

[warning] 9-9: Unused import 'java.util.Optional'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

[warning] 36-36: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

[warning] 22-22: Avoid unused private fields such as 'yaw'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 27-27: Avoid unused private fields such as 'pitch'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 32-32: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

🔇 Additional comments (12)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (7)

9-13: LGTM: Well-structured abstract class declaration

The abstract class declaration and the private member variable are well-defined. The offset variable is correctly encapsulated and initialized.

15-21: LGTM: Clear and correct implementation of getRotation3dRadians()

The method name clearly indicates the return unit (radians), and the implementation correctly combines the raw rotation with the offset. This addresses the previous concern about unit clarity.

23-27: LGTM: Well-defined abstract method for raw rotation

The getRawRotation3dRadians() method is appropriately declared as abstract, allowing subclasses to implement their specific way of getting raw rotation. The method name clearly indicates its purpose and return unit.

29-33: LGTM: Correct return type for acceleration method

The getAccelerationMetersPerSecondSquared() method now correctly returns a Translation3d object, which is appropriate for representing acceleration. This addresses the previous issue of using an incorrect return type (Rotation3d). The method name clearly indicates the unit of measurement.

35-49: LGTM: Well-implemented getter and setter for offset

The setOffset() and getOffset() methods are correctly implemented. They provide a proper way to update and access the offset, which is crucial for calibration and maintaining encapsulation.

51-59: LGTM: Clear abstract methods for calibration and reset

The calibrate() and reset() abstract methods are well-defined, allowing subclasses to implement specific procedures for these operations. The method names are clear and self-explanatory.

71-76: LGTM: Well-defined enum for IMU axes

The IMUAxis enum is simple, clear, and correctly represents the three axes of a gyroscope. Its placement within the IMU class is appropriate given its close relation to IMU functionality.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (4)

1-32: LGTM: Well-structured class with proper imports and documentation

The class structure, imports, and field declarations look good. The use of @NotNull annotations enhances null safety, which is a good practice.

🧰 Tools

🪛 GitHub Check: pmd

[warning] 22-22: Avoid unused private fields such as 'yaw'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 27-27: Avoid unused private fields such as 'pitch'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 32-32: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

102-105: LGTM: Correct implementation of getAccelerationMetersPerSecondSquared

The getAccelerationMetersPerSecondSquared method correctly uses the Translation3d class to represent acceleration, which is appropriate for vector quantities. The implementation looks good.

107-120: LGTM: Proper delegation in calibrate, reset, and getRawIMU methods

The calibrate, reset, and getRawIMU methods correctly delegate to the underlying IMU instance. This implementation is clean and follows good design principles.

122-133: LGTM: Effective use of switch expression in fromIMUAxis method

The fromIMUAxis method is well-implemented, using a modern Java switch expression for a concise and readable conversion between IMUAxis types.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1)

1-168: Overall implementation review

The ADIS16448IMU class provides a comprehensive wrapper for the ADIS16448 IMU, offering flexibility in initialization and axis configuration. The implementation is generally solid, with proper use of annotations, modern Java features, and clear method implementations.

Key points to address:

Remove the unused import of java.util.Optional.

Verify and document the axis assignments in the constructors, removing the TODO comments once confirmed.

Consider the usage of the 'roll' field, either by adding a TODO comment explaining its future use or removing it if unnecessary.

Addressing these points will enhance the code quality and reliability of the IMU implementation.

Great job on implementing this IMU wrapper! Once the minor issues are addressed, this class will be a robust and flexible solution for working with the ADIS16448 IMU.

🧰 Tools

🪛 GitHub Check: pmd

[warning] 9-9: Unused import 'java.util.Optional'
Reports import statements that can be removed. They are either unused,
duplicated, or the members they import are already implicitly in scope,
because they're in java.lang, or the current package.

If some imports cannot be resolved, for instance because you run PMD with
an incomplete auxiliary classpath, some imports may be conservatively marked
as used even if they're not to avoid false positives.

UnnecessaryImport (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryimport

[warning] 36-36: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

…) method to IMU

…N in switch statement

…0-0/gyros

coderabbitai

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (5)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (2)
16-28: LGTM with a minor suggestion: Methods for angle and rotation retrieval.

The getAngleRadians and getRotation3dRadians methods are well-defined. The concrete implementation of getRotation3dRadians correctly combines the raw rotation with the offset.

Consider adding a @param tag in the JavaDoc for getAngleRadians to describe the axis parameter:
/**
 * Gets the specified angle's rotation in radians.
 * @param axis The axis for which to retrieve the angle.
 * @return the angle's rotation in radians.
 */
public abstract double getAngleRadians(SingleAxisGyroscope.Axis axis);
58-76: LGTM with a minor suggestion: Abstract methods for calibration, reset, and raw IMU access.

The calibrate, reset, and getRawIMU methods are appropriately defined as abstract, allowing for device-specific implementations in subclasses. The getRawIMU method's return type of Object aligns with the provided learnings, allowing for flexibility without imposing generics on consumers of the IMU class.

For consistency, consider adding brief descriptions to the calibrate and reset method JavaDocs, similar to other methods in the class:
/**
 * Calibrates the gyroscope.
 * Implementations should define the specific calibration procedure.
 */
public abstract void calibrate();

/**
 * Resets the gyroscope.
 * Implementations should define the specific reset procedure.
 */
public abstract void reset();
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (3)
22-33: Document the intended future use of yaw, pitch, and roll fields

The yaw, pitch, and roll fields are currently unused in the implementation, which is flagged by static analysis tools. However, based on the PR objectives and past comments, these fields are intended for future use in SingleAxisGyroscope implementation.

To improve code clarity and suppress static analysis warnings, consider adding a comment explaining their intended future use. For example:
/**
 * The YAW axis.
 * Note: This field is intended for future use in SingleAxisGyroscope implementation.
 */
private final IMUAxis yaw;

// Similar comments for pitch and roll
This documentation will help other developers understand the purpose of these fields and prevent accidental removal.

🧰 Tools

🪛 GitHub Check: pmd

[warning] 23-23: Avoid unused private fields such as 'yaw'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 28-28: Avoid unused private fields such as 'pitch'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 33-33: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

39-92: Ensure consistency in default calibration time across constructors

The constructors use different default calibration times:

The no-arg constructor uses the default from ADIS16470_IMU(), which is 500ms.

The constructor with port parameter uses ADIS16470_IMU.CalibrationTime._4s.

Consider using a consistent default calibration time across all constructors to avoid potential confusion. You could either:

Use 4s as the default for all constructors:
public ADIS16470IMU() {
    this(IMUAxis.Z, IMUAxis.X, IMUAxis.Y, SPI.Port.kMXP, ADIS16470_IMU.CalibrationTime._4s);
}
Or use 500ms as the default for all constructors:
public ADIS16470IMU(IMUAxis yaw, IMUAxis pitch, IMUAxis roll, SPI.Port port) {
    this(yaw, pitch, roll, port, ADIS16470_IMU.CalibrationTime._512ms);
}
Choose the option that best fits your use case and update the constructor chain accordingly.

159-166: Improve error message in toIMUAxis method

The toIMUAxis method has a good implementation, but the error message in the default case could be more informative. Consider updating it to list all valid axis options:
default -> throw new IllegalArgumentException("Invalid IMU axis. Must be one of kX, kY, or kZ, but got: " + axis);
This change provides more context about the expected valid inputs, making it easier for developers to identify and fix issues when an invalid axis is provided.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3881104 and 8dd30a7.

📒 Files selected for processing (4)

build.gradle (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

build.gradle

🧰 Additional context used

📓 Learnings (2)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1)

Learnt from: Baconing
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java:121-123
Timestamp: 2024-10-03T15:11:52.897Z
Learning: The methods `imu.getAccelX()`, `imu.getAccelY()`, and `imu.getAccelZ()` return values in meters per second squared, not in Gs.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1)

Learnt from: Trip-kun
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java:61-69
Timestamp: 2024-10-03T15:33:18.879Z
Learning: In this codebase, the `IMU` class should not be made generic, as this would require all consumers to be aware of the specific gyroscope types or to be generic themselves. Returning `Object` from `getRawIMU()` is acceptable, allowing callers to perform type checking as needed.

🪛 GitHub Check: pmd

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

[warning] 23-23: Avoid unused private fields such as 'yaw'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 28-28: Avoid unused private fields such as 'pitch'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

[warning] 33-33: Avoid unused private fields such as 'roll'.
Detects when a private field is declared and/or assigned a value, but not used.

Since PMD 6.50.0 private fields are ignored, if the fields are annotated with any annotation or the
enclosing class has any annotation. Annotations often enable a framework (such as dependency injection, mocking
or e.g. Lombok) which use the fields by reflection or other means. This usage can't be detected by static code analysis.
Previously these frameworks where explicitly allowed by listing their annotations in the property
"ignoredAnnotations", but that turned out to be prone of false positive for any not explicitly considered framework.

UnusedPrivateField (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_bestpractices.html#unusedprivatefield

🔇 Additional comments (11)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (5)

1-6: LGTM: Package declaration and imports are appropriate.

The package declaration and imports are well-structured and relevant to the IMU implementation. The use of edu.wpi.first.math.geometry classes indicates proper integration with the WPILib framework.

7-14: LGTM: Class declaration and offset field are well-defined.

The abstract class declaration for IMU is appropriate, providing a common interface for various IMU implementations. The offset field is correctly encapsulated as private and initialized to zero, which is a good practice.

30-40: LGTM: Abstract methods for raw rotation and acceleration.

The getRawRotation3dRadians and getAccelerationMetersPerSecondSquared methods are well-defined abstract methods. Their signatures and return types (Rotation3d and Translation3d) are appropriate for representing 3D rotation and acceleration. The JavaDoc comments provide clear descriptions of the methods' purposes.

42-56: LGTM: Offset getter and setter methods.

The setOffset and getOffset methods are well-implemented for managing the gyroscope's offset. The methods are properly documented with clear JavaDoc comments, and their implementations are straightforward and correct.

78-83: LGTM: IMUAxis enum definition.

The IMUAxis enum is well-defined, representing the three axes (X, Y, Z) of a gyroscope. This simple enumeration provides a clear and type-safe way to refer to specific axes in the IMU implementation.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (6)

1-9: LGTM: Clean imports and correct package declaration.

The package declaration and imports are appropriate for the class implementation. All imported classes are used within the code, and there are no unnecessary imports.

110-132: LGTM: Well-implemented getAngleRadians method.

The getAngleRadians method is efficiently implemented using nested switch expressions. It correctly returns the appropriate angle based on the given axis and the IMU's configuration. The use of modern Java features makes the code concise and readable.

134-141: LGTM: Correct implementation of getRawRotation3dRadians.

The getRawRotation3dRadians method correctly converts the gyro angles from degrees to radians using Math.toRadians(). It then returns a new Rotation3d object with these converted values, which is the expected behavior for this method.

143-146: LGTM: Correct implementation of getAccelerationMetersPerSecondSquared.

The getAccelerationMetersPerSecondSquared method correctly returns a new Translation3d object with acceleration values. As per the provided learnings, the methods imu.getAccelX(), imu.getAccelY(), and imu.getAccelZ() already return values in meters per second squared, so no conversion is necessary.

148-190: LGTM: Well-implemented utility methods.

The utility methods in this class are correctly implemented:

calibrate() and reset() appropriately delegate to the underlying imu object.

getRawIMU() correctly returns the underlying imu object.

fromIMUAxis() and toIMUAxis() provide correct conversions between different IMU axis representations.

These methods enhance the usability and flexibility of the ADIS16448IMU class.

1-191: Overall assessment: Solid implementation with minor improvements needed.

The ADIS16448IMU class is well-structured and provides a comprehensive implementation for the ADIS16448 IMU. The code is generally clean, uses modern Java features effectively, and provides good flexibility through multiple constructors.

Key points to address:

Verify and document the axis assignments in the constructors.

Consider refactoring the repeated axis assignment logic into a private method.

Review the usage of the roll field, which is currently unused in the class methods.

Addressing these points will further improve the code quality and maintainability of this class.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

…AxisGyroscope.Axis) and fix getRawRotation3dRadians() by using yaw pitch roll instead of XYZ

coderabbitai

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (5)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (3)
14-33: Consider documenting the purpose of yaw, pitch, and roll fields

The yaw, pitch, and roll fields are well-declared and documented. However, they were previously flagged as unused by static analysis. Since you mentioned they'll serve a purpose in later commits when SingleAxisGyroscope is implemented, consider adding a comment to explain their future use to prevent confusion.

Example:
/**
 * The YAW axis.
 * Note: This field will be used in future implementations with SingleAxisGyroscope.
 */
private final IMUAxis yaw;
94-126: LGTM: Public methods are well-implemented

The public methods are correctly implemented and use the configured axes appropriately. The getRawRotation3dRadians method now correctly uses the configured yaw, pitch, and roll fields, addressing a previous concern.

One minor suggestion for clarity:

In the getAccelerationMetersPerSecondSquared method, consider adding a comment to clarify that the imu.getAccelX(), imu.getAccelY(), and imu.getAccelZ() methods return values in meters per second squared, as this isn't immediately obvious from the method names.

128-166: Improve error message in toIMUAxis method

The utility methods are well-implemented and use modern switch expressions effectively. However, the error message in the toIMUAxis method could be more informative:
default -> throw new IllegalArgumentException("IMU axis must be one of  " + axis);
Consider updating it to:
default -> throw new IllegalArgumentException("Unexpected IMU axis: " + axis);
This change makes it clear that the provided axis is unexpected, rather than suggesting it should be one of the axis that caused the error.
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (2)
1-13: Remove unused import.

The import java.util.Optional is not used in this class and can be removed to keep the code clean.
-import java.util.Optional;
13-35: Consider utilizing or removing the unused roll field.

The roll field is declared but not used in any of the class methods. If it's intended for future use, consider adding a TODO comment explaining its purpose. Otherwise, you may want to remove it to keep the code clean.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 8dd30a7 and 648adab.

📒 Files selected for processing (3)

build.gradle (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

build.gradle

🧰 Additional context used

📓 Learnings (1)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1)

Learnt from: Baconing
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java:121-123
Timestamp: 2024-10-03T15:11:52.897Z
Learning: The methods `imu.getAccelX()`, `imu.getAccelY()`, and `imu.getAccelZ()` return values in meters per second squared, not in Gs.

🔇 Additional comments (4)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (2)

1-13: LGTM: Imports and class declaration are well-structured

The imports are comprehensive and appropriate for the class implementation. The class declaration and documentation are clear and concise.

1-167: Overall: Well-implemented ADIS16470IMU class with minor suggestions for improvement

The ADIS16470IMU class is comprehensively implemented with good structure, documentation, and functionality. The code addresses previous concerns about unused fields and axis usage in rotation calculations. The suggested improvements are minor and focus on enhancing clarity, consistency, and error handling:

Document the future use of yaw, pitch, and roll fields.

Ensure consistent default calibration time across constructors.

Add a clarifying comment about acceleration units.

Improve the error message in the toIMUAxis method.

These small changes will further improve the already solid implementation.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (2)

166-169: LGTM: Correct implementation of getAccelerationMetersPerSecondSquared.

The method correctly returns a new Translation3d object with acceleration values in meters per second squared. The implementation is concise and accurate.

186-213: LGTM: Well-implemented utility methods.

The fromIMUAxis and toIMUAxis methods are correctly implemented using switch expressions. They provide a clean and efficient way to convert between IMUAxis and ADIS16448_IMU.IMUAxis enums.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

…sses

coderabbitai

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (6)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (3)
64-96: Rotation3d methods are correctly implemented, but consider future cleanup.

The getRotation3dRadians method correctly applies offset and handles inversion. The deprecated methods getRawRotation3d and getRotation3d are maintained for YAGSL compatibility.

Consider removing the deprecated methods getRawRotation3d and getRotation3d in a future update once YAGSL compatibility is no longer required. This will simplify the API and reduce potential confusion.

98-119: Acceleration methods are well-defined, but consider future cleanup.

The abstract methods for getting acceleration data are appropriately defined. The deprecated getAccel method is maintained for YAGSL compatibility.

Consider removing the deprecated getAccel method in a future update once YAGSL compatibility is no longer required. This will streamline the API and reduce potential confusion.

191-207: Consider improving type safety for getIMU method.

The getIMU method provides access to the raw gyroscope object, which is useful for advanced use cases. The IMUAxis enum is appropriately defined for representing the three axes.

Consider using generics to improve type safety for the getIMU method. This would eliminate the need for type casting in subclasses. For example:
public abstract class IMU<T> {
    // ...
    public abstract T getIMU();
}
Subclasses could then extend the generic IMU class with their specific IMU type:
public class ADIS16448IMU extends IMU<edu.wpi.first.wpilibj.ADIS16448_IMU> {
    @Override
    public edu.wpi.first.wpilibj.ADIS16448_IMU getIMU() {
        // Implementation
    }
}
This change would provide better type safety and reduce the risk of runtime errors due to incorrect casting.
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1)

39-41: Consider standardizing default calibration time across constructors

The no-arg constructor uses a default calibration time of 500ms (as per the ADIS16470_IMU JavaDoc), while the constructor with port parameter uses a 4s calibration time. Consider standardizing the default calibration time across all constructors for consistency. If different default times are intentional, please document the reasoning in the constructor JavaDocs.

Also applies to: 62-67, 77-84

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (2)

35-35: Consider adding @NotNull annotation to the 'roll' field.

For consistency with the other axis fields (yaw and pitch), consider adding the @NotNull annotation to the 'roll' field. This helps in maintaining a uniform approach to null-safety across the class.

220-223: Add @NotNull annotation to getRollAxis method.

For consistency with the getYawAxis and getPitchAxis methods, consider adding the @NotNull annotation to the getRollAxis method. This maintains a uniform approach to null-safety across the class.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 648adab and 28dfa6c.

📒 Files selected for processing (3)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1 hunks)

🧰 Additional context used

📓 Learnings (1)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1)

Learnt from: Baconing
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java:121-123
Timestamp: 2024-10-03T15:11:52.897Z
Learning: The methods `imu.getAccelX()`, `imu.getAccelY()`, and `imu.getAccelZ()` return values in meters per second squared, not in Gs.

🔇 Additional comments (10)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (4)

1-22: Class structure and member variables look good.

The abstract class IMU extends SwerveIMU and includes well-documented member variables for inversion and offset. This provides a solid foundation for specific IMU implementations.

24-62: getAngleRadians methods are well-implemented.

The overloaded getAngleRadians methods provide flexibility by accepting different axis enums (IMUAxis and SingleAxisGyroscope.Axis). They correctly apply the offset to the raw angle readings.

121-151: Offset and inversion methods are correctly implemented.

The methods for setting and getting the offset and inversion state are simple, straightforward, and provide the necessary functionality for managing the IMU's calibration and orientation.

153-189: Abstract methods for IMU operations are well-defined.

The abstract methods for calibration, reset, factory default, clearing sticky faults, and getting yaw, pitch, and roll axes provide a comprehensive interface for IMU operations. This ensures that specific IMU implementations will cover all essential functionalities.
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16470IMU.java (1)
125-131: Verify the order of angles in Rotation3d constructor

The current implementation passes the angles to the Rotation3d constructor in the order: roll, pitch, yaw. However, the Rotation3d constructor typically expects the angles in the order: yaw, pitch, roll. Please verify if this is intentional or if it needs to be adjusted to:
return new Rotation3d(
    Math.toRadians(imu.getAngle(fromIMUAxis(this.yaw))),
    Math.toRadians(imu.getAngle(fromIMUAxis(this.pitch))),
    Math.toRadians(imu.getAngle(fromIMUAxis(this.roll)))
);
This change would align with the conventional order of rotation angles.
src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (5)
176-178: LGTM: Correct implementation of getAccelerationMetersPerSecondSquared.

The implementation correctly returns a Translation3d object with acceleration values in meters per second squared. No conversion is needed as the ADIS16448 IMU already provides the values in the correct units.

189-203: LGTM: Correct implementation of calibrate, reset, and factoryDefault methods.

The calibrate and reset methods correctly delegate to the underlying IMU object. The factoryDefault method appropriately calibrates the IMU and resets the offset, ensuring a complete reset of the device state.

230-242: LGTM: Correct and efficient implementation of fromIMUAxis.

The fromIMUAxis method is well-implemented, using a modern switch expression for clear and concise conversion between axis types. The method is also properly documented with JavaDoc, enhancing code readability and maintainability.

244-257: LGTM: Correct and efficient implementation of toIMUAxis.

The toIMUAxis method is well-implemented, using a modern switch expression for clear and concise conversion between axis types. The method is also properly documented with JavaDoc, enhancing code readability and maintainability.

52-62: 🛠️ Refactor suggestion

Refactor duplicated axis assignment logic and verify assignments.

The axis assignment logic is duplicated in two constructors. Consider refactoring this into a private static method to improve maintainability.

There are TODO comments indicating uncertainty about the correctness of pitch and roll axis assignments. It's crucial to verify these assignments to ensure proper IMU functionality.

Here's a suggested refactor:
private static IMUAxis[] assignAxes(IMUAxis yaw) {
    IMUAxis pitch;
    IMUAxis roll;

    if (yaw == IMUAxis.X) {
        pitch = IMUAxis.Y;
        roll = IMUAxis.Z;
    } else if (yaw == IMUAxis.Y) {
        pitch = IMUAxis.X;
        roll = IMUAxis.Z;
    } else {
        pitch = IMUAxis.X;
        roll = IMUAxis.Y;
    }

    return new IMUAxis[]{pitch, roll};
}
Then, in your constructors, you can use this method:
IMUAxis[] axes = assignAxes(this.yaw);
this.pitch = axes[0];
this.roll = axes[1];
This refactoring will eliminate code duplication and make it easier to verify and update the axis assignment logic if needed.

Also applies to: 95-105

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

… utility method

coderabbitai

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1)
204-229: Remove unnecessary qualifiers in assignAxes method

The assignAxes method contains unnecessary IMUAxis qualifiers for enum values. Since these values are already in scope within the enum, the qualifiers can be safely removed to improve readability.

Apply this diff to remove the unnecessary qualifiers:
 public static IMUAxis[] assignAxes(IMUAxis yaw) {
     IMUAxis pitch;
     IMUAxis roll;

-    if (yaw == IMUAxis.X) {
-        pitch = IMUAxis.Y;
-        roll = IMUAxis.Z;
-    } else if (yaw == IMUAxis.Y) {
-        pitch = IMUAxis.X;
-        roll = IMUAxis.Z;
+    if (yaw == X) {
+        pitch = Y;
+        roll = Z;
+    } else if (yaw == Y) {
+        pitch = X;
+        roll = Z;
     } else {
-        pitch = IMUAxis.X;
-        roll = IMUAxis.Y;
+        pitch = X;
+        roll = Y;
     }

     return new IMUAxis[]{pitch, roll};
 }
🧰 Tools

🪛 GitHub Check: pmd

[warning] 216-216: Unnecessary qualifier 'IMUAxis': 'X' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 217-217: Unnecessary qualifier 'IMUAxis': 'Y' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 218-218: Unnecessary qualifier 'IMUAxis': 'Z' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 219-219: Unnecessary qualifier 'IMUAxis': 'Y' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 220-220: Unnecessary qualifier 'IMUAxis': 'X' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 221-221: Unnecessary qualifier 'IMUAxis': 'Z' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 223-223: Unnecessary qualifier 'IMUAxis': 'X' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 224-224: Unnecessary qualifier 'IMUAxis': 'Y' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 28dfa6c and 5722c6b.

📒 Files selected for processing (2)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1 hunks)

🧰 Additional context used

📓 Learnings (1)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (2)

Learnt from: Baconing
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java:205-208
Timestamp: 2024-10-07T13:56:20.208Z
Learning: In the `ADIS16448IMU` class, the `clearStickyFaults()` method should be left empty and not throw an exception, as there are no side effects from it doing nothing on the ADIS16448.

Learnt from: Baconing
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java:121-123
Timestamp: 2024-10-03T15:11:52.897Z
Learning: The methods `imu.getAccelX()`, `imu.getAccelY()`, and `imu.getAccelZ()` return values in meters per second squared, not in Gs.

🪛 GitHub Check: pmd

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java

[warning] 216-216: Unnecessary qualifier 'IMUAxis': 'X' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 217-217: Unnecessary qualifier 'IMUAxis': 'Y' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 218-218: Unnecessary qualifier 'IMUAxis': 'Z' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 219-219: Unnecessary qualifier 'IMUAxis': 'Y' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 220-220: Unnecessary qualifier 'IMUAxis': 'X' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 221-221: Unnecessary qualifier 'IMUAxis': 'Z' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 223-223: Unnecessary qualifier 'IMUAxis': 'X' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

[warning] 224-224: Unnecessary qualifier 'IMUAxis': 'Y' is already in scope because it is declared in an enclosing type
Import statements allow the use of non-fully qualified names. The use of a fully qualified name
which is covered by an import statement is redundant. Consider using the non-fully qualified name.

UnnecessaryFullyQualifiedName (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#unnecessaryfullyqualifiedname

🔇 Additional comments (3)

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java (2)

1-35: LGTM! Well-structured class declaration and fields.

The class structure is well-designed with appropriate use of final fields for immutability. The Javadoc comments provide clear documentation for the class and its fields.

157-240: LGTM! Well-implemented methods and utility functions.

The remaining methods in the class, including getAccelerationMetersPerSecondSquared, calibrate, reset, factoryDefault, and the axis getters, are well-implemented and straightforward. The utility methods fromIMUAxis and toIMUAxis provide clear conversion between different axis representations.

The empty implementation of clearStickyFaults is correct, as per the comment and the learning that there are no side effects from this function not doing anything on the ADIS16448.
src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java (1)
191-199: 🛠️ Refactor suggestion

Consider using generics for improved type safety in getIMU()

The getIMU() method currently returns Object, which provides flexibility but sacrifices type safety. Consider using generics to improve type safety and reduce the need for casting in subclasses. This change would align with the documentation suggestion and enforce proper typing in subclasses.

However, implementing this change would require modifications to the class definition and all its subclasses. Given the potential impact on the codebase, this change should be carefully considered and discussed with the team.

If you decide to implement generics, here's a possible refactoring:
-public abstract class IMU extends SwerveIMU {
+public abstract class IMU<T> extends SwerveIMU {
    // ... other methods ...

    /**
     * Returns the raw gyroscope object.
     * Should only be used when you know the subclass of the gyroscope.
     * <br><br>
-     * Implementations should override the return type to the type of the gyroscope used in the vendor's API.
+     * The return type is specified by the generic type T, which should be the type of the gyroscope used in the vendor's API.
     * (e.g the {@link ADIS16448IMU} would return {@link edu.wpi.first.wpilibj.ADIS16448_IMU})
-     * @return the raw gyroscope object.
+     * @return the raw gyroscope object of type T.
     */
-    public abstract Object getIMU();
+    public abstract T getIMU();
}
Subclasses would then extend the generic IMU class:
public class ADIS16448IMU extends IMU<edu.wpi.first.wpilibj.ADIS16448_IMU> {
    @Override
    public edu.wpi.first.wpilibj.ADIS16448_IMU getIMU() {
        // Implementation
    }
}
⛔ Skipped due to learnings
Learnt from: Trip-kun
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java:61-69
Timestamp: 2024-10-03T15:33:18.879Z
Learning: In this codebase, the `IMU` class should not be made generic, as this would require all consumers to be aware of the specific gyroscope types or to be generic themselves. Returning `Object` from `getRawIMU()` is acceptable, allowing callers to perform type checking as needed.

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java

all the functionality from SingleAxisGyroscope exists in IMU, and it just complicates things

Changes reflected: IMUAxis -> CartesianAxis SingleAxisGyroscope -> Attitude

…ianAxis.assignAxes(CartesianAxis yaw)

Trip-kun · 2024-10-07T17:40:36Z

src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java

I would run back over the getAngleRadians method. It may be better to "apply" the rotation to the entire 3d rotation and then extract the axis you want rather than just adding the offset to the raw output. I'm not entirely sure which would be correct here but it is notable that 3d chained rotations are complicated (there's a reason they can be done with quaternions and quaternion multiplication is not communicative.

https://math.stackexchange.com/questions/2016937/why-is-the-product-of-two-rotation-matrices-not-commutative

an example of what I'm talking about, by the way

Trip-kun · 2024-10-07T17:43:23Z

src/main/java/net/frc5183/librobot/hardware/gyro/SingleAxisGyroscope.java

I would not just remove single axis gyroscopes. They can be useful for tracking rotating parts like arms or grabbers. In addition, you could "get" a single axis gyroscope from IMUs to allow more adaptability.

Signed-off-by: Brenden Freier <[email protected]>

…essary keep my family's name out of your mouth, quaternions

coderabbitai

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (3)

src/main/java/net/frc5183/librobot/hardware/gyro/NavXIMU.java (1)
157-159: Handle unsupported operations appropriately

The clearStickyFaults() method currently contains a comment stating that the NavX IMU does not support this functionality. To make this clear to users of your API, consider throwing an UnsupportedOperationException. This approach explicitly informs users that the operation is not supported.

Apply this diff to implement the exception:
 public void clearStickyFaults() {
-    // The NavX IMU does not have a method for clearing sticky faults.
+    throw new UnsupportedOperationException("clearStickyFaults is not supported by NavXIMU.");
 }
src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java (1)

142-148: Review Redundant Calibration in factoryDefault()

The factoryDefault() method calls imu.calibrate(), and the calibrate() method also calls imu.calibrate(). This could lead to redundant calibration steps, increasing initialization time. Consider whether it is necessary to call imu.calibrate() in both methods to optimize performance.
src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java (1)
226-228: Rename getInverted() to isInverted() for clarity

In Java, it's standard convention to prefix boolean getters with is or has. Renaming getInverted() to isInverted() enhances readability and aligns with common naming practices.

Apply this diff to rename the method:
-public boolean getInverted() {
+public boolean isInverted() {
     return inverted;
 }
Ensure all references to getInverted() are updated accordingly.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5722c6b and fa12c52.

📒 Files selected for processing (4)

src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16470IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/NavXIMU.java (1 hunks)

🧰 Additional context used

📓 Learnings (1)

src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java (1)

Learnt from: Baconing
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java:205-208
Timestamp: 2024-10-07T13:56:20.208Z
Learning: In the `ADIS16448IMU` class, the `clearStickyFaults()` method should be left empty and not throw an exception, as there are no side effects from it doing nothing on the ADIS16448.

🔇 Additional comments (5)

src/main/java/net/frc5183/librobot/hardware/gyro/NavXIMU.java (2)

142-143: Review the reset and calibration methods for completeness

The calibrate(), reset(), and factoryDefault() methods all call imu.reset(). Verify that this method effectively performs the intended action in each context:

Calibrate: Typically involves a procedure to minimize sensor bias. Confirm if additional calibration steps are required beyond resetting.

Reset: Resets the sensor's yaw, pitch, and roll angles. Ensure this method resets all necessary state.

Factory Default: Should restore the sensor to its original factory settings. Consider if imu.reset() suffices or if more comprehensive actions are needed.

Also applies to: 147-148, 152-154

190-193: Ensure consistent axis mapping in conversion methods

In the toCartesian and fromCartesian static methods, you map between AHRS.BoardAxis and CartesianAxis. Verify that all possible axis values are correctly mapped and that the mapping is consistent in both directions. This ensures reliable conversions and prevents potential mismatches.

You can run the following script to check for consistency:

Also applies to: 203-207

src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java (3)

49-57: Ensure Proper Axis Assignment in Constructor

In the constructor, yaw is initialized using imu.getYawAxis(), and then pitch and roll are assigned using CartesianAxis.assignAxes(yaw). Please verify that the assignment of pitch and roll axes is correct and aligns with the physical orientation of the IMU. Incorrect axis mapping could lead to inaccurate readings.

151-153: Empty Implementation of clearStickyFaults() is Appropriate

As per previous learnings, leaving the clearStickyFaults() method empty is acceptable because the ADIS16448 IMU does not have a mechanism for clearing sticky faults. This implementation aligns with the hardware capabilities.

89-96: ⚠️ Potential issue

Ensure Compatibility of Switch Expressions with Project's Java Version

The use of switch expressions in methods like getRawAngleRadians() relies on features introduced in Java 14 or newer. Please confirm that the project's target Java version supports switch expressions to prevent potential compilation issues.

Run the following script to check the target Java version specified in your build files:

src/main/java/net/frc5183/librobot/hardware/gyro/NavXIMU.java

coderabbitai · 2024-10-14T13:08:32Z

src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16470IMU.java

+ case kYaw -> Attitude.YAW;
+ case kPitch -> Attitude.PITCH;
+ case kRoll -> Attitude.ROLL;
+ default -> throw new IllegalArgumentException("ADIS16470_IMU Axis must be one of kYaw, kPitch, or kRoll. Was" + axis);


⚠️ Potential issue

Typo in exception message in toAttitude method

The exception message in the toAttitude method is missing a colon and space before the axis variable. It should be:

- default -> throw new IllegalArgumentException("ADIS16470_IMU Axis must be one of kYaw, kPitch, or kRoll. Was" + axis); + default -> throw new IllegalArgumentException("ADIS16470_IMU Axis must be one of kYaw, kPitch, or kRoll. Was: " + axis);

📝 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

default -> throw new IllegalArgumentException("ADIS16470_IMU Axis must be one of kYaw, kPitch, or kRoll. Was" + axis);

default -> throw new IllegalArgumentException("ADIS16470_IMU Axis must be one of kYaw, kPitch, or kRoll. Was: " + axis);

coderabbitai · 2024-10-14T13:08:33Z

src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java

+ public Rotation3d getOffset() {
+ if (offset != null) return offset;
+
+ Map<CartesianAxis, Double> offset = Map.of(
+ CartesianAxis.X, offsetX,
+ CartesianAxis.Y, offsetY,
+ CartesianAxis.Z, offsetZ
+ );
+
+ return new Rotation3d(
+ offset.get(getYawAxis()),
+ offset.get(getPitchAxis()),
+ offset.get(getRollAxis())
+ );
+ }


⚠️ Potential issue

Remove deprecated offset field to prevent inconsistencies

The getOffset() method uses the deprecated offset field, and if it's null, it reconstructs the offset from offsetX, offsetY, and offsetZ. This can lead to inconsistencies if both are used. To maintain clarity and prevent potential errors, remove the deprecated offset field and adjust getOffset() to always compute the offset from offsetX, offsetY, and offsetZ.

Apply this diff to simplify getOffset():

-@Deprecated -@Nullable -private Rotation3d offset; ... -@Deprecated -public Rotation3d getOffset() { - if (offset != null) return offset; - - Map<CartesianAxis, Double> offset = Map.of( - CartesianAxis.X, offsetX, - CartesianAxis.Y, offsetY, - CartesianAxis.Z, offsetZ - ); - - return new Rotation3d( - offset.get(getYawAxis()), - offset.get(getPitchAxis()), - offset.get(getRollAxis()) - ); -} +public Rotation3d getOffset() { + Map<CartesianAxis, Double> offsetMap = Map.of( + CartesianAxis.X, offsetX, + CartesianAxis.Y, offsetY, + CartesianAxis.Z, offsetZ + ); + + return new Rotation3d( + offsetMap.get(getYawAxis()), + offsetMap.get(getPitchAxis()), + offsetMap.get(getRollAxis()) + ); +}

📝 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 Rotation3d getOffset() {

if (offset != null) return offset;

Map<CartesianAxis, Double> offset = Map.of(

CartesianAxis.X, offsetX,

CartesianAxis.Y, offsetY,

CartesianAxis.Z, offsetZ

);

return new Rotation3d(

offset.get(getYawAxis()),

offset.get(getPitchAxis()),

offset.get(getRollAxis())

);

}

public Rotation3d getOffset() {

Map<CartesianAxis, Double> offsetMap = Map.of(

CartesianAxis.X, offsetX,

CartesianAxis.Y, offsetY,

CartesianAxis.Z, offsetZ

);

return new Rotation3d(

offsetMap.get(getYawAxis()),

offsetMap.get(getPitchAxis()),

offsetMap.get(getRollAxis())

);

}

…fault IMU methods. ADIS16470 acceleration tests are broken due to a [bug in wpilibj](wpilibsuite/allwpilib#7180).

…ed gyroscopes

coderabbitai

Actionable comments posted: 13

🧹 Outside diff range and nitpick comments (7)

src/test/java/net/frc5183/librobot/hardware/gyro/IMUTests.java (1)
10-21: LGTM: Comprehensive test cases with clear assertions.

The test cases cover all possible yaw inputs (X, Y, Z) and check both pitch and roll assignments for each case. The assertions use clear and descriptive error messages.

Consider adding a blank line between test cases for improved readability:
 assertEquals(IMU.CartesianAxis.Y, yawX[1], "Incorrect roll when yaw is X.");

+
 IMU.CartesianAxis[] yawY = IMU.CartesianAxis.assignAxes(IMU.CartesianAxis.Y);
 assertEquals(IMU.CartesianAxis.X, yawY[0], "Incorrect pitch when yaw is Y.");
 assertEquals(IMU.CartesianAxis.Z, yawY[1], "Incorrect roll when yaw is Y.");

+
 IMU.CartesianAxis[] yawZ = IMU.CartesianAxis.assignAxes(IMU.CartesianAxis.Z);
 assertEquals(IMU.CartesianAxis.X, yawZ[0], "Incorrect pitch when yaw is Z.");
 assertEquals(IMU.CartesianAxis.Y, yawZ[1], "Incorrect roll when yaw is Z.");
src/main/java/net/frc5183/librobot/hardware/gyro/NavXIMU.java (1)
113-144: Consider adding a comment explaining NavX limitations

The getRawRotation3dRadians method is correctly implemented. However, for the getRateDegreesPerSecond methods, it would be helpful to add a comment explaining why pitch and roll rates are not supported by the NavX IMU. This would provide clarity for users of the class.

Consider adding a comment above the getRateDegreesPerSecond methods:
/**
 * Gets the rate of change of the specified attitude in degrees per second.
 * Note: NavX IMU only supports yaw rate. Pitch and roll rates are not available.
 * @param axis The attitude axis to get the rate for.
 * @return The rate of change in degrees per second.
 * @throws UnsupportedOperationException if the requested axis is not supported.
 */
src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java (1)

38-38: Inconsistent documentation of default calibration time

The comment on line 38 mentions a "500ms calibration time," but elsewhere in the constructors (e.g., line 75), the calibration time is set to 512ms using ADIS16448_IMU.CalibrationTime._512ms. Please verify the default calibration time and update the documentation to consistently reflect the correct value.
src/test/java/net/frc5183/librobot/hardware/gyro/NavXTests.java (2)
582-583: Rephrase unprofessional comment in documentation

The comment contains informal language ("but I'm lazy") that might not be appropriate for the codebase. Consider rephrasing it to maintain a professional tone.

Suggested change:
-     * I could grab this value programmatically, but I'm lazy and the current delay seems fine.
+     * The delay duration is set to accommodate the sensor's update frequency.
199-201: Remove unnecessary casts to (double)

Casting a Double to (double) is redundant because unboxing happens automatically. Removing unnecessary casts improves code readability.

Apply this diff to remove the redundant casts:
-    imu.setOffsetX((double) offset);
-    imu.setOffsetY((double) offset);
-    imu.setOffsetZ((double) offset);
+    imu.setOffsetX(offset);
+    imu.setOffsetY(offset);
+    imu.setOffsetZ(offset);
src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16470Tests.java (2)
207-211: Simplify lambda expressions for better readability

The lambda expressions for setAngle and verifyRot can be simplified by removing explicit type declarations, enhancing code readability.

Apply this diff to simplify the lambdas:
-Consumer<Double> setAngle = (Double angle) -> {
+Consumer<Double> setAngle = angle -> {
     sim.setGyroAngleX(angle);
     sim.setGyroAngleY(angle);
     sim.setGyroAngleZ(angle);
 };

-Consumer<Double> verifyRot = (Double angle) -> {
+Consumer<Double> verifyRot = angle -> {
     assertEquals(
         new Rotation3d(
             Math.toRadians(angle),
             Math.toRadians(angle),
             Math.toRadians(angle)
         ),
         imu.getRotation3dRadians(),
         "Rotation3d failed."
     );
 };
Also applies to: 214-224

606-608: Ensure testIMU provides meaningful assertions

The testIMU method currently checks for reference equality, which might not catch underlying issues. Consider adding more detailed assertions to verify the state and behavior of the IMU instance.

For example, you could assert that certain properties of wpiIMU match expected values.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between fa12c52 and 7ae15eb.

📒 Files selected for processing (8)

src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16470IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java (1 hunks)
src/main/java/net/frc5183/librobot/hardware/gyro/NavXIMU.java (1 hunks)
src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16448Tests.java (1 hunks)
src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16470Tests.java (1 hunks)
src/test/java/net/frc5183/librobot/hardware/gyro/IMUTests.java (1 hunks)
src/test/java/net/frc5183/librobot/hardware/gyro/NavXTests.java (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16470IMU.java

🧰 Additional context used

📓 Learnings (1)

src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java (1)

Learnt from: Baconing
PR: frc5183/librobot#9
File: src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java:205-208
Timestamp: 2024-10-07T13:56:20.208Z
Learning: In the `ADIS16448IMU` class, the `clearStickyFaults()` method should be left empty and not throw an exception, as there are no side effects from it doing nothing on the ADIS16448.

🔇 Additional comments (14)

src/test/java/net/frc5183/librobot/hardware/gyro/IMUTests.java (3)
1-5: LGTM: Package declaration and imports are correct.

The package declaration is consistent with the file path, and the imports are appropriate for a JUnit 5 test class.

7-9: LGTM: Test class and method declarations are well-structured.

The class name IMUTests is descriptive and follows Java naming conventions. The test method testAssignCartesianAxes() is properly annotated with @Test and has a clear, descriptive name.

1-22: Overall, excellent test implementation for IMU axis assignment.

The IMUTests class provides a comprehensive test for the assignAxes method of the IMU.CartesianAxis class. It covers all possible yaw inputs and verifies the correct assignment of pitch and roll axes. The test structure is clear, and the assertions are well-written with descriptive error messages.

To ensure that this test class covers all the necessary functionality of the IMU implementation, let's verify the existence of the IMU class and its CartesianAxis enum:

This script will help us confirm that the tested functionality is properly implemented in the main code.

✅ Verification successful

Verification Complete: IMU Class and CartesianAxis Enum Exist

The IMU class and its CartesianAxis enum are present in the codebase as expected. All necessary components are correctly implemented, ensuring that the tests in IMUTests are valid and effectively verify the assignAxes method.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the existence of IMU class and CartesianAxis enum

# Test: Search for IMU class definition
echo "Searching for IMU class definition:"
rg --type java "class IMU"

# Test: Search for CartesianAxis enum definition within IMU class
echo "Searching for CartesianAxis enum definition:"
rg --type java "enum CartesianAxis" -A 5
Length of output: 971
src/main/java/net/frc5183/librobot/hardware/gyro/NavXIMU.java (6)

1-37: LGTM: Class structure and field definitions are well-defined

The class declaration, imports, and field definitions are correctly implemented and well-documented. The use of the AHRS class from the Kauai Labs library and the definition of CartesianAxis fields for yaw, pitch, and roll provide a solid foundation for the NavXIMU implementation.

39-86: LGTM: Constructors provide flexible initialization options

The multiple constructors offer various initialization options, allowing users to create a NavXIMU instance using different ports (SPI, I2C, Serial) or an existing AHRS instance. The main constructor correctly sets up the axis configuration based on the IMU's board yaw axis.

89-111: LGTM: Angle retrieval methods correctly implemented

The getRawAngleRadians methods are correctly implemented for both Attitude and CartesianAxis parameters. The conversion from degrees to radians is accurate, and the use of the convertNavXDegrees method ensures the correct range of angles (0 to 360 degrees).

146-162: Acceleration conversion issue already addressed

The acceleration conversion issue in the getAccelerationMetersPerSecondSquared methods has been previously flagged in past review comments. Please refer to those comments for the necessary changes to convert the acceleration values from G's to m/s².

164-183: LGTM: IMU state management methods are well-implemented

The calibrate, reset, factoryDefault, and clearStickyFaults methods are correctly implemented. The use of imu.reset() for calibration and resetting is appropriate, and the additional offset reset in factoryDefault is a good practice. The comment explaining why clearStickyFaults is empty provides clarity for users of the class.

185-245: LGTM: Utility methods and axis conversions are well-implemented

The getter methods for axes and IMU are correctly implemented. The convertNavXDegrees method properly adjusts the angle range from -180 to 180 degrees to 0 to 360 degrees. The static methods for converting between CartesianAxis and AHRS.BoardAxis provide useful utility functions for working with different axis representations.

src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java (2)

171-173: clearStickyFaults() method correctly left empty

As per the retrieved learnings, the clearStickyFaults() method is appropriately left empty because the ADIS16448 IMU does not support clearing sticky faults. This implementation avoids unnecessary exceptions and aligns with the hardware capabilities.

162-168: ⚠️ Potential issue

Potential NullPointerException in factoryDefault()

In line 164, super.setOffset(null); is called. If setOffset() does not handle null values properly, this could lead to a NullPointerException. Please verify whether passing null is acceptable. If not, consider initializing the offset to a default value or modifying setOffset() to safely handle null.

src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java (1)

1-346: Well-Structured Implementation of the IMU Class

The IMU abstract class is well-designed, providing a comprehensive framework for various IMU functionalities. The methods are clearly defined, and the documentation enhances understandability.
src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16470Tests.java (1)
497-593: Verify if the wpilibj issue is resolved to re-enable acceleration tests

The acceleration tests are currently disabled due to a known bug in wpilibj (issue #7180). Check if the issue has been resolved so that these tests can be re-enabled.

Run the following script to check the status of the issue:

✅ Verification successful

The wpilibj issue #7180 is still OPEN. Acceleration tests should remain disabled until the issue is resolved.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check if issue #7180 in wpilibsuite/allwpilib is closed

# Expected result: The issue state should be "CLOSED" if resolved.
gh issue view 7180 --repo wpilibsuite/allwpilib --json state --jq '.state'
Length of output: 79
src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16448Tests.java (1)

1-638: Overall, the test suite is comprehensive and well-structured

The ADIS16448Tests class provides thorough coverage of the ADIS16448IMU functionalities. The tests are well-organized, cover a wide range of scenarios, and follow good practices in unit testing.

src/main/java/net/frc5183/librobot/hardware/gyro/ADIS16448IMU.java

src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java

src/test/java/net/frc5183/librobot/hardware/gyro/NavXTests.java

coderabbitai · 2024-10-14T16:33:55Z

src/test/java/net/frc5183/librobot/hardware/gyro/NavXTests.java

+ private void delay() {
+ try {
+ Thread.sleep(waitDuration);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ }


🛠️ Refactor suggestion

Avoid using Thread.sleep() in unit tests

Using Thread.sleep() in unit tests can lead to longer test execution times and potential flakiness. Consider using synchronization mechanisms or mocking to simulate the delay more effectively.

You might refactor the delay() method to eliminate Thread.sleep(). For example, you can use latches or other synchronization tools to wait for the simulation to update without an explicit sleep.

What methods would you recommend other than a while loop? Most won't work due to the loss of precision between the AHRS and simulation communication

@Baconing, thank you for the clarification. Given the loss of precision between the AHRS and simulation communication, I understand that synchronization can be challenging.

One alternative to Thread.sleep() is to poll the simulation state until it updates, using a timeout to prevent indefinite waits. This can help reduce test flakiness and avoid unnecessary delays. Here's a possible approach:

private void waitForSimulationUpdate() { final long timeout = 1000; // Timeout in milliseconds long startTime = System.currentTimeMillis(); double initialYaw = yaw.get(); // 'yaw' is the SimDouble representing the yaw angle while (yaw.get() == initialYaw) { if (System.currentTimeMillis() - startTime > timeout) { throw new RuntimeException("Timed out waiting for simulation update."); } Thread.yield(); // Yield to prevent busy-waiting } }

This method waits until the yaw value changes, indicating that the simulation has updated. By using a timeout and yielding control, it minimizes potential flakiness and doesn't rely on arbitrary sleep durations.

Alternatively, if the simulation provides any hooks or callbacks upon data updates, you could leverage them to synchronize the test execution without active waiting.

If these options aren't feasible due to the precision constraints, and Thread.sleep() remains the most reliable method, documenting the reasoning in your code comments—as you've done—helps maintain clarity for future maintenance.

Let me know if this helps or if you'd like to explore other possibilities.

(_/)
(•‿•) Thinking...

Well, to start, you have to get the value from the actual AHRS object, not the simulation value, as we're waiting for updates on the AHRS. Secondly, if you add the AHRS as an instance variable and get its value, you also need to apply a delta to account for precision loss in communication between the simulator and AHRS object. You also need to redefine this function for each variable being updated.

@Baconing, thank you for the detailed explanation. I understand that due to the necessity of retrieving values from the actual AHRS object and accounting for precision loss in communication between the simulator and AHRS, alternative synchronization methods may not be practical. Using Thread.sleep() with proper documentation is acceptable in this context.

(_/)
(•◡•) Thanks for the explanation!

✏️ Learnings added

Learnt from: Baconing PR: frc5183/librobot#9 File: src/test/java/net/frc5183/librobot/hardware/gyro/NavXTests.java:585-591 Timestamp: 2024-10-14T17:22:11.266Z Learning: In unit tests for `NavXTests.java` in `net.frc5183.librobot.hardware.gyro`, when waiting for updates on the `AHRS` object during simulations, it's necessary to use `Thread.sleep()` because values must be obtained from the actual `AHRS` object, not the simulation values, and precision loss in communication must be accounted for.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

coderabbitai · 2024-10-14T16:33:56Z

src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16448Tests.java

+
+ sim.setGyroAngleZ(TEST_ANGLE_3);
+ assertEquals(TEST_ANGLE_3, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.YAW)), DELTA);
+
+ sim.setGyroAngleZ(TEST_ANGLE_4);
+ assertEquals(TEST_ANGLE_4, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.YAW)), DELTA);
+
+ sim.setGyroAngleZ(TEST_ANGLE_5);
+ assertEquals(TEST_ANGLE_5, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.YAW)), DELTA);
+ }
+
+ @Test
+ void testAnglePitch() {
+ sim.setGyroAngleX(TEST_ANGLE_1);
+ assertEquals(TEST_ANGLE_1, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.PITCH)), DELTA);
+
+ sim.setGyroAngleX(TEST_ANGLE_2);
+ assertEquals(TEST_ANGLE_2, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.PITCH)), DELTA);
+
+ sim.setGyroAngleX(TEST_ANGLE_3);
+ assertEquals(TEST_ANGLE_3, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.PITCH)), DELTA);
+
+ sim.setGyroAngleX(TEST_ANGLE_4);
+ assertEquals(TEST_ANGLE_4, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.PITCH)), DELTA);
+
+ sim.setGyroAngleX(TEST_ANGLE_5);
+ assertEquals(TEST_ANGLE_5, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.PITCH)), DELTA);
+ }
+
+ @Test
+ void testAngleRoll() {
+ sim.setGyroAngleY(TEST_ANGLE_1);
+ assertEquals(TEST_ANGLE_1, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.ROLL)), DELTA);
+
+ sim.setGyroAngleY(TEST_ANGLE_2);
+ assertEquals(TEST_ANGLE_2, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.ROLL)), DELTA);
+
+ sim.setGyroAngleY(TEST_ANGLE_3);
+ assertEquals(TEST_ANGLE_3, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.ROLL)), DELTA);
+
+ sim.setGyroAngleY(TEST_ANGLE_4);
+ assertEquals(TEST_ANGLE_4, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.ROLL)), DELTA);
+
+ sim.setGyroAngleY(TEST_ANGLE_5);
+ assertEquals(TEST_ANGLE_5, Math.toDegrees(imu.getAngleRadians(IMU.Attitude.ROLL)), DELTA);
+ }
+
+ @Test
+ void testAngleCartesianX() {
+ sim.setGyroAngleX(TEST_ANGLE_1);
+ assertEquals(TEST_ANGLE_1, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.X)), DELTA);
+
+ sim.setGyroAngleX(TEST_ANGLE_2);
+ assertEquals(TEST_ANGLE_2, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.X)), DELTA);
+
+ sim.setGyroAngleX(TEST_ANGLE_3);
+ assertEquals(TEST_ANGLE_3, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.X)), DELTA);
+
+ sim.setGyroAngleX(TEST_ANGLE_4);
+ assertEquals(TEST_ANGLE_4, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.X)), DELTA);
+
+ sim.setGyroAngleX(TEST_ANGLE_5);
+ assertEquals(TEST_ANGLE_5, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.X)), DELTA);
+ }
+
+ @Test
+ void testAngleCartesianY() {
+ sim.setGyroAngleY(TEST_ANGLE_1);
+ assertEquals(TEST_ANGLE_1, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Y)), DELTA);
+
+ sim.setGyroAngleY(TEST_ANGLE_2);
+ assertEquals(TEST_ANGLE_2, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Y)), DELTA);
+
+ sim.setGyroAngleY(TEST_ANGLE_3);
+ assertEquals(TEST_ANGLE_3, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Y)), DELTA);
+
+ sim.setGyroAngleY(TEST_ANGLE_4);
+ assertEquals(TEST_ANGLE_4, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Y)), DELTA);
+
+ sim.setGyroAngleY(TEST_ANGLE_5);
+ assertEquals(TEST_ANGLE_5, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Y)), DELTA);
+ }
+
+ @Test
+ void testAngleCartesianZ() {
+ sim.setGyroAngleZ(TEST_ANGLE_1);
+ assertEquals(TEST_ANGLE_1, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Z)), DELTA);
+
+ sim.setGyroAngleZ(TEST_ANGLE_2);
+ assertEquals(TEST_ANGLE_2, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Z)), DELTA);
+
+ sim.setGyroAngleZ(TEST_ANGLE_3);
+ assertEquals(TEST_ANGLE_3, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Z)), DELTA);
+
+ sim.setGyroAngleZ(TEST_ANGLE_4);
+ assertEquals(TEST_ANGLE_4, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Z)), DELTA);
+
+ sim.setGyroAngleZ(TEST_ANGLE_5);
+ assertEquals(TEST_ANGLE_5, Math.toDegrees(imu.getAngleRadians(IMU.CartesianAxis.Z)), DELTA);
+ }


🛠️ Refactor suggestion

Refactor angle tests to reduce code duplication

The angle test methods (testAngleYaw, testAnglePitch, testAngleRoll, testAngleCartesianX, testAngleCartesianY, testAngleCartesianZ) share similar structures and repetitive code. Refactoring these tests using parameterized tests or helper methods can enhance maintainability and readability.

Suggestion:

Use JUnit 5's @ParameterizedTest along with @MethodSource or @ValueSource to run the same test logic with different parameters (e.g., axis and test angles).

Create a helper method that accepts the axis and expected values to eliminate repetitive code.

Example refactoring using @ParameterizedTest:

@ParameterizedTest @MethodSource("provideAnglesForTesting") void testAngle(IMU.Axis axis, Consumer<Double> setAngleFunction, Function<IMU, Double> getAngleFunction) { for (double testAngle : new double[]{TEST_ANGLE_1, TEST_ANGLE_2, TEST_ANGLE_3, TEST_ANGLE_4, TEST_ANGLE_5}) { setAngleFunction.accept(testAngle); assertEquals(testAngle, Math.toDegrees(getAngleFunction.apply(imu)), DELTA); } } static Stream<Arguments> provideAnglesForTesting() { return Stream.of( Arguments.of( IMU.CartesianAxis.X, (Consumer<Double>) sim::setGyroAngleX, (Function<IMU, Double>) imu -> imu.getAngleRadians(IMU.CartesianAxis.X) ), // Add entries for other axes ); }

coderabbitai · 2024-10-14T16:33:56Z

src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16448Tests.java

+
+ sim.setGyroRateX(TEST_ANGLE_3/2);
+ assertEquals(TEST_ANGLE_3/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.X), DELTA);
+
+ sim.setGyroRateX(TEST_ANGLE_4/2);
+ assertEquals(TEST_ANGLE_4/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.X), DELTA);
+
+ sim.setGyroRateX(TEST_ANGLE_5/2);
+ assertEquals(TEST_ANGLE_5/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.X), DELTA);
+ }
+
+ @Test
+ void testRateCartesianY() {
+ sim.setGyroRateY(TEST_ANGLE_1/2);
+ assertEquals(TEST_ANGLE_1/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Y), DELTA);
+
+ sim.setGyroRateY(TEST_ANGLE_2/2);
+ assertEquals(TEST_ANGLE_2/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Y), DELTA);
+
+ sim.setGyroRateY(TEST_ANGLE_3/2);
+ assertEquals(TEST_ANGLE_3/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Y), DELTA);
+
+ sim.setGyroRateY(TEST_ANGLE_4/2);
+ assertEquals(TEST_ANGLE_4/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Y), DELTA);
+
+ sim.setGyroRateY(TEST_ANGLE_5/2);
+ assertEquals(TEST_ANGLE_5/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Y), DELTA);
+ }
+
+ @Test
+ void testRateCartesianZ() {
+ sim.setGyroRateZ(TEST_ANGLE_1/2);
+ assertEquals(TEST_ANGLE_1/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Z), DELTA);
+
+ sim.setGyroRateZ(TEST_ANGLE_2/2);
+ assertEquals(TEST_ANGLE_2/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Z), DELTA);
+
+ sim.setGyroRateZ(TEST_ANGLE_3/2);
+ assertEquals(TEST_ANGLE_3/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Z), DELTA);
+
+ sim.setGyroRateZ(TEST_ANGLE_4/2);
+ assertEquals(TEST_ANGLE_4/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Z), DELTA);
+
+ sim.setGyroRateZ(TEST_ANGLE_5/2);
+ assertEquals(TEST_ANGLE_5/2, imu.getRateDegreesPerSecond(IMU.CartesianAxis.Z), DELTA);
+ }
+
+ @Test
+ void testRateYaw() {
+ sim.setGyroRateZ(TEST_ANGLE_1/2);
+ assertEquals(TEST_ANGLE_1/2, imu.getRateDegreesPerSecond(IMU.Attitude.YAW), DELTA);
+
+ sim.setGyroRateZ(TEST_ANGLE_2/2);
+ assertEquals(TEST_ANGLE_2/2, imu.getRateDegreesPerSecond(IMU.Attitude.YAW), DELTA);
+
+ sim.setGyroRateZ(TEST_ANGLE_3/2);
+ assertEquals(TEST_ANGLE_3/2, imu.getRateDegreesPerSecond(IMU.Attitude.YAW), DELTA);
+
+ sim.setGyroRateZ(TEST_ANGLE_4/2);
+ assertEquals(TEST_ANGLE_4/2, imu.getRateDegreesPerSecond(IMU.Attitude.YAW), DELTA);
+
+ sim.setGyroRateZ(TEST_ANGLE_5/2);
+ assertEquals(TEST_ANGLE_5/2, imu.getRateDegreesPerSecond(IMU.Attitude.YAW), DELTA);
+ }
+
+ @Test
+ void testRatePitch() {
+ sim.setGyroRateX(TEST_ANGLE_1/2);
+ assertEquals(TEST_ANGLE_1/2, imu.getRateDegreesPerSecond(IMU.Attitude.PITCH), DELTA);
+
+ sim.setGyroRateX(TEST_ANGLE_2/2);
+ assertEquals(TEST_ANGLE_2/2, imu.getRateDegreesPerSecond(IMU.Attitude.PITCH), DELTA);
+
+ sim.setGyroRateX(TEST_ANGLE_3/2);
+ assertEquals(TEST_ANGLE_3/2, imu.getRateDegreesPerSecond(IMU.Attitude.PITCH), DELTA);
+
+ sim.setGyroRateX(TEST_ANGLE_4/2);
+ assertEquals(TEST_ANGLE_4/2, imu.getRateDegreesPerSecond(IMU.Attitude.PITCH), DELTA);
+
+ sim.setGyroRateX(TEST_ANGLE_5/2);
+ assertEquals(TEST_ANGLE_5/2, imu.getRateDegreesPerSecond(IMU.Attitude.PITCH), DELTA);
+ }
+
+ @Test
+ void testRateRoll() {
+ sim.setGyroRateY(TEST_ANGLE_1 / 2);
+ assertEquals(TEST_ANGLE_1 / 2, imu.getRateDegreesPerSecond(IMU.Attitude.ROLL), DELTA);
+
+ sim.setGyroRateY(TEST_ANGLE_2 / 2);
+ assertEquals(TEST_ANGLE_2 / 2, imu.getRateDegreesPerSecond(IMU.Attitude.ROLL), DELTA);
+
+ sim.setGyroRateY(TEST_ANGLE_3 / 2);
+ assertEquals(TEST_ANGLE_3 / 2, imu.getRateDegreesPerSecond(IMU.Attitude.ROLL), DELTA);
+
+ sim.setGyroRateY(TEST_ANGLE_4 / 2);
+ assertEquals(TEST_ANGLE_4 / 2, imu.getRateDegreesPerSecond(IMU.Attitude.ROLL), DELTA);
+
+ sim.setGyroRateY(TEST_ANGLE_5 / 2);
+ assertEquals(TEST_ANGLE_5 / 2, imu.getRateDegreesPerSecond(IMU.Attitude.ROLL), DELTA);
+ }


🛠️ Refactor suggestion

Refactor rate tests to eliminate redundant code

The rate test methods (testRateCartesianX, testRateCartesianY, testRateCartesianZ, testRateYaw, testRatePitch, testRateRoll) contain repetitive patterns. Refactoring these tests using parameterized tests or helper methods will improve code efficiency and readability.

Suggestion:

Implement parameterized tests to iterate over different axes and test values.

Utilize helper methods to encapsulate common logic.

Example refactoring:

@ParameterizedTest @MethodSource("provideRatesForTesting") void testRate(IMU.Axis axis, Consumer<Double> setRateFunction, Function<IMU, Double> getRateFunction) { for (double testRate : new double[]{TEST_ANGLE_1 / 2, TEST_ANGLE_2 / 2, TEST_ANGLE_3 / 2, TEST_ANGLE_4 / 2, TEST_ANGLE_5 / 2}) { setRateFunction.accept(testRate); assertEquals(testRate, getRateFunction.apply(imu), DELTA); } } static Stream<Arguments> provideRatesForTesting() { return Stream.of( Arguments.of( IMU.CartesianAxis.X, (Consumer<Double>) sim::setGyroRateX, (Function<IMU, Double>) axis -> imu.getRateDegreesPerSecond(IMU.CartesianAxis.X) ), // Add entries for other axes ); }

src/test/java/net/frc5183/librobot/hardware/gyro/ADIS16448Tests.java

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e6b51db and 2de1188.

📒 Files selected for processing (1)

src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java (1 hunks)

🧰 Additional context used

coderabbitai · 2024-10-15T13:47:15Z

src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java

+ Map<CartesianAxis, Double> offset = Map.of(
+ CartesianAxis.X, offsetX,
+ CartesianAxis.Y, offsetY,
+ CartesianAxis.Z, offsetZ
+ );


⚠️ Potential issue

Avoid variable shadowing of offset

In the getOffset() method, the local variable offset shadows the class field offset, which can lead to confusion and potential bugs. Renaming the local variable will improve code clarity.

Apply this diff to rename the local variable:

public Rotation3d getOffset() { if (offset != null) return offset; - Map<CartesianAxis, Double> offset = Map.of( + Map<CartesianAxis, Double> offsetMap = Map.of( CartesianAxis.X, offsetX, CartesianAxis.Y, offsetY, CartesianAxis.Z, offsetZ ); - return new Rotation3d( - offset.get(getYawAxis()), - offset.get(getPitchAxis()), - offset.get(getRollAxis()) + return new Rotation3d( + offsetMap.get(getYawAxis()), + offsetMap.get(getPitchAxis()), + offsetMap.get(getRollAxis()) ); }

📝 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

Map<CartesianAxis, Double> offset = Map.of(

CartesianAxis.X, offsetX,

CartesianAxis.Y, offsetY,

CartesianAxis.Z, offsetZ

);

Map<CartesianAxis, Double> offsetMap = Map.of(

CartesianAxis.X, offsetX,

CartesianAxis.Y, offsetY,

CartesianAxis.Z, offsetZ

);

coderabbitai · 2024-10-15T13:47:15Z

src/main/java/net/frc5183/librobot/hardware/gyro/IMU.java

+ return !inverted ? getRawRotation3dRadians().minus(getOffset()) : getRawRotation3dRadians().minus(getOffset()).unaryMinus();
+ }


🛠️ Refactor suggestion

Refactor getRotation3dRadians() for improved readability

The current implementation of getRotation3dRadians() uses a complex ternary operation, which can reduce readability and maintainability. Refactoring the method can enhance clarity.

Apply this diff to simplify the method:

public Rotation3d getRotation3dRadians() { - return !inverted ? getRawRotation3dRadians().minus(getOffset()) : getRawRotation3dRadians().minus(getOffset()).unaryMinus(); + Rotation3d adjustedRotation = getRawRotation3dRadians().minus(getOffset()); + return inverted ? adjustedRotation.unaryMinus() : adjustedRotation; }

📝 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

return !inverted ? getRawRotation3dRadians().minus(getOffset()) : getRawRotation3dRadians().minus(getOffset()).unaryMinus();

}

public Rotation3d getRotation3dRadians() {

Rotation3d adjustedRotation = getRawRotation3dRadians().minus(getOffset());

return inverted ? adjustedRotation.unaryMinus() : adjustedRotation;

}

Baconing added 2 commits October 2, 2024 13:49

refactor(hardware/gyro): separate IMUs and single axis gyros

626bfd6

feat(hardware/gyro/imu): implement ADIS16448IMU and ADIS16470IMU

a699ec7

Baconing requested a review from Trip-kun October 2, 2024 17:53

Baconing self-assigned this Oct 2, 2024

Baconing added the Kind/Feature New functionality label Oct 2, 2024

coderabbitai bot reviewed Oct 2, 2024

View reviewed changes

Baconing added 7 commits October 3, 2024 08:12

refactor(hardware/gyro/imu): remove NotNull annotations from abstract…

f2445de

… methods

refactor(hardware/gyro/imu): use Translation3d instead of Rotation3d …

1b9402f

…for acceleration.

refactor(hardware/gyro/imu): use radians instead of degrees

045bb2c

feat/refactor(hardware/gyro/imu/adis16*): finish implementations

d180bee

style(hardware/gyro/imu/adis16470): remove unused import

4f91bc7

chore(build.gradle): add wpiunits dependency

a49c171

refactor(hardware/gyro/imu/adis16470): use unqualified name instead o…

2e5b57d

…f FQDN in switch statement

coderabbitai bot mentioned this pull request Oct 3, 2024

Discussion: Use Generics in Hardware Classes #10

Open

coderabbitai bot reviewed Oct 3, 2024

View reviewed changes

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java Outdated Show resolved Hide resolved

src/main/java/net/frc5183/librobot/hardware/gyro/imu/IMU.java Outdated Show resolved Hide resolved

coderabbitai bot reviewed Oct 3, 2024

View reviewed changes

Baconing added this to the Version: 2025-1.0.0 milestone Oct 3, 2024

refactor(hardware/gyro/imu): put unit in method name for IMU.getRotat…

3881104

…ion3d() and IMU.getRawRotation3d()

coderabbitai bot reviewed Oct 3, 2024

View reviewed changes

Baconing added 8 commits October 3, 2024 11:56

feat(hardware/gyro/imu): add getAngleRadians(SingleAxisGyroscope.Axis…

e26558e

…) method to IMU

Merge branch 'master' into v2025-1-0-0/gyros

2b1123a

refactor(hardware/imu/adis16448): use unqualified name instead of FQD…

a95c639

…N in switch statement

Merge remote-tracking branch 'origin/v2025-1-0-0/gyros' into v2025-1-…

a39601e

…0-0/gyros

Merge branch 'master' into v2025-1-0-0/gyros

77d2f72

Merge branch 'master' into v2025-1-0-0/gyros

fe3c8b6

Merge branch 'master' into v2025-1-0-0/gyros

6556a2a

Merge branch 'master' into v2025-1-0-0/gyros

8dd30a7

coderabbitai bot reviewed Oct 3, 2024

View reviewed changes

Merge branch 'master' into v2025-1-0-0/gyros

935a74f

Baconing added 6 commits October 3, 2024 12:43

Merge branch 'master' into v2025-1-0-0/gyros

426ea9c

Merge branch 'master' into v2025-1-0-0/gyros

d4743f9

Merge branch 'master' into v2025-1-0-0/gyros

0837d8d

Merge branch 'master' into v2025-1-0-0/gyros

73d6fd0

Merge branch 'master' into v2025-1-0-0/gyros

f48f6c7

feat/fix(hardware/gyro/imu/adis16*): implement getAngleRadians(Single…

648adab

…AxisGyroscope.Axis) and fix getRawRotation3dRadians() by using yaw pitch roll instead of XYZ

coderabbitai bot reviewed Oct 4, 2024

View reviewed changes

feat(hardware/gyro/imu): finish implementation of IMU and it's subcla…

28dfa6c

…sses

coderabbitai bot reviewed Oct 7, 2024

View reviewed changes

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java Outdated Show resolved Hide resolved

src/main/java/net/frc5183/librobot/hardware/gyro/imu/ADIS16448IMU.java Outdated Show resolved Hide resolved

refactor(hardware/gyro/imu): move yaw pitch roll axis assignment to a…

5722c6b

… utility method

coderabbitai bot reviewed Oct 7, 2024

View reviewed changes

Baconing added 4 commits October 7, 2024 12:41

refactor(hardware/gyro): remove SingleAxisGyroscope, just use IMU.

213fecf

all the functionality from SingleAxisGyroscope exists in IMU, and it just complicates things

feat(hardware/gyro): implement KauaiLabs NavX

898776e

docs(hardware/gyro/adis16*): update documentation to reflect changes

64eb036

Changes reflected: IMUAxis -> CartesianAxis SingleAxisGyroscope -> Attitude

style(hardware/gyro/imu): remove unnecessary qualifiers in IMU.Cartes…

9ba4834

…ianAxis.assignAxes(CartesianAxis yaw)

Baconing changed the title ~~WIP: feat(hardware/gyro): implement gyroscopes~~ feat(hardware/gyro): implement gyroscopes Oct 7, 2024

refactor(hardware/gyro): clean up some of the long methods

7108bbc

Trip-kun requested changes Oct 7, 2024

View reviewed changes

Baconing added 2 commits October 10, 2024 13:24

Merge branch 'master' into v2025-1-0-0/gyros

336dd89

Signed-off-by: Brenden Freier <[email protected]>

refactor(hardware/gyro/imu): only use Rotation3d where absolutely nec…

fa12c52

…essary keep my family's name out of your mouth, quaternions

coderabbitai bot reviewed Oct 14, 2024

View reviewed changes

Baconing added 4 commits October 14, 2024 09:19

test(hardware/gyro): implement tests for ADIS16448, ADIS16470, and de…

9669e48

…fault IMU methods. ADIS16470 acceleration tests are broken due to a [bug in wpilibj](wpilibsuite/allwpilib#7180).

fix(hardware/gyro): implement getRate() function

ed1a44c

fix(hardware/gyro): (re)implement angle rate logic

4e72ffa

test(hardware/gyro): finish implementing unit tests for all implement…

7ae15eb

…ed gyroscopes

coderabbitai bot reviewed Oct 14, 2024

View reviewed changes

Baconing added 2 commits October 14, 2024 12:34

fix(hardware/gyro/navx): use Gs not m/ss

e6b51db

docs(hardware/gyro): annotate deprecated members

2de1188

coderabbitai bot reviewed Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hardware/gyro): implement gyroscopes #9

feat(hardware/gyro): implement gyroscopes #9

Baconing commented Oct 2, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 2, 2024 •

edited

Loading

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

Trip-kun Oct 7, 2024

Trip-kun Oct 7, 2024

Trip-kun Oct 7, 2024

coderabbitai bot left a comment

coderabbitai bot Oct 14, 2024

coderabbitai bot Oct 14, 2024

coderabbitai bot left a comment

coderabbitai bot Oct 14, 2024 •

edited

Loading

Baconing Oct 14, 2024

coderabbitai bot Oct 14, 2024

Baconing Oct 14, 2024

coderabbitai bot Oct 14, 2024

coderabbitai bot Oct 14, 2024

coderabbitai bot Oct 14, 2024

coderabbitai bot left a comment

coderabbitai bot Oct 15, 2024

coderabbitai bot Oct 15, 2024

	default -> throw new IllegalArgumentException("ADIS16470_IMU Axis must be one of kYaw, kPitch, or kRoll. Was" + axis);
	default -> throw new IllegalArgumentException("ADIS16470_IMU Axis must be one of kYaw, kPitch, or kRoll. Was: " + axis);

		return !inverted ? getRawRotation3dRadians().minus(getOffset()) : getRawRotation3dRadians().minus(getOffset()).unaryMinus();
		}

- return !inverted ? getRawRotation3dRadians().minus(getOffset()) : getRawRotation3dRadians().minus(getOffset()).unaryMinus();
- }
+ public Rotation3d getRotation3dRadians() {
+ Rotation3d adjustedRotation = getRawRotation3dRadians().minus(getOffset());
+ return inverted ? adjustedRotation.unaryMinus() : adjustedRotation;
+ }

feat(hardware/gyro): implement gyroscopes #9

Are you sure you want to change the base?

feat(hardware/gyro): implement gyroscopes #9

Conversation

Baconing commented Oct 2, 2024 • edited by coderabbitai bot Loading

Summary by CodeRabbit

coderabbitai bot commented Oct 2, 2024 • edited Loading

Walkthrough

Changes

Poem

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

Trip-kun Oct 7, 2024

Choose a reason for hiding this comment

Trip-kun Oct 7, 2024

Choose a reason for hiding this comment

Trip-kun Oct 7, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Baconing Oct 14, 2024

Choose a reason for hiding this comment

coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

Baconing Oct 14, 2024

Choose a reason for hiding this comment

coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

Baconing commented Oct 2, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 2, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

coderabbitai bot Oct 14, 2024 •

edited

Loading