Variable value getting clobbered?

Hello! I have implemented a simple function that will start the CurieTimerOne if it has not been started already.

boolean StartTimerOne(unsigned long tickDurationMicroSeconds)
{
  if(timerStarted == false)
  {
    CurieTimerOne.initialize(tickDurationMicroSeconds);
    CurieTimerOne.start();
    timerStarted = true;
    return true;
  }
  else
  {
    return false;
  }
 }

I set the value for timerStarted to false initially. If I print out the value of timerStarted WITHOUT calling my function in the loop, I get a value of '0' as expected. If I do call the function, I never see the value of the variable switch from '0' to '1'. It's initially a '1' and never changes. Can anyone shed some light on why this happening?

Thanks!

You’re going to have to show the rest of the code. The issue obviously isn’t in this snippet.

Here you go. I just tested it a few times over, and one single time I got the values that I expected…

#include <Adafruit_NeoPixel.h>
#include <CurieTimerOne.h>

#define LEDS_PIN    7

#define NUM_LEDS    2
#define EYE_LEFT     0
#define EYE_RIGHT    1
#define EYE_BOTH     2

#define DEBUG_PRINTING
#ifdef DEBUG_PRINTING
#define DEBUG_PRINT(x) Serial.print((x))
#define DEBUG_PRINTLN(x) Serial.println((x))
#else
#define DEBUG_PRINT(x)
#define DEBUG_PRINTLN(x)
#endif

Adafruit_NeoPixel leds = Adafruit_NeoPixel(NUM_LEDS, LEDS_PIN, NEO_GRB + NEO_KHZ800);

static boolean timerStarted = 0;

/*
* SetEyeLedColor()- Executes action sequence at index
* 
* Parameters:
*    eyeId-  defines right, left or both eyes; must be one of OOMIYU_EYE_XXX
*    red         value (0-255) of red component     
*    green       value (0-255) of green component
*    blue        value (0-255) of blue component
*    brightness  value (0-100) percent
* Return: true on success, false otherwise (invalid eyeId)
* 
*/
boolean SetEyeLedColor(unsigned long eyeId, unsigned long int red, unsigned long int green, unsigned long int blue, int brightness)
{
 if (eyeId > NUM_LEDS)
 {
   return false;
 }

 //limit LED brightness to between 0-100%
 int redValue = map(red, 0, 255, 0, (255 * brightness) / 100);
 int greenValue = map(green, 0, 255, 0, (255 * brightness) / 100);
 int blueValue = map(blue, 0, 255, 0, (255 * brightness) / 100);

 leds.setPixelColor(eyeId, redValue, greenValue, blueValue);
 leds.show();
 return true;
}

/*
* StartTimerOne()- Starts ARC Timer 1
* 
* Parameters:
*    tickDurationMicroSeconds - tick duration in milliseconds
* Return: true on success, false if already started
* 
*/
boolean StartTimerOne(unsigned long tickDurationMicroSeconds)
{
 DEBUG_PRINT("In StartTimerOne");
 DEBUG_PRINTLN("");
 
 if(timerStarted == false)
 {
   DEBUG_PRINT("In IF");
   DEBUG_PRINTLN(""); 
   CurieTimerOne.initialize(tickDurationMicroSeconds);
   CurieTimerOne.start();
   timerStarted = true;

   DEBUG_PRINT("PeriodInUsec: ");
   DEBUG_PRINT(CurieTimerOne.periodInUsec);
   DEBUG_PRINTLN("");
   
   return true;
 }
 else
 {
   DEBUG_PRINT("In ELSE");
   DEBUG_PRINTLN("");  
   return false;
 }
}

/*
* StopTimerOne()- Stops ARC Timer 1
* 
* Parameters:
*    none
* Return: true on success, false if not started
* 
*/
boolean StopTimerOne()
{
 if(timerStarted)
 {
   CurieTimerOne.kill();
   timerStarted = false;
   return true;
 }
 else
 {
   return false;
 }
}

void initialize()
{
   DEBUG_PRINT("In initialize");
     DEBUG_PRINTLN("");
 //setup LEDS
 leds.begin();
 //StartTimerOne(100000);
}

void setup() {
 // put your setup code here, to run once:

#ifdef DEBUG_PRINTING
 Serial.begin(9600);
#endif

 initialize();
}

void loop() {
 // put your main code here, to run repeatedly:
 SetEyeLedColor(0, 0, 25, 0, 50);

 DEBUG_PRINT("In loop");
 delay(1000);
 DEBUG_PRINT("BEFORE CALL timerStarted= ");
 DEBUG_PRINT(timerStarted);
 DEBUG_PRINTLN("");

 StartTimerOne(10000);
 DEBUG_PRINT("AFTER CALL timerStarted= ");
 DEBUG_PRINT(timerStarted);
 DEBUG_PRINTLN("");

 delay(1000);

}

