Simple clock settings menu not working as expected

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

Hi, I intend to do a menu to set the time and date on a clock (only for clock in this test code below) and the only ideas I had were using functions if/else or switch/case. After reading a lot about both I think switch/case cannot work as I want so I try only if/else, my code below, I will post all code if needed. I want to select with SET_MAIN what I want to adjust and then adjust the value with SET_UP and SET_DOWN. When I run the code I can switch the menu items when push SET_MAIN but nothing happens if I push UP or DOWN, hours or minutes in the clock doesn't change. As I read, the if function should work inside another if but in my case it doesn't work, so either it cannot work like this or I made a mistake somewhere. Please somebody help me to understand why is it not working.

while (1)
	{
		if (!(SET_PIN & (1<<SET_MAIN)))
		{		
			s++;
			if (s > 2)
			{	s = 0;
				TEXTGOTO(4,2);
				WRITESTRING("           ");
			}			
			TEXTGOTO(2,2);
			itoa(s%10,temp,10);
			WRITESTRING(temp);
			_delay_ms(1000);
			
			if (s == 1)
			{
				TEXTGOTO(4,2);
				WRITESTRING("SET HOURS  ");
				
				if(!(SET_PIN & (1<<SET_UP)))
					{
					hour++;
					_delay_ms(250);
					if(hour > 23)
					hour = 0;
					}
				if(!(SET_PIN & (1<<SET_DOWN)))
					{
					hour--;
					_delay_ms(250);
					if(hour < 0)
					hour = 23;
					}
		
			}
			if (s == 2)
			{	
				TEXTGOTO(4,2);
				WRITESTRING("SET MINUTES");
				
					if(!(SET_PIN & (1<<SET_UP)))
					{
					minute++;
					_delay_ms(250);
					if(minute > 59)
					minute = 0;
					}
					if(!(SET_PIN & (1<<SET_DOWN)))
					{
					minute--;
					_delay_ms(250);
					if(minute < 0)
					minute = 59;
					}
			}
		}			   
	}

 

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

A few things...

 

Buttons bounce.  I guess your long delay after acting on an UP/DOWN might take care of it.

 

I don't see where you re-display.  How are you deciding "not working"?

 

Put a little wrapper around the posted code, and run it in the simulator.

 

Ideally, show a complete test program.  In this case, what type are the variables "minute" and "hour"?

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

This is entire test code. I use atmega32 and graphic LCD display, the clock is on the display, also added a counter for the menu items and names of the menu items are displayed and can change them. If I go the first menu item "1 SET HOURS" and push SET_UP or SET_DOWN the value of the hours are not changing. Thank you for reply.

#define  F_CPU 4000000UL
#include "RA8835_Init.h"
#include <avr/io.h>
#include <util/delay.h>
#include <avr/pgmspace.h>
#include <avr/interrupt.h>
#include <math.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include <avr/wdt.h>
#include <string.h>

#define SET_PORT	PORTA
#define SET_DDR		DDRA
#define SET_PIN		PINA

#define SET_DOWN	PINA2
#define SET_UP		PINA1
#define SET_MAIN	PINA0

unsigned char hour = 0;
unsigned char minute = 0;
unsigned char second = 0;

void Update_Time(void);
ISR(TIMER1_COMPA_vect);

