Observations

[soypat]

Hey Adam! Been looking a bit through the code and I thought to take note of some things that called out to me.

Documentation

EuroPi struct needs better naming or documentation. I'd guess K1 and K2 are the knobs, but it is hard to be sure. Adding a comment or changing them to Knob1 and Knob2 would be pretty good (I'd opt for the second since it would make comments superfluous, which is an especially good thing!). Same goes for NewAI and NewDI functions, their naming seems evil to me hahah 🙃

API

I feel like you should be making better use of the tinygo drivers package and it's interfaces. A lot of stuff here has wrappers that feel superfluous to me. An example is the Display. Instead of having a *Display in the EuroPi struct could you not use a drivers.Displayer and have the font stored in the EuroPi struct too if needed?

I also feel like the DigitalReader could be a struct instead of an interface. Though this would prevent mocking (never mocked tinygo programs myself though).

After some thinking... Shouldn't all of Europi's fields be non-interface types? What exactly is the advantage of having them as interface types? Seems to me they should be at most structs that embed interfaces which implement the IO functionality.

Heap

yep, I think strconv.Itoa is causing those awful heap alloc. Instead I'd have a static byte array of the size of the display and use the strconv.AppendInt function to avoid heap allocations altogether.

type display struct {
     // ...
     buf [16]byte  
}
// ...
n := copy(disp.buf[:],  "my knob is at ")
strconv.AppendInt(a[:n:16], number, 10) // if append exceeds capacity then you have a problem

[awonak]

Thank you so much for all of these thoughtful comments! I'm digging to each up them and slowly making the changes. I'll report back with any findings on the heap allocation performance improvements as I go.

[awonak]

EuroPi struct needs better naming or documentation

Agreed! I have added docstrings to each of the peripheral/component singletons. The terse naming is a carryover from the official repo (which has better documentation). I wanted to keep the naming conventions the same for naming parity, though I am considering changing to your suggesting since this is a completely separate repo and a fresh start.

Instead of having a *Display in the EuroPi struct could you not use a drivers.Displayer

This was an ah-ha moment for me, since tinydraw accepts a device.Displayer, then that would have been the correct interface to use! I did end up removing the EuroPi struct in favor of peripheral/component singletons, so I'm not using that interface at the moment.

Shouldn't all of Europi's fields be non-interface types? What exactly is the advantage of having them as interface types?

Yeah, my original intention was to make it easy to fake out parts of the EuroPi struct for testing. The interfaces aren't really doing anything for me now, so I'll remove them and only add them back if I truly need them.

Seems to me they should be at most structs that embed interfaces which implement the IO functionality.

This is a really good observation. Early on I was exploring using composition like this but I never got around to implementing it.

[awonak]

Seems to me they should be at most structs that embed interfaces which implement the IO functionality.

This is a really good observation. Early on I was exploring using composition like this but I never got around to implementing it.

I ended up implementing this for the DigitalReader and it makes so much more sense! Thanks for the nudge. Unfortunately the hardware implementations for Analog Input and Knob are different enough that they need separate implementations.

I'm applying my changes in my existing Singleton refactor branch: #3

[soypat]

Yeah, my original intention was to make it easy to fake out parts of the EuroPi struct for testing. The interfaces aren't really doing anything for me now, so I'll remove them and only add them back if I truly need them.

If you do this remember to keep fields unexported so that if you change them back to a interface they don't break programs!

I'll look at your PR #3 now