pio: handle options on devices

[quinte17]

you probably know of https://commandcenter.blogspot.de/2014/01/self-referential-functions-and-design.html how about using some kind of implementation of that? i implemented something for my bme driver at https://github.com/quinte17/bme280 things i gained:

• the user finally can control the complete device even on creation.
• there is no need to have many parameters for the NewFunctions
• forced mode is also available.
• what i still not like much is how to be able to wait for the device in some cases. (reset, forced mode)

the benefit for your implementation is to have a possibility for the user to change the device's address. (because that could in theory depend on hw-configuration) in our two cases we already need other addresses. yours need 0x76 i need 0x77

[maruel]

Hey thank for the comment, it's a very interesting route to dive into.

I'm open to the option.

Thoughts

• No I hadn't read it. Thanks for the link.
• I'm not a fan of returning unexported symbols but I can live with that.
• The fact one had to specify all arguments when a user just want: "just give me sane defaults" was annoying to me too.
  • This directly help with this issue.
• One thing I care about is the orthogonality of flags (or the lack of).
  • I think it doesn't make sense to specify one oversampling (let's say temperature) without the others, so the option would still accept 4 values, the 3 oversampling settings and the one for the filter.
  • This reduces chatter on the I²C bus by changing all these options all at once.
  • This reduces the API surface area, something I care a lot about.
  • Lack of non-orthogonal flags grouping is something that bothered me with exp/io/spi/driver.Conn; some flags must be specified together.
• I feel weird a bit about Reset() as an option. I understand the rationale for generality but there's an issue for "option" vs "action".
  • The difference between both makes sense to explore this more.
  • Is the distinction actually important for the users and implementers, or not? Maybe not!
• This may not work well for interfaces like Conn-like interfaces.
  • Maybe it's not a problem? I care a lot about strict typing and I don't want to fall into interface{} use unless strictly needed.
  • I need to think more specifically about it the "app has an interface handle, not a concrete object" case.
• I think it's a good idea.

Action Items

• For consistency, switching to this pattern would mean to do it for pretty much all drivers.
• Add bme280.Settings(t, p, h, f) function.

Paging @tve for thoughts.

[quinte17]

I'm not a fan of returning unexported symbols but I can live with that.

Thats what i am also unsure about. but i think making it public is no problem, because every driver has its own Option-Type. So the compiler will bother if someone misuses it. And golint will also be quiet.

I think it doesn't make sense to specify one oversampling (let's say temperature) without the others, so the option would still accept 4 values, the 3 oversampling settings and the one for the filter.

This i think depends on the use case. Sure humidity and pressure depend on temperature so temperature is the minimum to measure. But one could decide if he wants the others measured. Chapter 5.5 of the bme data-sheet suggests different modes of operation. And some of them wont measure one of the values. I think it should still be possible to have advanced configurations.

Lack of non-orthogonal flags grouping is something that bothered me with exp/io/spi/driver.Conn; some flags must be specified together.

This depends on the hardware i think.

I feel weird a bit about Reset() as an option. I understand the rationale for generality but there's an issue for "option" vs "action".

I am still unsure about that reset option too. I thought that this is something what isnt called very often. It only makes sense after a restart or if something with the sensor is "weird". I could also live with action like options. The forced mode should then be an action too. And there the mixing between options/actions starts.

How would you solve the "wait for a specific register/bitchange problem"? I had also thought providing something like the time.After returning a channel instead of just blocking. The poor thing is that you always have to poll the device. And i do not think that all devices can handle concurrent access..

thank you for listening.

[tve]

Mhh, to be honest, this seems too clever for me.. A scheme that requires reading two pages of pretty tricky stuff is not a good scheme if there are simpler alternatives. Consider that someone is going to have to explain this to every contributor to pio.

What I've seen that is simple is to provide an opts struct. Something like:

type BME280Opts struct {
  option1 option1Type
  option2 option2Type
  ...
}

func NewBM280(requiredArg type1, opts BME280Opts) { ... }

A call with one option specified then looks like:

bme, err := NewBME280(v1, BME280Opts{option1: v2})

If you want, you can make the opts arg be varargs and then a call to NewBME280 with no options simply leaves the struct off entirely.

Pros:

