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

Builder Pattern for MpuConfig #20

Open
tchamelot opened this issue Mar 25, 2020 · 3 comments
Open

Builder Pattern for MpuConfig #20

tchamelot opened this issue Mar 25, 2020 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@tchamelot
Copy link
Collaborator

Currently, MpuConfig<MODE> tries to follow the Builder Pattern. However, to fully match the pattern, MpuConfig<MODE> is missing a function like

impl MpuConfig<MODE> {
    pub fn consume(&self) -> Self {
        self
    }
}

The problem is that currently MpuConfig<MODE> has only default builders which return Self or setters which return &mut Self. That makes it really difficult to use. Would a refactor of the configuration be possible. My current idea would be to add:

impl MpuConfig<MODE> {
    pub fn build<Device>(&self,  dev: Device) -> Mpu9250<D, MODE> {
        Mpu9250 {
            dev,
            raw_mag_sensitivity_adjustments: [0; 3],
            mag_sensitivity_adjustments: [0.0; 3],
            gyro_scale: self.gyro_scale.unwrap_or_default(),
            accel_scale: self.accel_scale.unwrap_or_default(),
            mag_scale: self.mag_scale.unwrap_or_defaut(),
            accel_data_rate: self.accel_data_rate
                .unwrap_or_default(),
            gyro_temp_data_rate: self.gyro_temp_data_rate
                .unwrap_or_default(),
            sample_rate_divisor: self.sample_rate_divisor,
            _mode: PhantomData,
        }
    }
}

Actually, just that would pose a problem as the Device creation is now to be handled by the final user. But we could imagine having two build for I2C and SPI which create the Device.

Like for my previous issue, I am up for it.

@little-arhat
Copy link
Member

Hey, @tchamelot! I was thinking recently about something like this, though not sure if it's possible to achieve without breaking changes.

@tchamelot
Copy link
Collaborator Author

Yeah, I have seen this article on reddit, I don't know if everything he uses is stable or not (neither if all that this crate use is stable too as I work with nightly) but this would be a good idea.

@little-arhat
Copy link
Member

@tchamelot btw, "Like for my previous issue, I am up for it" — sure, I agree that current approach to configuration is not ideal and would love to improve it, so feel free to take a stab (either by improving builder pattern or by using "struct init" or maybe other approaches)! %)

@tchamelot tchamelot self-assigned this Jun 10, 2020
@tchamelot tchamelot added the enhancement New feature or request label Jun 10, 2020
tchamelot added a commit to tchamelot/mpu9250 that referenced this issue Jun 13, 2020
tchamelot added a commit to tchamelot/mpu9250 that referenced this issue Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants