Strange behaviour when returning value

Using Arduino Programming Questions
1

Hello,

For my project i use a rotary encoder. I've try to make a small library for this in order to get the number of rotation perfomed since last call.

But for obscure reason sometimes the function GetEncoderCounterSinceLastCall()
return -256 only when i turn the rotaryEncoder counterClockwise

I've attempted to put some log to find the cuplrit whithout much success log randomly goes like this:

Update: -1
Get: -1
Loop: -256

while i attempt to be in all case

Update: -1
Get: -1
Loop: -1

I can't find the culprit and i see nothing wrong here. Any help would be greatly appreciated

Here is my code :

Main :

#include "RotaryEncoder/RotaryEncoder.h"

#define pinA 2 // CLK Output A Do not use other pin for clock as we are using interrupt
#define pinB 3 // DT Output B

volatile unsigned int encoder0Pos = 0;
volatile unsigned int oldencoder0Pos = 0;
long unsigned tempsA;
int aLastState;

RotaryEncoder rotaryEncoder(pinA, pinB);

void setup()
{
  Serial.begin(115200);
  rotaryEncoder._streamRef = &Serial;
}

void loop()
{
  short rotaryValue = rotaryEncoder.GetEncoderCounterSinceLastCall();
  if (rotaryValue != 0)
  {
     Serial.print("Loop: "); Serial.println(rotaryValue);
  }
}

RotaryEncoder.h

#ifndef RotaryEncoder_h
#define RotaryEncoder_h
#include "Arduino.h"

class RotaryEncoder
{
public:
  RotaryEncoder(int aPin, int bPin);
  void UpdateEncoder();
  short GetEncoderCounterSinceLastCall();
  static RotaryEncoder *sEncoder;
  Stream *_streamRef;
private:
  int _aPin;
  int _bPin;
  volatile short _counter;
  long unsigned _aTemps;
  int aLastState;
  static void updateEncoderISR();
};

#include "RotaryEncoder.cpp"
#endif

RotartyEncoder.cpp

#include "Arduino.h"
#include "RotaryEncoder.h"

RotaryEncoder *RotaryEncoder::sEncoder = 0;

RotaryEncoder::RotaryEncoder(int aPin, int bPin)
{
    _aPin = aPin;
    _bPin = bPin;
    sEncoder = this;
    pinMode(_aPin, INPUT_PULLUP);
    pinMode(_bPin, INPUT_PULLUP);
    attachInterrupt(digitalPinToInterrupt(_aPin), RotaryEncoder::updateEncoderISR, CHANGE);
    _aTemps = millis();
}

// Static function
void RotaryEncoder::updateEncoderISR()
{
    if (sEncoder != 0)
        sEncoder->UpdateEncoder();
}

void RotaryEncoder::UpdateEncoder()
{
    int acurrentState = digitalRead(_aPin);
    int bcurrentState = digitalRead(_bPin);
    if (abs(millis() - _aTemps) > 10 && aLastState != acurrentState)
    {
        if (bcurrentState == HIGH && acurrentState == LOW)
        {
            --_counter;
            _streamRef->print("Update: ");
            _streamRef->println(_counter);
        }
        else if (bcurrentState == LOW && acurrentState == LOW)
        {
            ++_counter;
        }
        _aTemps = millis();
    }
    aLastState = acurrentState;
}

short RotaryEncoder::GetEncoderCounterSinceLastCall()
{
    short current = _counter;
    if (_counter != 0)
    {
        _streamRef->print("Get: ");
        _streamRef->println(_counter);
        _counter = 0;
    }
    return current;
}

Laser.ino (605 Bytes)
RotaryEncoder.cpp (1.3 KB)
RotaryEncoder.h (461 Bytes)

2

This function

Should implement a critical section

Side note, don’t include your .cpp in your .h….

2 Likes
3

Thanks a lot J-M-L !!!!

I wasn't expecting race condition on a single thread/core execution.
Correct me if i'm wrong :
After some reading about interrupt and the fact that the board is a 8 bit microcontroller ( wich means two instruction to get the 16 bit from the short ), this can lead to race condition where interrupt is triggered between two read instruction.

By implementing critical section like encapsulate GetEncoderCounterSinceLastCall with

noInterrupts(); 
//code
interrupts();

properly fix the problem.

I wonder if there is any unexpected behaviour with noInterrupts and if a mutex::lock could avoid this.

Side note : I know i shouldn't include my .cpp in .h files unfortunatly this is the only way i found to make VSCode Arduino extension properly build the file othewise it does not get linked to compiler...

4

You should not be printing from an interrupt routine. Printing is slow and can hang if the print buffer gets full.

5

You should keep the code in the critical section super short, like

short RotaryEncoder::GetEncoderCounterSinceLastCall() 
{
  short counterCopy;

  // -------------------------
  // critical section
  // -------------------------
  noInterrupts(); 
  counterCopy = _counter;
  _counter = 0;
  interrupts();
  // -------------------------

  _streamRef->print("Get: ");
  _streamRef->println(_counter);
  return counterCopy;
}

then your risk is just the likelihood of getting two interrupts during the critical section execution because you'll handle only one after the critical section. We are talking a fraction of a micro-second on a Uno type arduino so it should be an acceptable risk esp. as you ignore possible bouncing anyway for 10ms...

6

Yes this is for debug purpose only _streamRef will be completly remove from the final code and keep it fast as possible

1 Like
7

Yes i get it like in all informatic languages the shortest time the lock/stopinterrupt is taken the better it is.

8

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.