Help with atmega328p programming

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

Hello, I was solving a task, but I want to know from you if I have solved this task well.

 

Implement this project with two Arduino plates (MCU-A and MCU-B).
This project should be built using atmega328p in the programming language c

 

MCU A

 

To implement the program which simulates the working principle of the traffic light for pedestrian crossing.

 

Use LEDs instead of lights.
To implement the possibility that enables the reduction of the time for execution of the traffic light with minimal changes in the program for 10 times.

This is the specification of the traffic light work:

1. There are two modes of traffic light operation: "day" and "night":

a) The "night" mode is activated by receiving the message through UART where the bit with index 1 and the bit with index 2, have the value 1.

b) The "day" mode is activated by activating the receipt of the message through UART where the bit with index 1 and the bit with index 2, have the value 0.

c) An LED in Pins B7 (PORTB) has been implemented to monitor the correct reception of messages. If a message is received that does not belong to any operating mode ("day" or "night") then set the status LED to the on state (LED connected to pin B7). Otherwise stop the LED in B7.

2. In "day" mode, turn on the red light and turn off the green light for 2 minutes, then turn on the green light and turn off the red light for 25 seconds.

3. In "night" mode the red and green lights are off

 Use Output Compare interrupt for program implementation. Use UART with 38400 baud speed, 8 data bits, even parity, 1 start bit and 1 stop bit. Use of Arduino IDE functions is not permitted. Use pins C2, and B6 as outputs for red and green light. Pins B7 to be used for the LED to signal the receipt of the wrong message.

 

 

#define LED_I 7 //portb if incorrect message from uart
#define LED_K 2 //portc red
#define LED_GJ 6 //port b  green

#define PRESCALER 1
#define F_OSC 5000000
#define REDUKTIMI 10
#define TICKS_PER_INTERVAL (F_OSC/PRESCALER/REDUKTIMI)
#define checkbit 

#define BAUD 38400
#define MYUBRR F_OSC/16/BAUD-1

#define modi_day
#define modi_night

unsigned char rxdata;

unsigned char data;
unsigned char sec=0;

void setupIO(void)
{
DDRB |= (1<<LED_I) | (1<<LED_GJ);
DDRC |=(1<<LED_K);
}

void initiUART(void)
{
UBRR0H = MYUBRR >>8;
UBRR0L = MYUBRR & 0x00FF;
UCSR0B = (1<<RXEN0) | (1<<RXCIE0); //ENABLE RX AND RX INTERRUPT
UCSR0C = (1<<UCSZ01) | (1<<UCSZ00); //CHARACTER SIZE 8 BIT
UCSR0C &= ~(1<<USBS0); //ONE STOP BIT
UCSR0C |=(1<<UPM01); //EVEN PARITY
}

ISR(USART_RX_vect)
{
rxdata = UDR0;
if(checkbit(rxdata,1) & checkbit(rxdata,2))
{
modi_night();
}else if((data & (0<<1)) == 0 & (data & (0<<2)) == 0)){
modi_day();
}else{
PORTB &=~(1<<LED_GJ);
PORTB |=1<<LED_I;
PORTC |=(1<<LED_K);
}
}
void initTimer(void){
TCCR1A |= 1<<COMP1A0; //OUTPUT COMPARE MODE ON
TCCR1B |= 1<<C10; // NO PRESCALER
OCR1A = TICKS_PER_INTERVAL; //WHEN TO INTERRUPT
TIMSK1 = 1<<OCIE1A; //OUTPUT COMPARE INTERRUPT ENABLE FOR CHANNEL A
sei(); // set enable interrupt
}

ISR(TIMER1_COMP_vect)
{
OCR1A +=TICKS_PER_INTERVAL;
sec++;
}

unsigned bool checkbit(unsigned char data, unsigned char index)
{
return (data & (1<<index)) != 0;
}

modi_dita(){
sec=0;
if(sec >=0 & sec <=120){
PORTB &=~(1<<LED_GJ) & ~(1<<LED_I);
PORTC &=~(1<<LED_K);
}else if(sec > 120 & sec <= 145){
PORTB |=(1<<LED_GJ) 
PORTB &=~(1<<LED_I);
PORTC &=~(1<<LED_K);
}else{
sec=0;
}
}

