1
\$\begingroup\$

I'm currently writing an API for our codebase that allows us to use an Arduino Leonardo for our tests. This code is uploaded to the Arduino, and we read through the analog for work.

As stated in the docstring, this function calls a passed function once a threshold has been met. However, I need there to be a delay between function calls, as signals read from the analog can fluctuate and cause the callback function to be called more than expected. So, I devised a solution that requires the signals to depreciate 1/10 of the threshold value in order for callback to be executed again. The secondFactor is if the user wants to define their own limit of when they want the function to be called again. For example:

NeuroBoard board;
board.setTriggerOnEvenlope(700, Serial.println("Reached!");

This executes the statement once 700 is reached. Then once 630 is reached (700 - (700 x .10)), it set a boolean flag so that once 700 is reached again, it will be called. For another example:

NeuroBoard board;
board.setTriggerOnEnvelope(700, Serial.println("Reached!"), 500);

Same as above, however the boolean flag will only be set once the signals reach 500.

I hope I've explained everything so that it's easily understandable. If anything is unclear, please leave a comment and I'll address it! Below is the code:

NeuroBoard.hpp (code omitted for brevity)

class NeuroBoard {

    private:
        bool thresholdMet = false;

    public:
        /**
         * Calls the passed function when the envelope value is greater 
         * than the passed threshold.
         * 
         * @param threshold Threshold for envelope value.
         * @param callback Function to call when threshold is reached.
         * @param secondFactor Optional parameter for the second threshold the data must pass.
         * 
         * @return void.
        **/
        void setTriggerOnEnvelope(const unsigned int& threshold, void callback(), const unsigned int& secondFactor=0);
}

NeuroBoard.cpp (code omitted for brevity)

The ISR is basically a timer that runs on the Arduino. I've configured it so every second I get about ~50,000 readings from the analog per second. The envelopeValue is the highest reading recorded, then subtracted by one every time a reading isn't equal to or greater than it.

int envelopeValue;

ISR (TIMER1_COMPA_vect) {
    int reading = analogRead(channel);
    envelopeValue = (reading >= envelopeValue) ? reading : envelopeValue - 1;
}

void NeuroBoard::setTriggerOnEnvelope(const unsigned int& threshold, void callback(), const unsigned int& secondFactor=0) {

    // TODO: Call callback when passed threshold is met by envelope value.

    const int THRESHOLD_SCALE_FACTOR = threshold / 10;
    int secondThreshold = secondFactor;

    if (secondFactor == 0) {
        secondThreshold = threshold - THRESHOLD_SCALE_FACTOR;
    }

    if (envelopeValue >= threshold) {
        if (!this->thresholdMet) {
            this->thresholdMet = true;
            callback();
        }
    } else {
        if (envelopeValue <= secondThreshold) {
            this->thresholdMet = false;
        }
    }

}

Note: Because this code is for an Arduino, I cannot use any standard library functions or imports. However, because this section of our code is pure c++, I don't think it will be an issue. But please consider that if you decide to write an answer. Thanks!

\$\endgroup\$
1
\$\begingroup\$

Pass small trivial types by value.

void f(unsigned ui);  // GOOD
void f(const unsigned& ui);  // WORSE

C and C++ don't do parameters of function type; they do parameters of function pointer type.

void f(void callback());     // BAD
void f(void (*callback)());  // GOOD

See Linus Torvalds' take on "parameters of array type", and then apply the same attitude to "parameters of function type." Don't write them. Write what you mean, instead.


Prefer signed int over unsigned int, unless you're doing bit-twiddling. Especially in your case, where the user might legitimately pass in a secondThreshold of zero or negative. See "The unsigned for value range antipattern" (2018-03-13).


Default function arguments are the devil. Prefer overloading:

void setTriggerOnEnvelope(int threshold, void (*cb)(), int cooldown);

void setTriggerOnEnvelope(int threshold, void (*cb)()) {
    return setTriggerOnEnvelope(threshold, cb, threshold - threshold/10);
}

Notice how I just shortened your original code by about 7 lines, because now you don't need all those helper variables (secondFactor, THRESHOLD_SCALE_FACTOR) and you don't need any mutation.


In fact, you should consider giving those two functions different names. Right now, it is much too easy for a careless programmer or refactorer to write

board.setTriggerOnEnvelope(700, [](){ Serial.println("Reached!"); });

when they actually meant

board.setTriggerOnEnvelope(700, [](){ Serial.println("Reached!"); }, 350);

If the two-argument version were named setTriggerOnEnvelopeWithDefaultCooldown, it'd be a lot harder for the programmer to run into a bug by forgetting the right number of arguments. See "Inheritance is for sharing an interface (and so is overloading)" (2020-10-09).


// TODO: Call callback when passed threshold is met by envelope value.

This TODO comment seems "done", doesn't it? Anyway, you should certainly not have any TODO comments left in code that you're posting to the CodeReview Stack Exchange, because we explicitly don't review non-working code.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the answer! The TODO comments are just a little reminder what the function needs to do so I don't have to keep flipping back to the documentation. The overloading tip works great, thanks! \$\endgroup\$
    – Linny
    Dec 23 '20 at 5:39
  • 1
    \$\begingroup\$ IIUC, you're saying that you habitually write comments of the form // TODO: Find the mean of the inputs where a typical programmer would write // Find the mean of the inputs? Don't do that! Reserve TODO (and also FIXME) for commenting on things that are actually to do in the codebase, like // TODO: find an algorithm that handles overflow better. (Imagine you woke up to find your spouse had gone out for the day and left behind a list of tasks labeled "TO DO"... but it was mostly stuff they'd already done. Confusing, right?) \$\endgroup\$ Dec 23 '20 at 15:21

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.