Maths doens't work unless i use serial output?

Using Arduino Programming Questions
#1

Hey Guys!

My first arduino project and i’m currently writing a little program to be used on my bike for controlling the lights, the code is below -

#define GREENPIN 11
#define REDPIN 10
#define buttonPin 12
#define buttonPin2 8
int buttonCount=0;
char COLOUR;
int levels[0];
int arrayKey = 0;
int count = 0;
void setup() {
  pinMode(REDPIN, OUTPUT);
  pinMode(GREENPIN, OUTPUT);
  pinMode(buttonPin,INPUT_PULLUP); 
  pinMode(buttonPin2,INPUT_PULLUP);  
  Serial.begin(9600);
}

void loop(){
   int held=0;
   int brightness=0;  
   if (buttonCount==0){
     COLOUR = GREENPIN;
     arrayKey = 11;
   }else if (buttonCount==1){
     COLOUR = REDPIN;     
     arrayKey = 10;     
   }//buttonCount being 2 is the normal state of operation
   else if (buttonCount==3){
     COLOUR = 0;
     for (count=10;count<12;count++) {
      //kill the lights!
      levels[count] = 0;
     }
     buttonCount = 0;
   }
   while (digitalRead(buttonPin)==LOW){  
     held++;
     //slow the fade up a little.
     brightness=held/5;
     Serial.print(brightness);
     if (brightness < 252){
       analogWrite(COLOUR,brightness);
     }
     if (brightness >253){
       //max value reached, flash to show that.
       analogWrite(COLOUR,0);
       delay(300);
       analogWrite(COLOUR,254);
       delay(300); 
       brightness = 254;
     }
     //record that LEDs state.
     levels[arrayKey] = brightness;
  }
  while (digitalRead(buttonPin2)==LOW){  
      analogWrite(GREENPIN,254); 
      analogWrite(REDPIN,254);
  }
     for (count=10;count<12;count++) {
      //and have it flash
     analogWrite(count,levels[count]);
     delay(200);
     analogWrite(count,0);
  }
  if(held>1){
   buttonCount++;
  }  
}

the issue i have is if i take out the line -

Serial.print(brightness);

then the brightness variable just equals the held value, so scales up way to quickly! I can’t see any reason for that - anyone have any ideas? Any other comments/suggestions on the code are of course welcome :slight_smile:

Thanks!

Chris

#2

Printing brightness takes time which effectively slows down the fade. Try replacing it with a short delay() and you can experiment with the delay() to see the effect. Once you have that working consider using the technique used in the BlinkWithoutDelay example in the IDE to avoid the delay() blocking the execution of the code.

Another suggestion. Change the variables that don’t need to be ints to bytes. This will save space which may not be important in this program but could be in larger ones.

Most important of all

int levels[0];

Why do you have an array with zero levels in it and what will happen when you put values in memory locations that the program does not have ownership of, such as

    for (count=10;count<12;count++) {
      //kill the lights!
      levels[count] = 0;
     }

and

levels[arrayKey] = brightness;
#3

Thanks, i’ve removed the console and added a small delay (delay 10;) and it’s working! :slight_smile:

Regarding the changes from int to byte, i’ve made those changes as well. With the array (i’m new to c++!) the purpose of that is to store the brightness of the LED for it to then flash at that level - if there is a better way / cleaner way that would be great, but i wanted to keep the two levels serperate as i quite often only have the rear lights on (in the morning when it’s light enough but still don’t trust those drivers ;))

Thanks for the help!

Current code below -

#define GREENPIN 11
#define REDPIN 10
#define buttonPin 12
#define buttonPin2 8
byte buttonCount=0;
char COLOUR;
int levels[0];
byte arrayKey = 0;
byte count = 0;
void setup() {
  pinMode(REDPIN, OUTPUT);
  pinMode(GREENPIN, OUTPUT);
  pinMode(buttonPin,INPUT_PULLUP); 
  pinMode(buttonPin2,INPUT_PULLUP);  
}

void loop(){
  byte held=0;
  if (buttonCount==0){
    COLOUR = GREENPIN;
    arrayKey = 11;
  }
  else if (buttonCount==1){
    COLOUR = REDPIN;     
    arrayKey = 10;     
  }//buttonCount being 2 is the normal state of operation
  else if (buttonCount==3){
    COLOUR = 0;
    for (count=10;count<12;count++) {
      //kill the lights!
      levels[count] = 0;
    }
    buttonCount = 0;
  }
  while (digitalRead(buttonPin)==LOW){  
    held++;
    //slow the fade up a little.
    delay(10);
    if (held < 252){
      analogWrite(COLOUR,held);
    }
    if (held >253){
      //max value reached, flash to show that.
      analogWrite(COLOUR,0);
      delay(300);
      analogWrite(COLOUR,254);
      delay(300); 
      held = 254;
    }
    //record that LEDs state.
    levels[arrayKey] = held;
  }
  while (digitalRead(buttonPin2)==LOW){  
    analogWrite(GREENPIN,254); 
    analogWrite(REDPIN,254);
  }
  for (count=10;count<12;count++) {
    //and have it flash
    analogWrite(count,levels[count]);
    delay(200);
    analogWrite(count,0);
  }
  if(held>1){
    buttonCount++;
  }  
}
#4

Using an array is OK, but

int levels[0];

declares an array with zero elements. Later on in your code you store values in it but there is no space allocated for the storage. C++ will not stop you doing this. It is your responsibility to declare the array to be large enough and to manage the array indices in your program.

How many array levels do you need in your program

    for (count=10;count<12;count++) {
      //kill the lights!
      levels[count] = 0;
     }

