Reorganize task scheduling and fix bug in use of volatile variables

[t0mpr1c3]

The suggestions by @jpcornil-git are:

• To change main.cpp to something like:

void setup() {
  // Objects running in async context
  GlobalCom::init();
  GlobalKnitter::init();
  GlobalLedGreen::init();
  GlobalLedYellow::init();
  GlobalBeeper::init();

  // Objects running in interrupt context
  GlobalEncoders::init(); 
}

void loop() {
  // Non-blocking methods/cooperative RR scheduling
  GlobalCom::update();
  GlobalKnitter::update();
  GlobalLedGreen::update();
  GlobalLedYellow::update();
  GlobalBeeper::update();
}

• fsm should really be part of the knitter object while isr code should move from knitter to encoders

• care should be taken to synchronize handover of machine state data between interrupt and main contexts

An example of a bug is the following:

At the moment this is done in the interrupt context (https://github.com/AllYarnsAreBeautiful/ayab-firmware/blob/v1.0-dev/src/ayab/knitter.cpp#L107) but it is not safe because if an interrupt fires between L402 and L403 of the same file, m_direction may change in-between.

To fix this, Knitter::update() should call, before running the fsm::update(), an encoder update() method that enter a critical section/disable interrupts, update machine state data, exit the critical section and return.

This bug is mostly hidden today as most of the code is executed after and interrupt/before the next one but if something takes more time than expected and delay fsm code such that it hits the next interrupt it may expose it (and a webserver will add more tasks moving forward).

[t0mpr1c3]

It's not possible to combine the Fsm object with Knitter because the FSM governs both the knitting and testing states.

I'm thinking that you could cache GlobalEncoders at the top of Fsm::dispatch() and only use the cached values in the methods called from Fsm::dispatch() like GlobalKnitter::knit(). That way, interrupt-driven updates to GlobalEncoders would not affect any of the derived values.

[jpcornil-git]

I would do the following:

• Move setUpInterrupt() from knitter.cpp to encoders.cpp

/*!
 * \brief Initialize interrupt service routine for Encoders object.
 */
void Encoders::setUpInterrupt() {
  // (re-)attach ENC_PIN_A(=2), interrupt #0
  detachInterrupt(digitalPinToInterrupt(ENC_PIN_A));
#ifndef AYAB_TESTS
  // Attaching ENC_PIN_A, Interrupt #0
  // This interrupt cannot be enabled until
  // the machine type has been validated.
  attachInterrupt(digitalPinToInterrupt(ENC_PIN_A), GlobalEncoders::encA_interrupt(), CHANGE);
#endif // AYAB_TESTS
}

• Change Knitter::isr() to Knitter::update() method to fetch encoder state data within a critical section;
  • ATOMIC_BLOCK(ATOMIC_RESTORESTATE) macro for avr arch (https://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html)
  • using a library supporting different architecture, e.g. https://github.com/wizard97/SimplyAtomic

/*!
 * \brief Update machine state data.
 */
void Knitter::update() {
  // update machine state data
  ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
    m_position = GlobalEncoders::getPosition();
    m_direction = GlobalEncoders::getDirection();
    m_hallActive = GlobalEncoders::getHallActive();
    m_beltShift = GlobalEncoders::getBeltShift();
    m_carriage = GlobalEncoders::getCarriage();
  }
}

This way you don't need cached values (Knitter values are safe/nerver overwritten from ISR) and these values are always consistent (updated within a critical section).