Some strange behaviour on a condition statement

Hi,

I’m new to the Arduino world but not new to embedded systems programming.

I’m building a simple metronome using a Mega 2560 and I’m using a 470K potential resistor attached to an analogue input to change the tempo. The set tempo is displayed on 3 7 segment LEDs. I’ve had to add some debounce code to avoid the pot bouncing between readings. The idea being that the pot reading has to remain stable for a certain period before it’s reported as a new value, or the pot reading has to be greater than a debounce amount to identify if the user is turning the dial around (otherwise they would not see the tempo increasing on the led until they stop turning the dial).

I’ve written my code so the loop() calls get_pot_bpm() and updates the 7 seg LED on each iteration.

For some unexplainable reason my condition to check if the difference in the old reported reading and the new reading is greater than the debounce amount doesn’t work…until a slight delay is put in! In the case below I’ve inserted a delay(10). Without the delay() it won’t work. I’ve tried adding some serial output to debug the code but with the debug it also works - because it adds a delay. I’ve also unit tested the function in XCode and I can’t see anything wrong in the fundamental code.

Any ideas would be appreciated please :slight_smile:

#define POT_DEBOUNCE_PERIOD 10
#define POT_DEBOUNCE_AMOUNT 2
#define MAX(a,b) (a>b?a:b)
#define MIN(a,b) (a<b?a:b)

unsigned int get_pot_bpm()
{
unsigned int current_reading = analogRead(POT_PIN);
static unsigned int last_reading = 0;
unsigned long current_millis = millis();
static unsigned int reported_reading = 0;
static boolean debouncing = FALSE;
static unsigned long debounce_start_millis = 0;

/* If current_reading is different to last_reading, it either needs to stay
static for the debounce period or exceed the debounce amount to be reported /
if (current_reading != last_reading)
{
/
Without this delay(), the next condition will never evaluate to TRUE /
delay(10);
/
If we exceed the debounce amount, that can be reported immediately /
if ((MAX(current_reading, last_reading)-MIN(current_reading, last_reading)) > POT_DEBOUNCE_AMOUNT)
{
reported_reading = current_reading;
debouncing = FALSE;
debounce_start_millis = current_millis;
last_reading = current_reading;
}
else
/
If we are within the debounce amount, store current time and reading */
{
debouncing = TRUE;
debounce_start_millis = current_millis;
last_reading = current_reading;
}
}

/* If we are debouncing and we’ve reached the debounce period, report reading */
if ((debouncing==TRUE) && ((current_millis-debounce_start_millis) > POT_DEBOUNCE_PERIOD))
{
reported_reading = current_reading;
last_reading = current_reading;
debouncing = FALSE;
}

return ((float(reported_reading)/float(1023))*(HIGHEST_BPM-LOWEST_BPM)+LOWEST_BPM);
}

I've written my code so the loop() calls get_pot_bpm()

But we can't see "loop".

