bxparks / AceButton

An adjustable, compact, event-driven button library for Arduino that debounces and dispatches events to a user-defined event handler.
MIT License
385 stars 37 forks source link

Singleton system button config can be an issue #41

Closed aleroy closed 4 years ago

aleroy commented 4 years ago

Why is the default config implemented as a Singleton? If you have more than one button and they both point to the same config, then they also all point to the same handler. So you get several buttons that provide a single behavior. I'm just failing to see a use case that would benefit from this.

Wouldn't a factory method be more appropriate? ...or perhaps just making the handler a member of the button class? For now, I can simply assign my own instantiations. So, this is more of a comment than an issue (although, it could be an issue for unsuspecting users).

There's some nice code here. Thanks for sharing. It's refreshing to see some OOP in this world of microcontrollers.

bxparks commented 4 years ago

If you have multiple buttons, you can distinguish them through their pin number. See "Option 2: Multiple Button Discriminators" of https://github.com/bxparks/AceButton#multiple-buttons. You could also use multiple instances of ButtonConfig, see Option 1 in the same section, but in most cases, it's simpler to use just a single EventHandler.

You must have a different mental model of buttons than I do, because having multiple buttons share a single ButtonConfig is the common case in my mind. See the example code in https://github.com/bxparks/AceButton/tree/develop/examples/TunerButtons, where you have 6 radio station buttons, sharing a single ButtonConfig. They all have the same timing parameters, they all essentially do the same thing, except for selecting a different radio station.

The situations where you'd want different ButtonConfig for different buttons seems to me more rare, for example, if you want one button to have a different delay for the LongPress event than another, and you want a slightly different click delay for a DoubleClick on one button versus another.

With regards to why the EventHandler is not a method inside AceButton, I didn't want to make AceButton be polymorphic, which adds a vtable pointer (extra RAM), and I didn't want to force the end-user to subclass the AceButton class just to implement a different EventHandler, that seemed like a lot of unnecessary boilerplate code. Furthermore, the EventHandler often turns into the intersection of the AceButton code and the external world (e.g. LEDs, GPIO pins timers, etc). It didn't feel right to pull in all that external dependencies into the whatever subclass of AceButton would be created by the library consumer.

I'm not sure what you mean by a factory method in this context, assuming that we are using the term 'factory method' is the same way. Maybe some code can help explain what you mean. Keep in mind that I want to avoid all dynamic allocation of memory in an embedded environment.

With regards to the OOP, there are people who are strongly against it, especially using the virtual keyword in an embedded environment. I personally find OOP methodologies quite useful as long as we don't abuse it, and we keep in mind the extremely limited resources of our embedded chips. The Arduino programming framework uses OOP, so it's also kinda hard to avoid it completely.

seisfeld commented 4 years ago

Brian, as a non expert embedded developer, I have to say that just reading your replies and documentation etc. is really “inspirational“ if that makes any sense. Probably doesn’t. I don’t know why. 🤓

bxparks commented 4 years ago

Thanks @seisfeld!

But full disclosure, I'm not an embedded developer either. This is a hobby. I usually work with systems with millions users, thousands of cores, and TB's storage and RAM. But it's fun to work on systems with just a few kB of RAM, where shaving a few bytes can be critical.

seisfeld commented 4 years ago

You are welcome. A hobby eh? 😉 This lib looks so damn professional... but considering your day job I am not surprised at all now hehe. 🤯

aleroy commented 4 years ago

Thank you for the excellent explanation! It makes much more sense now.

Looking back, I may not have been thinking clearly when I originally posted. I'm not sure what I meant by factory method either. I wanted separate handlers for each button. It surprised me to find the singleton rather than a copy or new ButtonConfig instance. Now I see how that is unnecessary.

I'll go ahead and close this now. Thank you again for your prompt response and for this contribution!

bxparks commented 4 years ago

Yeah, the separation of the ButtonConfig from the AceButton is probably one of the trickier parts of this library for new users. (In my defense I try to explain that here: https://github.com/bxparks/AceButton#buttonconfig-class). Most people expect the event handler to be part of the button, and the various timing parameters to be part of the button too. And it occurs to me that using normal OOP on systems with GBs of RAM, that's probably how it'd be implemented. But on an embedded evironment, we have to make some compromises to save resources.

If it helps you think about this better, you can consider ButtonConfig to be the objectification of the class object of the AceButton class. When you create multiple ButtonConfig, you are essentially creating multiple types (or classes) of buttons. In C++ however, classes are not first-class objects, so we have to use another class to represent this object. And in fact, a pure OOP would have a createButton() factory method on the ButtonConfig object, which returns buttons bound to that particular ButtonConfig instance. That's a lot cleaner from an abstract theoretical perspective, but it is not really practical on a chip with 2kB of RAM.

Hope that helps explain my thought process that went into this library.