juliangaal / mpu6050

MPU6050 embedded-hal driver written in Rust
https://crates.io/crates/mpu6050
MIT License
50 stars 31 forks source link

MPU6886 suppoort #54

Open tana opened 1 year ago

tana commented 1 year ago

Summary

This PR allows the crate work for MPU6886 sensor, which is commonly found in M5Stack products.

Details

MPU6886 is almost similar to MPU6050 except lack of DMP and HPF, slight difference in motion detection interrupt, etc. Registers for basic functions (just reading acceleration and angular rate) are identical.

The main problem was difference of value of WHOAMI register. It caused unmodified library to fail during initialization.

This PR allows changing expected WHOAMI value through new type parameter of Mpu6050 struct, and adds Mpu6886 type alias for convenience.

Concerns

This PR uses const generics, which is added in Rust 1.5.1. This may contradict current Rust Edition 2018 setting. Also, I couldn't run the Linux test code, because I didn't have MPU6886 sensor breakout board usable from Linux.

tana commented 1 year ago

Thank you for reviewing my PR, but I don't think my code is wrong. Because of backward compatibility, Mpu6050<S> should be alias of Mpu6050<S, DEFAULT_SLAVE_ADDR>, which is for MPU-6050 sensors. If your suggested change is used, Mpu6050<S> will expect WHOAMI_VALUE_MPU6886 and fail with MPU-6050.

By the way, I'm going to add the missing newline.

rursprung commented 1 year ago

Thank you for reviewing my PR, but I don't think my code is wrong. Because of backward compatibility, Mpu6050<S> should be alias of Mpu6050<S, DEFAULT_SLAVE_ADDR>, which is for MPU-6050 sensors. If your suggested change is used, Mpu6050<S> will expect WHOAMI_VALUE_MPU6886 and fail with MPU-6050.

yeah, sorry, scratch my comment - i totally mis-read that... i somehow expected WHOAMI and the I2C address to be two different values (i have seen other devices which have similar WHOAMI APIs but then return some other values) and thus just read "oh, you replaced the WHOAMI with an I2C address" and missed the "MPU6886" part in it. my fault!

juliangaal commented 1 year ago

sorry for not reacting at all. Totally missed this PR. I will check it out when I handle #59 and #56

MPU6886 is almost similar to MPU6050 except lack of DMP and HPF, slight difference in motion detection interrupt, etc. Registers for basic functions (just reading acceleration and angular rate) are identical.

Do you have a suggestion on how to handle this difference?

tana commented 1 year ago

I think it is possible to handle the difference of 6886 and 6050 using traits, like

trait Mpu6050Specific { ... }
trait Mpu6886Specific { ... }

impl<I> Mpu6050Specific for Mpu6050<I> { ... }
impl<I> Mpu6886Specific for Mpu6886<I> { ... }