piglow: Setup needs to be done at Open

[rakyll]

Open functions are also responsible to run the initialization sequences. I don't see a good reason why there is both Open and Setup.

[mattetti]

Agreed, I think that's a pattern that was used in GoBot which might be why it was done here. I don't think it's needed and would also prefer to have Setup moved into Open

[zankich]

I don't know if I agree with having logic in the Open. If your open has logic in it about resetting it's internal state, what do we do in the case of a reconnect? Say for whatever reason your program crashes or you lose connection to the device, do you want to completely wipe the internal state of your registers, or do you want to attempt to recover?

[mattetti]

That's the approach we've taken for all the existing libs. Note that it is not a good argument it itself, consistency however is important. I do think it's worth talking through the API.

There are 3 main arguments being brought up so far:

• simplicity of the API (less methods)
• consistency (alway use the same open/setup sequence)
• reconnection

I personally don't think that reconnection is worth adding a new method and therefore increasing the API complexity. Reconnection is or at least should be an exceptional scenario, in which case, I think a reset is not really an issue.

@zankich you have more experience using GoBot, how often and in what kind of scenarios did disconnections/reconnections happen?

[zankich]

Not having logic in your Open is the inverse of #25 (comment). If I reset the world whenever I opened my device, I would lose all the information on my registers. This example was with the PiGlow, which is a pretty trivial type device, but I can still imagine the same sentiment with a more "advanced" microcontroller or device.

Granted most of the time I've had to deal with reconnects have been with devices over serial/http/(some remote type thing). Having something time out unexpectedly while you're reading or waiting for an event does happen. In the case of an i2c device connected directly to your raspberry pi, if you unexpectedly lose connection to it, chances are something bad just happened.

I 100% agree with having a consistent API, which makes sense and is not more complicated than it has to be. I do however want to expose as many of the raw operations that the device support and not try to mash functionality together into a larger method because the program reads a little cleaner. In my experience this creates libraries which work really well up until the point you need to do something advanced, and then you're out of luck. These device drivers should be fairly low level and give the consumer the ultimate say in how their program interacts with their hardware.

[rakyll]

What about naming Setup to Clear or Reset? I don't think Setup reflects what the method is doing and hence there is this issue. Setup gives me an impression this is a startup-time sequence I need to run.

[zankich]

The Setup function needs to be run the first time you use the PiGlow. It disables the software shutdown mode and enables the pins on each LED control register.

I'm completely open to changing the name to something more descriptive.

What about instead of needing to explicitly call Setup the first time, there can be an optional parameter passed to the Open function which will initialize the PiGlow or not?

[mattetti]

Activate or Enable might be better names

On Thu, Jun 23, 2016, 18:10 Adrian Zankich notifications@github.com wrote:

The Setup function needs to be run the first time you use the PiGlow.
It disables the software shutdown mode and enables the pins on each LED
control register.

I'm completely open to changing the name to something more descriptive.

What about instead of needing to explicitly call Setup the first time,
there can be an optional parameter passed to the Open function which will
initialize the PiGlow or not?

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#26 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAAAcUPaZ4ss-JkY7eQQV5MbwHPMrWe3ks5qOy59gaJpZM4I8doC
.

[rakyll]

An optional argument to Open sounds best to me.