int main(void)
{
	LCD_INIT();
	CLEARTEXT();
	CLEARGRAPHIC();
	SET_PORT = (1<<SET_MAIN | 1<<SET_DOWN | 1<<SET_UP);

	TEXTGOTO(2,2);
	WRITESTRING("0");
	TEXTGOTO(2,4);
	WRITESTRING("00:00.00");

	TCCR1B = (1<<CS12|1<<WGM12);
	OCR1A = 15625-1;
	TIMSK = 1<<OCIE1A;
	sei();

	int s = 0;
	char *temp = 0;

while (1)
	{
		if (!(SET_PIN & (1<<SET_MAIN)))
		{
			s++;
			if (s > 2)
			{	s = 0;
				TEXTGOTO(4,2);
				WRITESTRING("           ");
			}
			TEXTGOTO(2,2);
			itoa(s%10,temp,10);
			WRITESTRING(temp);
			_delay_ms(1000);

			if (s == 1)
			{
				TEXTGOTO(4,2);
				WRITESTRING("SET HOURS  ");

				if(!(SET_PIN & (1<<SET_UP)))
					{
					hour++;
					_delay_ms(250);
					if(hour > 23)
					hour = 0;
					}
				if(!(SET_PIN & (1<<SET_DOWN)))
					{
					hour--;
					_delay_ms(250);
					if(hour < 0)
					hour = 23;
					}

			}
			if (s == 2)
			{
				TEXTGOTO(4,2);
				WRITESTRING("SET MINUTES");

					if(!(SET_PIN & (1<<SET_UP)))
					{
					minute++;
					_delay_ms(250);
					if(minute > 59)
					minute = 0;
					}
					if(!(SET_PIN & (1<<SET_DOWN)))
					{
					minute--;
					_delay_ms(250);
					if(minute < 0)
					minute = 59;
					}
			}
		}
	}
}

void Update_Time()
{
	char *temp = 0;

	TEXTGOTO(2,4);

	itoa(hour/10,temp,10);
	WRITESTRING(temp);
	itoa(hour%10,temp,10);
	WRITESTRING(temp);
	WRITESTRING(":");

	itoa(minute/10,temp,10);
	WRITESTRING(temp);
	itoa(minute%10,temp,10);
	WRITESTRING(temp);
	WRITESTRING(".");

	itoa(second/10,temp,10);
	WRITESTRING(temp);
	itoa(second%10,temp,10);
	WRITESTRING(temp);
}

ISR(TIMER1_COMPA_vect)
{
	second++;

	if(second > 59){
		second = 0;
		minute++;}
	if(minute > 59){
		minute = 0;
		hour++;}
	if(hour > 23){
		hour = 0;}

	Update_Time();
}

 

Last Edited: Tue. Sep 19, 2017 - 01:55 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:
Put a little wrapper around the posted code, and run it in the simulator.

Or step through it in the debugger (which will probably slow it down enough for the switch bounce not to matter)

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

I will try debugger, I do not have a simulator. Using AVR Studio 5, I do not know if this has simulator option.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
	char *temp = 0;
...
	itoa(hour/10,temp,10);

Really BAD idea!

Stefan Ernst

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

VIO47 wrote:
push SET_UP or SET_DOWN the value of the hours are not changing.
theusch wrote:
I don't see where you re-display. How are you deciding "not working"?
theusch wrote:
In this case, what type are the variables "minute" and "hour"?
VIO47 wrote:
unsigned char hour = 0;
VIO47 wrote:
if(hour < 0)

How do you expect an unsigned variable to be less than zero?

 

Shared variables with an ISR need to be volatile.

http://www.avrfreaks.net/forum/t...

 

VIO47 wrote:
Using AVR Studio 5, I do not know if this has simulator option.

1)  Studio5 is very buggy.  Short answer:  Don't use it.

2)  Yes, all versions of AVRStudio and AtmelStudio have a simulator.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Sternst, Can you please let me know why, it only works like this, maybe because of WRITESTRING function which was not written by me, I am not experienced in this area. 

 

Teusch, yes, this is right, missed it, thank you.

Last Edited: Tue. Sep 19, 2017 - 02:23 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It's your code - so you need to be the main actor in debugging it!

 

Have you run it in the debugger yet? 

 

The whole point of stepping in the debugger is the you will see what is happening - and, thus, answer the question!

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

VIO47 wrote:

ISR(TIMER1_COMPA_vect);

Funny how the world revolves. Just the other day there was a thread about this very thing - no you don't give declarations for ISR() because ISR() is not a function it is a macro. If you had actually typed:

void ISR(TIMER1_COMPA_vect);

you would have found out about this :-)

 

Anyway the reason you declare functions is so that the interface to call is known to the compiler before the implementation is encountered - but nothing (in your control) calls ISR()s anyway.

