Arduino 'constrain' issues

[PSR B1257]

Hello there.

I am currently working on a Arduino-sketch to generate a configurable PWM-signal. Frequency and pulsewidth can be independently adjustet via a little menu-function. All in all it is working sufficently and accurate enugh for the given application.
It would be a nice touche, to limit the possible set-values, which I indendet to do with a constrain-function (seems easy enugh). But since the code is a litte convoluted , the constrain-function does not work (and I commented out the according lines).

Here is the code. I left the original autor (credit where credit's due) in the header and mengelt my lines in between (hence the englisch an german commentry  ).

Code: [Select]

/*******Interrupt-based Rotary Encoder Menu Sketch*******
 * by Simon Merrett, based on insight from Oleg Mazurov, Nick Gammon, rt and Steve Spence, and code from Nick Gammon
 * 3,638 bytes with debugging on UNO, 1,604 bytes without debugging
 */
#include <Wire.h>
#include <LiquidCrystal_I2C.h> // Library for LCD

//#define LED 13

// Wiring: SDA pin is connected to A4 and SCL pin to A5.
// Connect to LCD via I2C, default address 0x27 (A0-A2 not jumpered)
LiquidCrystal_I2C lcd = LiquidCrystal_I2C(0x27, 16, 2);
 
// Rotary encoder declarations
static int pinA = 2; // Our first hardware interrupt pin is digital pin 2
static int pinB = 3; // Our second hardware interrupt pin is digital pin 3
volatile byte aFlag = 0; // let's us know when we're expecting a rising edge on pinA to signal that the encoder has arrived at a detent
volatile byte bFlag = 0; // let's us know when we're expecting a rising edge on pinB to signal that the encoder has arrived at a detent (opposite direction to when aFlag is set)
volatile byte encoderPos = 0; //this variable stores our current value of encoder position. Change to int or uin16_t instead of byte if you want to record a larger range than 0-255
volatile byte oldEncPos = 0; //stores the last encoder position value so we can compare to the current reading and see if it has changed (so we know when to print to the serial monitor)
volatile byte reading = 0; //somewhere to store the direct values we read from our interrupt pins before checking to see if we have moved a whole detent
// Button reading, including debounce without delay function declarations
const byte buttonPin = 4; // this is the Arduino pin we are connecting the push button to
byte oldButtonState = HIGH;  // assume switch open because of pull-up resistor
const unsigned long debounceTime = 10;  // milliseconds
unsigned long buttonPressTime;  // when the switch last changed state
boolean buttonPressed = 0; // a flag variable
// Menu and submenu/setting declarations
byte Mode = 0;   // This is which menu mode we are in at any given time (top level or one of the submenus)
const byte modeMax = 2; // This is the number of submenus/settings you want
byte setting1 = 0;  // a variable which holds the value we set
byte setting2 = 0;  // a variable which holds the value we set

unsigned long Frequenz = 0;
unsigned long Pulsbreite = 0;
unsigned long Pausenbreite = 0;
unsigned long previousMillis = 0;
const int PWMpin=13;
bool PWMstate = false;


/* Note: you may wish to change settingN etc to int, float or boolean to suit your application.
 Remember to change "void setAdmin(byte name,*BYTE* setting)" to match and probably add some
 "modeMax"-type overflow code in the "if(Mode == N && buttonPressed)" section*/

void setup() {
  //Display initial beschreiben
  lcd.begin();
  lcd.backlight();
  lcd.setCursor(0, 0);
  lcd.print("Pulsbreite=");
  lcd.setCursor(0, 1);
  lcd.print("Frequenz="); 
 
  //Rotary encoder section of setup
  pinMode(pinA, INPUT_PULLUP); // set pinA as an input, pulled HIGH to the logic voltage (5V or 3.3V for most cases)
  pinMode(pinB, INPUT_PULLUP); // set pinB as an input, pulled HIGH to the logic voltage (5V or 3.3V for most cases)
  attachInterrupt(0,PinA,RISING); // set an interrupt on PinA, looking for a rising edge signal and executing the "PinA" Interrupt Service Routine (below)
  attachInterrupt(1,PinB,RISING); // set an interrupt on PinB, looking for a rising edge signal and executing the "PinB" Interrupt Service Routine (below)
  // button section of setup
  pinMode (buttonPin, INPUT_PULLUP); // setup the button pin
 
  pinMode(PWMpin, OUTPUT);
  digitalWrite(PWMpin,PWMstate);

}

void loop() {
  rotaryMenu();
  PWM();
  // carry out other loop code here
}

void rotaryMenu() { //This handles the bulk of the menu functions without needing to install/include/compile a menu library
  //DEBUGGING: Rotary encoder update display if turned
  if(oldEncPos != encoderPos) { // DEBUGGING
    oldEncPos = encoderPos;// DEBUGGING
   
    //Pfeile löschen
    if (Mode == 0 && encoderPos == 0){
        lcd.setCursor(15, 0);
        lcd.print(" ");
        lcd.setCursor(15, 1);
        lcd.print(" ");
    }
    //Pfeil zeichnen auf Wert "Pulsbreite"
    if(Mode == 0 && encoderPos == 1){
        lcd.setCursor(15, 0);
        lcd.print("<");
        lcd.setCursor(15, 1);
        lcd.print(" ");       
    }
    //Wert von Drehencoder an Stelle "Pulsbreite" schreiben
    if(Mode == 1){
        lcd.setCursor(11, 0);
        if (encoderPos <10) lcd.print(" ");
        lcd.print(encoderPos); 
        lcd.print("ms");
    }
    //Pfeil zeichnen auf Wert "Frequenz"   
    if(Mode == 0 && encoderPos == 2){
        lcd.setCursor(15, 0);
        lcd.print(" ");
        lcd.setCursor(15, 1);
        lcd.print("<");         
    }
    //Wert von Drehencoder an Stelle "Frequenz" schreiben
    if(Mode == 2){
        lcd.setCursor(11, 1);
        if (encoderPos <10) lcd.print(" ");
        lcd.print(encoderPos); 
        lcd.print("Hz");
    }
   
  }// DEBUGGING
  // Button reading with non-delay() debounce - thank you Nick Gammon!
  byte buttonState = digitalRead (buttonPin);
  if (buttonState != oldButtonState){
    if (millis () - buttonPressTime >= debounceTime){ // debounce
      buttonPressTime = millis ();  // when we closed the switch
      oldButtonState =  buttonState;  // remember for next time
      if (buttonState == LOW){
        buttonPressed = 1;
      }
      else {
        buttonPressed = 0; 
      } 
    }  // end if debounce time up
  } // end of state change

  //Main menu section
  if (Mode == 0) {

    //Berechnung der Pausenbreite in µs
    Pausenbreite=(1000000/Frequenz)-1000*Pulsbreite;

    if (encoderPos > (modeMax+10)) encoderPos = modeMax; // check we haven't gone out of bounds below 0 and correct if we have
    else if (encoderPos > modeMax) encoderPos = 0; // check we haven't gone out of bounds above modeMax and correct if we have
    if (buttonPressed){
      Mode = encoderPos; // set the Mode to the current value of input if button has been pressed
      buttonPressed = 0; // reset the button status so one press results in one action
      if (Mode == 1) {
          encoderPos = setting1;  //letzten eingestellten Wert wiederherstellen     
      }
      if (Mode == 2) {
          encoderPos = setting2;  //letzten eingestellten Wert wiederherstellen   
      }

    }
  }
  if (Mode == 1 && buttonPressed) {
    setting1 = encoderPos; // Einstellung Pulsbreite
    //setting1 = constrain (setting1,2,15);
    Pulsbreite = setting1;
    setAdmin(1,setting1);
    }
  if (Mode == 2 && buttonPressed) {
    setting2 = encoderPos; // Einstellung Frequenz
    //setting2 = constrain (setting2,1,50);
    Frequenz = setting2;
    setAdmin(2,setting2);
    }
}

// Carry out common activities each time a setting is changed
void setAdmin(byte name, byte setting){
 

  encoderPos = 0; // reorientate the menu index - optional as we have overflow check code elsewhere
  buttonPressed = 0; // reset the button status so one press results in one action
  Mode = 0; // go back to top level of menu, now that we've set values

}

//Rotary encoder interrupt service routine for one encoder pin
void PinA(){
  cli(); //stop interrupts happening before we read pin values
  reading = PIND & 0xC; // read all eight pin values then strip away all but pinA and pinB's values
  if(reading == B00001100 && aFlag) { //check that we have both pins at detent (HIGH) and that we are expecting detent on this pin's rising edge
    encoderPos --; //decrement the encoder's position count
    bFlag = 0; //reset flags for the next turn
    aFlag = 0; //reset flags for the next turn
  }
  else if (reading == B00000100) bFlag = 1; //signal that we're expecting pinB to signal the transition to detent from free rotation
  sei(); //restart interrupts
}

//Rotary encoder interrupt service routine for the other encoder pin
void PinB(){
  cli(); //stop interrupts happening before we read pin values
  reading = PIND & 0xC; //read all eight pin values then strip away all but pinA and pinB's values
  if (reading == B00001100 && bFlag) { //check that we have both pins at detent (HIGH) and that we are expecting detent on this pin's rising edge
    encoderPos ++; //increment the encoder's position count
    bFlag = 0; //reset flags for the next turn
    aFlag = 0; //reset flags for the next turn
  }
  else if (reading == B00001000) aFlag = 1; //signal that we're expecting pinA to signal the transition to detent from free rotation
  sei(); //restart interrupts
}

void PWM()
{
  unsigned long currentMillis = micros();
 

 if( PWMstate == HIGH )
 {
    if( (unsigned long)(currentMillis - previousMillis) >= 1000*Pulsbreite){   
    PWMstate = LOW;// change the state of LED
    digitalWrite(PWMpin,PWMstate);
    previousMillis=micros();
    }
 }
 else
 {   
    if( (unsigned long)(currentMillis - previousMillis) >= Pausenbreite){     
    PWMstate = HIGH;// change the state of LED
    digitalWrite(PWMpin,PWMstate);
    previousMillis=micros();
    }
 }
}



Any ideas, how to solve this issue, are greatly appreciated.

Best wishes, Matt

[Pack34]

Are you getting an error? Or is it not behaving as intended?

[ledtester]

You are constraining setting1:

Code: [Select]

  if (Mode == 1 && buttonPressed) {
    setting1 = encoderPos; // Einstellung Pulsbreite
    //setting1 = constrain (setting1,2,15);

but you are not also constraining encodePos which is what gets displayed:

Code: [Select]

    if(Mode == 1){
        lcd.setCursor(11, 0);
        if (encoderPos <10) lcd.print(" ");
        lcd.print(encoderPos);
        lcd.print("ms");

Is that what your problem is?

This code looks suspicious:

Code: [Select]

    if (encoderPos > (modeMax+10)) encoderPos = modeMax; // check we haven't gone out of bounds below 0 and correct if we have
    else if (encoderPos > modeMax) encoderPos = 0; // check we haven't gone out of bounds above modeMax and correct if we have

The bounds on encodePos depend on what Mode is set to and in menu mode you want it wrap around, but when setting the PWM parameters probably not.

[PSR B1257]

Or is it not behaving as intended?

The latter.

but you are not also constraining encodePos which is what gets displayed:

Yep, that's because I want a live updating display, as the encoder is turned.

This code looks suspicious:

It's part of the original sketch (which I admittedly not fully understand).

[newbrain]

Or is it not behaving as intended?

The latter.

Care to elaborate?
What is the expected result and what are the differences with the actual outcome?

The program logic is quite messy - but being Arduino, that's to be expected.

I agree with ledtester that the display should be constrained too (though there's no constrain function in the code as shown).

Their second point on suspicious code is instead moot: limiting encoderPos is only performed when Mode is 0 (menu mode).

[Pack34]

I noticed all your variables are in bytes, but the constrain function uses ints. I'm not sure if that would be an issue with the compiler or not. Have you tried just using two IF statements instead of the constrain function? It's just bounding a number. It's not immediately clear why you're using bytes there. You seem to go up to using unsigned_long variables in the PWM function.

Your setAdmin function isn't doing anything with the passed in variables.

[PSR B1257]

Care to elaborate?

It kinda sorta works. The value is constrained, but not during the adjustment via the encoder. You can (at first glance) input i.e. 20ms pulsewith (maximum should be 15ms) and the display reads 20ms. However if you recall the value (by the "setAdmin" function, I suspect), in order to change it, the correcty constrained value of 15ms is shown.
So it is working in the background (verified by frequencycounter), but displayed wrong. I didn't even noticed that before   

[ledtester]

There are a bunch of race conditions in this code -- encodePos is updated in the ISRs but accessed and modified multiple times in the main program.

This library looks like it does a better job:

https://github.com/0xPIT/encoder

[PSR B1257]

There are a bunch of race conditions in this code

Well possible...I'm to much of a non-programmer to assess this.

This library looks like it does a better job:

I tried a bunch of different encoder-reading-sketches and found this one beeing the most reliable. Regarding your link, I'm sure it's fine, but I don't even remotly understand it 

To make my code even more messy, I added a few IF-statements to at least limit the displayed value (under the motto 'Keep it stupid simple'  ).

Code: [Select]

    if(Mode == 1){
        lcd.setCursor(11, 0);
        if (encoderPos <3)
        {
          lcd.print(" 2ms");
        }
        else if (encoderPos <16)
        {
          if (encoderPos <10) lcd.print(" ");
          lcd.print(encoderPos); 
          lcd.print("ms");
        }
        else
        {
          lcd.print("15ms");
        }

#worstcodeinhistory 

[Pack34]

There are a bunch of race conditions in this code

Well possible...I'm to much of a non-programmer to assess this.

This library looks like it does a better job:

I tried a bunch of different encoder-reading-sketches and found this one beeing the most reliable. Regarding your link, I'm sure it's fine, but I don't even remotly understand it 

To make my code even more messy, I added a few IF-statements to at least limit the displayed value (under the motto 'Keep it stupid simple'  ).

Code: [Select]

    if(Mode == 1){
        lcd.setCursor(11, 0);
        if (encoderPos <3)
        {
          lcd.print(" 2ms");
        }
        else if (encoderPos <16)
        {
          if (encoderPos <10) lcd.print(" ");
          lcd.print(encoderPos); 
          lcd.print("ms");
        }
        else
        {
          lcd.print("15ms");
        }

#worstcodeinhistory 

You're changing what's printed to the LCD, not the actual value.

[brucehoult]

There are a bunch of race conditions in this code

Well possible...I'm to much of a non-programmer to assess this.

This library looks like it does a better job:

I tried a bunch of different encoder-reading-sketches and found this one beeing the most reliable. Regarding your link, I'm sure it's fine, but I don't even remotly understand it 

To make my code even more messy, I added a few IF-statements to at least limit the displayed value (under the motto 'Keep it stupid simple'  ).

Code: [Select]

    if(Mode == 1){
        lcd.setCursor(11, 0);
        if (encoderPos <3)
        {
          lcd.print(" 2ms");
        }
        else if (encoderPos <16)
        {
          if (encoderPos <10) lcd.print(" ");
          lcd.print(encoderPos); 
          lcd.print("ms");
        }
        else
        {
          lcd.print("15ms");
        }

#worstcodeinhistory 

You're still doing it. You're using the value of "volatile byte encoderPos" at multiple places in the code and it might be different every time because the interrupt routine can be changing it.

For example when you do "if (encoderPos < 3)" it might be greater than 3, but by the time you test "if (encoderPos < 16) it might have changed to something less than 3.

Similarly, when you test if it is less than 10 to print a leading space, by the time you print the actual number it might have changed. So you can print a leading space for a number that doesn't need one by the time you print it, or vice versa.

What you *should* be doing is something like this:

Code: [Select]

byte constrainedPos = encoderPos;
if (constrainedPos < 3) constrainedPos = 3;
if (constrainedPos > 15) constrainedPos = 15;
if (oldEncPos != constrainedPos){
  if (constrainedPos <10) lcd.print(" ");
  lcd.print(constrainedPos); 
  lcd.print("ms");
  oldEncPos = constrainedPos;
}


[PSR B1257]

You're still doing it.

Yes, since 'encoderPos' is live updating.

For example when you do "if (encoderPos < 3)" it might be greater than 3, but by the time you test "if (encoderPos < 16) it might have changed to something less than 3.

You would have to turn the encoder really fast, to achieve this behavior, wouldn't you?

What you *should* be doing is something like this:

Unfortunately, the controller goes bananas, if I try this. It prints " 2ms" all over the LCD 

[brucehoult]

You're still doing it.

Yes, since 'encoderPos' is live updating.

And you don't want it to update in the middle of one set of operations on it, only once per loop.

For example when you do "if (encoderPos < 3)" it might be greater than 3, but by the time you test "if (encoderPos < 16) it might have changed to something less than 3.

You would have to turn the encoder really fast, to achieve this behavior, wouldn't you?

No. The value has to change sometime. Sometimes it will happen in a safe place, sometimes it will happen in a place that breaks your logic.

It might only break once every five seconds, or once a day, or once a month, but it *will* eventually happen at the worst possible time for your program logic. Or it might break 100 times a second. It depends how much time your program spends in which part of the code. It's bad programming to allow this to happen.

What you *should* be doing is something like this:

Unfortunately, the controller goes bananas, if I try this. It prints " 2ms" all over the LCD 

Then you presumably need to do an lcd.setCursor() somewhere. I'm interested in the constraining the value part, which you are having problems with.

[PSR B1257]

And you don't want it to update in the middle of one set of operations on it, only once per loop.

Not really. 'encoderPos' is updated live to the LCD, while the encoder is turned (as you would expect from any menu-function. Only after pressing the encoder button, the value is transferred to 'setting1' and 'Pulsbreite' (pulse width) for example.
Even if the code glitches during the adjustment, it wouldn't be the end of the world.

[brucehoult]

And you don't want it to update in the middle of one set of operations on it, only once per loop.

Not really. 'encoderPos' is updated live to the LCD, while the encoder is turned (as you would expect from any menu-function. Only after pressing the encoder button, the value is transferred to 'setting1' and 'Pulsbreite' (pulse width) for example.
Even if the code glitches during the adjustment, it wouldn't be the end of the world.

Why write code that can glitch, when it's just as easy to write code that can't glitch?

[PSR B1257]

when it's just as easy to write code that can't glitch?

Easier for who?
I did not get your code-example to work.

[free_electron]

i typically use an array for parameters and use named indexes to refer to them

int settings [5][3];
#define minval  0
#define currentval  1
#define maxval  2

#define nothing 0
#define  frequency  1
#define  dutycycle  2
#define  level  3
int parameter;


void rotaryhandler_up (current_parameter)
     {
        if (settings(currentparameter,currentval) < settings{currentparameter,maxval)) settings(currentparameter,currentval) = settings(currentparameter,currentval) +1;
        event = evnt_rotary_up;
     }
void rotaryhandler_down (current_parameter)     
     {
        if (settings(currentparameter,currentval) < settings{currentparameter,maxval)) settings(currentparameter,currentval) = settings(currentparameter,currentval) -1;
        event = evnt_rotary_down;
     }
void rotary_handler_click (void)
    {
     current_parameter = 0;
     event = evnt_rotary_click;
    }



void eventhandler(event)
{
   switch (event){
     event_rotary_up:
     event_rotary_down:
             switch(parameter) {
             frequency :
                lcdprint (0,0,'Frequency = %i',settings(parameter,currentval);
                break;
             dutycycle :
                lcdprint (0,0,'Dutycycle = %i',settings(parameter,currentval);
                break;
             level :
                lcdprint (0,0,'Level = %i',settings(parameter,currentval);
                break;

            }
             break;
    event_rotary_click:
    default:
   }
   event = 0;
       

}


something along those lines.

basically create an array that stores your settings. each ' setting has a minimum , maximum and current_value
the menuhandler sets an integer that determines which of the array entries is being manipulated. : current_parameter

the rotary encoder handler has 3 hardware events : up , down and click. it uses 'current_parameters to index the array and determines the new value of 'current_value' based on which direction and the minimum and maximum.
the 'click event' sets the index to zero. you can turn all you want : nothing increments or decrements. the zero index is a dummy.

There is a software event handler that is responsible for postprocessing. That one updates the display , sends the data into registers or peripherals and whatnot. The hardware handlers set a software_event so it can be post-processed.
upon post-processing the software_event is set to zero.

you can have multiple hardware rotary encoders. lets say you have an encoder for mulitple functions , and one dedicate to the output level.
the hardware handler for output level would simply 'force' the current_parameter to 'level' prior to changing the values. upon calling the softwareeventhandler , the correct parameter will be displayed as the current_parameter has changed.

the software eventhandler is not limited to rotary encoders. i can have pushbutton_events in there or other events

event output_overload : display ' overload '
event numberkey
    number_key_0
    number_key_1
    number_key_2
    ....
    numerical = numerical * 10 + numberkey;
event backspace
    numerical = numerical \10
event enter
    if numerical > settings ( current_parameter,minval) and numerical < settings ( current_parameter,manval)   settings(current_parameter,current_value) = numerical;


I split things between hardware interrupts ( encoders, buttons , scanned events ). those hardware interrupts do minimal processing and critical processing and then fire a software event that deal with the rest.
critical events would be an overload : the hardware handler would switch off output , then fire the software event to notify the user.
scanned events are events triggered by a hardware timer. for example a loop that reads data over i2c or spi or io pins on a periodical sweep. those are for critical scans only.
i have a general purpose system_tick as well. that one can fire software scanned events.

[PSR B1257]

i typically use an array for parameters and use named indexes to refer to them

Sound's interesting. Thanks for your comprehensive reply.

But once again, silly me can't really reenact your code. I'm more the having-a-working-code-and-modify-it-somewhat-programmer.

[Ground_Loop]

You're still doing it.

Yes, since 'encoderPos' is live updating.

For example when you do "if (encoderPos < 3)" it might be greater than 3, but by the time you test "if (encoderPos < 16) it might have changed to something less than 3.

You would have to turn the encoder really fast, to achieve this behavior, wouldn't you?

What you *should* be doing is something like this:

Unfortunately, the controller goes bananas, if I try this. It prints " 2ms" all over the LCD 

The display problem may be related to using interrupts to service the encoder position.  I've had lcds get interrupted in the middle of updates then they're scrambled until I can send a reset.

[brucehoult]

The display problem may be related to using interrupts to service the encoder position.  I've had lcds get interrupted in the middle of updates then they're scrambled until I can send a reset.

There are easy ways to program to avoid that.

He's refusing to consider them, even after it being demonstrated.

[PSR B1257]

He's refusing to consider them

I'm not. I "just" can't implement them.

Whilst being messy, the code works fine, without glitches or something. Apart from the initial constraining-problem of course.

The display only gets scrambled, if I try to make the *should*-code work. Obviously, I don't know, what I'm doing....