While loop does not recognize variable change

Go To Last Post
22 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello,

I have some trouble with my code lightning some LED Apa102. The problem might be the while loop but for a better understanding of the code I will give you a brief explanation.

 

The main.c function just calls the setup function of led.c and calls start_light_pattern(), set_led_pattern.

In the led.c we have an array called led_array. This array stores the values we want to write to the pin according to the APA102 Docu (first the start bits, then brightness, b,g,r for each led, end bits). 

Because we want to have more than one pattern, we define different pattern arrays. In these we define the different steps and what to do like: set led 0 to brightness 10, b=10, g=200, r=45. So this pattern is a three dimensional array. First dimension shows how much steps we have in this pattern, the second dimension could be as big as the number of leds, because we might want to change the value of every led in one step. The third dimension is 5 (first index of led, then brightness, and rgb).

For every pattern there is a duration array, which gives us the number of timer0 overflows we want to wait between the single pattern steps.

 

In each timer0 overflow interrupt we check if the duration of the current pattern step is over. If so, we update the leds in the led_array with the corresponding values set in the current led pattern array.

Like: look at the second step of the pattern in led_pattern_1. For each led configution in this step, update the led values in the general led_array.

If we have set some new led configurations in the interrupt routine, a variable called write_update is set to 1 to indicate that we need to write some new byte values to the pin.

 

 set_led_pattern() sets the current led pattern index. If it's zero no patten will be executed. Right now we only have 1 pattern.

start_light_pattern() starts a while loop which check if led_pattern is unequal to 0 and write_update is 1. If so, the led_array is written to the pin.

 

So here is the code:

 

 

main.c

# include "led.c"
...
...


void setup() {
    setup_led();
};

int main(){
    setup();
    start_light_pattern();
    set_led_pattern(1);
    while(1) {
       // 
    }
    
    
}

    led.c

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <avr/io.h>
#include <avr/interrupt.h>
#include <util/delay.h>

// uint8 goes up to 255
#define CLI_PIN (uint8_t)5 //pin 13
#define DATA_PIN (uint8_t)3 //pin 11
#define NUM_LEDS (uint8_t)12
#define LOW (uint8_t)0
#define HIGH (uint8_t)1

// brightness can only go up to 31 (since it's 5 bits)
// which led to change, then bright, rgb
const uint8_t led_pattern_1[12][NUM_LEDS][5] = {
    {{0, 10 , 255, 255, 0}},
    {{1, 10 , 0, 255, 0}},
    {{2, 10 , 0, 0, 255}},
    {{3, 10 , 0, 255, 0}},
    {{4, 10 , 255, 0, 0}},
    {{5, 10 , 0, 255, 0}},
    {{6, 10 , 255, 0, 255}},
    {{7, 10 , 255, 0, 0}},
    {{8, 10 , 255, 0, 0}},
    {{9, 10 , 255, 0, 0}},
    {{10, 10 , 255, 0, 0}},
    {{11, 10 , 255, 0, 0}},
};

const uint16_t duration_1[NUM_LEDS] = {
    200,
    200,
    200,
    200,
    200,
    200,
    200,
    200,
    200,
    200,
    200,
    200,
};

// which led to change, then bright, rgb
volatile uint8_t led_array[NUM_LEDS][4] = {
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
        {0, 0, 0, 0},
};

volatile int8_t counter = -1;
volatile int8_t duration = -1;
volatile uint8_t write_update = 0;
volatile int8_t light_pattern = -1;

//set pin high or low
void write_to_pin(uint8_t value, uint8_t pin){
    if (value == HIGH){
        PORTB |= (HIGH<<pin);
    } else {
        PORTB &= ~(HIGH<<pin);
    }
}

//step through each bit of a byte
void handle_byte(uint8_t byte){
    for(uint8_t i=8; i>0; --i){
        write_to_pin( ((byte) & (1 << i)) != 0 ? HIGH : LOW, DATA_PIN);
        write_to_pin(HIGH, CLI_PIN);
        write_to_pin(LOW, CLI_PIN);
    }
}

//step through each byte in the array
void handle_byte_array(){
     write_update = 0;

     // write starting bytes, 32 zeros
     handle_byte(0);
     handle_byte(0);
     handle_byte(0);
     handle_byte(0);

    for(uint8_t i=0; i < NUM_LEDS; ++i){
        handle_byte((0b111 << 5) | led_array[i][0]);
        handle_byte(led_array[i][3]);
        handle_byte(led_array[i][2]);
        handle_byte(led_array[i][1]);
    }

    write_to_pin(HIGH, DATA_PIN);
    for (uint8_t i = 0; i < NUM_LEDS/2; ++i)
    {
        write_to_pin(HIGH, CLI_PIN);
        write_to_pin(LOW, CLI_PIN);
    }
}

