feat(timer): Improve periodicity of the software timer

[finger563]

Description

Motivation and Context

How has this been tested?

Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Update
• Hardware (schematic, board, system design) change
• Software change

Checklist:

• My change requires a change to the documentation.
• I have added / updated the documentation related to this change via either README or WIKI

Software

• I have added tests to cover my changes.
• I have updated the .github/workflows/build.yml file to add my new test to the automated cloud build github action.
• All new and existing tests passed.
• My code follows the code style of this project.

Hardware

• I have updated the design files (schematic, board, libraries).
• I have attached the PDFs of the SCH / BRD to this PR
• I have updated the design output (GERBER, BOM) files.

[github-actions]

✅Static analysis result - no issues found! ✅

[Copilot]

Pull request overview

This PR updates the espp::Timer component to improve periodic scheduling accuracy by tracking an explicit wakeup_time_ (instead of waiting relative to the start of each callback), and extends the timer example to exercise periodicity under simulated long callbacks.

Changes:

• Add wakeup_time_-based scheduling to reduce drift and improve periodic alignment in the software timer.
• Introduce a mutex_ intended to protect timer state updates (period/delay/wakeup time).
• Update the timer example and its sdkconfig defaults to better demonstrate/measure timer periodicity (and reduce log verbosity noise).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
components/timer/src/timer.cpp	Implements wakeup_time_ scheduling and adds locking around some timer state updates.
components/timer/include/timer.hpp	Adds mutex_ and wakeup_time_ members to support the new scheduling approach.
components/timer/example/sdkconfig.defaults	Tunes example configuration (optimization + 1ms FreeRTOS tick) to support periodicity testing.
components/timer/example/main/timer_example.cpp	Adds a periodicity-testing block and adjusts timer log levels / minor constants.

Suppressed comments (1)

components/timer/src/timer.cpp:184

• cv.wait_until(lock, wakeup_time_, ...) uses wakeup_time_ directly without synchronization with the new state mutex, and the subsequent wakeup_time_ += period_ can race with set_period() (and may apply a partially-updated period). Snapshot wakeup_time_/period_ under mutex_ before waiting, and update wakeup_time_ under the same mutex after the wait.

  {
    std::unique_lock<std::mutex> lock(m);
    cv.wait_until(lock, wakeup_time_, [&task_notified] { return task_notified; });
    // reset the task_notified flag
    task_notified = false;
  }
  // now that we've waited, make sure the next wakeup time is the next multiple
  // of the period after the last
  wakeup_time_ += period_;

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Suppressed comments (5)

components/timer/src/timer.cpp:69

• This is a classic check-then-act race: two threads can call start() concurrently, both observe !is_running(), and both attempt task_->start(). Even if task_->start() fails for the second caller, this can still lead to inconsistent initialization (e.g., wakeup_time_ being overwritten). Consider serializing start() (e.g., guard the whole start sequence with mutex_) or using running_.compare_exchange_strong(false, true) to ensure only one caller proceeds, and revert running_ to false if task_->start() fails.

bool Timer::start() {
  if (is_running()) {
    logger_.info("timer is already running, not starting");
    return true;
  }

components/timer/src/timer.cpp:84

• This is a classic check-then-act race: two threads can call start() concurrently, both observe !is_running(), and both attempt task_->start(). Even if task_->start() fails for the second caller, this can still lead to inconsistent initialization (e.g., wakeup_time_ being overwritten). Consider serializing start() (e.g., guard the whole start sequence with mutex_) or using running_.compare_exchange_strong(false, true) to ensure only one caller proceeds, and revert running_ to false if task_->start() fails.

  if (task_->start()) {
    running_ = true;
    return true;
  }

components/timer/include/timer.hpp:104

• Changing start() overloads from void to bool is a breaking public API change (callers that take a pointer-to-member, override wrappers, or rely on the exact signature will break). If backward compatibility is important, consider keeping the old void start()/void start(delay) as wrappers (possibly deprecated) that call the new bool versions, or introduce differently named methods (e.g., try_start) to avoid a signature-breaking change.

  /// @return true if the timer was started or is already running, false if the
  ///         timer could not be started.
  bool start();

components/timer/include/timer.hpp:115

• Changing start() overloads from void to bool is a breaking public API change (callers that take a pointer-to-member, override wrappers, or rely on the exact signature will break). If backward compatibility is important, consider keeping the old void start()/void start(delay) as wrappers (possibly deprecated) that call the new bool versions, or introduce differently named methods (e.g., try_start) to avoid a signature-breaking change.

  /// @return true if the timer was started or restarted, false if the timer
  ///         could not be started.
  bool start(const std::chrono::duration<float> &delay);

components/timer/src/timer.cpp:79

• Using delay_float > 0 / period_float > 0 to decide scheduling behavior ties logic to the float representation (which is primarily for logging) rather than the authoritative std::chrono::microseconds values. For clearer intent and fewer precision/representation edge cases, prefer checking delay_.count() > 0 and period_.count() > 0 here (and similarly elsewhere).

    if (delay_float > 0) {
      wakeup_time_ += delay_;
    }
    if (period_float > 0) {
      wakeup_time_ += period_;
    }

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

Suppressed comments (3)

components/timer/include/timer.hpp:104

• Changing start() overloads from void to bool is a source-level breaking API change. If this component is consumed outside this module, either (a) keep the void versions and add new try_start()/start_checked() methods, or (b) clearly document the breaking change (e.g., changelog/release notes) and ensure call sites are updated to handle/ignore the return intentionally.

  bool start();

components/timer/include/timer.hpp:115

• Changing start() overloads from void to bool is a source-level breaking API change. If this component is consumed outside this module, either (a) keep the void versions and add new try_start()/start_checked() methods, or (b) clearly document the breaking change (e.g., changelog/release notes) and ensure call sites are updated to handle/ignore the return intentionally.

  bool start(const std::chrono::duration<float> &delay);

components/timer/src/timer.cpp:203

• Catching up missed periods by incrementing in a while loop can become very costly if the system is delayed for a long time or period is small (potentially thousands/millions of iterations). Prefer computing the number of missed intervals in O(1) (e.g., based on (end - wakeup_time_) / period) and jumping wakeup_time_ forward in a single step.

      std::lock_guard<std::recursive_mutex> lock(mutex_);
      while (wakeup_time_ < end) {
        wakeup_time_ += period;
      }
    }

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

