Conversation
…h that they need fully separate implementations of the interface.
analog_reader.go
Outdated
| // A struct for handling the reading of knob voltage and position. | ||
| type Knob struct { | ||
| type knob struct { | ||
| machine.ADC |
There was a problem hiding this comment.
Careful with exposing a machine.ADC here. I'd unexport this field or switch to an interface. This will prevent from users depending on tinygo directly and make mock-tests possible.
There was a problem hiding this comment.
I'll ditto that remark - in my fork of the project I've been fighting against the lack of testability quite often because things are so intrinsically linked to machine and other TinyGo packages.
My main personal failing until now is that I've just been building runtime testing apparatus frameworks instead of just abstracting away the TinyGo linkages.... though those don't hurt to have around for final-pass QA, I guess.
analog_reader.go
Outdated
| // The analogue input allows you to 'read' CV from anywhere between 0 and 12V. | ||
| type AnalogInput struct { | ||
| type analogInput struct { | ||
| machine.ADC |
There was a problem hiding this comment.
Same as below, unexport or interface seems like a good idea.
digital_reader.go
Outdated
| type DigitalReader interface { | ||
| Handler(func(machine.Pin)) | ||
| HandlerWithDebounce(func(machine.Pin), time.Duration) | ||
| Handler(callback) |
There was a problem hiding this comment.
This is an odd API design choice. having an unexported type in an exported interface. Do you want to define semantics on the callback type? Also be careful with how you use this interface- it's present in digitalInput and button as an exported embedded field on an unexported type which are present as an exported field on EuroPi, which means the underlying DigitalReader on the button and digitalInput is accesible to users to reference. I'd consider hiding this in button and digitalInput behind an unexported field and allowing users to set them on initialization.
| b.debounceDelay = delay | ||
| b.Pin.SetInterrupt(machine.PinFalling, b.debounceWrapper) | ||
| type button struct { | ||
| DigitalReader |
There was a problem hiding this comment.
Unexport this field maybe so that users can't access the field through EuroPi. I'd prefer something like this be set on EuroPi initialization, maybe more on that in another comment.
| // Display is a wrapper around `ssd1306.Device` for drawing graphics and text to the OLED. | ||
| type Display struct { | ||
| type display struct { | ||
| ssd1306.Device |
There was a problem hiding this comment.
Another exported field on an unexported type which is readily accesible to users.
europi.go
Outdated
| } | ||
| } | ||
|
|
||
| func GetInstance() *EuroPi { |
There was a problem hiding this comment.
OK, so when I said singletons are great... Ummm... is there a thing as taking things too far?
Hahaha, kidding aside this pattern is questionable from a configuration and mock-test standpoint. Since you only have one EuroPi ever and more importantly no ways to initialize it there's no way to mock EuroPi and also no way to configure it either. Consider returning to the New() function and letting the user say when they want to start the EuroPi.
The reason I draw the line on singletons on the EuroPi is because the EuroPi type is not a direct abstraction on hardware as is a PWM, I2C and such, but rather a type that holds the direct HW abstractions and operates on them. There is no reason for EuroPi to be a singleton really unless you really want it to be a singleton, which is OK, I guess.
| } | ||
|
|
||
| // New will return a new EuroPi struct. | ||
| func New() *EuroPi { |
There was a problem hiding this comment.
If you choose to return to New (see comment on singletons below), then we could get some neat function-callback-configuration parameter thing going on. This is what I'm talking about. So something like
knobNumber := 0
ep := europi.New(
europi.WithKnob(knobNumber, mockADC), // each of these package level option functions returns *another* function!
europi.WithButton(buttonNumber, mockDigitalReader),
europi.WithDisplay(myHDDisplay),
)This would be pretty neat since your API would then stay pretty much the same into the future, and if a user wanted to initialize it ez-pz they could just go ep := europi.New()
|
I didn't get around to reading the whole PR, but here goes the main things I noticed I might have done differently, hope it helps! |
soypat
left a comment
soypat
left a comment
There was a problem hiding this comment.
PR is looking real good :) Let me know if you want help with anything
3 participants
Additional changes:
EuroPistructEuroPias a Singleton viaeuropi.GetInstance()