Last Edited: Tue. Sep 19, 2017 - 02:48 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you all for replies, I am just doing this as a hobby and looking to learn something else. I am not able to write a code head-end, many times looking in examples from others or pieces of code.

The debugger didn't show me something wrong, at least as I can use it. Maybe somebody can tell me if this is a good idea to follow or should I drop it. If it's good I will try to find out what is wrong there. I made all corrections you advised me, anyway still not working.

What else I should use instead of AVR Studio 5? I got it with the STK500, should I look for a newer version or another software?

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

VIO47 wrote:
The debugger didn't show me something wrong

So, as you stepped through the code it did exactly as you expected - including updating the display ... ?

 

 should I look for a newer version 

The latest is Atmel Studio 7.

 

The are loads of tutorials - including YouTube videos

 

http://www.microchip.com/development-tools/atmel-studio-7

 

http://www.atmel.com/microsite/atmel-studio/

 

http://www.atmel.com/tools/atmelstudio.aspx

 

 

 

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

VIO47 wrote:
The debugger didn't show me something wrong, at least as I can use it.

So, which of the recommended changes did you make?  Shared variables are volatile, and of a suitable type for the comparisons?

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

This is the code with all changes.

I cannot change char *temp because WRITESTRING will no more work. Changed shared variables to volatile but regarding suitable type for comparison, don't know what is wrong, maybe have to learn more.

Also installed Atmel Studio 7 and already using it.

 

About updating the display, I am not sure I understand exactly what do you mean, if variable hour increase then I should see this already on display, at least is what I am thinking.

#define  F_CPU 4000000UL
#include "RA8835_Init.h"
#include <avr/io.h>
#include <util/delay.h>
#include <avr/pgmspace.h>
#include <avr/interrupt.h>
#include <math.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include <avr/wdt.h>
#include <string.h>

#define SET_PORT	PORTA
#define SET_DDR		DDRA
#define SET_PIN		PINA

#define SET_DOWN	PINA2
#define SET_UP		PINA1
#define SET_MAIN	PINA0

volatile int hour = 0;
volatile int minute = 0;
volatile int second = 0;

void Update_Time(void);


int main(void)
{
	LCD_INIT();
	CLEARTEXT();
	CLEARGRAPHIC();
	
	TEXTGOTO(2,2);
	WRITESTRING("0");
	TEXTGOTO(2,4);
	WRITESTRING("00:00.00");
	
	SET_PORT = (1<<SET_MAIN | 1<<SET_DOWN | 1<<SET_UP);
	
	TCCR1B = (1<<CS12|1<<WGM12);
	OCR1A = 15625-1;
	TIMSK = 1<<OCIE1A;
	sei();
	
	int s = 0;
	char *temp = 0;
	
	while (1)
	{
		if (!(SET_PIN & (1<<SET_MAIN)))
		{
			s++;
			if (s > 2)
			{	s = 0;
				TEXTGOTO(4,2);
				WRITESTRING("           ");
			}
			TEXTGOTO(2,2);
			itoa(s%10,temp,10);
			WRITESTRING(temp);
			_delay_ms(1000);
			
			if (s == 1)
			{
				TEXTGOTO(4,2);
				WRITESTRING("SET HOURS  ");
				
				if(!(SET_PIN & (1<<SET_UP)))
				{
					hour++;
					_delay_ms(250);
					if(hour > 23)
					hour = 0;
				}
				if(!(SET_PIN & (1<<SET_DOWN)))
				{
					hour--;
					_delay_ms(250);
					if(hour < 0)
					hour = 23;
				}
				
			}
			if (s == 2)
			{
				TEXTGOTO(4,2);
				WRITESTRING("SET MINUTES");
				
				if(!(SET_PIN & (1<<SET_UP)))
				{
					minute++;
					_delay_ms(250);
					if(minute > 59)
					minute = 0;
				}
				if(!(SET_PIN & (1<<SET_DOWN)))
				{
					minute--;
					_delay_ms(250);
					if(minute < 0)
					minute = 59;
				}
			}
		}
	}
}


