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.