Skip to content

Conversation

@tchamelot
Copy link
Collaborator

Hey, this is a proposition to improve the builder pattern. It does not break the previous building method.

Now, one is able to build an instance with

let mut mpu9250 = MpuConfig::dmp().dmp_rate(DmpRate::_20Hz).build(i2c);
mpu9250.init(&mut Delay, &dmp_firmware).unwrap();

or

let mut mpu9250 = MpuConfig::imu().build(spi, ncs);
mpu9250.init(&mut Delay).unwrap();

If you have any suggestion, I will be glad to ear about it!

Copy link
Member

@little-arhat little-arhat left a comment

Choose a reason for hiding this comment

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

LGTM!

It would be nice to maybe update dmp example to show how to use new builders.

FYI, this branch increases size of a mpu code, compiled in release mode by 100bytes (according to cargo bloat):
with status quo version:
0.1% 1.3% 538B mpu9250

with this branch:
0.1% 1.6% 686B mpu9250

Not a huge increase, but maybe worth tracking that down to confirm increase is coming from useful functions.

Thanks!


#[cfg(not(feature = "i2c"))]
/// Build new Mpu9250 from current configuration using the provided device
pub fn build<E, SPI, NCS>(self,
Copy link
Member

Choose a reason for hiding this comment

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

For SPI connected devices we also provide *_with_reinit family of constructors. Although they are a bit ugly, they serve practical need -- maybe worth adding support for reinit to builders?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you give me an example of how you would use *with_reinit() so I can try to design something ergonomic.

@tchamelot
Copy link
Collaborator Author

FYI, this branch increases size of a mpu code, compiled in release mode by 100bytes (according to cargo bloat):
with status quo version:
0.1% 1.3% 538B mpu9250

with this branch:
0.1% 1.6% 686B mpu9250

Not a huge increase, but maybe worth tracking that down to confirm increase is coming from useful functions.

I did not know about cargo bloat, so i gave it a try on the rpi example.

master builder legacy init builder new init
0.2% 2.0% 4.8KiB mpu9250 0.2% 2.0% 4.8KiB mpu9250 0.2% 2.0% 4.9KiB mpu9250

First of all, the size of the mpu9250 crate is much bigger in my case. I do not see any change between this branch and the master when using the legacy init system. However, there is a 0.1KiB increase when using the new init system based on the builder pattern.

I can add it in the readme or in the doc if you think that such a small amount of memory should be documented.

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.

2 participants