void Update_Time()
{
	char *temp = 0;
	
	TEXTGOTO(2,4);
	
	itoa(hour/10,temp,10);
	WRITESTRING(temp);
	itoa(hour%10,temp,10);
	WRITESTRING(temp);
	WRITESTRING(":");
	
	itoa(minute/10,temp,10);
	WRITESTRING(temp);
	itoa(minute%10,temp,10);
	WRITESTRING(temp);
	WRITESTRING(".");
	
	itoa(second/10,temp,10);
	WRITESTRING(temp);
	itoa(second%10,temp,10);
	WRITESTRING(temp);
}

ISR(TIMER1_COMPA_vect)
{
	second++;
	
	if(second > 59)
	{
		second = 0;
		minute++;
	}
	if(minute > 59)
	{
		minute = 0;
		hour++;
	}
	if(hour > 23)
	{
		hour = 0;
	}
	
	Update_Time();
}

 

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

VIO47 wrote:
About updating the display, I am not sure I understand exactly what do you mean

Go on - you expect the display to change (ie, "update") when you use the buttons - don't you?!

 

if variable hour increase then I should see this already on display

Exactly - and do you?

 

 

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

Don't update_time() from the ISR. Just set a flag then service it in main()

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

Thank you, I do not know how to do this, already reading about it.

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

I cannot change char *temp because WRITESTRING will no more work

While WRITESTRING does in fact expect a pointer to char, that doesn't mean that all you need to do is declare temp to be a pointer to char.

 

Since this is just a hobby, I guess you haven't had any formal (computer) language training, and specifically no C training, and that you haven't studied it enough on your own to know what you need to know.

 

If you plan on pursuing this hobby, you would do well to remedy that.  Get a copy of K&R's "The C Programming Language (Second Edition)".  There is also this excellent free online resource:

http://publications.gbdirect.co.uk/c_book/

 

The long and the short of it is that in C, strings are just arrays of char.  To declare an array temp of size 32 bytes, you:

char temp[32];

This sets aside enough memory for 32 char in a row, and calls it temp.  The name of the array also serves as a pointer to the array (actually, to the first element of it), so you can pass it to functions which expect a pointer to char.

 

What you have done is declare a pointer to char without allocating any memory for an array of char.  What's more, you initialised it to 0, which means it will point to the beginning of the SRAM memory map.  On the AVR, that actually is where the cpu keeps all of its general purpose registers.  As soon as you dereference temp (which WRITESTRING undoubtedly does), you will drive the AVR off of a cliff.

 

Make the change I suggest above, see what happens.

 

Others have made several other suggestions.  Heed them.

 

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

Thank you joeymorin for explanation and advises, code modified and works. I have a C book, something I could find in my local language (I think I can understand better since I am not english native) but will try to find also the one you recommended. 

 

I did set a flag in ISR and moved Update_Time() in main, this is the only way I could do it  to have the clock still working, but still cannot set the time.

#define  F_CPU 4000000UL
#include "RA8835_Init.h"
#include <avr/io.h>
#include <util/delay.h>
#include <avr/pgmspace.h>
#include <avr/interrupt.h>
#include <math.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include <avr/wdt.h>
#include <string.h>

#define SET_PORT	PORTA
#define SET_DDR		DDRA
#define SET_PIN		PINA

#define SET_DOWN	PINA2
#define SET_UP		PINA1
#define SET_MAIN	PINA0

volatile int hour = 0;
volatile int minute = 0;
volatile int second = 0;
volatile int flag = 0;
char temp[32];
int s = 0;

void Update_Time(void);


