luni64 / TeensyTimerTool

Generic Interface to Teensy Timers
MIT License
82 stars 18 forks source link

Crashing when beginning timers #24

Closed mnissov closed 1 year ago

mnissov commented 1 year ago

I'm trying to write a class which captures a period timer (PIT) and a oneshot timer (GPT1) but there is some weird behavior depending on where the periodic is begin() which I can't quite understand.

My demo code:

HEADER

#pragma once

#include <Arduino.h>
#include <TeensyTimerTool.h>

class Demo
{
private:
  TeensyTimerTool::OneShotTimer one_shot_timer_;
  TeensyTimerTool::PeriodicTimer timer_;
  uint8_t pin_;
  unsigned long interval_;

public:
  Demo(const uint8_t & pin, const unsigned long & interval);
  ~Demo() {}

  void begin();
  void callback();
};

Demo::Demo(const uint8_t & pin, const unsigned long & interval)
: one_shot_timer_(TeensyTimerTool::GPT1),
  timer_(TeensyTimerTool::PIT),
  pin_(pin),
  interval_(interval)
{
  pinMode(pin_, OUTPUT);
}

void Demo::begin()
{
  timer_.begin([this]() { callback(); }, interval_, false);
  timer_.start();
}
void Demo::callback() { digitalWriteFast(pin_, !digitalReadFast(pin_)); }

CPP

#include "demo.hpp"

Demo t1(LED_BUILTIN, 1'000'000);
void setup() { t1.begin(); }

void loop() {}

As it stands this works fine, causing the LED on my teensy4.1 to blink at 1 Hz

Crash 1

Moving the begin line into the constructor, such that the header is changed to:

Demo::Demo(const uint8_t & pin, const unsigned long & interval)
: one_shot_timer_(TeensyTimerTool::GPT1),
  timer_(TeensyTimerTool::PIT),
  pin_(pin),
  interval_(interval)
{
  pinMode(pin_, OUTPUT);
  timer_.begin([this]() { callback(); }, interval_, false);
}

void Demo::begin() { timer_.start(); }

Crash 2

I want to rewrite the begin function to take over more responsibility rather than assigning these things in the constructor. Changing the files as such also causes the LED to no longer blink

#pragma once

#include <Arduino.h>
#include <TeensyTimerTool.h>

class Demo
{
private:
  TeensyTimerTool::OneShotTimer one_shot_timer_;
  TeensyTimerTool::PeriodicTimer timer_;
  uint8_t pin_;
  unsigned long interval_;

public:
  Demo();
  ~Demo() {}

  void begin(const uint8_t & pin, const unsigned long & interval);
  void callback();
};

Demo::Demo() : one_shot_timer_(TeensyTimerTool::GPT1), timer_(TeensyTimerTool::PIT)
{
  pinMode(pin_, OUTPUT);
}

void Demo::begin(const uint8_t & pin, const unsigned long & interval)
{
  pin_ = pin;
  interval_ = interval;

  timer_.begin([this]() { callback(); }, interval_, false);
  timer_.start();
}
void Demo::callback() { digitalWriteFast(pin_, !digitalReadFast(pin_)); }
#include "demo.hpp"

Demo t1;
void setup() { t1.begin(LED_BUILTIN, 1'000'000); }

void loop() {}

Conclusion

Resetting to the original files after uploading either of the crashing setups instantly results in the LED blinking again.

Note this is with the v1.0.0 release of TeensyTimerTool and a teensy 4.1

luni64 commented 1 year ago

Crash 1 In embedded programming it is not a good idea to access the hardware in constructors because constructors of global objects may be called before the complete hardware is intialized. Moving timer_.begin into the constructor will generate access to some timer registers and I assume that the corresponding clocks are not yet running. Some things work without problems (e.g. pinMode), others don't. This problem is the reason why all those begin functions exist, they move the hardware access out of the early called constructors into code which will be called after the hardware is set up completely.

Crash 2 This actually is not a crash. Since you moved the storing of the pin number from the constructor to the begin function, pin_ is not yet set in the constructor and may have some arbitrary number. Thus, pinMode will set the mode of the wrong pin and you won't see the LED blinking (its pin mode is still INPUT). Just move the call to pinMode to the begin function (or set _pin in the constructor) and you should be good.

