-
Notifications
You must be signed in to change notification settings - Fork 230
si5351: add support for si5351 #810
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
Conversation
si5351/si5351.go
Outdated
| "math" | ||
|
|
||
| "tinygo.org/x/drivers" | ||
| "tinygo.org/x/drivers/internal/legacy" |
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.
This is known to cause heap allocations, shall we try and avoid this from start?
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.
Good point @ysoldak I just added another commit to use the regmap package instead.
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.
Updated again with the latest code from #801
Adds support for the si5351 I2C programmable clock generator using code from @chiefMarlin which used code from @conotto which somehow never got merged. Thank you everyone! Signed-off-by: deadprogram <[email protected]>
99907b5 to
2c55886
Compare
soypat
left a comment
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.
Very nice! Love to see regmap in use! FYI got a couple suggestions on how to avoid most if not all allocations
Signed-off-by: deadprogram <[email protected]>
2c55886 to
a48aaf0
Compare
|
Appears that there are no further heap allocations now. Should be ready to go. |
|
|
||
| // Power down all output drivers | ||
| buf := []byte{CLK0_CONTROL, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80} | ||
| d.bus.Tx(uint16(d.Address), buf, nil) |
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.
First real use of regmap in the wild!
I was trying to wrap my head around it and how could I use it in cases like #800 (as suggested).
I basically hit this same problem -- can't just use regmap functions everywhere, consistently, have to fall back to raw d.bus.Tx here and there => have to keep both rw and bus. And that sort of defies the purpose, to me at least...
We may be better off implementing helper methods for reading and writing variable size slices, like this, but in internal package somewhere to be reusable?
https://github.com/tinygo-org/drivers/blob/dev/lsm9ds1/lsm9ds1.go#L216
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 think that @soypat is working on something like that. In the meantime, we can add in the future if we find it is needed.
|
Any other feedback or can this get merged now? |
ysoldak
left a comment
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.
LGTM
|
Good stuff! Curious how the compiler is managing to not escape an array that is being passed into an interface. Very curious indeed |
Adds support for the si5351 I2C programmable clock generator using code from @chiefMarlin which used code from @conotto which somehow never got merged.
Thank you everyone!