int main(void)
{
	LCD_INIT();
	CLEARTEXT();
	CLEARGRAPHIC();
	
	TEXTGOTO(2,2);
	WRITESTRING("0");
	TEXTGOTO(2,4);
	WRITESTRING("00:00.00");
	
	SET_PORT = (1<<SET_MAIN | 1<<SET_DOWN | 1<<SET_UP);
	
	TCCR1B = (1<<CS12|1<<WGM12);
	OCR1A = 15625-1;
	TIMSK = 1<<OCIE1A;
	sei();
		
	
	while (1)
	{
		if (!(SET_PIN & (1<<SET_MAIN)))
		{
			s++;
			if (s > 2)
			{	s = 0;
				TEXTGOTO(4,2);
				WRITESTRING("           ");
			}
			TEXTGOTO(2,2);
			itoa(s%10,temp,10);
			WRITESTRING(temp);
			_delay_ms(1000);
			
			if (s == 1)
			{
				TEXTGOTO(4,2);
				WRITESTRING("SET HOURS  ");
				
				if(!(SET_PIN & (1<<SET_UP)))
				{
					hour++;
					_delay_ms(200);
					if(hour > 23)
					hour = 0;
				}
				if(!(SET_PIN & (1<<SET_DOWN)))
				{
					hour--;
					_delay_ms(200);
					if(hour < 0)
					hour = 23;
				}

			}
			if (s == 2)
			{
				TEXTGOTO(4,2);
				WRITESTRING("SET MINUTES");
				
				if(!(SET_PIN & (1<<SET_UP)))
				{
					minute++;
					_delay_ms(200);
					if(minute > 59)
					minute = 0;
				}
				if(!(SET_PIN & (1<<SET_DOWN)))
				{
					minute--;
					_delay_ms(200);
					if(minute < 0)
					minute = 59;
				}
			}
		}
		
		if (flag)
		{
			flag = 0;
			Update_Time();
		}
	}
	
	
	
}


void Update_Time()
{
	TEXTGOTO(2,4);
	
	itoa(hour/10,temp,10);
	WRITESTRING(temp);
	itoa(hour%10,temp,10);
	WRITESTRING(temp);
	WRITESTRING(":");
	
	itoa(minute/10,temp,10);
	WRITESTRING(temp);
	itoa(minute%10,temp,10);
	WRITESTRING(temp);
	WRITESTRING(".");
	
	itoa(second/10,temp,10);
	WRITESTRING(temp);
	itoa(second%10,temp,10);
	WRITESTRING(temp);
}

ISR(TIMER1_COMPA_vect)
{
	second++;
	
	if(second > 59)
	{
		second = 0;
		minute++;
	}
	if(minute > 59)
	{
		minute = 0;
		hour++;
	}
	if(hour > 23)
	{
		hour = 0;
	}
		
	flag = 1;
}

 

It is something I do not understand, if I change the time setting part of the code to this one it works, of course not as I want to, it can only increase hour or minute and will have to use too many pins of uC if I want to set also the date. What is the difference?

while (1)
	{
		if(!(SET_PIN & (1<<SET_UP)))
		{
			hour++;
			if(hour > 23)
			hour = 0;
		}
		if(!(SET_PIN & (1<<SET_DOWN)))
		{
			minute++;
			if(minute > 59)
			minute = 0;
		}
		_delay_ms(1000);
		
		if (flag)
		{
			flag = 0;
			Update_Time();
		}
	}

 

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

As is, your code to set the hours and minutes depends upon the main button being pressed.  However, there is a loop counter 's' which changes between each mode (hours, minutes, none) which also depends on the main button.  You need to disassociate these two functions.

 

I think you need to take a step back and stop writing code.  Try to describe the clock in your native language.  Do you know what a flow chart is?  Do you know what a state machine is?  If not, it would be a good time to look into it.

 

Think about how a simple, cheap digitial wristwatch works.  Many have only two buttons.  How do you set the time on one of those?  A good starting place would be the user's manual for such a wristwatch.  It will provide a procedure for setting the time and date.  Now, describe that procedure in more detail for yourself on a piece of paper.  These are the steps you should take before you ever start typing code into an editor.

 

 

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

Hi, thank you. Sorry for all my stupid questions, maybe these are so simple things for you. Of course I know what is a flowchart, I have imagined all settings process only, didn't put it on paper, will try this too. I have read something about state machine too and this was my attempt to do one, obviously it was a bad idea. Maybe I am trying to do things too complicated. 

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

VIO47 wrote:
I have imagined all settings process only, didn't put it on paper

Never do that!

 

How can you check that your code actually matches the design?!

 

Of course, it doesn't have to be actual paper.