search

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

Multi-button input resistor ladder #43

Closed alf45tar closed 4 years ago

alf45tar commented 4 years ago

May I submit a Pull Request for adding LadderConfigButton class to your wonderful library?

Thanks alf45tar

bxparks commented 4 years ago

Hi, Can you explain what the LadderConfigButton is?

alf45tar commented 4 years ago

http://www.ignorantofthings.com/2018/07/the-perfect-multi-button-input-resistor.html https://hackaday.com/2012/03/02/r2r-ladder-connects-multiple-buttons-to-one-adc-pin/

bxparks commented 4 years ago

Got it. I thought you were talking about Ladder Logic in the context of PLC's, but that didn't make sense.

Initially, I was skeptical of adding support for buttons using a resistor ladder, because there are some fundamental assumptions in the code base that buttons are connected to digital inputs. But thinking about it some more, I think we could add an abstraction layer which is thin enough that it would fit nicely into the current architecture.

Since I will have to maintain and fix any bugs in your code in the future, there are a few things I would request, before merging in your code:

1) Design: We need to agree on the design of your ladder code. I have a rough design in my own mind. It involves one additional class called LadderConfig or LadderButtonConfig (not LadderConfigButton). This LadderButtonConfig would be somewhat similar with the existing EncodedButtonConfig class. 2) Code quality: I request unit tests for all major functionality, and this is a major functionality. I would expect the tests for this class to end up looking very much like tests/EncodedButtonConfigTest. I would like example sample code that's been verified in real-life, that shows how to use the new class. See examples/Encoded16To4Buttons/. I would like the new code to follow the same coding style as the existing code to preserve style consistency. 3) Documentation: I believe documentation is just as important as code. This means doxygen docs within the code, and a separate User documentation about how to use the new class. (See for example docs/binary_encoding)

If these expectations are ok with you, we can start discussing the design of the class. I'd be interested to know how you propose to handle 10-bit versus 12-bit ADC's, and how you would handle resistance variability and ADC calibration, for example, due to temperature fluctuations.

alf45tar commented 4 years ago

Did you check my fork of your repository? LadderButtonConfig is already implemented. I am not familiar with unit tests but I tested LadderButtonConfig in my PedalinoMini project and it works.

Documentation not ready yet but I know it is important.

10-bit or 12-bit is client specific. It is not requested any assumption by the library. The only hardware function used is AnalogRead. The analog resolution setup has to be done in the user code. The ladder analog table (AnalogButtons_t) is defined by user too according to his decision of analog resolution.

I think temperature fluctuations is out of scope of the library and must be manages by the user via ladder analog table.

Cheers alf45tar

bxparks commented 4 years ago

I didn't know you had a fork. I took a look at your LadderButtonConfig, and it's a good start. But it is not an implementation that I would incorporate into this library. I think it would take me longer and more effort to explain what I would like this code to look like, compared to just writing it myself.

So I have started my own implementation of this, because I think it's an interesting problem. When I push a new version of the AceButton library, you can continue to use your own implementation for your own project. But you may need to rename it to something else e.g. MyLadderButtonConfig, since my version will also be called LadderButtonConfig.

I'll keep this issue open so that I can reference it in my git commits.

Thanks for providing the motivation to work on this.

bxparks commented 4 years ago

This has been pushed out with v1.5. Closing.