components/timer/src/timer.cpp:146

• The log line reports the input period.count() (seconds) instead of the actual stored period after duration_cast to microseconds. For sub-microsecond inputs, the stored period_ may truncate to 0 (one-shot) while the log still prints a non-zero value, which is misleading for debugging. Consider logging a local copy of period_float computed from period_ (captured under the mutex) so the message reflects the real schedule.

    std::lock_guard<std::recursive_mutex> lock(mutex_);
    period_ = std::chrono::duration_cast<std::chrono::microseconds>(period);
    period_float = std::chrono::duration<float>(period_).count();
  }
  logger_.info("Period set to {:.3f} s", period.count());

components/timer/src/timer.cpp:51

• Input validation for negative period/delay is applied only in the AdvancedConfig constructor. The Config constructor (which is also used by the example/test suite via Timer({...})) still allows negative period_ / delay_, leading to inconsistent behavior (e.g., negative delay currently results in “no delay” but leaves delay_ negative in state). Consider applying the same clamping/validation logic in the Config constructor as well so both construction paths behave consistently.

  if (period_float < 0) {
    logger_.warn("period cannot be negative, setting to 0");
    period_ = std::chrono::microseconds(0);
    period_float = 0;
  }

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

components/timer/src/timer.cpp:136

• The start() info log computes "Will wake up in" using wakeup_time_ (which includes the period when period>0). For timers configured with both delay and period, this reports delay+period even though the first callback fires after just the delay, making the log misleading.

    local_wakeup_time = wakeup_time_;
    local_start_time = start_time_;
  }
  if (task_->start()) {
    logger_.info("Started with period {:.3f} s and delay {:.3f} s. Will wake up in {:.3f} s",

components/timer/example/main/timer_example.cpp:687

• This comment says set_period(0) stops "after the current callback", but the Timer implementation waits until the current period’s scheduled wakeup before the next loop iteration and only then observes the new period. So set_period(0) can still allow one more scheduled wake/callback depending on when it is called. The comment should match the actual semantics to avoid misleading readers.

    // 13) set_period(0) turns a running periodic timer into a one-shot (it stops
    //     after the current callback).

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

components/timer/src/timer.cpp:114

• start() sets running_=true before attempting task_->start(), and concurrent callers that observe running_=true return success immediately. If task_->start() fails (OOM/invalid config), a concurrent start() can still return true even though the timer never actually started. Consider a 3-state start flag (stopped/starting/running) or otherwise synchronizing concurrent start() callers so the return value reflects whether the task really started.

  bool expected = false;
  if (!running_.compare_exchange_strong(expected, true)) {
    logger_.info("timer is already running, not starting");
    return true;
  }

components/timer/src/timer.cpp:285

• The "skipping periods" warning can fire on every iteration during sustained overload (e.g. when callbacks consistently overrun by multiple periods), which can spam logs and further degrade timing. Since the overrun warning above is rate-limited and the docs mention a rate-limited warning, this should be rate-limited as well.

    // only log if we are skipping more than one period
    if (n > 1) {
      logger_.warn("Already passed expected wakeup time, skipping {} periods", n);
    }