2
\$\begingroup\$

I have two similar libraries for use in an embedded system (Teensy, sort of like Arduino). One is for debouncing a digital pin, the other is for reading a capacitive pin as if it is a debounced digital pin.

They have similar APIs:

  • constructor (with pin number as parameter)
  • void update() hides reading the pin and internal state
  • bool pressed() indicates a "button press" occurred since the last update()
  • bool released() indicates a "button release" occurred since last update()

The main difference between them is in their respective update() implementations. SimpleSwitch.h is based on a timeout; CapSwitch.h is based on a moving average.

How should I factor out a parent class, so that CapSwitch and SimpleSwitch can inherit from some sort of GeneralPurposeSwitch?

Here are the two libraries, minus #includes and comments:

SimpleSwitch.h

class SimpleSwitch
{
    public:
        // Default constructor. Takes a digital pin.
        SimpleSwitch(uint8_t p)
        {
            pin = p;
            pinMode(pin,INPUT_PULLUP);
            previousState = digitalRead(pin);
            currentState = previousState;
            hiLoTransition = false;
            acceptNextPress = true;
            currentTime = 0;
        }
        ~SimpleSwitch() {}

        // update internal state
        void update(uint32_t now = micros())
        {
            currentTime = now;
            // Update after DEBOUNCE_INTERVAL or more microseconds.
            if (currentTime - previousTime >= DEBOUNCE_INTERVAL) {
                previousTime += DEBOUNCE_INTERVAL;
                currentState = digitalRead(pin);

                // Okay to update the value returned by "pressed()".
                if (acceptNextPress) {
                    acceptNextPress = false;
                    hiLoTransition = previousState && !currentState; // reset in pressed()
                    loHiTransition = !previousState && currentState; // reset in released()
                }

                // Remember switch state.
                previousState = currentState;
            }
        }

        // Return true once, then always false until next valid transition.  Sometimes called
        // an "immediate" debounce, because it responds to the first transition and ignores further
        // transitions for a set period.
        bool pressed() { return reset_edge(hiLoTransition); }
        bool released() { return reset_edge(loHiTransition); }

        bool getState() { return currentState; } // subject to bounce

    private:
        // Takes an edge (rising or falling) BY REFERENCE and resets it.
        // Edge remains false until next update().
        bool reset_edge(bool& edge)
        {
            bool t = edge;
            edge = false;
            acceptNextPress = true;
            return t;
        }

        bool acceptNextPress,currentState,previousState,hiLoTransition,loHiTransition;
        uint8_t pin;
        uint32_t currentTime,previousTime;
        const uint32_t DEBOUNCE_INTERVAL{10000}; // 10ms

};

CapSwitch.h

class CapSwitch 
{
 public:
    // constructor requires touch-enabled pin number
 CapSwitch(uint8_t _pin) : pin(_pin),THRESH(200){}
    ~CapSwitch(){}

    void update(void)
    {
        int val = touchRead(pin);
        static int idx{0};
        buf[++idx%BUFLEN] = val; // update ring buffer
        if (accept) { // queue empty
            accept = false;
            double avg = get_avg();
            if (val - avg > THRESH) { bstate = false; } // cap. increasing
            if (avg - val > THRESH) { bstate = true; } // cap. decreasing
            hilo = prev_state && !bstate; // falling edge
            lohi = bstate && !prev_state; // rising edge
        }
        prev_state = bstate;
    }

    bool pressed(void) { return reset_edge(hilo); }
    bool released(void) { return reset_edge(lohi); }

 private:
    bool reset_edge(bool& edge)
    {
        bool t = edge;
        edge = false;
        accept = true;
        return t;
    }

    double get_avg(void)
    {
        double sum{0};
        for (uint8_t i=0;i<BUFLEN;++i) { sum+=buf[i]; }
        return sum/BUFLEN;
    }

    uint8_t pin;
    const int THRESH;
    static const uint8_t BUFLEN{7};
    int buf[BUFLEN]{0};
    bool bstate,prev_state,hilo,lohi;
    bool accept{true};
};
\$\endgroup\$
3
\$\begingroup\$

