what's wrong with this code? (AVR)

[foxbrain]

This circuit is about atmega16L:
PORTA is connected to switches.
PORTB is connected to two 7segments.
PORTC is connected to on led on pin 2 for testing.
PORTD is connected to two switch transistors to choice which 7segment to work.
The code is:

Code:
#include <util/delay.h>
#include <avr/io.h>
#define SETBIT(ADDRESS,BIT) (ADDRESS |= (1<<BIT))
#define CLEARBIT(ADDRESS,BIT) (ADDRESS &= ~(1<<BIT))
#define CHECKBIT(ADDRESS,BIT) (ADDRESS & (1<<BIT))
void makeit (int my)
{
switch (my)
{
case 0 :
PORTB = ~(0b01111110);
break;

case 1:
PORTB = ~0b00010010;
SETBIT(PORTC,2);
break;

case 2:
PORTB = ~0b10111101;
break;

case 3:
PORTB = ~0b10110111;
break;

case 4:
PORTB = ~0b11010011;
break;

case 5:
PORTB = ~(0b11100111);
break;

case 6:
PORTB = ~0b11101111;
break;

case 7:
PORTB = ~0b00110010;
break;

case 8:
PORTB = ~0b11111111;
SETBIT(PORTC,2);
break;

case 9:
PORTB = ~0b11110111;
break;
}
}
int main(void)
{
DDRA=0x00;
DDRB=0xFF;
DDRC=0xFF;
DDRD=0xFF;
int num=00;
int ten;
int one;
PORTB=0xFF;
PINA=0b11111111;
PORTA=0xFF;
do
{
if(PINA==0b11111110)num=01;
else if(PINA==0b11111101)num=12;
else if(PINA==0b11111011)num=23;
else if(PINA==0b11110111)num=34;
else if(PINA==0b11101111)num=45;
else if(PINA==0b11011111)num=56;
else if(PINA==0b10111111)num=67;
else if(PINA==0b01111111)num=78;
else PINA=0b11111111;
SETBIT(PORTC,2);
_delay_ms(1000);
CLEARBIT(PORTC,2);
_delay_ms(1000);
} while (PINA =0b11111111);
do
{
SETBIT(PORTC,2);
asm("NOP");
} while (PINA!=0xFF);
while(1)
{
if(PINA==0b11111110) num++;
//else if(PORTA==0b01111111)num=89;
else if(PINA==0b11111101)num--;
ten=num/10;
one=num%10;
SETBIT(PORTD,4);
makeit(ten);
_delay_ms(10);
CLEARBIT(PORTD,4);
SETBIT(PORTD,1);
makeit(one);
_delay_ms(10);
CLEARBIT(PORTD,1);
}
}

It should work like this :
1- Nothing is displayed on the 7segments , and the led begins to light on and off.
2- When I press a bottom on any of portA it should get out of the first while loop and keeps the led turned on and a number is saved in the integer num and the led turns off so it is on the second do while loop.
3- When I stop pressing the bottom the number appears on the 7segment and I can increase or decrease it when I press the bottom on PINA,0 or PINA,1.
The problem is:
1- The operation execution is still stuck in the first loop even when I press any bottom.
2- if I skipped the first do while loop , the increment and decrement doesn’t appear working good. Is it because it is increasing too fast ? what is the solution?
3- also I’m having some warning:
• #warning "F_CPU not defined for <util/delay.h>"
• #warning "Compiler optimizations disabled; functions from <util/delay.h> won't work as designed"
• large integer implicitly truncated to unsigned type
How can I solve all of these problems?
Thanks in advanced.

 

[luben111]

Re: what's wrong with this code?