Hope that helps

mnissov commented 1 year ago

Sorry for the delay.

Both these points make sense, and implementing so seems to resolve the problems in the basic case. However the problem persists in the more complicated implementation. Something closer resembling the final, intended functionality is:

#include "TriggerGenerator.hpp"

// configuration parameters
const int g_fast_output_pin = 0;               // pin number
const int g_fast_output_interval = 5000 * 1;   // us
const int g_fast_output_high_time = 10;        // us
const int g_slow_output_pin = 1;               // pin number
const int g_slow_output_interval = 50000 * 1;  // us
const int g_slow_output_high_time = 250;       // us

TriggerGenerator trigger_fast;
TriggerGenerator trigger_slow;

void setup()
{
  Serial.begin(9600);

  pinMode(LED_BUILTIN, OUTPUT);
  digitalWrite(LED_BUILTIN, HIGH);

  trigger_fast.begin(0, g_fast_output_pin, g_fast_output_interval, g_fast_output_high_time);
  trigger_slow.begin(1, g_slow_output_pin, g_slow_output_interval, g_slow_output_high_time);
}

int g_led_state = LOW;
unsigned long g_prev_millis = 0;
void loop()
{
  unsigned long current_millis = millis();
  if (current_millis - g_prev_millis > 1000) {
    g_prev_millis = current_millis;

    g_led_state = g_led_state == LOW ? HIGH : LOW;
    digitalWriteFast(LED_BUILTIN, g_led_state);
  }
}
 #pragma once

#include <Arduino.h>
#include <TeensyTimerTool.h>

class TriggerGenerator
{
public:
  TriggerGenerator();
  ~TriggerGenerator() {}

  void begin(
    const uint8_t & type, const uint8_t & pin, const unsigned long & interval,
    const unsigned long & width);

private:
  void TriggerFallingEdge();
  void PeriodicCallback();
  void OneShotCallback();

  TeensyTimerTool::OneShotTimer one_shot_timer_;
  TeensyTimerTool::PeriodicTimer periodic_timer_;

  uint32_t count_;

  // Parameters
  uint8_t type_;
  uint8_t pin_;
  unsigned long interval_;
  unsigned long width_;
};

TriggerGenerator::TriggerGenerator()
: one_shot_timer_(TeensyTimerTool::GPT1), periodic_timer_(TeensyTimerTool::PIT)
{
}

void TriggerGenerator::begin(
  const uint8_t & type, const uint8_t & pin, const unsigned long & interval,
  const unsigned long & width)
{
  type_ = type;
  pin_ = pin;
  interval_ = interval;
  width_ = width;

  pinMode(pin_, OUTPUT);

  one_shot_timer_.begin([this]() { this->OneShotCallback(); });
  periodic_timer_.begin([this]() { this->PeriodicCallback(); }, interval_);
}

inline void TriggerGenerator::TriggerFallingEdge() { one_shot_timer_.trigger(width_); }

inline void TriggerGenerator::OneShotCallback() { digitalWriteFast(this->pin_, LOW); }

inline void TriggerGenerator::PeriodicCallback()
{
  digitalWriteFast(pin_, HIGH);

  TriggerFallingEdge();
}

While still not "finished", this is largely representative of a finalized version for my purposes. And this suffers from similar problems, primarily that only the timer which is started through begin() first will actually show any output on my oscilloscope.

E.g. in the above case, pin 0 shows a 200Hz signal and one expects 20Hz on pin 1, but it is constantly high.

Edit: Note also, if I reverse the order to be

  trigger_slow.begin(1, g_slow_output_pin, g_slow_output_interval, g_slow_output_high_time);
  trigger_fast.begin(0, g_fast_output_pin, g_fast_output_interval, g_fast_output_high_time);

then I see a 20Hz signal on pin 1 and constant high on pin 0.

mnissov commented 1 year ago

Realized I was using the same timer for both instances. Switching from GPT to something with multiple channels (e.g. TMR1) works.

luni64 commented 1 year ago

It is always a good idea to place a

TeensyTimerTool::attachErrFunc(TeensyTimerTool::ErrorHandler(Serial)) ;

in setup() while developing. This would spit out an error message in such and other cases. (You can use any stream as output channel).