-
Notifications
You must be signed in to change notification settings - Fork 192
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
Conversation
…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>
Conflicts resolved, can now be squashed and merged. |
This branch should be ready for review and then squash and merge. |
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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this 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.
Squashing and merging. |
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