Hi
SETBIT(ADDRESS,BIT) (ADDRESS |= (1<<BIT)) looks incorrect for me
It should be something like this (you need to use pointer

SETBIT(ADDRESS,BIT) (((uint8_t *)(ADDRESS))* |= (1<<BIT))

Right now you simply amnipulate the address value, not the cell pointed by this address

 

[foxbrain]

Re: what's wrong with this code?

no,i'm sure about it , i made before this circuit : a circuit that check if a buttom is pressed turn on a led or turn it off .
also i tried on that make the leds turns on and off with a time delay and it worked good..
anyway as i wrote up the led mission is just to know where the atmega16L is doing (executing)in the code.

 

[alexan_e]

Re: what's wrong with this code?

Hi
SETBIT(ADDRESS,BIT) (ADDRESS |= (1<<BIT)) looks incorrect for me
It should be something like this (you need to use pointer

SETBIT(ADDRESS,BIT) (((uint8_t *)(ADDRESS))* |= (1<<BIT))

Right now you simply amnipulate the address value, not the cell pointed by this address

#define SETBIT(ADDRESS,BIT) (ADDRESS |= (1<<BIT)) is a macro so when you use SETBIT(PORTD,4); it is translated to (PORTD|= (1<<4)) , there is nothing wrong with that , it is a very common bitwise operation

This is a list of some macros that make coding easier

---

Code C - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
// add this defines
#define SETBIT(ADDRESS,BIT) (ADDRESS |= (1<<BIT)) 
#define CLEARBIT(ADDRESS,BIT) (ADDRESS &= ~(1<<BIT)) 
#define FLIPBIT(ADDRESS,BIT) (ADDRESS ^= (1<<BIT)) 
#define CHECKBIT(ADDRESS,BIT) (ADDRESS & (1<<BIT)) 
#define WRITEBIT(RADDRESS,RBIT,WADDRESS,WBIT) (CHECKBIT(RADDRESS,RBIT) ? SETBIT(WADDRESS,WBIT) : CLEARBIT(WADDRESS,WBIT))
 
// and then you can use
SETBIT(PORTX,2);  // set bit2 of portx (set to 1)
SETBIT(PORTY,0);  // set bit0 of porty
 
CLEARBIT(PORTX,5); // clear bit5 of portx (set to 0)
 
FLIPBIT(PORTX,2);  // invert bit2 of portX, if it was 1 it becomes 0, and if 0 becomes 1
                    // example for portx 0x10101010 the result is 0x10101110
                    
if (CHECKBIT(PORTX,4)) {} else {} // result is true if bit4 of portx is 1, false if it is 0
if (!CHECKBIT(PORTX,4)) {} else {} // result is true if bit4 of portx is 0, false if it is 1
 
WRITEBIT(PORTX,0,PORTY,2);   // copy the bit0 of portx to bit2 of porty

---

Alex

---------- Post added at 16:22 ---------- Previous post was at 16:04 ----------

I assume you are using AVRstudio with winavr,

In every project you have to define the clock, select project -> configuration options , from there set your clock speed (for 8MHz enter 8000000) ans also the optimization level (default Os), this will take care of the warnings

 

[alexxx]

Re: what's wrong with this code?

1- The operation execution is still stuck in the first loop even when I press any bottom.

Your first loop is:

do
{
//..........
//..........
//..........
} while (PINA =0b11111111);

Don't you get the following warning?
use of "=" where "==" may have been intended

My compiler gives me warning about that, just replace "=" with "==" and tell us what happened.

Regards,
Alexis

 

[alexan_e]

Re: what's wrong with this code?

Another note,
when you want to check if a button is pressed on not don't use

---

Code C - [expand]
1
2
if(PINA==0b11111110) num++;
 else if(PINA==0b11111101)num--;

---

because the condition you are using needs the complete port to be exactly as specified.
If your buttons are in bit 0 and 1 then check only these bits

---

Code C - [expand]
1
2
if (!CHECKBIT(PINA,0)) num++;  // check if bit0 is 0
 else if (!CHECKBIT(PINA,1)) num--;  // check if bit1 is 0

---

Alex

---------- Post added at 17:18 ---------- Previous post was at 16:31 ----------

This coding style using delays is not very good because you are making the cpu wait until it is time to show a different value, you can't do anything else in the meantime so a better idea is to use a timer that gives an interrupt when it is time to refresh the digits.
So a better way would be:

define an array
char display_digit[10]= (~0b01111110,~0b00010010,~0b10111101,~0b10110111,~0b11010011,~0b11100111,~0b11101111,~0b00110010,~0b11111111,~0b11110111); (these are your values, I hope they are correct)
in which you define the digits 0..9, these represent the values of the port connected to the led segmets
for example to light the number 5 you do PORTX =display_digit[5] , or for 0 PORTX =display_digit[0] etc

then you define an array of 2 char.
char my_number[2];

You store the two numbers you want in this array, for example for 25
my_number[0]=2;
my_number[1]=5;

When you want to show the first digit you use
PORTB = display_digit[ my_number[0] ]
and for the second number
PORTB = display_digit[ my_number[1] ]

Now you can use a timer interrupt to do the following (the refresh rate will be controlled by the timer interrupt rate)

---

Code C - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
ISR(TIM0_OVF)   // interrupt function
{   static digit;
 
    write here the code to turn off all displays
 
    if (digit==0)
    {   digit=1;
        PORTB = display_digit[my_number[0]]; // set the segment port
        write here the code to turn on the first display
    }
 
    else
    {   digit=0;
        PORTB = display_digit[my_number[1]]; // set the segment port
        write here the code to turn on the second display
    }
}

---

Alex

 

[foxbrain]

Re: what's wrong with this code?

alexan i think you are write of using checkbit than using pina==.......
but i didn't understand what do u mean by the reason:because the condition you are using needs the complete port to be exactly as specified.?
.....
anyway: i edit on my code CHECKBIT(PINA,..) on every if statment as alexan said, and I put == instead of = in while statement and here is what happened:
the led is on and off , i put 0v on one of the A pins the led turns on , i remove the 0v from the A pins the 7segment gives 0 then 0 and the operation starts again from the main function!!!!
and when i repeat is again and again it appears write and stops changes just the numbers (works write).
So why is it sometimes working and other not?!
is there like scramble while putting 0v?
about using interrupt function : i don't prefer that because i still don't know about it , maybe later.... (thanks)

 

[alexan_e]

Re: what's wrong with this code?

alexan i think you are write of using checkbit than using pina==.......
but i didn't understand what do u mean by the reason:because the condition you are using needs the complete port to be exactly as specified.?

It is quite simple, when you use this if (PINA==0b11111110) then it will only be true if portA input value is 0b11111110 only,
on the other hand this if (!CHECKBIT(PINA,0)) will be translated to if (!(PINA & (1<<0))), this will be true when portA input value is 0bXXXXXXX0 where X can be any value so this code check only one bit of the port.

Another thing I noticed is that you are writing values to PINA, this is wrong PINA is only used to read the port value.
When you set

---

Code C - [expand]
1
2
3
DDRA=0x00;  // portA as input
PORTA=0xFF; // enable all pull up resistors in portA 
// PINA=0b11111111;  you shouldn't use this, PINA is only to read the input and the value is 0xFF already because you have enabled the pullups in the previous line.

---

You also have it

---

Code C - [expand]
1
2
3
4
5
6
7
8
9
if(PINA==0b11111110)num=01;
else if(PINA==0b11111101)num=12;
else if(PINA==0b11111011)num=23;
else if(PINA==0b11110111)num=34;
else if(PINA==0b11101111)num=45;
else if(PINA==0b11011111)num=56;
else if(PINA==0b10111111)num=67;
else if(PINA==0b01111111)num=78;
//  else PINA=0b11111111; remove it from here too

---

I have no idea of your circuit, I think you have one display and you show tens and ones alternating with 10ms delay between them, can you try with 500ms or 1000ms so that you can see clearly what is happening because with 10ms delay you change the display 500 times/sec.

When you say

the operation starts again from the main function

how do you know, how are you debugging the code?

Alex

 

[foxbrain]

Re: what's wrong with this code?

1 : about my circuit : well i'm using 2 7segments (common anode), they are connected to PORTB , pins 1 and 4 of PORTD are connected to two switching transistors (2n2222) so i get the char ten display it on the first 7segment then turn it off and on the another one to display the char one and so on....
2 : i forgot to tell you about the warnings , i'm using avr studio 5 not avr studio 4 with winavr and everything seems different that you had told me.
3 :
quote: "It is quite simple, when you use this if (PINA==0b11111110) then it will only be true if portA input value is 0b11111110 only, "
as i know that a pin that is non-connected to anything it will take 1 logic , other if it may have some scrambles so it can take 0 or 1 but by using SETBIT(..,..)i'm checking one bit so no way for scrambling??
4 :
quote : "how do you know, how are you debugging the code?"
when i'm in the first do while loop the led will turn on and off when i'm in the second loop the led is on and when i'm in the infinite loop the led is off , so here i know where am i in the executing code.
................
anyway i had edit the things you had told me about (not using PINA=.....)and everything is working good except num is keeping the value 00 ..but if i pressed the buttom before turning on the power to the circuit everything is great.
so as I see is that the execution of the first loop is out of it before changing values of num .
i tried to put another condition to get out of the first loop : while (PINA == 0b11111111 && num==00); but it didn't work!!!

foxbrain

 

[alexan_e]

Re: what's wrong with this code?

You shouldn't be in such a hurry to install the AVRstudio 5, it is still a beta version so there may be bugs, there is nothing wrong with version 4.
There are two way to do things , one is something that works and the other is something that is correct, when you are interested in the value of one bit what is the purpose of checking the entire port?
And if you decide tomorrow to add a led to the port of any other device why would you put yourself in the position to have to go back and edit all your code, and to answer your question yes all unconnected pins will have the value of 1 because of the pull up resistors.
A small note, I didn't use SETBIT but CHECKBIT.

Post your code as it is now so I can see what is wrong because you have changes many things from the original code

Alex

---------- Post added at 23:51 ---------- Previous post was at 23:29 ----------

What I see wrong is in this part

---

Code C - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
do
    {
        if(PINA==0b11111110)num=01;
        else if(PINA==0b11111101)num=12;
        else if(PINA==0b11111011)num=23;
        else if(PINA==0b11110111)num=34;
        else if(PINA==0b11101111)num=45;
        else if(PINA==0b11011111)num=56;
        else if(PINA==0b10111111)num=67;
        else if(PINA==0b01111111)num=78;
        SETBIT(PORTC,2);         // if you press your button anywhere after this line you will get out of the loop without setting the value
        _delay_ms(1000);
        CLEARBIT(PORTC,2);
        _delay_ms(1000);
    } while (PINA =0b11111111);

---

A simple solution would be to use the num assignment after the loop

---

Code C - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
do
{
    SETBIT(PORTC,2);
    _delay_ms(1000);
    CLEARBIT(PORTC,2);
    _delay_ms(1000);
} while (PINA =0b11111111);  // if any change you will go to the next line to assign the num
if(PINA==0b11111110)num=01;
else if(PINA==0b11111101)num=12;
else if(PINA==0b11111011)num=23;
else if(PINA==0b11110111)num=34;
else if(PINA==0b11101111)num=45;
else if(PINA==0b11011111)num=56;
else if(PINA==0b10111111)num=67;
else if(PINA==0b01111111)num=78;

---

but I still don't like that you have these large delays (_delay_ms(1000) and you can easily have to wait over one second before your input is detected

I would prefer smaller delays like the following and storing the input would also be a good idea because there is always the chance that the input may change in the wrong moment

---

Code C - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
uint8_t delay_counter;
uint8_t PINA_storage;
do
{
    if (delay_counter==100) SETBIT(PORTC,2);
    else if (delay_counter==200)
    {
        CLEARBIT(PORTC,2);
        delay_counter=0;
    }
    _delay_ms(10);
    delay_counter++;
    PINA_storage=PINA;  // store the input and use it in the following code
 
} while (PINA_storage =0b11111111); // now this will be checked every 10ms instead of 2000ms
 
if(PINA_storage==0b11111110) num=01;
else if(PINA_storage==0b11111101) num=12;
else if(PINA_storage==0b11111011) num=23;
else if(PINA_storage==0b11110111) num=34;
else if(PINA_storage==0b11101111) num=45;
else if(PINA_storage==0b11011111) num=56;
else if(PINA_storage==0b10111111) num=67;
else if(PINA_storage==0b01111111) num=78;

---

Alex

 

[foxbrain]

i liked avr studio 5 just because it doesn't need to add winavr to write in c language which sometimes makes problems (errors)...but what your wrote is right about this.
ya that's right but what about if i want two or three buttoms to be pressed to act something?should i put if(CHECKBIT)&&(CHECKBIT)&&(CHECKBIT)?....
quote:"I didn't use SETBIT but CHECKBIT."
sorry i wrote it by wrong....
now it's done after i put setbit and clearbit before the if else statement..it's a good idea to use the code you had written (to use it instead of the delay "thanks")
there is a last small thing but first let's look at the code how it is:

#include <util/delay.h>
#include <avr/io.h>
#define SETBIT(ADDRESS,BIT) (ADDRESS |= (1<<BIT))
#define CLEARBIT(ADDRESS,BIT) (ADDRESS &= ~(1<<BIT))
#define CHECKBIT(ADDRESS,BIT) (ADDRESS & (1<<BIT))
void makeit (int my)
{
switch (my)
{
case 0 :
PORTB = ~(0b01111110);
break;

case 1:
PORTB = ~0b00010010;
SETBIT(PORTC,2);
break;

case 2:
PORTB = ~0b10111101;
break;

case 3:
PORTB = ~0b10110111;
break;

case 4:
PORTB = ~0b11010011;
break;

case 5:
PORTB = ~(0b11100111);
break;

case 6:
PORTB = ~0b11101111;
break;

case 7:
PORTB = ~0b00110010;
break;

case 8:
PORTB = ~0b11111111;
SETBIT(PORTC,2);
break;

case 9:
PORTB = ~0b11110111;
break;
}
}
int main(void)
{
DDRA=0x00;
DDRB=0xFF;
DDRC=0xFF;
DDRD=0xFF;
int num=00;
int ten;
int one;
PORTB=0xFF;
PORTA=0xFF;

do
{
SETBIT(PORTC,2);
_delay_ms(1000);
CLEARBIT(PORTC,2);
_delay_ms(1000);
} while (PINA ==0b11111111); // if any change you will go to the next line to assign the num
if(PINA==0b11111110)num=01;
else if(PINA==0b11111101)num=12;
else if(PINA==0b11111011)num=23;
else if(PINA==0b11110111)num=34;
else if(PINA==0b11101111)num=45;
else if(PINA==0b11011111)num=56;
else if(PINA==0b10111111)num=67;
else if(PINA==0b01111111)num=78;
else PINA=0b11111111;

do
{
SETBIT(PORTC,2);
asm("NOP");
} while (PINA!=0xFF);
while(1)
{
CLEARBIT(PORTC,2);
//if(!CHECKBIT(PINA,6)) num--;
//else if(!CHECKBIT(PINA,7))num++;
ten=num/10;
one=num%10;
SETBIT(PORTD,4);
makeit(ten);
_delay_ms(1000);
CLEARBIT(PORTD,4);
SETBIT(PORTD,1);
makeit(one);
_delay_ms(1000);
CLEARBIT(PORTD,1);
}
}


when i add :
//if(!CHECKBIT(PINA,6)) num--;
//else if(!CHECKBIT(PINA,7))num++;
i see the two 7segment gives the value of num and the led begins turning on and off(starts the program from the beginning)

foxbrain

 

[alexan_e]

When you want to detect multiple buttons simultaneously you can either use multiple CHECKBIT or use If(!(PINA & 0b0000111)) for example to detect bit 0-1-2 pressed together

Why do you use
//if(!CHECKBIT(PINA,6)) num--;
//else if(!CHECKBIT(PINA,7))num++;

the buttons were in bits 0 and 1, did you change them?

Alex

---------- Post added at 00:55 ---------- Previous post was at 00:49 ----------

Replace your code with the last one in post #10 and also remove PINA=0b11111111;, I forgot it when I copied your code from the first post

 

[foxbrain]

now it's clear thanks....
now the number is that i had chosen is displayed on the two 7segment and what i want from:
//if(!CHECKBIT(PINA,6)) num--;
and
//else if(!CHECKBIT(PINA,7))num++;
is when i press PINA,6 i decrease it (the displayed number) and when i press PINA,7 i increase it...

 

[alexan_e]

I would also add a check so that num stays between 0-99

if(!CHECKBIT(PINA,6)) num--;
else if(!CHECKBIT(PINA,7))num++;

if(Num<0) Num=0;
else if(Num>99) Num=99;

Also in case 1 and 8 you have SETBIT(PORTC,2); , why?

Alex

---------- Post added at 01:07 ---------- Previous post was at 01:01 ----------

I don't see anything else wrong in the code, there is no reason for the code to restart, it can't exit the while loop

 

[foxbrain]

i added :
if(Num<0) Num=0;
else if(Num>99) Num=99;
but i had the same result
quote : Also in case 1 and 8 you have SETBIT(PORTC,2); , why?
just ignore it ...before this code i was testing the display of the seven segment....
foxbrain

 

[alexan_e]

So now you set the number correctly, and you see the displays alternating the number.

if you use the CHECKBIT does it work the same until you press one of the buttons or it doesn't work at all?

Alex

 

[foxbrain]

i put CHECKBIT but no changes happened......
but as i am trying the code it appears everything is good only when i press the button on PINA,1...
and when i increase it it increases from 12 to 18 and then everything turns off....
does that shows us smething??
that the most strange thing is to restart the program.....!!!!!!!!!!!!!

 

[alexan_e]

you wrote PINA,1, but the buttons are in bit 6 and 7 right, did you mean 6 or 7?

when you say it increases from 12 to 18 you mean it jumps from 12 to 18 without seeing 13,14,15,16,17?

did you try with another number 23,34,45 etc?

Alex

 

[foxbrain]

no i'm talking about the beginning when i want to choose which value num will take....when i choose pina,1 (num takes 12)
and everything is going good and i can increase it to 18 (numbers from 12 to 18 appears) and then everything goes off.
but when i choose at the beginning any other value to store in num (any pin of A except pina,1)the 7segment display num and the it starts again (the code led turns on and off)....
foxbrain

 

[alexan_e]

Repost the latest version of your code

Alex

---------- Post added at 01:51 ---------- Previous post was at 01:48 ----------

And I hope the schematic is correct, you have resistors for the display, to the led and to the base of the transistors right?