void setup_led() {
    cli();

    DDRB = (1 << DATA_PIN); // digital pin 11
    DDRB = (1 << CLI_PIN); //digital pin 13

    // Set Normal mode (Waveform Generator Mode)
    TCCR0A |= (0 << WGM01) | (0 << WGM00);
    TCCR0B |= (0 << WGM02);

    // Set Prescaling to Clock / 1024 (CS)
    TCCR0B |= (1 << CS00) | (0 << CS01) | (1 << CS02);

    // TOIE0 Timer 0 Overflow Interrupt Enable
    TIMSK0 = 0;
    TIMSK0 |= (1 << TOIE0);

    // enable interrupts
    sei();
};


void set_led_pattern(pattern_id){
    light_pattern = 1;
}

void start_light_pattern(){
while(1){
    if (light_pattern > 0 && write_update == 1){
         handle_byte_array();
    }
}
}

void reset_led_array () {
    for (uint8_t i = 0; i < NUM_LEDS; ++i) {
        for (uint8_t j = 0; j < 4; ++j) {
             led_array[i][j] = 0;
        }
    }
}

void handle_current_pattern_step () {
    // for debuggin purpose we change all generic variables to the
    //values for pattern _1
    if (counter + 1 < 12) {
        ++counter;
        duration = duration_1[counter];

        // ((int) (sizeof(led_pattern_1[counter]) / sizeof(led_pattern_1[counter])[0]))

        for(uint8_t i=0; i<1; ++i){
            uint8_t curr_idx = led_pattern_1[counter][i][0];

            for (uint8_t j=1; j<5; ++j) {
                led_array[curr_idx][j-1] = led_pattern_1[counter][i][j] ;
            }
        }
        write_update = 1;
    } else {
        write_update = 1;
        counter = -1;
        duration = -1;
        reset_led_array();
    }
}

ISR (TIMER0_OVF_vect)
{
    if (light_pattern > 0 & duration == -1) {
        switch(light_pattern) {
            case 1: handle_current_pattern_step(); break;
            default: handle_current_pattern_step();
        }
    } else if (light_pattern > 0 & duration > 0) {
        --duration;
    }
}

So here is what happens. If we run this, no led is lightning. 

We check it, and the interrupt service routine never goes into "if (light_pattern > 0 & duration == -1)". 

So it seems like as soon as we start the while loop with:  start_light_pattern(), the while loop does not recognize any changes of the variables:

light_pattern, write_update and the led array led_array.

 

If we change the order so we first set the pattern index and the change the while loop, the first led lights but the orders do not. So it seems like

before the while loop starts the first pattern step is executed via the interrupt routine, but then the while loop again does not notice any changes in

write_update and the led_array.

 

Is this assumption correct? If so, what can we do? I thought defining these variables as "volatile" should be the solution, but it does not work?

Are there any other problems?

 

We tested it, and in general the service routine and the "writing to the pin methods" are working if we just change one led and trigger the "handle byte array" manually an not starting the while loop. So there shouldn't be any problem with these functions.

 

Thanks for any help!

Last Edited: Sat. Nov 30, 2019 - 10:00 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

michac1995 wrote:
if (light_pattern > 0 & duration == -1)

 

This is a quite common C error.

 

& means AND, the two expressions are anded together.

 

&& means logical AND. Replace & with && to make it work. 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Suggestion: parenthesis do not cost anything. That is, they do not make the compiled code any bigger. While the C language rules of precedence fully apply in this statement, you can remove a lot of  potential confusion if you simply do:

 

if ( (light_pattern > 0) && (duration == -1) )

Also, white space costs nothing. Note the white spaces at each end and the middle. Now, the grouping is completely unambiguous to almost all readers.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:
Suggestion: parenthesis do not cost anything

Just playing Devil's Advocate.

Suppose you write

if (a = 2 && b == 3)

where you really meant a == 2, you get a warning from the compiler.

If, however, you write

if ((a = 2) && (b == 3))

you don't get the warning.

So extra brackets are not always a good thing!

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

MrKendo wrote:

Suppose you write

if (a = 2 && b == 3)

where you really meant a == 2, you get a warning from the compiler.

...

 

I've seen some writers advocate the use of...

 

if ( constant == variable)

...to avoid such errors.

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

Last Edited: Sat. Nov 30, 2019 - 05:33 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

I hate the look of it but you should use "2 = a" anyway as "2 == a" is a valid test but 2 = a will always be an error whether parenthesised or not.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yes, Yoda style is safest way, if you can put up with it. I don't like it so have to rely on the compiler warnings :)

 

As to using & instead of &&