modi_nata(){
PORTB &=~(1<<LED_GJ) & ~(1<<LED_I);
PORTC &=~(1<<LED_K);
}

int main(){
setup();
initTimer();
initUART();
while(1){

}
}

 

Can you tell me if I solved well

 

 

 

M.K

Last Edited: Sun. Jan 24, 2021 - 07:22 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Please see Tip #1 in my signature, below, for how to properly post source code:

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: 2

Metro1 wrote:
Can you tell me if I solved well

 

At a quick glance the code will not compile as there are a number of defects. Atmel Studio7 has a simulator, maybe you might want to try compiling your code and debugging it.

 

I can also note that (data & (0<<1)) will always give a 0 result. Shift 0 as many times as you like - it will always be 0. Where is 'data' defined??

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

I don't know the assessment criteria but I would fail you because of the lack of comments.

#1 Hardware Problem? https://www.avrfreaks.net/forum/...

#2 Hardware Problem? Read AVR042.

#3 All grounds are not created equal

#4 Have you proved your chip is running at xxMHz?

#5 "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."

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

F_OSC is 5MHz? You sure? It's a pretty unusual frequency.

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

Yes, I have given the frequency, because it is not given in the project

M.K

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

Yes, I have given the frequency, because it is not given in the project.

M.K

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

For modin "Nata"
I have defined "rxdata" to receive the message via UART if the bit with index 1 and the bit with index 2 have the value 1.
In the "Day" mode
I have defined "date" to receive the message via UART if the bit with index 1 and the bit with index 2 have the value 0.

M.K

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

Please do NOT post the same questions in multiple forums.

 

One needs to keep the discussion all in one place so that people do not answer the same question in multiple sites, or answer questions that have already been answered in the other thread.

 

JC

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

DocJC wrote:
Please do NOT post the same questions in multiple forums

Especially without paying any attention to any of the advice already received.

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: 1

@Metro1...the code posted in #1 does not compile as it is full of errors. I count at least 7 10 errors which stop it compiling and that is before we look at any errors that stop it working.

 

If you really want help then YOU need to put in some work and make sure people can check your code. It is obvious that you have not tried to compile the code.

 

Also, please sort out the indentation by using the 'Code' button on the forum editor (The "<>" button on the toolbar).

 

I'm sure people will help but ONLY if you put in some effort first.

#1 Hardware Problem? https://www.avrfreaks.net/forum/...

#2 Hardware Problem? Read AVR042.

#3 All grounds are not created equal

#4 Have you proved your chip is running at xxMHz?

#5 "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."

Last Edited: Mon. Jan 25, 2021 - 08:19 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Brian Fairchild wrote:
I count at least 7 10 errors which stop it compiling
I tried too. Things that need fixing:

 

1) It needs system includes of at least:

#include <avr/io.h>
#include <avr/interrupt.h>

2) Because of this:

#define modi_day
#define modi_night

followed by this:

ISR(USART_RX_vect)
{
    rxdata = UDR0;
    if(checkbit(rxdata,1) & checkbit(rxdata,2))
    {
        modi_night();
    }
    else if((data & (0<<1)) == 0 & (data & (0<<2)) == 0)){
        modi_day();
    }

it looks extremely suspicious. As you define those symbols as the null string this effectively becomes:

    if(checkbit(rxdata,1) & checkbit(rxdata,2))
    {
        ();
    }
    else if((data & (0<<1)) == 0 & (data & (0<<2)) == 0)){
        ();

3) You are going to have to pick a language and to stick to it. Now I don't know your native language but when you do:

#define modi_day
#define modi_night

then later:

modi_dita(){
    sec=0;
modi_nata(){
    PORTB &=~(1<<LED_GJ) & ~(1<<LED_I);

I rather suspect that in your native language the words for "day" and "night" are "dita" and "nata" ? You can't use one in one place and the other in another place - it needs to be consistent. A compiler doesn't know that night and nata are really the same thing!

 

I assume that the #define's are just plain wrong and that in the ISR for RX you mean to call modi_nata and modi_dita? If you are going to do that and place the ISR() before the definition of those functions you will need pre-declaration of their interfaces.

 

4) You have ISR(USART_RX_vect) but within initiUART() I don't see the bit that would arm the RXCIE interrupt to happen?

 

5) A bit like those null definitions of modi_day and modi_night you do much the same with:

#define checkbit

and then later attempt to use this with:

    if(checkbit(rxdata,1) & checkbit(rxdata,2))

I see you do have an implementation of checkbit() later:

unsigned bool checkbit(unsigned char data, unsigned char index)
{
    return (data & (1<<index)) != 0;
}

so because it is used before it has been defined it needs to be pre-declared. Replace the #define of checkbit with:

unsigned bool checkbit(unsigned char data, unsigned char index);

The ; rather than "{ code }" on the end says "I'm just telling you about this now but not defining it yet just so you'll know that when you come to a call to checkbit() expect it to have two char parematers and to return an "unsigned bool"

 

6) Only DON'T do that because there's no such thing as an "unsigned bool". It might return an "unsigned char" or an "unsigned int" or simply a "bool" but not "unsigned bool"

 

7) Assuming you had a checkbit() that works your ISR probably starts out OK:

ISR(USART_RX_vect)
{
    rxdata = UDR0;
    if(checkbit(rxdata,1) & checkbit(rxdata,2))

but very shortly afterwards it changes to:

    else if((data & (0<<1)) == 0 & (data & (0<<2)) == 0)){

I think you are going to have to make your mid up (again!) is it called "rxdata" or is it just "data"

 

8) Whatever it is, your way of checking that bit two bits are not set is wrong anyway. When you use:

(0<<2)

in C it does not matter whether it is 2 or 5 or 1 or 3192 the fact is that 0 shifted to the left by any amount is still always 0. So what you are really checking is:

else if((data & 0) == 0 & (data & 0) == 0)){

well again anything that is ANDd with 0 is 0 so this further becomes:

else if(0 == 0 & 0 == 0)){

9) Well 0 is always equal to 0 so in theory both logical clauses here are "true". However you also made the classic mistake of making a compound logical test using "&" when it should have been "&&" in the middle:

else if(0 == 0 && 0 == 0)){

10) This is possibly more of a "style" thing than an actual error but in something like:

void initTimer(void){
    TCCR1A |= 1<<COMP1A0; //OUTPUT COMPARE MODE ON
    TCCR1B |= 1<<C10; // NO PRESCALER

use = not |= unless you know the register has already been set and you simply want to add bits to it.

 

11) Also unless you have the C operator precedence table committed to memory and absolutely know that "<<" binds stringer than "|=" then make use of parentheses:

void initTimer(void){
    TCCR1A = (1 << COMP1A0); //OUTPUT COMPARE MODE ON
    TCCR1B = (1 << C10); // NO PRESCALER

(oh and always put spaces on either side of an operator like "<<", it makes the code easier to read).

 

12) Just sit and contemplate this for a while:

modi_dita(){
    sec=0;
    if(sec >=0 & sec <=120){

So you set "sec" to 0 and then immediately test if it is >= 0 and <=120. Well I can tell you now that after sec=0 both these conditions are met. So there's no question it would execute the first conditional block. Or rather it would have if you hadn't made the & rather than && mistake yet again !

 

14) Now I see elsewhere:

ISR(TIMER1_COMP_vect)
{

    sec++;

So presumably the idea is that you set "sec = 0" then you are expecting something to happen to increment it in the timer interrupt so that by the time you get to the conditionals it might be beyond 0? But as written, it immediately goes into the test - there's no time for it to increment. Even if the interrupt fired and it did increment the foreground code would not "see" this as you made the classic error of only using:

unsigned char sec=0;

when, for a variable that is shared between the main code and an ISR() this MUST be volatile:

volatile unsigned char sec=0;

15) If it really is the case that "sec" can be changed by the interrupt at any moment it is very unwise to write code that does:

    if(sec >=0 & sec <=120){
        PORTB &=~(1<<LED_GJ) & ~(1<<LED_I);
        PORTC &=~(1<<LED_K);
    }
    else if(sec > 120 & sec <= 145){
        PORTB |=(1<<LED_GJ)
        PORTB &=~(1<<LED_I);
        PORTC &=~(1<<LED_K);
    }else{
        sec=0;
    }

You actually re-read the value of "sec" four ties in this but, between one read of it and the next the value may have changed. If you are going to use a volatile variable that is being updated elsewhere then in the code that uses it you should probably read it just once into a "local" variable then do your tests on that - the actual read will then only happen once.

 

15) You have the sei() in the initTimer() but then in main() you do:

