search

sajattack / bitbang-hal

Implements embedded-hal traits by bitbanging
MIT License
41 stars 11 forks source link

expose low-level i2c control for not-quite-i2c devices #26

Closed agrif closed 5 months ago

agrif commented 5 months ago

Hello!

I'm trying to talk to a Beken BK1080 FM receiver. This chip claims to speak I2C, but it's not quite exactly right. It uses the same pins and the same signaling, but the order is slightly different from a normal I2C transaction. It can share the I2C bus with other, "real" I2C devices just fine.

This seems like exactly the right use-case for a bit-bang I2C implementation, but I needed to expose a few of the previously private methods on I2cBB to make it work. I've added documentation comments with a strong warning that these are low-level calls, and to prefer the embedded-hal traits.

I can understand you might be reluctant to expose these methods, but I think I2C is sufficiently messy and unstandardized that it's worth a modest API change to talk to these devices. Otherwise, I would have to copy and paste the bitbang-hal source into my crate. I'd rather make these changes here so everybody can benefit.

Please let me know if there are any changes I can make to get this merged.

sajattack commented 5 months ago

I think this makes sense. It's tough to balance hiding internals and letting endusers get things done. However I think @eldruin made most of these inline functions so I'd like to check his opinion as well.

sajattack commented 5 months ago

Perhaps we should prefix with underscores? Is that a rust-suitable convention or a python one? I'm not sure

eminence commented 5 months ago

If you really didn't want these functions to be widely known, maybe they could be public, but annotated with #[doc(hidden)] to prevent them from showing up in the docs?

agrif commented 5 months ago

I've never seen underscores used that way in rust. I did consider changing the names of the functions to raw_*, and I can still make that change easily.

I'd be uncomfortable hiding the documentation for them, though. If these changes had already been made, but hidden in the docs, I wouldn't have thought to go hunting for undocumented but still public functions.

eldruin commented 5 months ago

I think this use-case makes sense. I am concerned about people using it wrong as well, though. I would indeed add a raw_* prefix to the methods. (underscore prefixes are a python convention)

agrif commented 5 months ago

I've prefixed the exposed interfaces with raw_*. I believe both that and the bold warning should be effective deterrents.

I also think that most people using this crate will be coming at it from a different direction, anyway. As in, they have a specific device crate they want to use, which itself uses the embedded-hal traits.

agrif commented 5 months ago

Thanks for the quick merge! Now back to wrestling with this receiver IC.