Another note: If I comment out the call in the loop() function to StartTimerOne(), compile and upload, I get an output of false everytime (0) as expected. If I then put the call back in, I will get the values that I expect one time only. Could this be some type of buffer issue?

I don't think this is your problem, but in your code you have:

/*
* SetEyeLedColor()- Executes action sequence at index
* 
* Parameters:
*    eyeId-  defines right, left or both eyes; must be one of OOMIYU_EYE_XXX
*    red         value (0-255) of red component     
*    green       value (0-255) of green component
*    blue        value (0-255) of blue component
*    brightness  value (0-100) percent
* Return: true on success, false otherwise (invalid eyeId)
* 
*/
boolean SetEyeLedColor(unsigned long eyeId, unsigned long int red, unsigned long int green, unsigned long int blue, int brightness)
{

The values are such that they could be handled with a byte data type. When you call this function, you are using:

 SetEyeLedColor(0, 0, 25, 0, 50);

the compiler will default to an int data type for the data, not an unsigned long. The arguments cause 10 bytes to be pushed on the stack, but your function expects 20 bytes. I doubt that it is the source of your problem. Still, I would change the arguments to byte values, or use:

 SetEyeLedColor(0UL, 0UL, 25UL, 0UL, 50);

so you are documenting the code to use unsigned longs.

Those ints wouldn't get promoted?

If that were a problem how come we never have to do that with calls to functions like delay?

The compiler knows that the function wants unsigned longs and it will give it unsigned longs.

Besides,

#include <CurieTimerOne.h>

tells that this sketch is for Arduino 101, which has 32-bit ints.

The function always returns the same value as what it is stored in timerStarted, so it could be much simpler:

boolean StartTimerOne(unsigned long tickDurationMicroSeconds)
{
  if(timerStarted == false)
  {
    CurieTimerOne.initialize(tickDurationMicroSeconds);
    CurieTimerOne.start();
    timerStarted = true;
  }
  return timerStarted;
 }

However, it makes little sense to call the function when you already KNOW whether the function needs to be called, so it really should look like:

void StartTimerOne(unsigned long tickDurationMicroSeconds)
{
    CurieTimerOne.initialize(tickDurationMicroSeconds);
    CurieTimerOne.start();
    timerStarted = true;
 }

and you should call the function only if !timerStarted.

One possible problem is that Adafruit_NeoPixel is writing to memory outside it's declared footprint and clobbering 'timerStarted'. If you change the declaration of timerStarted to before Adafruit_NeoPixel this may change things.

As a matter of style (it should not make a runtime difference), timerStarted should be declared as

static boolean timerStarted = false;

and I prefer to see

 if( ! timerStarted)
{
    ...
}

but I'm not a fanatic in this respect.

econjack:
...the compiler will default to an int data type for the data, not an unsigned long. The arguments cause 10 bytes to be pushed on the stack, but your function expects 20 bytes.

No, that is not true. The compiler KNOWS that function expects unsigned long arguments, so it will promote them to unsigned longs implicitly. That is the whole reason c/c++ uses function prototypes.

Regards,
Ray L.

stowite:
One possible problem is that Adafruit_NeoPixel is writing to memory outside it's declared footprint and clobbering 'timerStarted'. If you change the declaration of timerStarted to before Adafruit_NeoPixel this may change things.

Thank you, this fixed my issue! If you have a minute, would you please elaborate on why/how this happens?

 if (eyeId > NUM_LEDS)
 {
   return false;
 }

If eyeId equals NUM_LEDS, you'll have a problem, too.

PaulS:
If eyeId equals NUM_LEDS, you'll have a problem, too.

Ah you're right! If eyeId equals NUM_LEDS then the led command should work on both leds. I'll need to handle that differently. Thanks!!

claudiaa:
Thank you, this fixed my issue! If you have a minute, would you please elaborate on why/how this happens?

Either there is a bug in the Adafruit_NeoPixel library or you are using wrongly. I have never used the Adafruit_NeoPixel library but would bet that the problem is yours. Double check your code to make sure you are using the library correctly.

RayLivingston:
No, that is not true. The compiler KNOWS that function expects unsigned long arguments, so it will promote them to unsigned longs implicitly. That is the whole reason c/c++ uses function prototypes.

Regards,
Ray L.

I originally thought that, too, but then I started wondering why floating point constants don't get promoted. Also, I think all of us have had situations where function arguments where screwed up until we placed an explicit function prototype at the top of the sketch. I haven't had this happen lately, so maybe that's been cleared up. Anyway, if you look at the last line of my post, the reason for the UL type designator is to document what your intentions are. From silent casts to "unnecessary" type specifiers, I think self-documenting code is a good thing.