ssd1xxx: break dependency from machine package #812
Conversation
| wr: wr, | ||
| cs: cs, | ||
| rst: rst, | ||
| func New(rs, wr, cs, rst pin.Output, bus Bus) *Device { |
There was a problem hiding this comment.
Big compile-time bang here, as we return pointer now.
There was a problem hiding this comment.
I guess returning a pointer would agree with the ways of other drivers. Are we OK with breaking our users on this? We'd be opening the door to breaking other drivers that do this. It's fine by me if we all agree
There was a problem hiding this comment.
On second thought, I'd perform this change in one fell swoop on all drivers
There was a problem hiding this comment.
We did this multiple times, so 🤷 As I understood practice is "if it breaks in compile time, then fine". We don't want to break ppl silently, keeping API, but changing behaviour.
There was a problem hiding this comment.
Note: the function signature change is not a breaking change unless someone thought of storing the function in a data structure, which I believe is very, VERY unlikely. Most (read as all) people use drivers as seen in examples so this should not be a breaking change
There was a problem hiding this comment.
That's true, good point, compile-time breaks only if they specified type, not inferred it.
So, if they infer type -- it just works for them, a variable stops being struct and becomes pointer to struct.
If they define variable type explicitly, it will break for them in compile time. I'd say that's acceptable.
I may try and dig older PRs there we did such change w/o much fuzz, so I understood we are fine with this.
|
The following shows how to do the same thing by combining package ssd1306
import (
"time"
"tinygo.org/x/drivers"
"tinygo.org/x/drivers/internal/legacy"
"tinygo.org/x/drivers/internal/pin"
)
type SPIBus struct {
wire drivers.SPI
dcPin pin.OutputFunc
resetPin pin.OutputFunc
csPin pin.OutputFunc
buffer []byte // buffer to avoid heap allocations
}
// NewSPI creates a new SSD1306 connection. The SPI wire must already be configured.
func NewSPI(bus drivers.SPI, dcPin, resetPin, csPin pin.Output) *Device {
legacy.ConfigurePinOut(dcPin)
legacy.ConfigurePinOut(resetPin)
legacy.ConfigurePinOut(csPin)
return &Device{
bus: &SPIBus{
wire: bus,
dcPin: dcPin.Set,
resetPin: resetPin.Set,
csPin: csPin.Set,
},
}
}
// configure pins with the SPI bus and allocate the buffer
func (b *SPIBus) configure(address uint16, size int16) []byte {
b.csPin.Low()
b.dcPin.Low()
b.resetPin.Low()
b.resetPin.High()
time.Sleep(1 * time.Millisecond)
b.resetPin.Low()
time.Sleep(10 * time.Millisecond)
b.resetPin.High()
b.buffer = make([]byte, size+1) // +1 for a command
return b.buffer[1:] // return the image buffer
}
// command sends a command to the display
func (b *SPIBus) command(cmd uint8) error {
b.buffer[0] = cmd
return b.tx(b.buffer[:1], true)
}
// flush sends the image to the display
func (b *SPIBus) flush() error {
return b.tx(b.buffer[1:], false)
}
// tx sends data to the display
func (b *SPIBus) tx(data []byte, isCommand bool) error {
b.csPin.High()
b.dcPin(!isCommand)
b.csPin.Low()
err := b.wire.Tx(data, nil)
b.csPin.High()
return err
}Note that it does not need to add any additional types. 😄 |
|
+1 to proposed scheme by Ron. No reason to intentionally avoid all the benefits of |
|
Yes, @deadprogram @soypat this going to work, but then we are configuring pins as outputs in I mean, it was like this before, but only by mistake -- pins configuration must have happened in Leaving aside the whole "configure pin mode in drivers was a big mistake from start". I'm just trying to find a model approach to apply on majority of drivers breaking from |
| dcPin.Configure(machine.PinConfig{Mode: machine.PinOutput}) | ||
| resetPin.Configure(machine.PinConfig{Mode: machine.PinOutput}) | ||
| csPin.Configure(machine.PinConfig{Mode: machine.PinOutput}) | ||
| func NewSPI(bus drivers.SPI, dcPin, resetPin, csPin pin.Output) *Device { |
There was a problem hiding this comment.
we should stick to the original place of configuration. if original author configured pins in New, we should also configure in New
There was a problem hiding this comment.
I sort of agree, in style like "the hell with it, legacy behavior anyway".
But I see it was a mistake and scout in me rebels.
| legacy.ConfigurePinOut(d.dcPin) | ||
| legacy.ConfigurePinOut(d.resetPin) | ||
| legacy.ConfigurePinOut(d.csPin) | ||
| legacy.ConfigurePinOut(d.enPin) | ||
| legacy.ConfigurePinOut(d.rwPin) |
There was a problem hiding this comment.
the way I was going about configuring pins when storing function pointers is shown in #753, might help you out
There was a problem hiding this comment.
The solution you are referring to is configurePins function stored as a field in struct and called in Configure.
Can't say it's either more elegant or less code 🤷 But it's an alternative, yes.
There was a problem hiding this comment.
I did an experiment. Built the ssd1306 example for xiao-rp2040 three times:
- baseline (current code in dev)
OutputStructstyle (this PR)configurePinsstyle andpin.OutputFunc(proposed alternative)
Here are the results:
OutputStruct adds 1.5% on flash, configurePins adds 2.5% on flash. Of course, this is just a small example, in real application difference going to be negligible. But it was fun to verify the effect of both approaches.
Baseline
$ tinygo build -size full -o ./build/test.hex -target=xiao-rp2040 ./examples/ssd1306/
code rodata data bss | flash ram | package
------------------------------- | --------------- | -------
0 17 3 6 | 20 9 | (padding)
999 0 7 26 | 1006 33 | (unknown)
240 0 0 0 | 240 0 | /Users/ysoldak/code/tinygo/targets
1016 0 0 0 | 1016 0 | C compiler-rt
0 168 0 0 | 168 0 | C interrupt vector
80 0 0 0 | 80 0 | C picolibc
0 0 0 4096 | 0 4096 | C stack
64 0 0 0 | 64 0 | Go interface method
0 668 0 0 | 668 0 | Go types
58 0 0 0 | 58 0 | device/arm
64 0 0 0 | 64 0 | device/rp
20 0 0 0 | 20 0 | internal/binary
532 86 0 0 | 618 0 | internal/task
1946 79 4 330 | 2029 334 | machine
490 0 0 7 | 490 7 | machine/usb/cdc
0 0 93 0 | 93 93 | machine/usb/descriptor
294 18 0 0 | 312 0 | main
4036 637 5 767 | 4678 772 | runtime
756 0 0 0 | 756 0 | runtime/volatile
142 0 0 0 | 142 0 | sync/atomic
204 0 0 0 | 204 0 | time
546 0 0 0 | 546 0 | tinygo.org/x/drivers/ssd1306
------------------------------- | --------------- | -------
11487 1673 112 5232 | 13272 5344 | total
OutputStruct
$ tinygo build -size full -o ./build/test.hex -target=xiao-rp2040 ./examples/ssd1306/
code rodata data bss | flash ram | package
------------------------------- | --------------- | -------
0 17 3 6 | 20 9 | (padding)
1127 0 7 26 | 1134 33 | (unknown)
240 0 0 0 | 240 0 | /Users/ysoldak/code/tinygo/targets
1016 0 0 0 | 1016 0 | C compiler-rt
0 168 0 0 | 168 0 | C interrupt vector
80 0 0 0 | 80 0 | C picolibc
0 0 0 4096 | 0 4096 | C stack
132 0 0 0 | 132 0 | Go interface method
0 720 0 0 | 720 0 | Go types
58 0 0 0 | 58 0 | device/arm
64 0 0 0 | 64 0 | device/rp
20 0 0 0 | 20 0 | internal/binary
532 86 0 0 | 618 0 | internal/task
1798 79 4 330 | 1881 334 | machine
490 0 0 7 | 490 7 | machine/usb/cdc
0 0 93 0 | 93 93 | machine/usb/descriptor
306 18 0 0 | 324 0 | main
4020 637 5 767 | 4662 772 | runtime
750 0 0 0 | 750 0 | runtime/volatile
142 0 0 0 | 142 0 | sync/atomic
204 0 0 0 | 204 0 | time
32 0 0 0 | 32 0 | tinygo.org/x/drivers/internal/pin
608 0 0 0 | 608 0 | tinygo.org/x/drivers/ssd1306
------------------------------- | --------------- | -------
11619 1725 112 5232 | 13456 5344 | total
configurePins func
$ tinygo build -size full -o ./build/test.hex -target=xiao-rp2040 ./examples/ssd1306/
code rodata data bss | flash ram | package
------------------------------- | --------------- | -------
0 17 3 6 | 20 9 | (padding)
1179 0 7 26 | 1186 33 | (unknown)
240 0 0 0 | 240 0 | /Users/ysoldak/code/tinygo/targets
1016 0 0 0 | 1016 0 | C compiler-rt
0 168 0 0 | 168 0 | C interrupt vector
80 0 0 0 | 80 0 | C picolibc
0 0 0 4096 | 0 4096 | C stack
32 0 0 0 | 32 0 | Go interface method
0 708 0 0 | 708 0 | Go types
58 0 0 0 | 58 0 | device/arm
64 0 0 0 | 64 0 | device/rp
20 0 0 0 | 20 0 | internal/binary
532 86 0 0 | 618 0 | internal/task
1768 79 4 330 | 1851 334 | machine
490 0 0 7 | 490 7 | machine/usb/cdc
0 0 93 0 | 93 93 | machine/usb/descriptor
290 18 0 0 | 308 0 | main
4046 637 5 767 | 4688 772 | runtime
758 0 0 0 | 758 0 | runtime/volatile
142 0 0 0 | 142 0 | sync/atomic
204 0 0 0 | 204 0 | time
22 0 0 0 | 22 0 | tinygo.org/x/drivers/internal/legacy
40 0 0 0 | 40 0 | tinygo.org/x/drivers/internal/pin
798 0 0 0 | 798 0 | tinygo.org/x/drivers/ssd1306
------------------------------- | --------------- | -------
11779 1713 112 5232 | 13604 5344 | total
This is the diff for the configurePins func approach, based on this PR:
diff --git a/ssd1306/ssd1306_spi.go b/ssd1306/ssd1306_spi.go
index 1b50c8bc..fc740451 100644
--- a/ssd1306/ssd1306_spi.go
+++ b/ssd1306/ssd1306_spi.go
@@ -9,11 +9,12 @@ import (
)
type SPIBus struct {
- wire drivers.SPI
- dcPin pin.OutputStruct
- resetPin pin.OutputStruct
- csPin pin.OutputStruct
- buffer []byte // buffer to avoid heap allocations
+ wire drivers.SPI
+ dcPin pin.OutputFunc
+ resetPin pin.OutputFunc
+ csPin pin.OutputFunc
+ buffer []byte // buffer to avoid heap allocations
+ configurePins func()
}
// NewSPI creates a new SSD1306 connection. The SPI wire must already be configured.
@@ -21,9 +22,14 @@ func NewSPI(bus drivers.SPI, dcPin, resetPin, csPin pin.Output) *Device {
return &Device{
bus: &SPIBus{
wire: bus,
- dcPin: pin.OutputStruct{Output: dcPin},
- resetPin: pin.OutputStruct{Output: resetPin},
- csPin: pin.OutputStruct{Output: csPin},
+ dcPin: dcPin.Set,
+ resetPin: resetPin.Set,
+ csPin: csPin.Set,
+ configurePins: func() {
+ legacy.ConfigurePinOut(dcPin)
+ legacy.ConfigurePinOut(resetPin)
+ legacy.ConfigurePinOut(csPin)
+ },
},
}
}
@@ -32,9 +38,7 @@ func NewSPI(bus drivers.SPI, dcPin, resetPin, csPin pin.Output) *Device {
func (b *SPIBus) configure(address uint16, size int16) []byte {
// configure GPIO pins (on baremetal targets only, for backwards compatibility)
- legacy.ConfigurePinOut(b.dcPin)
- legacy.ConfigurePinOut(b.resetPin)
- legacy.ConfigurePinOut(b.csPin)
+ b.configurePins()
b.csPin.Low()
b.dcPin.Low()
@@ -64,7 +68,7 @@ func (b *SPIBus) flush() error {
// tx sends data to the display
func (b *SPIBus) tx(data []byte, isCommand bool) error {
b.csPin.High()
- b.dcPin.Set(!isCommand)
+ b.dcPin(!isCommand)
b.csPin.Low()
err := b.wire.Tx(data, nil)
b.csPin.High()
There was a problem hiding this comment.
Thank you for testing this! I was curious as to how much flash the closures added. I knew there had to be an added load but clueless on how much. Very nice to get a feel for the cost of these abstractions. Eventually I'd like to do as you say yurii and do away with pin configuration and these ugly closures. When that day comes we should prepare to do all breakage together in one fell swoop
|
To answer your question Yurii, yes it is surprise behaviour. I don't like configuration in New calls- but changing that behaviour may break users and we're already striving to not break our users with the proposed pin HAL API so we could try to not break anyone until we're ready to do a major announcement. For now lets focus on porting drivers to new pin HAL API like we're doing without stepping on too many toes- at least that's my take |
This breaks dependency of ssd1xxx drivers from
machinepackage, more specifically frommachine.Pin.See #795
To workaround the need to convert all
High()andLow()calls toSet(true)andSet(false)respectively, I've opted for implementing a simple helper structOutputStruct. A bit clunky, so I'm happy to apply a better approach if there is one. At least it's insideinternal.Psst, I've considered to use
OutputFuncinternally that hasHigh()andLow()implemented already, but then hit the configure-for-output wall (need to maintain reference to the variable anyway and call the legacy method later).Yes, by the way, legacy backwards-compatible pin configuration for baremetal targets moved to
Configuremethods, asNewshall not touch hardware (at least we state this in many places and it is seem to be sane pattern). I don't expect this simple change shall break anyone.Breaking change though: returning pointer to struct from
New()inssd1289case -- such changes we did before and it is very easy to fix on consumer side and it breaks with big bang on compile time, so shall be fine.We are hopefully and gradually apply such changes to all drivers.
I propose we do this in batches of similar drivers to help with reviews.