You asked, "How should I factor out a parent class, so that CapSwitch and SimpleSwitch can inherit from some sort of GeneralPurposeSwitch?" — but don't you already know how to do that?

class GeneralPurposeSwitch {
    virtual void update() = 0;
    virtual bool pressed() = 0;
    virtual bool released() = 0;
    virtual ~GeneralPurposeSwitch() = default;
};

The question I want to ask back to you is, "Why do you need CapSwitch and SimpleSwitch to inherit from some sort of GeneralPurposeSwitch?"

You shouldn't write OOP code for its own sake (in fact, you shouldn't write any kind of code "for its own sake", at least in a production codebase). You should start from some sort of problem in need of solution, and then write exactly the code that solves that problem.

For example, if your problem is that you won't know until runtime what kind of switch is hooked up to your hardware, then you might need this kind of code to work:

void f(GeneralPurposeSwitch& s) {
    ... s.update(); ...
}

int main(int argc, char **argv)
{
    if (argv[1] == std::string("cap")) {
        f(CapSwitch(0x18));
    } else {
        f(SimpleSwitch(0x23));
    }
}

In which case, sure, factor out a base class.

But if you don't need that kind of thing to work, then you probably shouldn't worry about adding code to make it work. Instead, just focus on making the two codepaths as understandable as possible.

For example, right now you've got parameters named p and _pin that both seem to be doing the same thing. Consider renaming them both to pin, so that a human reader who has already learned what the English word "pin" means will immediately know what's going on in your code, instead of first having to learn what p means (namely, "pin") and then later have to learn again what _pin means (namely, "pin") and then wondering why there are two different words for what seems like exactly the same concept.

You've got member variables named hilo and hiLoTransition. Consider renaming them both to the same thing.

And so on. Once the two codepaths start looking more similar, so that the human reader can look at one of them and then look at the other and say, "Okay, I see what's going on in CapSwitch by analogy with SimpleSwitch"... well, at that point, it might make sense to look at whether CapSwitch can reuse any of the actual code inside of SimpleSwitch — via a non-abstract base class, or CRTP, or any other design pattern of that general nature. But it's still very premature to worry about that, I'd say.

\$\endgroup\$
  • \$\begingroup\$ In this case I'll always know at compile-time what type of switch I'm using, so your advice of cleaning up for readability works for me. \$\endgroup\$ – hoosierEE Jan 4 '16 at 14:36
1
\$\begingroup\$

Since you've said you will always know the type of switch at compile time, you might want to consider what I'd think of as middle ground between the status quo and supporting run-time polymorphism.

The middle ground that seems obvious (at least to me) would be to factor the parts that are truly unique to a type of switch into a pair of classes, then have a small template that you instantiate over one of those classes, which provides a single interface to either type of switch. This lets you change between the two at compile time, so only one little bit of code needs to change, and avoids repeating code between the two where it isn't strictly necessary.

This isn't complete (by any means), but I hope it gives the general idea:

template <class SwitchType>
class Switch { 
    SwitchType t;
    int pin;
    bool prev_state;
public:
    Switch(int pin) : pin(pin) {
        // delegate to read correctly for the type of switch:
        prev_state = t.read(pin);
        // ...
    }

    // these appear to be common between the two:
    bool pressed(void) { return reset_edge(hilo); }
    bool released(void) { return reset_edge(lohi); }

    bool reset_edge(bool) {
        bool t = edge;
        edge = false;
        accept = true;
        return t;
    }
};

struct CapSwitch {
    int read(int pin) { 
        // capRead(pin);
    }
    // ...
};

struct SimpleSwitch { 
    int read(int pin) { 
        // digitalRead(pin);
    }
    // ...
};

So, the common functions end up in the template itself, and anything specific to one or the other ends up in the class for that specific type (with a front-end in the template the delegates appropriately) so client code works the same regardless of which type of switch is in use.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.