if (light_pattern > 0 & duration == -1)

I get a warning for that too, although I think it will actually work as intended in this case, each of the comparisons produces a result with value either 0 or 1, which as a bitwise  and will still work  (but you don't get the short-circuit behaviour of logical and).

 

This looks suspicious.

Assuming light_pattern > 1 [EDIT I meant > 0], you do something once which sets duration > 0, it will then be decremented back to 0, but will never go below 0. I'm wondering if you meant that duration should get decremented back to -1.

michac1995 wrote:

ISR (TIMER0_OVF_vect)
{
    if (light_pattern > 0 & duration == -1) {
        switch(light_pattern) {
            case 1: handle_current_pattern_step(); break;
            default: handle_current_pattern_step();
        }
    } else if (light_pattern > 0 & duration > 0) {
        --duration;
    }
}

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for your tips.
I tried it but nothing changed.:/

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your while loop in main does nothing, just an endless loop.

So the only thing that will do anything is the timer overflow ISR.

That does one of three things depedning on the values of light_pattern and duration,

call handle_current_pattern_step, decrement duration, or do nothing.

Are any of those 3 three things actually changing the LED hardware?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

There is a while loop in led.c . This starts as soon as the main.c calls start_led_pattern.

The while loop writes the values which stand in led_array to the pin and the interrupt Updates the led_array

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

michac1995 wrote:
There is a while loop in led.c

Not in the code you posted in #1.

If there is a while loop in some other code, OK, but we can't see it :)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'm sorrY, you are absolutely right.
Due to some debugging issues I loaded up the wrong version.
I've edit it. Now there is a while loop in the start_led_pattern function in led.c

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

OK, I see it now.

Having an endless whie loop in start_light_pattern function is really not the right way to be going about it.

When your main function calls start_light_pattern, that call will never return.

Anything you have in main after that will never be executed.

So it never even gets to calling set_led_pattern and never reaches the while(1) loop in main.

 

You should only have one endless loop in the program, that should be the while(1) loop in main.

 

You need a re-think,

Two possibilities that spring to mind.

1) Your while loop in main tests for a flag that is set in the timer ISR, and does something when the flag is set.

2) Your while loop in main does nothing, everything is driven from the ISR (this method is generally only appropriate for simple case where your program only does the one thing).

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

michac1995 wrote:
I've edit it.

Please don't edit a post after people have replied to it!

 

It makes things really confusing, as the comments cease to make sense because the post they referred to has changed!

 

 

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
ISR (TIMER0_OVF_vect)
{
    if ( (light_pattern > 0) && (duration == -1) )
        {
        switch(light_pattern)
          {
            case 1:
              handle_current_pattern_step();
              break;

            default:
              handle_current_pattern_step();
          }
       }
    else if ( (light_pattern > 0) && (duration > 0) )
       {
        --duration;
       }
}

What changes did you make? This is what was intended methinks.

 

 

 

Also:

 

void setup_led() {
    cli();

    DDRB = (1 << DATA_PIN) | (1 << CLI_PIN); // digital pin 11 and 13

Your original code wrote to DDRB twice - only the last write stuck.

Last Edited: Sat. Nov 30, 2019 - 11:51 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The main.c calls the start_led_pattern, and now there is a while loop inside that function.

void start_light_pattern(){
while(1){
if (light_pattern > 0 && write_update == 1){
handle_byte_array();
}
}
}

So the while loop calls handle_byte_array if the conditions are true. Handle_byte_array looks at the led_array and writes to the pins. In the interrupt we change the values of this led_array.

With regard to your last point with writing to DDRB that actually seems to work like that.

Thanks!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

If you run your code in the atmel studio simulator you’ll be able to get some insight into what your code is actually doing.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Taking what you have already, if you do something like this I think you are not too far off

int main()
{
    setup();

    set_led_pattern(1);

    while(1)
    {
        start_light_pattern();
    }
     

Your start_light_pattern function is where you are checking the update flag.

I would give the function a better name. EDIT and remove the while(1) loop from start_light_pattern.

Last Edited: Sun. Dec 1, 2019 - 11:12 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 2

Problem is solved:
As soon as the connection estblishes the arduino resets. This was the reason why the led turn offs and on again ( so this wasn't triggered by pyserial).
Because we send the 0 immediatley after the connetion , the arduino still was reseting.

After putting pyserial code in the while loop it worked.

Thanks!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

michac1995 wrote:
Problem is solved

Then see Tip #5.

 

However,

After putting pyserial code in the while loop it worked.

What do you mean by that?

 

Not sure how that solves the problem as stated?

 

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I think it's mixed up with this thread

https://www.avrfreaks.net/forum/...

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are absolutely right,
I'm sorr,!