(When posting code, please use the # (code) icon on the editor's toolbar)

if (current_reading != last_reading)

Analogue readings are notoriously jittery.

Hi,

I’ve simplified the whole sketch so it is less hw dependant. With the delay in, it works fantastically with no jitter:

#define POT_DEBOUNCE    TRUE
#define POT_PIN         0
#define TRUE            1
#define FALSE           0
#define POT_DEBOUNCE_PERIOD 10
#define POT_DEBOUNCE_AMOUNT 2
#define MAX(a,b) (a>b?a:b)
#define MIN(a,b) (a<b?a:b)

#define LOWEST_BPM      40
#define HIGHEST_BPM     350

#define TRACE_INIT  Serial.begin(9600); Serial.flush();
#define TRACE(x)    Serial.print(x)
#define TRACELN(x)  Serial.println(x)

unsigned int bpm;


/**********************************************************************************/
/* BPM Display Task Functions                                                     */
/* Displays the BPM on the 7 seg display                                          */
/**********************************************************************************/
void task_bpm_display()
    {
    static unsigned int displayed_bpm = 0;

    if (displayed_bpm != bpm)
        {
        TRACELN(bpm);
        displayed_bpm = bpm;
        }
    }

/**********************************************************************************/
/* Pot Pin Task Functions                                                         */
/* Monitors pot pin for changes. Modify BPM based on value and switch from tap to */
/* pot pin tempo mode                                                             */
/**********************************************************************************/
void task_pot_pin()
    {
    /* BPM can either be set using pot or tap. pot_bpm is the pot value for bpm */
    static unsigned int pot_bpm = 0;
    unsigned int pot_pin_reading = get_pot_bpm();
    
    /* If the pot changes, store latest bpm and change bpm control to pot */
    if (pot_pin_reading != pot_bpm)
        {
        pot_bpm = pot_pin_reading;
        bpm = pot_bpm;
        }       
    }

unsigned int get_pot_bpm()
    {
    unsigned int current_reading = analogRead(POT_PIN);
    static unsigned int last_reading = 0;
    unsigned long current_millis = millis();
    static unsigned int reported_reading = 0;
    static boolean debouncing = FALSE;
    static unsigned long debounce_start_millis = 0;             

    /* For some crazy reason, if the delay doesn't go in, the debounce amount condition fails */
    delay(10);        
    
    /* If current_reading is different to last_reading, it either needs to stay 
	 static for the debounce period or exceed the debounce amount to be reported */
    if (current_reading != last_reading)
	{
        /* If we exceed the debounce amount, that can be reported immediately */     
	if ((MAX(current_reading, last_reading)-MIN(current_reading, last_reading)) > POT_DEBOUNCE_AMOUNT)
	    {
            reported_reading = current_reading;
            debouncing = FALSE;
            debounce_start_millis = current_millis;
            last_reading = current_reading;
	    }
        else
	    /* If we are within the debounce amount, store current time and reading */
	    {
            debouncing = TRUE;
            debounce_start_millis = current_millis;
            last_reading = current_reading;
	    }
	}

    /* If we are debouncing and we've reached the debounce period, report reading */
    if ((debouncing==TRUE) && ((current_millis-debounce_start_millis) > POT_DEBOUNCE_PERIOD))
	{
        reported_reading = current_reading;
        last_reading = current_reading;
        debouncing = FALSE;
	}
	
    return ((float(reported_reading)/float(1023))*(HIGHEST_BPM-LOWEST_BPM)+LOWEST_BPM);
    }

/**********************************************************************************/
/* Setup and Main loop                                                            */
/**********************************************************************************/

void setup()
    {
    TRACE_INIT;
    bpm = get_pot_bpm();
    }
    
void loop()
    {
    task_pot_pin();
    task_bpm_display();
    }

Well my software skills are not good enough to really critic your program, but I have a few general comments. You seem to be treating analog input pins like one would treat digital input switches requiring debouncing timing, etc. A analog input reading is a snap shot of a voltage on the pin and does not exhibit any of the problems as a bouncing switch contact can cause. Why are you doing that?

Also comparing analog input readings for equality in a if statement is kind of questionable. Due to A/D converter issues, even two readings in a row reading a fixed stable voltage is bound to have a +/- LSB error due to the conversion method used. Best to compare two analog input values are within a range of equality of +/- several counts?

Lefty

Hi Lefty,

You seem to be treating analog input pins like one would treat digital input switches requiring debouncing timing, etc. A analog input reading is a snap shot of a voltage on the pin and does not exhibit any of the problems as a bouncing switch contact can cause. Why are you doing that?

I performed some measurements on the values I got from a readAnalog() with the pot connected and these tended to vary by +/-1. What my debounce code is intended to do is stabilise the reading that is passed back to display on the 7 seg led.

I feel that the problem isn’t in the values passed back from readAnalog() but in the fact that I have to insert a delay() to be able to get the condition to function correctly. The only difference in the case of it working and not working is the insertion of that delay.

Some potentiometers are pretty noisy. In some cases it would be a great idea.

So, after some more messing with it, it does actually seem to be readAnalog() that causes the condition to fail. If instead of calling readAnalog() I call a function which increments by 5 on each call, the condition works correctly without a delay.

I found this in the help texts relating to analog pins:

The Atmega datasheet also cautions against switching analog pins in close temporal proximity to making A/D readings (analogRead) on other analog pins. This can cause electrical noise and introduce jitter in the analog system. It may be desirable, after manipulating analog pins (in digital mode), to add a short delay before using analogRead() to read other analog pins.

I'm not reading any other analog pins but I wonder if the fact I'm reading the pin so frequently is causing the issue. The delay can be reduced to 1msec with correct functionality.

Analog reading can and are frequently noisy. One can deal with it by external filtering (resistor/cap) or by software averaging. If you take 8 readings in a row and add them to a variable and then divide it by 8 you will get a smoother value. Just be sure to zero out the average before doing the next set of 8 reads.

Lefty

Many thanks for all of your help