Skip to content
This repository has been archived by the owner on Jun 19, 2024. It is now read-only.

feat(devices): V5 Inertial Sensor support #16

Merged
merged 25 commits into from
Jan 8, 2024

Conversation

Tropix126
Copy link
Member

@Tropix126 Tropix126 commented Dec 31, 2023

Adds high level bindings to the PROS IMU API through the InertialSensor struct. Currently untested on actual hardware.

Limitations:
- The fields on pros_sys::imu_raw_s appear to be private, making it impossible to implement the InertialSensor::gyro_rate and InertialSensor::accel without manually modifying the bindings. As such, i've left these methods commented out for now.

Let me know if any of the naming for things here seem off. (specifically, should data structs be InertialStruct or InertialSensorStruct?)

@Tropix126 Tropix126 marked this pull request as ready for review December 31, 2023 17:49
@Gavin-Niederman Gavin-Niederman self-assigned this Dec 31, 2023
@Gavin-Niederman
Copy link
Member

Your naming is perfectly fine as it is.

Copy link
Member

@Gavin-Niederman Gavin-Niederman left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! there are only 2 things that I would change.

pros/src/sensors/imu.rs Outdated Show resolved Hide resolved
pros/src/sensors/imu.rs Outdated Show resolved Hide resolved
@Gavin-Niederman Gavin-Niederman added the scope:bindings Modifies pros-sys bindings label Dec 31, 2023
@Gavin-Niederman
Copy link
Member

I think this is ready to merge unless lewis disagrees

@Gavin-Niederman
Copy link
Member

Thanks for doing these bindings, I was putting it off.

@Tropix126
Copy link
Member Author

Tropix126 commented Dec 31, 2023

Okay, seems we have a choice on how we want to handle calibration (currently we call the non-blocking function):
image

Any thoughts?

@Gavin-Niederman
Copy link
Member

Ah, I say we use the blocking function for the sync API and the non blocking one to create a future for the async API.

@Tropix126
Copy link
Member Author

Sounds good, will do.

@Tropix126
Copy link
Member Author

Tropix126 commented Jan 1, 2024

Any thoughts on bringing in a crate like mint which provides standard container types for other math-related crates to interop with?

  • Quaternion -> mint::Quaternion
  • Euler -> mint::EulerAngles
  • InertialRaw is essentially a mint::Vector3, serving as either an angular velocity vector with gyro_rate or an acceleration vector with accel.

Could also make it an optional feature and just provide conversion types rather than straight up returning them.

@Gavin-Niederman
Copy link
Member

I think that if we do that it should definitely be a feature. I also think that there are many crates that provide similar functionality to mint, such as nalgebra, so there should be discussion on which one we would use. For now I think we should continue without bringing in a library like that, but I am open for discussion about it later.

@Tropix126 Tropix126 changed the title feat: initial support for the V5 IMU feat(devices): V5 Inertial Sensor support Jan 5, 2024
Copy link
Member

@Gavin-Niederman Gavin-Niederman left a comment

Choose a reason for hiding this comment

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

After this and testing, it should be ready to merge.

pros/src/sensors/imu.rs Show resolved Hide resolved
pros/src/sensors/imu.rs Outdated Show resolved Hide resolved
Copy link
Member

@Gavin-Niederman Gavin-Niederman left a comment

Choose a reason for hiding this comment

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

Everything looks good! If you think that testing isn't necessary I will approve and merge.

@Tropix126
Copy link
Member Author

Everything looks good! If you think that testing isn't necessary I will approve and merge.

Still need to test the newer async stuff against the executor fix branch, which i'll try to get to tonight, tomorrow, or monday. Everything else has been tested to my knowledge.

@Gavin-Niederman
Copy link
Member

Alright sounds good. I hope we can get this merged soon.

Copy link
Member

@Gavin-Niederman Gavin-Niederman left a comment

Choose a reason for hiding this comment

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

LGTM (Lets Get That Motion)

@Gavin-Niederman Gavin-Niederman merged commit bb841bb into vexide:main Jan 8, 2024
3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
scope:bindings Modifies pros-sys bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants