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

Fix quaternion mapping. #26

Merged
merged 1 commit into from
Jan 28, 2024
Merged

Fix quaternion mapping. #26

merged 1 commit into from
Jan 28, 2024

Conversation

alexbohm
Copy link
Contributor

@alexbohm alexbohm commented Oct 9, 2023

I'm using the quaternion output from the sensor with an esp32c6 and I noticed that the order of the data is wrong.

If I'm reading the mint docs correctly, the order should be x, y, z, w if building the Quat from an array. I switched to explicitly constructing the Quat so it's clearer how the fields are mapped.

Tested with hardware and now the sensor is reacting how I would expect.

I also had to disable the default features for the num-traits crate so that it would compile for a no_std environment.

Cargo.toml Outdated
@@ -22,7 +22,7 @@ byteorder = { version = "1", default-features = false }
serde = { version = "1.0", default-features = false, features = ["derive"], optional = true }
mint = "^0.5.4"
bitflags = "1"
num-traits = "0.2.15"
num-traits = { version = "0.2.15", default-features = false}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to enable libm feature too as it will enable f32 and f64 functions

Copy link
Contributor

Choose a reason for hiding this comment

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

See #27

Comment on lines +333 to +345
let x = x as f32 * scale;
let y = y as f32 * scale;
let z = z as f32 * scale;
let w = w as f32 * scale;

let quat = mint::Quaternion {
v: mint::Vector3 { x, y, z },
s: w,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, one of my interns also pointed this issue

@alexbohm
Copy link
Contributor Author

Thanks for taking a look!
Looks like the libm feature in #28 is exactly what I would do to update this pr.

Not sure what the workflow is but feel free to edit this pr as needed, otherwise after work I can make some changes.

@elpiel
Copy link
Contributor

elpiel commented Jan 23, 2024

@alexbohm I did open a new PR for it along side a few other to do various fixes, feature, etc.
I would love if @eupn has the capacity to take a look, otherwise I'm interested in taking maintainer status and eventually moving the repo to @AeroRust

@alexbohm alexbohm force-pushed the bugfix/quat-mapping branch from f5fa3d3 to 39ddabe Compare January 24, 2024 00:27
@alexbohm alexbohm changed the title Fix quaternion mapping and allow no_std with num_traits. Fix quaternion mapping. Jan 24, 2024
@alexbohm
Copy link
Contributor Author

I decided to just let #28 take care of the no default features needed for no_std.

@elpiel
Copy link
Contributor

elpiel commented Jan 24, 2024

I decided to just let #28 take care of the no default features needed for no_std.

Awesome! Can't want to try out the fix for the Quaternions!

@eupn eupn merged commit b05d476 into eupn:master Jan 28, 2024
1 check failed
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