PinInput+PinOutput HAL (#753, reloaded)

[soypat]

This PR is a redo of #753 which was too damn long for any person to understand.

It now includes more comments detailing benefits and the why of some of the ideas in previous PR.

[ysoldak]

Excuse my long comment, but I've tried to cover all aspects of this refactoring we are trying to make.

Problem

Package drivers depends on TinyGo's package machine due to use of machine.Pin (and PinMode, etc) type.
Dependency from machine package means drivers can only be compiled with TinyGo compiler.
We want drivers package can be used independently from TinyGo.
Example: Control peripherals (a display, etc) on Raspberry Pi where mainstream Go compiler to be used.

Requirements for a solution:

• No dependency from machine.Pin (and friends) in drivers;
• Existing consumer code must still work;
• Keep API simple and consistent (see below);
• Shall not sacrifice performance.

Context

Great Public API is:

• Simple - minimal number of concepts and assumptions
  helps with learning of the API, resoning about it and using it
• Consistent - similar things done similarly
  this has positive effect on Simplicity
• Implementation neutral
  internal implementation can be changed w/o need of API change

In drivers package now we have following bits of the public API:

• drivers.SPI
• drivers.I2C
• drivers.UART
• drivers.Sensor
• drivers.Displayer
  ...and they all are interfaces

Some drivers set pin mode (input/output) once in Configure() method and other drivers do that multiple times, in runtime (1-wire, dht thermometer, etc).
Must be noted though, there is no consistency, not all drivers confgure pin modes. In that case, pins expected to be passed pre-configured.

Solution

Pin modes & existing code

While output mode is more or less same across hardware, input mode is trickier: it can be simple PinInput, but can be also PinInputPullup or PinInputPulldown -- and the mode very much depends on driver consumer's hardware.

Proposal:
All new drivers shall not try and configure pin modes; it must be explicitly stated in documentation that pin modes must come pre-configured or pin mode configuration must be handled on consumer side when a pin can change mode in runtime.
Existing drivers that do configure pin modes now, shall proceed doing so, but only in the case when pin passed to the constructor is of type machine.Pin -- to avoid breaking existing consumer code.

Build tags trick (catch "baremetal", check for "machine.Pin" type and set pin mode) can be used to keep existing consumer code working.

Breaking dependency

It had been shown that it is possible to break dependency from the machine package.
For that we need to replace machine.Pin with a new type defined in drivers package.

Alternatives

Alternative 1: drivers.Pin interface (and must likely drivers.PinInput and drivers.PinOutput interfaces)

• Breaks dependency from machine.Pin
• Existing code still works
• Keeps API simple and consistent
  • Consistent with existing API bits (see Context)
  • Consistent across drivers, as old and new drivers going to use same approach in constructors
    Example: machine.Pin can still be passed as-is into constructors, into all drivers, old or new
• Performance hit due to interface lookups can be avoided in the drivers that require performance by storing pointers to functions internally
  such technique can be promoted as best practice and advised to be used in all new drivers, (some) existing drivers may be modified to use the approach.

Alternative 2: drivers.PinInput and drivers.PinOutput function types

• Breaks dependency from machine.Pin
• Existing code still works
• Brings inconsistency and confusion into API
  • Inconsistent with existing API bits (see Context)
  • Inconsistent across drivers, as old and new drivers going to use different approaches in constructors
  • Not simple, as it going to be mighty annoying to constantly switch between constrictor flavors in the same consumer code base
• Performace is kept, as function pointers add no penalty

Summary

Proposal

• Adopt drivers.Pin interface approach, alternative 1;
• New drivers shall not configure pin modes;
• Promote storing pointers to pin set and get functions internally in drivers as a best practice.

Note on false impressions

A worry expressed about potential risk of misunderstanding and false impression of (Tiny)Go being slow and non-suitable for embedded development, I find ungrounded. Sloppy code can be written in any language. Code in C, too, can be slow, if written w/o understanding of system programming, complexity theory and use of proper data types.
Function types API is not a panacea. Function types do not shield us from driver developers creating slices left and right that end up in heap and triggering GC cycles unnecessarily, just to give an example.
I can't see what benefits we gain by adopting function types API, yet it's clear we are going to sacrifice API consistency in that case.

[soypat]

Supposition: Due to performance gains, we need a function HAL exposed to users. The question is if we also want to add an interface HAL for pins.

Pros of interfaces

In drivers package now we have following bits of the public API [...]
...and they all are interfaces

This is the one and only argument in favor of using interfaces I find the slightest bit compelling, though the argument I feel is very debilitated due to the call site usage being identical for PinOutput and even more readable for PinInput. Are we taking a decision because we prefer type definitions that say "interface" over "func" ?

I'll also note that the argument "that's the way it's always been done" is not a great argument in the context of tech and embedded systems... especially when we ourselves are working on a project that defies everything about how embedded systems is done today in the industry.

Empty arguments in favor of interfaces

machine.Pin can still be passed as-is into constructors, into all drivers, old or new

We can still do this with an internal pin interface, this PR is such a demonstration of this.

Arguments not mentioned against interfaces

• I'll note interfaces are extremely hard to teach. I've spent two classes going over the concept of an interface with students who've programmed in Python (never used inheritance) and it has been an surprisingly hard concept to grasp. Interfaces are more complex than functions values.

yet it's clear we are going to sacrifice API consistency in that case.

We are also sacrificing API consistency if we have function and interface user-facing API. This is likely much more confusing for users than having just a function HAL: which HAL do they choose? We are exposing two HALs that have the exact same purpose. Recall we removed WriteRegister and ReadRegister methods from the I2C interface in the spirit of API consistency: You could do these operations with Tx already.

Having two HALs for the same concept goes against the most basic design principles. It is confusing to have two things that overlap over the same concept. We don't have several HALs for other concepts in TinyGo.

This PR shows we can literally do without one of these HALs... why are we still having this conversation?

Functions: Brick wall preventing bad designs

• Ensure interface signature growth avoided: Someone has already mentioned the possibility of adding Toggle method to PinOutput interface- there are reasons why we shouldnt do this.

• Ensure developers don't do interface type conversions in drivers which would break portability, this goes against concept of HAL.

Don't forget: Benefits of function HAL

• Implementing a novel HAL is trivial and requires minimal boilerplate and can be written next to the point of use for maximum readability. Also very easy to share HAL implementations due to conciseness
  • Trivial to write mock tests with absolutely no scaffolding
• Reads cleanly. PinOutput.High()/Low(); PinInput() -> cs.High(); isBusy := d.isBusy()

Proposal

We know we want the function HAL. Baby steps. Let's start there and see if the interface HAL need be exported. It has currently been demonstrated it need not be exported in this PR. We can always export it in the future without breaking our users. We are compromising on nothing by taking this approach, just delaying the potential addition of the interface HAL if it be required.

@ysoldak Insisting on adding the interface HAL by this point seems to me risking a whole lot of disadvantages for "I like when it says interface in the type definition because that's how it's always been done". If this proves to be the sentiment for the general public, we can always add the interface in the future.

[HattoriHanzo031]

Having two HALs for the same concept goes against the most basic design principles.

@soypat I think we all agree on this, the confusion is that @ysoldak and me think that function proposal will cause this inconsistency and not the interface proposal. For example, by your proposal, if we want to keep backward compatibility, old drivers will have following API:

func New(p1 pin.Input, p2 pin.Output) Device // pin.Input and pin.Output are interfaces

and new drivers will have following API (there is no example of implementing new driver so I'm presuming this is your intention):

func New(p3 drivers.PinInput, p4 drivers.PinOutput) Device // drivers.PinInput and drivers.PinOutput are function types

Both pin.Input/Output and drivers.PinInput/Output are used in user facing API, hence users are exposed to two different pin HALs (even if one is internal) so API is inconsistent. If we were to rewrite old drivers to use drivers.PinInput/Output than the API would be consistent, but we would lose backward compatibility and examples (which is also an option to consider).

Whereas if interfaces are used, both new and old drivers would all have APIs like this:

func New(p1 drivers.Input, p2 drivers.Output) Device // drivers.Input and drivers.Output are interfaces

PinInput and PinOutput could be internal helper types used internally when implementing drivers to store interface methods, but not exposed on the API. Constructor implementation would look something like this:

type Device struct {
	in  pin.PinInput
	out pin.PinOutput
	...
}

func New(p1 drivers.Input, p2 drivers.Output) Device {
	return Device {
		in : p1.Set
		out: p2.Get
		...
	}
}

I agree that downside of this is that driver developers are not forced to store interface methods in variables and use them for better performance, but if all drivers are rewritten in this fashion, it would be easier for them to copy this pattern.

Another concern I have with function HAL is how would API for drivers that use the same pin for both input and output look like. Would the Driver constructor take two arguments of type drivers.PinInput and drivers.PinOutput like this:

func New(get drivers.PinInput, set drivers.PinOutput) Device // drivers.PinInput and drivers.PinOutput are function types

and in user code both arguments would be created using the same pin?

In interface case the API would look like this:

func New(p drivers.Pin) Device // drivers.Pin is interface that combines drivers.Input and drivers.Output interfaces

which in my opinion looks much more logical

EDIT: added example of driver implementation

[soypat]

@HattoriHanzo031 I'll try to address all your points:

function proposal will cause this inconsistency and not the interface proposal.

The first sentence of my comment states the reasoning for this. We need to expose the function HAL so third parties who develop drivers are informed on TinyGo's preferred HAL (for performance reasons). We don't need to expose the interface HAL, as shown in this PR.

and new drivers will have following API

No. There is nothing in this PR that demands a new API for new drivers. Driver designers can choose the constructor API as they please.

Both pin.Input/Output and drivers.PinInput/Output are used in user facing API

From the perspective of driver users, they will be able to pass in a machine.Pin to the constructor, They need not know of the function API. These are the users who do not develop drivers.

From the perspective of anyone else who uses TinyGo for driver development they will use the drivers package. They need the function HAL but nothing requires they use the interface HAL.

This way we get a compromise:

• We only have one HAL, which is something we all want
• We can still develop drivers like we've been developing drivers
• Users can still use drivers like they have been using drivers
• We can still export the interface in the future if it need be. There is absolutely no reason to rush bad API design.

Recall: the users of the "tinygo.org/x/drivers" package are driver developers, not driver users. We can serve both the best with this compromise, and not risk not being able to add the interface in the future if need be.

If we were to rewrite old drivers to use drivers.PinInput/Output than the API would be consistent, but we would lose backward compatibility and examples (which is also an option to consider).

No one is suggesting breaking backwards compatibility. There is absolutely no reason to break backwards compatibility. We can still even choose to adopt this method of developing drivers with function HAL and not interfaces internally, but expose the pin.Input to users. This is a good compromise for the reasons detailed above.

PinInput and PinOutput could be internal helper types used internally when implementing drivers to store interface methods, but not exposed on the API. Constructor implementation would look something like this:

I agree that downside of this is that driver developers are not forced to store interface methods in variables and use them for better performance, but if all drivers are rewritten in this fashion, it would be easier for them to copy this pattern.

Yes! Sounds great! This PR shows how this could work!

Another concern I have with function HAL is how would API for drivers that use the same pin for both input and output look like. Would the Driver constructor take two arguments of type drivers.PinInput and drivers.PinOutput like this:

Here's a small demonstration of how that would work!
https://github.com/tinygo-org/drivers/pull/795/files#diff-4e8216eab651a4c20848ea784becc264c3fbff50652bd1c08a9b009a6d309e25R12-R30

which in my opinion looks much more logical

Please explain why func New(p drivers.Pin) Device is more logical than func New(p pin.Input) Device. I will hold the second is more "logical" because it also explicit on how the pin will be used.

[HattoriHanzo031]

We don't need to expose the interface HAL, as shown in this PR.

Interface HAL in this PR is exposed to users even if it is internal because it is used in the user facing API. If those internal interfaces are changed they would break the API compatibility, hence they are exposed.

Recall: the users of the "tinygo.org/x/drivers" package are driver developers, not driver users.

I am not sure I understand this statement, isn't there more people using than writing drivers? Maybe the main point of disagreement is should drivers API be optimised for implementers or driver users?

No. There is nothing in this PR that demands a new API for new drivers. Driver designers can choose the constructor API as they please.

From the perspective of driver users, they will be able to pass in a machine.Pin to the constructor, They need not know of the function API. These are the users who do not develop drivers.

Now I'm not sure I understand the intention of making function HAL public. As I mentioned previously there is no example of implementing new drivers so I was presuming that the intention of making function HAL public is to use it in (some or all) future driver constructor APIs in this drivers package. If this is the case, when in future some driver designers choose constructors with interfaces and some choose constructors with functions, that would create inconsistency. If this is not the case and the function HAL is intended only to be used inside the drivers and not in APIs can you elaborate the reason for making function HAL public?

We need to expose the function HAL so third parties who develop drivers are informed on TinyGo's preferred HAL

If the main reason to make function API public is to communicate to third parties how would TinyGo prefer they write drivers in their own repos, I don't see it as strong argument. Third parties do not have to follow any pattern from drivers package, or worry about compatibility with drivers package. They can develop the drivers any way they want, and also use some third solution (for example generics) that works the best for their use case. For example, in big Go there are many third party JSON libraries, but most of them do not have the same API as the one from standard library.

Please explain why func New(p drivers.Pin) Device is more logical than func New(p pin.Input) Device. I will hold the second is more "logical" because it also explicit on how the pin will be used.

Your example is completely fine (and preferable) if the pin should only be used as input, but as I mentioned, my concern is about the drivers that must use the same pin for both input and output (like onewire and dht) in which case your example would not work because pin.Input can only get the pin value.

[soypat]

If those internal interfaces are changed they would break the API compatibility, hence they are exposed.

I've commented above but restating since the point seems to not be getting across: you comment is true for all of tinygo interfaces, Go standard library interfaces and third party interfaces. Any user facing interface which is modified has the potential to break user code.

You aren't really saying anything that is not true for an arbitrary exported interface, like is the case for the interface you are also pushing for.

Recall: the users of the "tinygo.org/x/drivers" package are driver developers, not driver users.

I am not sure I understand this statement, isn't there more people using than writing drivers? Maybe the main point of disagreement is should drivers API be optimised for implementers or driver users?

The point is illustrated with this PR explicitly. Someone who uses the uc151 driver in this PR has absolutely 0 contact with the drivers.PinInput HAL. They instead come in contact with pin.Output which is not part of drivers package. Driver developers either third party or within the drivers ecosystem will use the drivers.PinInput HAL because of all the benefits outlined above. It is the "canon" HAL for a pin.

In doing this we optimise for both- users can have the luxury of passing in a machine.Pin instead of machine.Pin.Set (as if it were such a big ask, but OK, we can see if this worry holds in the future). Meanwhile, driver developers use drivers.PinOutput behind the scenes and get all the benefits outlined above. Everyone wins. No one breaks.

Now I'm not sure I understand the intention of making function HAL public.

To obtain benefits detailed so clearly above

As I mentioned previously there is no example of implementing new drivers

There is a driver for the uc151 included and #753 contains over 20 drivers as an example.

that would create inconsistency

A result of design decisions:

• The best pin HAL is the function HAL (for the reasons outlined in previous comments) This is enough reason to me for making it public- why expose sub-par HAL? Why would we want sub-par experience for driver developers?
• We don't want to break our users
• No more than one HAL API for the Pin concept

function HAL is intended only to be used inside the drivers

Function HAL can be used wherever users or driver developers want. One thing is for sure- it is the better HAL for now. In TinyGo we seem to prefer better, even when it "is not how it has always been done".

If the main reason to make function API public is to communicate to third parties how would TinyGo prefer they write drivers in their own repos, I don't see it as strong argument. Third parties do not have to follow any pattern from drivers package, or worry about compatibility with drivers package.

I disagree on the basis of collaboration and fostering a coherent ecosystem where code is reusable from project to project. The Pin abstraction is particularily suited to HAL composition. We could theoretically architect the following stack that sits on a single pin:

• A pin that bitbangs I2C to control:
• A pin multiplexer IC that can read digital signal that:
• Is debounced by a debouncing driver... etc.

Ideally in the future we'd be comfortable with the idea of using a single Pin HAL for everything, so passing in machine.Pin.Set to constructors (come on guys, it is really not that bad).

The function HAL is also more flexible and can receive a interface easily! The same does not go for a constructor that receives an interface. The list of reasons for a function HAL only get longer by the day.

And even if you don't agree with this point, all the other benefits detailed in my comment above should be enough to convince you the function HAL is simply better in ever technical aspect. I'll remind the reader by now we are on the fence regarding function vs. interface because "interfaces are how it has always been done".

Your example is completely fine (and preferable) if the pin should only be used as input, but as I mentioned, my concern is about the drivers that must use the same pin for both input and output (like onewire and dht) in which case your example would not work because pin.Input can only get the pin value.

Your point illustrates perfectly why the interface HAL for constructors is not well suited for solving the more complex use cases easily without creating new types and adding their methods+data wheareas the function HAL can be solved inline at the call site. Maybe yet another point for function HAL. More to the list.

---

Thank you all for your comments and for your valid worries with this PR. I hope I answered all your questions!

[HattoriHanzo031]

If I was to summarise this discussion I would say (and correct me if I'm wrong) that in general we are all advocating for the same thing which is to introduce both function and interface pin abstractions, with major difference being which abstraction would be public and which would be internal. This PR suggests having function abstraction public (but mostly not used in the API functions) and interface abstraction internal (but used in the API functions), @ysoldak's PR have both abstractions public (with only interface abstraction being used in API functions) and I am advocating for having interface abstraction public (and used exclusively in the API) and function abstraction internal (used for performance optimisation in the driver implementations).

For easier further discussion, I created draft PR with PoC of my suggestion which is based on this PR with function and interface HAL switched places (function HAL being moved to internal and interface HAL being public). I also refactored onewire, dht and uc8151 drivers in a way I think it would work with this suggestion.

One thing that is still not clear to me is how would onewire driver be refactored to eliminate machine package and also preserve backward compatibility and performance after this PR is merged. @soypat can you please also refactor onewire driver in this PR (or a branch) so that is clear how do you suggest that this class of drivers (where the same pin is used both as input and output) should be implemented. I refactored onewire, dht and uc8151 drivers in my PR in a way I think it would work with my suggestion.

[HattoriHanzo031]

Another concern, which is unrelated to function vs interface HAL discussion because it affects both equally, is the offload of pin configuration burden to user code. Again, this is not a big problem if each pin should be configured only once and then used like that in the driver, but what about the cases like onewire and dht where pin must be configured during the driver operation? If you look at the code, the pin Set and Get functions should be implemented differently for onewire and dht drivers.

To demonstrate this I created new implementation of onewire driver (onewire_v2) simulating some future driver that has to have the same pin be input and output. As you can see from the example the user code becomes much more complex and as if user must understand inner workings of specific driver. We could create some helpers for this cases in drivers package, but since the configuration can vary from driver to driver, it could be challenging to cover all cases.

I don't think we must have the solution for this problem right now, but maybe it would be good to at least have awareness of it and to have a general idea how to deal with it.

[soypat]

with major difference being which abstraction would be public and which would be internal.

This seems to be the major difference, yes.

For easier further discussion, I created #797 with PoC

Thank you for the PoC.

how would onewire driver be refactored to eliminate machine package and also preserve backward compatibility and performance after this PR is merged. @soypat can you please also refactor onewire driver in this PR (or a branch) so that is clear how do you suggest that this class of drivers (where the same pin is used both as input and output) should be implemented.

Sure thing. Here's the code for a bit-bang one-wire interface with the proposed HAL.

• https://github.com/tinygo-org/drivers/blob/293de2b61a9f4249d683ffcc7309cb1aa97b8eb2/onewire/onewire.go
• https://github.com/tinygo-org/drivers/blob/293de2b61a9f4249d683ffcc7309cb1aa97b8eb2/onewire/onewire_tinygo.go

is the offload of pin configuration burden to user code. [...] onewire and dht where pin must be configured during the driver operation?

The main value proposition of this entire PR is for tinygo drivers to be usable with native Go code on other embedded system projects. This requires burdening the user with pin configuration, as you've correctly observed.

If you look at the code, the pin Set and Get functions should be implemented differently for onewire and dht drivers.

I'm not too familiar with the intricate differences between the one-wire and dht protocol, but a cursory search on google shows that the DHT sensor uses a proprietary protocol, so it would make sense to implement different HALs for each, at least at a glance. What is the issue?

As you can see from the example the user code becomes much more complex and as if user must understand inner workings of specific driver.

Check out my one-wire implementation- it relies on the assumption the pin is configured as pull up when input called, otherwise respecting the simplicity of the documented pin HAL. As a result, my implementation is very simple and requires no explicit pin configuration, compared to yours.

Also regarding your PR, we will most certainly not have a drivers.Pin HAL if we can avoid it. There is a lot to gain in readability and simplicity from separating input from output HALs in function signatures. Also, machine.Pin does not respect such a interface without scaffolding as you've shown, this could potentially be confusing to someone who expects a machine.Pin to fulfill the drivers.Pin interface.

As a final note

Call for feedback: If you believe an interface is superior, please bring a concrete counter-example or benchmark (perf, code size, or ergonomics) that the function HAL can’t match. Otherwise, let’s move forward with the function HAL and keep interfaces as internal adapters.

In the absence of concrete counter-examples, I'd suggest we all start to get comfortable with the idea of a function HAL and start thinking of the best way of adopting such a HAL going forward.