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 I2C master mode (bypass disabled) #41

Open
pkb-pmj opened this issue Aug 17, 2023 · 1 comment
Open

Support I2C master mode (bypass disabled) #41

pkb-pmj opened this issue Aug 17, 2023 · 1 comment

Comments

@pkb-pmj
Copy link

pkb-pmj commented Aug 17, 2023

Currently the magnetometer can only be used in bypass mode. This means you can't use two MPUs on the same bus - even though you can change the address of one of them using AD0, their magnetometers still have the same address. Learned this the hard and long way.

The InvenSense driver (which I finally found inside a SparkFun library, lol) does this by setting the magnetometer to single read mode (not continuous 8 Hz or 100Hz), and using SLV0 to read, and SLV1 to trigger the next measurement. I guess it should be possible to do it using only SLV0 and continuous read mode, but I couldn't make it work and didn't need to, because the InvenSense solution already runs at slightly over 100 Hz.

Now that requires some work:

  • Slave configuration (why are SLV1-3 registers missing..?)
  • Bypass toggle
  • Configurable magnetometer read mode (not hardcoded)
  • Magnetometer read function (for setup the current magnetometer setup function should be enough)
  • Probably a new type to differentiate it from the current bypass mode? (Marg)
  • Setup functions will only work in bypass mode, and reading functions in master mode, so some way to differentiate that
    Maybe like this?
// types.rs

pub(crate) trait Marg {}
impl<T: Marg> MpuMode for T {}

pub struct MargBypass;
impl Marg for MargBypass {}

pub struct MargMaster;
impl Marg for MargMaster {}
// lib.rs
// omitted all the I2C-related type parameters for readability

impl<Mode: Marg> Mpu9250<Mode> {
    // most of the current functions except for magnetometer reading
}

impl Mpu9250<MargBypass> {
    pub fn disable_bypass(self) -> Mpu9250<MargMaster>;
    //  the current magnetometer reading function
}

impl Mpu9250<MargMaster> {
    pub fn enable_bypass(self) -> Mpu9250<Marg>;
    // a special function for reading magnetometer from the configured SLV register
    // and/or a generic function for reading any SLV register
}
  • A different implementation of NineDOFDevice - it can't be implemented for I2cDevice, because it depends on bypass state, which I2cDevice doesn't know about

What do you think?

@pkb-pmj
Copy link
Author

pkb-pmj commented Aug 17, 2023

It's even more complicated lol
With two MPUs on one bus, you must set one of them into master mode before configuring magnetometers. With how it currently works + what I came up with above, funny things happen:

let mut mpu1 = Mpu9250::new_marg(...);
let mut mpu2 = Mpu9250::new_marg(...);
// And now you realize that inside Mpu9250::new_marg, AK8963::init enables bypass,
// which means we've tried initializing two magnetometers at the same time twice,
// leaving them in unknown state.
// That's not good.

// Let's try again
let mut mpu1 = Mpu9250::new_marg(...);
// First we should make mpu1 magnetometer unreachable
let mut mpu1 = mpu1.disable_bypass();
// And now we can properly initalize mpu2
let mut mpu2 = Mpu9250::new_marg(...);
// Disconnect mpu2 magnetometer
let mut mpu2 = mpu2.disable_bypass();
// Note that we don't know if mpu2 was set to bypass when we initialized mpu1
// so we don't know if it initialized properly
let mut mpu1 = mpu1.enable_bypass();
// Initialize mpu1 again, maybe there is a better way than re-defining it
let mut mpu1 = Mpu9250::new_marg(...);
// And finally disable bypass
let mut mpu1 = mpu1.disable_bypass();
// And now we have two properly configured MPUs

To prevent that, MPU and magnetometer initialization probably would have to be separate functions, which requires significant changes to the API 🤔
And then it's up to the user to sequence everything properly for a single MPU, because it can't be controlled from inside one MPU when you have two. Not good.

Also, maybe I should first make magnetometer mode configurable in a separate PR, that's a much easier change

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

1 participant