Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wpilib] Refactor the ADIS IMU classes #6703

Closed
wants to merge 7 commits into from
Closed

[wpilib] Refactor the ADIS IMU classes #6703

wants to merge 7 commits into from

Conversation

Gold856
Copy link
Contributor

@Gold856 Gold856 commented Jun 5, 2024

Create acquire thread with method reference instead of Runnable class
Clean up comments and bad Javadoc (removed useless doc stubs, used single line comments instead of multi-line eveywhere, removed low value comments)
Clean up variable declarations (declared and assigned variables when needed instead of declaring upfront and assigning later, made needsFlash a local in Java)
Remove unused gyro data variables (mostly average gyro data in Java)
Remove and/or inline a bunch of the conversion functions
Make toInt in Java take four ints instead of varargs to reduce allocations
Remove unnecessary parentheses and casts
Use InputModulus or angleModulus and remove custom angle wrapping
Use hypotf instead of manual calculation
Remove a bunch of warning suppressions as they were either fixed or aren't needed
Call reset from calibrate in Java (change not done in C++ because I'm not sure how to do recursive locks)
Use array initialization syntax when possible and remove ByteBuffer usage

@Gold856 Gold856 requested a review from a team as a code owner June 5, 2024 05:49
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! (Side note- How had I not seen Java array initializers before? They've been around since JLS 3)

Comment on lines 652 to 653
// Check if frame is incomplete. Add 1 because of timestamp
int data_remainder = data_count % dataset_len;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could someone more knowledgable explain the "Add 1 because of timestamp" in more detail? (I don't see where 1 is being added) (Looks like @ThadHouse added this in #3777@0883e61)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Add 1 because of timestamp" seems to have been added in the original ADIS driver repo, but it has zero explanation, and the other original repo doesn't have it, so whatever it meant before clearly doesn't apply anymore.

wpilibc/src/main/native/cpp/ADIS16448_IMU.cpp Outdated Show resolved Hide resolved
@PeterJohnson
Copy link
Member

@juchong would you be willing to review?

@Gold856 Gold856 closed this by deleting the head repository Jun 9, 2024
@Gold856
Copy link
Contributor Author

Gold856 commented Jun 9, 2024

Now located at #6719.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants