modm::PeriodicTimer::execute() is very slow on first call after a longer pause

[chris-durand]

modm::PeriodicTimer::execute() returns the passed cycles since the timer last expired. It computes them in a counting loop:

size_t
execute()
{
	if (GenericTimeout<Clock, Duration>::execute())
	{
		size_t count{0};
		if (this->_interval.count())
		{
			const auto now = this->now();
			while(1)
			{
				this->_start += this->_interval;
				const auto diff{now - this->_start};
				if (diff != this->_interval) count++;
				if (diff < this->_interval) break;
			}
		}
		else {
			this->_start = this->now();
			count = 1;
		}
		this->_state = this->ARMED;
		return count;
	}
	return 0;
}

In case the timer hasn't been used for a longer period of time the first run of execute() afterwards will be very slow. For a 2 hour break and a 5 ms period timer it blocks for 12ms on a 480 MHz H7. That equates to about 1.4 million counting cycles. It can be worked around by calling restart() after a longer delay, but I think this behaviour is unexpected and rather undesirable.

My application doesn't require that counter. To quickly work around the issue in my code I've implemented a simplified version that only does:

template<class Clock, class Duration>
class GenericPeriodicTimer : public modm::GenericTimeout<Clock, Duration>
{
public:
    using modm::GenericTimeout<Clock, Duration>::GenericTimeout;

    bool execute()
    {
        if (GenericTimeout<Clock, Duration>::execute() {
            this->_start = this->now();
            this->_state = this->ARMED;
            return true;
        }
        return false;
    }
};

I'm unsure what's the best way of addressing this in the modm implementation preserving the current functionality. Using division to calculate the missed cycles would be less efficient in the common case. Maybe only run one step of the loop and then do division if more cycles were skipped?

[salkinium]

Maybe only run one step of the loop and then do division if more cycles were skipped?

Yes, but keep in mind that CM0 and AVR don't have hardware division.

We could also implement it as you did, and rename the current function to execute_count(). The timer period will then increase under load, ie. the latency of calling execute() will be added through this->now(). Therefore I would still do one iteration of the loop in the new execute(), as this will cover most common use cases (but still be efficient for yours).