This looks odd because you only use 2 levels so why not use levels 0 and 1 and declare the levels array like this ?

int levels[2];
#5

It's a bit of a hack to just use the pin numbers where the LEDs are located, hence using 10 & 11 :slight_smile:

#6

Please edit your post, select the code, and put it between [code] … [/code] tags.

You can do that by hitting the # button above the posting area.

How to use this forum

#7

You can do that by hitting the # button above the posting area.

Where would that button be on the new system ?

#8

woosey:
It's a bit of a hack to just use the pin numbers where the LEDs are located, hence using 10 & 11 :slight_smile:

OK, if that's how you want to do it, but you absolutely must fix the array declaration or your program will be unstable.

#9

Let’s look at the code, without the Serial output operation:

void loop()
   {
     int held = 0;
     ...some stuff
     while(button is held down)
       {
         held++;
         set LED to held intensity
        }
     ... more stuff

What you have said here is: look at the state of the held button (a few instructions), and if it is LOW, (one instruction), then increment held (one instruction), set the LED to that intensity (a few instructions), lather rinse, repeat. So you are executing

a few instructions
one instruction
one instruction
a few instructions

on a processor where you can generally think of running SIXTEEN MILLION instructions per second! So you will see that light ramp up from 0 to max perhaps a half-million times per second, or, in a case where I have guessed the instruction counts too low, maybe as slowly as 100,000 times per second. Hmm…is this what you really want to do?

The problem with the “processing” model is that if you put no explicit delays in, that loop will execute hundreds of thousands to millions of times per second. Furthermore, you have made “held” a local variable, and initialized it to 0, so it will be reset to 0 on every iteration through loop(). So you have to hold that state someplace OUTSIDE the loop, such as a (shudder) global variable. In addition, you have to take timing into consideration; to avoid the gratuitous delay() calls (which ultimately only get you in trouble), you can look at micros(), and based on the desired ramp-up time (e.g., 0 to full brightness in 3 seconds means you divide the 3,000,000 microseconds into 255 intervals, e.g., every 11,720 microseconds you increment the current brightness (stored outside the loop() function) you add 1 to the brightness. note that you CANNOT write

if(micros() % 11720 == 0)

because the value is not incrementing by 1; in fact, it is unpredictable. So what you do is

#define DELTA_T 11720 // for example
#define MAX_BRIGHTNESS 255 // for example

long t = micros();
if(t > last_time + DELTA_T)
    { /* time to increment */
      if(brightness <= MAX_BRIGHTNESS)
         { /* still not full */
           brightness++;
          ...write brightness
          last_time = t;
         } /* still not full */
     } /* time to increment */

Left as An Exercise For The Reader: what to do if the light has reached maximum brightness, how to change the colors, etc. This is just an outline of the idea.

I have found that fewer steps actually works much better. For example, for some displays, 0 and 31 are indistinguishable states (unless you are dark-adapted in a completely black room except for the LED). So the time spent going from 0 to 31 serves no purpose. Similarly, 32 to 47 might not be distinguishable. So you can go with bigger jumps than 1. In addition, the human eye is very nonlinear in its response curve, and it has different response curves for red and green (the maximum sensitivity is in yellow, and red is actually very hard to see). To create “trails” of LEDs in an animated display, I started at 255 and divided by 2 so I got the sequence 255,127,63,31,15. The 15 value is just barely visible. So the “tail” is only five pixels long on an 8x8 display; this is an “idle but ready” pattern that runs in a “circle” (as it were) around the edges of the display if it detects nothing interesting nearby. [Bonus points if you can square the circle].

In general, avoid delay() calls. When you add a sensor to your app, the delay() limits how often you read the sensor, and if the sampling rate drops low enough, you will begin to fail to respond to the sensor data.

#10

UKHeliBob:

You can do that by hitting the # button above the posting area.

Where would that button be on the new system ?

It’s the second to last icon on the top row, the “scroll” with the “<>” on top of it. It’s one of the many regressions in the usability of the new forum software… :frowning:

Ralf

#11

It’s the second to last icon on the top row, the “scroll” with the “<>” on top of it. It’s one of the many regressions in the usability of the new forum software… :frowning:

I know where it is and what it looks like, it’s just that it’s not a # anymore since the “upgrade”

#12 
#define DELTA_T 11720 // for example

#define MAX_BRIGHTNESS 255 // for example

long t = micros();
if(t > last_time + DELTA_T)
   { /* time to increment /
     if(brightness <= MAX_BRIGHTNESS)
        { / still not full /
          brightness++;
         …write brightness
         last_time = t;
        } / still not full /
    } / time to increment */

t is declared as a signed long so at some point it will become negative which will mess up the timing. It would be better to declare it as an unsigned long to avoid the problem. Then of course it will roll over to zero with micros() and mess up the timing again.

It is better to use an unsigned long but to do the test for the period being finished by subtracting the start time from the current time and comparing the result with the period (also declared as unsigned long). This avoids the problem with the rollover to zero.

#13

UKHeliBob:

You can do that by hitting the # button above the posting area.

Where would that button be on the new system ?

Well, Bob, I’ve amended my boilerplate now. :stuck_out_tongue:

Please edit your post, select the code, and put it between [code] … [/code] tags.

You can do that by hitting the “Code” button above the posting area (It looks like a scroll with < > inside it).

#14

UKHeliBob:
I know where it is and what it looks like, it’s just that it’s not a # anymore since the “upgrade”

Definition of “upgrade”: Exchange your old bugs for new ones.

#15