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

Support for more MPU devices #4

Open
fmckeogh opened this issue Feb 7, 2019 · 3 comments
Open

Support for more MPU devices #4

fmckeogh opened this issue Feb 7, 2019 · 3 comments

Comments

@fmckeogh
Copy link

fmckeogh commented Feb 7, 2019

I've been working on a fork of this repo that adds support for the MPU6050 and MPU6000. This means I have had to add an Interface trait implemented by I2C and SPI so that the library can work with both. However, I am unsure of how clear this is, especially since modules are either one interface or the other. As an example, let mpu6050 = Mpu9250::imu_default(i2c, delay).unwrap(); just doesn't seem very nice to me, maybe let mpu6050 = MpuX::imu_default(i2c, delay).unwrap();? The problem is that internally, there is lots of shared behaviour which would be nice not to duplicate.

What are your thoughts on this? Is it something you would like to see merged, or should I develop it as its own crate?

@little-arhat
Copy link
Member

@Chocol4te thanks for your interest in this crate, we would definitely love to see fixes and improvements.

Adding i2c would mean major version bump, so changing top level name is ok.

As for sharing functionality, I guess it make sense to create Transport struct/trait that would take care of writing/reading data and then use that object in MpuX.

@mciantyre
Copy link
Contributor

Semi-related: I’ve been adding I2C support for the current family of MPU sensors in my fork. I generalized the existing implementation on a Device trait, which is similar to the Interface trait described above. Then, I added functions to construct an I2C-based sensor that are behind an ”i2c” Cargo feature-flag. The end result is an internal implementation that supports both SPI and I2C with an unchanged public API, so we don’t necessarily need a major version bump right away. Users could opt-in to I2C support as desired.

My use-case is MPU support on the BeagleBone Blue, which has an MPU9250 on I2C2. I’d love to collaborate on the shared SPI/I2C approaches!

@little-arhat
Copy link
Member

@mciantyre that sounds awesome!

indeed, we can publish minor version bump with feature flag for i2c, and then remove it, as it doesn't seem too hard — we just need to provide ergonomic constructors.

Feel free to send pull request, would love to merge your changes and build on top!

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

No branches or pull requests

3 participants