int main(){
    setup();
    initTimer();
    initUART();

so interrupts will already be enabled as you go into initUART. Usually when using multiple interrupts you would configure ALL the possible interrupt sources first then switch on the interrupting system so move the sei() out of initTimer() and instead use:

int main(){
    setup();
    initTimer();
    initUART();
    sei();

so interrupts are not enabled until all configuration has been done.

 

============================

 

To be honest this looks like code that has "grown organically". You have written a bit then kept on adding in other bits. That is not the way to write C (or any program code). Instead sit down and things about the whole design at the start. All the functionality it will need and how it will operate together. Some people even draw diagrams at this stage. Only when you have a clear design for the whole thing should you actually start coding - it will be more organised, easier to test, easier to debug as a result.

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

Thank you very much for the explanation, I have translated the task from my language into English, to make it more understandable to you.

I have made some changes if you can look again and tell me.

 

#include <avr/io.h>

#include <avr/interrupt.h>

 

#define LED_I 7 // portb if incorrect message from uart

#define LED_K 2 //portc  red led

#define LED_GJ 6 //port b green led

 

#define PRESCALER 1

#define F_OSC 5000000

#define REDUKTIMI 10

#define TICKS_PER_INTERVAL (F_OSC/PRESCALER/REDUKTIMI)

#define checkbit

 

 

#define BAUD 38400

#define MYUBRR F_OSC/16/BAUD-1

#define mode

#define mode_day 1

#define mode_night 2

#define mode_error 3

 

unsigned char my_rxdata;

 

volatile unsigned char sec=0;

 

void setupIO(void)

{

DDRB |= (1<<LED_I) | (1<<LED_GJ);

DDRC |=(1<<LED_K);

}

 

void modeday(void);

void modenight(void);

void modeerror(void);

 

ISR(TIMER1_COMP_vect)

{

OCR1A +=TICKS_PER_INTERVAL;

sec++;

if(sec>145){

  sec=0;

}

if (mode== mode_day)

{

   void modidita();

}

else if (mode == mode_night)

{

   void modinata();

}

else (mode == mode_error)

{

   void modeerror();

}

 

}

 

void modeday

{

if(sec >=0 && sec sec <= 120)

{

PORTC |= (1<<LED_K); //turned on led red

PORTB &= (~(1<<LED_GJ)); // turned off led green

PORTB &= (~(1<<LED_I)); // turned off

}

if(sec > 120 && sec <= 145)

{

PORTB |= (1<<LED_GJ); // turned on led green

PORTB &= (~(1<<LED_I)); // turned off

PORTC &= (~(1<<LED_K)); // turned off red

}

}

 

void modenight(){

PORTC &= (~(1<<LED_K));// turned off led red

PORTB &= (~(1<<LED_GJ));// turned off green

}

 

void modeerror(){

PORTB |= (1<<LED_I); // turned on

PORTC &= (~(1<<LED_K));// turned off led red

PORTB &= (~(1<<LED_GJ)); //turned off led green

}

 

 

void initiUART(void)

{

UBRR0H = MYUBRR >>8;

UBRR0L = MYUBRR & 0x00FF;

UCSR0B = (1<<RXEN0) | (1<<RXCIE0); //ENABLE RX AND RX INTERRUPT

UCSR0C = (1<<UCSZ01) | (1<<UCSZ00); //CHARACTER SIZE 8 BIT

UCSR0C |= (1<<USBS0); //ONE STOP BIT

UCSR0C |=(1<<UPM01); //EVEN PARITY

}

 

 

 

void initTimer(void){

TCCR1A |= (1 << COMP1A0); //OUTPUT COMPARE MODE ON

TCCR1B |= (1 << C10); // NO PRESCALER

OCR1A = TICKS_PER_INTERVAL; //WHEN TO INTERRUPT

TIMSK1 = (1 << OCIE1A); //OUTPUT COMPARE INTERRUPT ENABLE FOR CHANNEL A

sei(); // set enable interrupt

}

 

 

ISR(USART_RX_vect)

{

rxdata = UDR0;

//"night" mode is activated by receiving the message via UART where the bit with index 1 and the bit with index 2 have the value 1

if(checkbit(my_rxdata,1)==1 && checkbit(my_rxdata,2) == 1)

{

    mode= mode_night;

}

 

//The "day" mode is activated by activating the receipt of the message via UART where the bit with index 1 and the bit with index 2 have the value 0.

else if(checkbit(my_rxdata,1)==0 && checkbit(my_rxdata,2) == 0){

    mode= mode_day;

}

else

{

//If a message is received that does not belong to any operating mode ("day" or "night") then set the status LED to the on state (LED connected to pin B7). Otherwise stop the LED in B7.

   mode = mode_error;

 

}

}

 

int main(void){

  setupIO(); 

  initiUART();

  initTimer();

  sei();

while(1){

}

 

}

M.K

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

Have you actually compiled your code?

#1 Hardware Problem? https://www.avrfreaks.net/forum/...

#2 Hardware Problem? Read AVR042.

#3 All grounds are not created equal

#4 Have you proved your chip is running at xxMHz?

#5 "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."

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

No I can not install atmel studio on laptop

M.K

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

Referring to your other (locked) thread, we need to know what line of code the error corresponds with. Again, we can’t see your computer. My guess is your use of checkbit. All you did was #define checkbit

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

This is error 
exit status 1
expected primary-expression before '==' token

I tried the code in the arduino studio.

M.K

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

What did I suggest in #16?

 

 

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

Metro1 wrote:

No I can not install atmel studio on laptop

So use the Arduino IDE instead. It will still allow you to write plain C code and compile it.

#1 Hardware Problem? https://www.avrfreaks.net/forum/...

#2 Hardware Problem? Read AVR042.

#3 All grounds are not created equal

#4 Have you proved your chip is running at xxMHz?

#5 "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."

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


When I paste #13 into AS7 and attempt to build for 328P I see:

        		../../../junk1/main.c: In function 'TIMER1_COMP_vect':
D:\junk1\main.c(36,1): warning: 'TIMER1_COMP_vect' appears to be a misspelled signal handler, missing __vector prefix [-Wmisspelled-isr]
		 ISR(TIMER1_COMP_vect)
		 ^
D:\junk1\main.c(43,9): error: expected expression before '==' token
		     if (mode== mode_day)
		         ^
D:\junk1\main.c(47,15): error: expected expression before '==' token
		     else if (mode == mode_night)
		               ^
D:\junk1\main.c(51,12): error: expected expression before '==' token
		     else (mode == mode_error)
		            ^
D:\junk1\main.c(52,5): error: expected ';' before '{' token
		     {
		     ^
		../../../junk1/main.c: At top level:
D:\junk1\main.c(59,1): error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
		 {
		 ^
		../../../junk1/main.c: In function 'initiUART':
D:\junk1\main.c(88,33): warning: suggest parentheses around '-' inside '>>' [-Wparentheses]
D:\junk1\main.c(89,33): warning: suggest parentheses around '-' in operand of '&' [-Wparentheses]
		../../../junk1/main.c: In function 'initTimer':
D:\junk1\main.c(99,20): error: 'COMP1A0' undeclared (first use in this function)
		     TCCR1A |= (1 << COMP1A0); //OUTPUT COMPARE MODE ON
		                    ^
D:\junk1\main.c(99,20): info: each undeclared identifier is reported only once for each function it appears in
D:\junk1\main.c(100,20): error: 'C10' undeclared (first use in this function)
		     TCCR1B |= (1 << C10); // NO PRESCALER
		                    ^
D:\junk1\main.c(101,12): warning: large integer implicitly truncated to unsigned type [-Woverflow]
		     OCR1A = TICKS_PER_INTERVAL; //WHEN TO INTERRUPT
		            ^
		../../../junk1/main.c: In function '__vector_18':
D:\junk1\main.c(109,5): error: 'rxdata' undeclared (first use in this function)
		     rxdata = UDR0;
		     ^
D:\junk1\main.c(111,18): warning: left-hand operand of comma expression has no effect [-Wunused-value]
		     if(checkbit(my_rxdata,1)==1 && checkbit(my_rxdata,2) == 1)
		                  ^
D:\junk1\main.c(111,38): warning: left-hand operand of comma expression has no effect [-Wunused-value]
		     if(checkbit(my_rxdata,1)==1 && checkbit(my_rxdata,2) == 1)
		                                      ^
D:\junk1\main.c(113,9): error: expected expression before '=' token
		         mode= mode_night;
		         ^
D:\junk1\main.c(117,23): warning: left-hand operand of comma expression has no effect [-Wunused-value]
		     else if(checkbit(my_rxdata,1)==0 && checkbit(my_rxdata,2) == 0){
		                       ^
D:\junk1\main.c(117,43): warning: left-hand operand of comma expression has no effect [-Wunused-value]
		     else if(checkbit(my_rxdata,1)==0 && checkbit(my_rxdata,2) == 0){
		                                           ^
D:\junk1\main.c(118,9): error: expected expression before '=' token
		         mode= mode_day;
		         ^
D:\junk1\main.c(123,9): error: expected expression before '=' token
		         mode = mode_error;
		         ^
		make: *** [main.o] Error 1

So the error you mention in #17 is just one of many.

 

Once again this all stems from this nonsense:

#define mode

You clearly do not understand what "#define" is used for. You do NOT use it to declare/define variables (or functions) !! #define is part of the pre-processor. At the end of the say it's really just a "string redefinition system". What the line is really saying is "each time you encounter "mode" in the following replace it". Now it is valid to do things like:

#define mode moda

that would say "each time you see "mode" change it to "moda"". That might be a valid kind of thing to do. But when you use just "#define something" and have nothing else following it you are saying "each time you see something just remove it". So in later code when you write:

    if (mode== mode_day)
...
    else if (mode == mode_night)

the preprocessor will change this so what the C compiler actually sees is:

    if (== mode_day)
...
    else if ( == mode_night)

That then causes the compiler to say:

D:\junk1\main.c(85,9): error: expected expression before '==' token
		     if (mode== mode_day)
		         ^

You may think you can see "mode" there but that is not what the C compiler gets to see after this has been through preprocessing. In fact if you use -save-temps you can get the compiler to leave behind a copy of the file after preprocessing. Here's just a taste of what the C compiler sees:

# 36 "../../../junk1/main.c" 3
void 
# 36 "../../../junk1/main.c"
TIMER1_COMP_vect 
# 36 "../../../junk1/main.c" 3
(void) __attribute__ ((signal,used, externally_visible)) ; void 
# 36 "../../../junk1/main.c"
TIMER1_COMP_vect 
# 36 "../../../junk1/main.c" 3
(void)

# 37 "../../../junk1/main.c"
{
    
# 38 "../../../junk1/main.c" 3
   (*(volatile uint16_t *)(0x88)) 
# 38 "../../../junk1/main.c"
         +=(5000000/1/10);
    sec++;
    if(sec>145){
        sec=0;
    }
    if (== 1)
    {
        void modidita();
    }
    else if ( == 2)
    {
        void modinata();
    }
    else ( == 3)
    {
        void modeerror();
    }

}

void modeday
{
    if(sec >=0 && sec sec <= 120)
    {
        
# 62 "../../../junk1/main.c" 3
       (*(volatile uint8_t *)((0x08) + 0x20)) 
# 62 "../../../junk1/main.c"
             |= (1<<2);
        
# 63 "../../../junk1/main.c" 3
       (*(volatile uint8_t *)((0x05) + 0x20)) 
# 63 "../../../junk1/main.c"
             &= (~(1<<6));

Of course the fault is not just the #define mode thing. There are others like:

void modeday
{
    if(sec >=0 && sec sec <= 120)

That is not how a function in C starts. I assume you meant:

void modeday(void)
{

A function is:

return_type function_name (parameters) {

parameters can be empty (that is "()") or just "void" or a list of types and names to be passed into the function.

 

Oh and there was this:

    if(sec >=0 && sec sec <= 120)

How many "sec" do you really want in there? Was this supposed to be something more like:

    if((sec >=0) && (sec <= 120))

If you put parentheses around each separate test I think it's clearer and it also removes the extra "sec" that wasn't supposed to be there.

 

Other errors like:

D:\junk1\main.c(99,20): error: 'COMP1A0' undeclared (first use in this function)
		     TCCR1A |= (1 << COMP1A0); //OUTPUT COMPARE MODE ON
		                    ^

just look plain lazy. If you are using AS7 then when you write something like "TCCR1A" in your code put the cursor on it and use:

 

 

when you do that you will get to:

 

 

You can see from this that the valid bit names in this register are called:

WGM10
WGM11
COM1B0
COM1B1
COM1A0
COM1A1

so your:

 

 

was probably supposed to read:

 

 

Note that when I changed this to symbols that are defined the "red squiggles" under the two things that were wrong disappear.

 

In fact those red squiggles should help to highlight other problems. If I hover the mouse over one I see for example:

 

 

That's even telling you what's wrong - there is nothing called "rxdata" in this program. I suspect it might have been this you meant to use:

 

 

The overall impression of all this is that it is sloppy and clumsy. These are all simple errors that seem to stem from hot having thought about the design of this code before you wrote it. 

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

You didn't pat attention to the instructions on how to post source code, then?

 

It's been said twice - see post #2 and post #11

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

Brian Fairchild wrote:
Have you actually compiled your code?

Metro1 wrote:
No  I can not install atmel studio on laptop

Then how on earth do expect to complete a programming exercise?

 

 

I can not install atmel studio on laptop

Why not?

 

Do you mean you've tried, but failed? Or that you don't have permission to do it?

 

Surely, if you've been set this homework, you must previously have installed a suitable compiler?

 

EDIT

 

Is there anywhere that has an online compiler - similar to mbed?

 

How about: https://create.arduino.cc/projecthub/Arduino_Genuino/getting-started-with-arduino-web-editor-on-various-platforms-4b3e4a

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...
Last Edited: Tue. Jan 26, 2021 - 11:47 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1


awneil wrote:
Is there anywhere that has an online compiler - similar to mbed?
I think most people would probably follow Curtvm's lead and use Godbolt. I just pasted the #13 code into Godbolt and set it to avr-gcc 5.4.0 (the same one as AS7) with a single compiler option of -mmcu=atmega328p and just like AS7 it said...

 

 

which is pretty much identical to what I got in #20 from AS7.

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

Well, first of all, the code's formatting is a complete mess here.
  There are two processes happening in this code:
 
 1) The mode is updated by a byte received on the USART.  Valid update values are
    0x00 and 0x03.  This is assuming that that "index one" equals bit 0 and that "index two"
    equals bit 1.  The bits are ordered 7 to 0, so this is an ambiguity in the code specification.
    Since there only two valid values, you don't need to test whether a specific bit is set or
    cleared: just test for either of the two valid byte values.
 
 2) At each second, the traffic lights are updated according to the last value received from the USART.
    And, a counter of the seconds is incremented and tested against its limits {120 and 145}. If the
    second count is at one of these limits, the traffic light's state is changed. At the higher limit,
    the second count is cleared to zero.   This should be documented in the application's header comment
    block that comes before any code.  With a comment block, a user can know what the application does
    without actually having to study the code in detail.
    
    
  Right away, the code won't work because 16-bit timer1 is (attempting to be) loaded with
  a value greater than 64K.  Here that is FOSC/1/10, or 500000.  I suggest using the 1024
  timer1 preset to get a timer interval of 1/ (5000000/1024), or about 200 microseconds
  per timer tick.  Set timer1's compare value to @5000 to get an interrupt interval of one
  second.  Then, in the timer IRQ, check if the mode has changed (from a new byte received
  from the USART) and update the traffic light LEDs according to the day/night/error IF and
  only IF the mode has changed from the last second.

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

Simonetta wrote:
This is assuming that that "index one" equals bit 0

why not assume that it's bit 1 ?

 

But, yes - it's ambiguous.

 

Presumably a convention has been established in the class - but there's no way for us to know.

 

 

Simonetta wrote:
    Since there only two valid values, you don't need to test whether a specific bit is set or
    cleared: just test for either of the two valid byte values.

No, that's not true - the spec also states that an invalid byte value must be detected & indicated.

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...