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

i2c: refactor all i2c drivers to use Address property of Driver. #30

Merged
merged 2 commits into from Apr 7, 2019

Conversation

deadprogram
Copy link
Member

This PR refactors all current i2c drivers to use Address property of Driver. It allows variations of a driver to override the default address as discussed in #14

…ws variations to override the default address as discussed in #14

Signed-off-by: Ron Evans <ron@hybridgroup.com>
Signed-off-by: Ron Evans <ron@hybridgroup.com>
@deadprogram
Copy link
Member Author

Conflicts resolved, can now be squashed and merged.

@deadprogram
Copy link
Member Author

This branch should be ready for review and then squash and merge.

@deadprogram
Copy link
Member Author

Please review me then squash/merge.

@@ -9,46 +9,52 @@ import (

// Device wraps an I2C connection to a BlinkM device.
type Device struct {
bus machine.I2C
bus machine.I2C
Address uint16
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using uint16 here?
I haven't yet seen an I2C device in the wild that uses 10 bit addressing. This Device struct is also driver specific so there is no shared interface to support here. I think it can be a uint8. This will also avoid having to replace ReadRegister with Tx.

(This comment also applies to all other drivers).

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I replaced ReadRegister is actually because it creates a memory leak on AVR, since the allocated slice is never freed.

Copy link
Member

@aykevl aykevl Mar 24, 2019

Choose a reason for hiding this comment

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

I have looked into this a bit, but switching to Tx does not seem to improve the situation. I can still see a runtime.alloc call in the bh1750 driver after this change, for example. Can you give a code example where switching to Tx avoids a heap allocation?

After some investigation, I think it's because the built-in escape analysis of LLVM is too conservative for our purposes:
https://llvm.org/doxygen/CaptureTracking_8cpp_source.html#L332
In safe Go, there is no crazy pointer arithmetic possible like in C. Therefore, a comparison against nil can never be used to let a pointer escape. I'm not sure what the best way is to fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for Address uint16 is because the i2c interface signature is Tx(addr uint16, w, r []byte) error.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Okay, looks good to me. But it looks like there are merge conflicts now.

@deadprogram
Copy link
Member Author

Squashing and merging.

@deadprogram deadprogram merged commit d9b426b into master Apr 7, 2019
@deadprogram deadprogram deleted the features/config-i2c-address branch April 7, 2019 08:50
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.

None yet

2 participants