suggested update: provide a default constructor about responsiveanalogread

[edgar-bonet]

I would like to “vote” for this issue, but I suggest renaming it “Provide a default constructor”, for two reasons:

1. The lack of a default constructor doesn't prevent these objects to be class members

You can use the provided constructor in the initialization of your class, within an initializer list:

class MyClass {
    public:
        MyClass(int pin) : analog_read(pin, true) {}
    private:
        ResponsiveAnalogRead analog_read;
};

Although a default constructor would for sure make your life easier.

2. The usefulness of a default constructor goes beyond using the objects as class members

For example, one may want to create an array of such objects to be initialized later, in setup():

ResponsiveAnalogRead analog_reads[ANALOG_INPUT_COUNT];

It would also make the API nicer when using an external ADC. Currently, the README suggests explicitly specifying pin number 0, which works but is kind of weird.

It would also avoid issues such as #8, which notes that, when using an external ADC, “the constructor still needs a pin, and messes with the pin mode.”

By the way, the fact that the constructor messes with the pin mode is still relevant. Ideally, the default constructor should not touch the pin mode. This should be left to a begin() or setPin() method.

As a side note, in Arduino land, the method used to initialize an object is conventionally called begin(), rather than setup().

[dxinteractive]

Hi @kschaffer and @edgar-bonet , really sorry I've missed this one. I've begun rewriting this library as the API of the current one is really quite bad, even if the algorithm in it works ok. The new version will not require any arguments to be passed to the constructor, and likely won't even have any contructor parameters at all. I can keep you updated in this thread if you like.

[kschaffer]

Thanks for the followup. I would love to know the general timeline you anticipate. We're depending on this library for a project that we are hoping to publicly "release" (in an amateur, DIY context of course) this summer. Temporarily, we've just copy-pasted the code into our own repository to use our own version of it (keeping the license intact). I was going to switch so that we are using my own forked version of your library as a "submodule," so that there is at least some formal github path back to the original. A far preferable approach would be to use the "official" version as a submodule, so if it's available I'd love to do that. I also would be happy to contribute some effort to the API upgrade if you just need some basic changes made.

[kschaffer]

Renamed - my wording was sloppy in the original issue/request, so thanks for pinning things down much more precisely. Initially I was not sure if Arduino supported initializer lists or not, since I kept reading that you needed to use begin() functions. But yes, it does. However, there can still be issues if the constructor involves hardware configuration calls, like in this case. And, like you said, there are other reasons to want a default constructor anyway.

I have a forked version in which I implemented the change using setup() instead of begin() but I can easily change the name, to be consistent with Arduino conventions (such as they are). Pull request maybe on the way? (I'm a casual git user still feeling out the typical protocol... but I think that's the thing to do??).

[edgar-bonet]

Yes, pull requests are the usual way to collaborate in GitHub. Make sure you mention the issue in it. Writing “closes #11” in a commit message would automatically close this issue if the pull request is accepted.

[kschaffer]

I went ahead and tried a pull request with this change. I am still not sure I have that set up right... but I would love if we could get this change implemented ASAP, even if there are going to be more involved upgrades soon. Working with the Arduino library manager and all, it would be so much cleaner if we can use the "official" library rather than asking users to install a custom version. This change would make that possible, even while we wait for more extensive changes.

[dxinteractive]

Estimated timeframe would be to have a new version by months end. Your PR looks good, I can adjust the docs and republish to platform.io once I can give it a bit of a go. Actually, considering this is a bit of a stop-gap solution I wouldn't mind overloading the constructor to also accept the old constructor arguments too. That way the existing API will continue to work, and I don't have to bump a major library version to indicate breaking changes have taken place.

[dxinteractive]

Hi @kschaffer and @edgar-bonet , this was pulled in and bumped, and is now available in version 1.2.0 https://platformio.org/lib/show/913/ResponsiveAnalogRead. I'll update the readme soon. Let me know if there is anything else and if not I'll close the issue off.