Added si5351 driver #474
Added si5351 driver #474
Conversation
|
Hello @conotto thank you for working on this driver. I have changed the branch to |
|
Please let me know if anything is missing |
Co-authored-by: Nils Stinnesbeck <[email protected]>
|
Is there anything else required ? Or can this be merged already ? |
| lastRdivValue [3]uint8 | ||
| } | ||
|
|
||
| var errNotInitialised = errors.New("Si5351 not initialised") |
There was a problem hiding this comment.
I believe that nowadays, the practice is to make these exported:
var Err...
|
|
||
| // Configure sets up the device for communication | ||
| // TODO error handling | ||
| func (d *Device) Configure() { |
There was a problem hiding this comment.
I'm surprised that Configure does not have an error return
|
|
||
| // Disable all outputs setting CLKx_DIS high | ||
| data[0] = 0xFF | ||
| d.bus.WriteRegister(d.Address, OUTPUT_ENABLE_CONTROL, data) |
There was a problem hiding this comment.
Can this get an error? I'm guessing yes?
|
|
||
| // Connected returns whether a device at SI5351 address has been found. | ||
| func (d *Device) Connected() bool { | ||
| err := d.bus.Tx(uint16(d.Address), []byte{}, []byte{0}) |
There was a problem hiding this comment.
Can Tx get an error that is not just about things not being found? It seems a bit dangerous to throw the error away, or is that the interface definition? I"m still catching up.
|
|
||
| func (d *Device) EnableSpreadSpectrum() (err error) { | ||
| data := d.buf[:1] | ||
| err = d.bus.ReadRegister(d.Address, SPREAD_SPECTRUM_PARAMETERS, data) |
There was a problem hiding this comment.
The more common practice, at least at google, would be an unnamed error return and then
if err := d.bus.ReadRegister(...); err != nil {
return err
}
...
return d.bus.WriteRegister
it avoids any possible problems around shadowing (which has caused trouble in practice)
| // Reset both PLLs | ||
| data = d.buf[:1] | ||
| data[0] = (1 << 7) | (1 << 5) | ||
| d.bus.WriteRegister(d.Address, PLL_RESET, data) |
There was a problem hiding this comment.
can this error? Do you want to return an error if it does?
| } | ||
| // Channel range | ||
| if !(output < 3) { | ||
| return errInvalidParameter |
There was a problem hiding this comment.
here's another case where more informative errors might be useful, viz:
return fmt.Errorf("output %d is < 3:%w", ErrInvalidParameter)
| switch output { | ||
| case 0: | ||
| baseaddr = MULTISYNTH0_PARAMETERS_1 | ||
| break |
There was a problem hiding this comment.
you don't need these breaks. That's only an issue in C.
| data[6] = uint8((p2 & 0xFF00) >> 8) | ||
| data[7] = uint8(p2 & 0xFF) | ||
| err = d.bus.WriteRegister(d.Address, baseaddr, data) | ||
| if err != nil { |
There was a problem hiding this comment.
again, strongly recommend the practice:
if err := d.bus.Write(...); err != nil {
return nil
}
avoid named return parameters is usual.
| switch output { | ||
| case 0: | ||
| register = CLK0_CONTROL | ||
| break |
There was a problem hiding this comment.
don't need the breaks.
btw, golangci-lint would likely make useful suggestions.
Driver for Si5351 clock module