• really simple to write and simple to implement
• serves as nice documentation of the options 'cause they're all in this struct (this is one thing that's awful with the self-referential function)

Cons:

• the null value of each option means that the option is not set, so the fields need to be phrased accordingly
• if one uses the varargs style a beginner could be confused.

I've first encountered this in the gorethink library, which I've used extensively and I've found it very nice to use, the code is very readable. See https://godoc.org/github.com/dancannon/gorethink and all the types ending in Opts. Specific example: Run takes a lot of options, this is the function https://godoc.org/github.com/dancannon/gorethink#Term.Run, click on the RunOpts type to see them all.

[quinte17]

how to make sure that every option is set correctly? if this struct then is just again a set of enums its hard to write. don’t forget the package names. (at least auto-completion would still work)

bme, err := bme280.NewBME280(v1, bme280.BME280Opts{option1: bme280.v2,
                             option2: bme280.v3})

if you make the types easier in the struct you have no use of auto-completion and have to control the values provided more carefully.

[tve]

I think the Go proverb is to optimize for reading code and not for writing. But I actually don't understand what you mean by enum types. Your BME case would have:

type NewOpts struct {
  humOversampling int
  tempOversamplig int
  mode string
}

and a call might be

bme, err := bme280.New(v1, bme280.NewOpts{humOversampling:16})

[quinte17]

that’s what i meant with my last sentence. the type is easier. struct of ints. but now auto-completion is not working for the correct values. you have to read in the docs what is valid. but maybe reading the docs when interacting with hardware is a good thing.

please don’t misunderstand me. i do not persist on the "self referencing" thingy. i just want a solution which had some thoughts before ;)

[quinte17]

with enum types i mean a struct of special types of:

type FilterOpt int

const (
        OptFilterOff FilterOpt = iota
        OptFilterX2
        OptFilterX4
)
type NewOpts struct {
  filter FilterOpt
  tempOversamplig ...
...
}

bme, err := bme280.New(v1, bme280.NewOpts{filter:bme280.OptFilterX2})

[maruel]

Filter and Oversampling are already "types".

The goals I have for the project is:

1. Optimize for Usability.
2. At usability expense, the user can chose to optimize for performance.
3. The driver's writer pleasure is dead last.

It's probably good to optimize according to the scale of the configuration. My rule of thumb would be:

• 1~2 flags is worth just putting as-is.
• 3~6 flags is better as a struct.

• 6 is probably worth exploring more complex design, including the proposal above.

(or something like that). I know that runs counter to what I stated above.

Structs require good default values, which may be tricky in some cases but can be fixed with setting it as 0. I think this is the case in bme280.

One thing worth noting is that using a struct pointer (or varargs) is just a proxy to use default arguments, so we should think about it this way. An alternative is to create two constructors like New() and NewWithDefaults() or something shorter. That doesn't scale well when the device already has two constructors due to dual I²C and SPI support. This is the case for bme280.

I'm against gorethink.RunOpts implementation on the basis of avoiding interface{}.

The only important thing is to make it accessible and usable for less experienced developers.

I think in bme280's case, a struct would be appropriate because of what I consider lack of orthogonality.

How would you solve the "wait for a specific register/bitchange problem"?

Yeah there's no way of getting out of polling with this kind of device. This can cause serious I²C bandwidth deterioration when the bus is shared with a high bandwidth device (let's say a ssd1306, which is my use case). The datasheets gives estimates on the amount of time to take a measurement, so this calculation should be used to determine the right amount of time to sleep. Then a smoke test should be created to verify this assumption.

About concurrency, we need to clarify the safety of each objects. We should assume the use case will likely be:

• open the device in the "main" goroutine
• pool in a separate goroutine

The safety should be optimized for that use case. This is particularly important for drivers which implement a Close(), as it will likely be called from the main goroutine, potentially concurrently to a Read().

[tve]

I'm against gorethink.RunOpts implementation on the basis of avoiding interface{}.

There is nothing is that scheme that requires interface{}, you can use proper types and I wasn't at all suggesting to use interface{}.

I actually think that even with many options the struct is better because it clearly documents the options available and passes them all at once.

[maruel]

I agree for bme280 specifically, the lack of orthogonality makes it more sensible to use a struct. That said, this is assuming that most users do not care about the flags and would use nil, as using a struct makes like more painful for those would want to use the flags. That's tricky, I'm not sure most people do not care.

One thing I realized is that the filter flag is actually hurtful. There's no real reason to do an alpha filtering on the device instead of on the computer. On the other hand oversampling is useful because it's done in rapid succession. So I'm thinking about removing the filter flag and adding a filter wrapper in Go.

[quinte17]

the device-filter does not loose a measure. on the pc side you cant guarantee that all measurements were seen. even with rapid polling. the bme280 has no interrupts to signal anything. if you want that to be done on the pc side you maybe need some other hardware which is capable of what you need.

[maruel]

Fair enough.

[maruel]

I'm reopening because I want to continue to brainstorm if there's value in having a common options pattern or not. I know this was an issue for exposing config.

[maruel]

I'll close it for now since there will be no more work here but I'll keep the idea in mind.