si5351: complete refactor for more complete interface #832
Conversation
deadprogram
force-pushed
the
si5351-refactored
branch
from
b03dbff to
fc08c37
Compare
|
|
deadprogram
force-pushed
the
si5351-refactored
branch
from
fc08c37 to
b5d4cf2
Compare
soypat
left a comment
soypat
left a comment
There was a problem hiding this comment.
Couple things I noticed to improve API
si5351/si5351.go
Outdated
| d.rw.Write8(CRYSTAL_INTERNAL_LOAD_CAPACITANCE, d.crystalLoad) | ||
| // Configure initializes the SI5351 with the specified crystal load capacitance, | ||
| // reference oscillator frequency, and frequency correction. | ||
| func (d *Device) Configure(xtalLoadC uint8, xoFreq uint32, correction int32) error { |
There was a problem hiding this comment.
Have you captured all configuration options here- or should we have a Config struct for future additions?
There was a problem hiding this comment.
Added Config type for consistency. Not sure if needed for expansion, but we're prepared just in case.
| // reference oscillator frequency, and frequency correction. | ||
| func (d *Device) Configure(xtalLoadC uint8, xoFreq uint32, correction int32) error { | ||
| // Check for device on bus | ||
| if err := d.bus.Tx(uint16(d.Address), []byte{}, []byte{0}); err != nil { |
There was a problem hiding this comment.
could use d.rw instead to ensure no allocation here, or even add a IsConnected method
There was a problem hiding this comment.
I do not think it allocates here.
si5351/si5351.go
Outdated
| } | ||
|
|
||
| // SetMultisynthSource sets the PLL source for a multisynth. | ||
| func (d *Device) SetMultisynthSource(clk uint8, pll uint8) error { |
There was a problem hiding this comment.
pll is a an enum- maybe define enums for all these types to make API usage simpler ?
There was a problem hiding this comment.
I added a number of types for clarify and safety. Good call!
si5351/si5351.go
Outdated
| } | ||
|
|
||
| // EnableOutput enables or disables a clock output. | ||
| func (d *Device) EnableOutput(clk uint8, enable bool) error { |
There was a problem hiding this comment.
clk seems to also be an enum
si5351/si5351.go
Outdated
| case pll == PLL_B && !d.pllbConfigured: | ||
| // Clock source options | ||
| const ( | ||
| ClockSourceXTAL = iota |
There was a problem hiding this comment.
Make this a ClockSource type?
si5351/si5351.go
Outdated
|
|
||
| // Clock output identifiers | ||
| const ( | ||
| Clock0 uint8 = iota |
si5351/si5351.go
Outdated
|
|
||
| // Reference oscillator identifiers | ||
| const ( | ||
| PLLInputXO = iota |
deadprogram
force-pushed
the
si5351-refactored
branch
2 times, most recently
from
db6fe38 to
d2eb60c
Compare
This completely refactors the interface and implementation for the si5351 clock generator. The interface based on the Arduino implementation was both somewhat hard to work with and also missing a number of important features that are needed to use this chip for RF communication. Instead this new implementation draws inspiration from the efforts of the Traquino community mostly using the rp2040 processor. The TinyGo implementation is based on the patterns and code in the drivers repo for other i2c devices. It also includes some basic unit tests which are not comprehensive but at least provide some coverage. Signed-off-by: deadprogram <ron@hybridgroup.com>
soypat
left a comment
soypat
left a comment
There was a problem hiding this comment.
Awesome stuff! thanks for the contribution ron1
deadprogram
force-pushed
the
si5351-refactored
branch
from
d2eb60c to
c3a56b7
Compare
|
Made one last change to the naming on the config fields for clarity. |
|
Thanks for all the review/feedback @soypat now merging. |
This PR completely refactors the interface and implementation for the
si5351clock generator. The interface based on the Arduino implementation was both somewhat hard to work with and also missing a number of important features that are needed to use this chip for RF communication.Instead this new implementation draws inspiration from the efforts of the Traquito community. They work entirely with the rp2040 processor.
However, the actual implementation in this PR is based on the patterns and code in the TinyGo drivers repo for other i2c devices. It also includes some basic unit tests which are not comprehensive but at least provide some coverage. Of course this implementation is not tied to any particular target platform.