Problem with "if" and "while" cycles

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

Hey, I just lack this part to finish my alarm clock project. So I can not turn off the alarm. The whole program works fine, it goes into the "alarm" function and when I press the button PB3 it turns off the alarm but the program does not return to the normal clock. stands at the time of the alarm. Why is that happening?

 

void alarm()
{
	if (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10 && (PINB & (1<<PB3)))
	{
		//sensor();
		
		PORTD |= (1<<5);
		_delay_ms(200);
		//PORTD &= ~(1<<5);
		//_delay_ms(900);
	}
	else if (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10 && (!(PINB & (1<<PB3))))
	{
		do
		{
			PORTD &= ~(1<<5);
		}
		while (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10);
	}
 	else PORTD &= ~(1<<5);
}

 

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

Since you have not shown the context where and when that function is called, what follows are based on my assumptions - which might be wrong!  This is why we often ask for minimal but complete code  (or a good and precise  description of the calling context).

 

I'll have a stab at it anyway...

 

Here are some general tips to make your code more readable, also for when you are reading and thinking about it yourself.

 

1. Remove commented out code. It's only function is to confuse the reader (both you and us). Now we have

void alarm()
{
	if (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10 && (PINB & (1<<PB3)))
	{
		PORTD |= (1<<5);
		_delay_ms(200);
	}
	else if (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10 && (!(PINB & (1<<PB3))))
	{
		do
		{
			PORTD &= ~(1<<5);
		}
		while (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10);
	}
 	else PORTD &= ~(1<<5);
}

2. Try to move repeated code out into a function. With a good function name this will make the code much more clear. In this case the test of if the time is the set alarm time is repeated (three times!). Let's break it out into a nicely named function:

 

bool isAlarmTime(int Tempo[], int minutos, int horas)
{
   return Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10;
}

void alarm()
{
    if (isAlarmTime(Tempo, minutos, horas) && (PINB & (1<<PB3)))
    {
        PORTD |= (1<<5);
        _delay_ms(200);
    }
    else if ( isAlarmTime(Tempo, minutos, horas) && (!(PINB & (1<<PB3))))
    {
        do
        {
            PORTD &= ~(1<<5);
        }
        while (isAlarmTime(Tempo, minutos, horas);
    }
    else PORTD &= ~(1<<5);
}

Now it stands out much more clearly (since it is much less "camouflaged" by too detailed surrounding code) why your alarm gets stuck: If you "get into" the do-while loop you won't get out of it until the time is no longer equal to the set alarm time.

 

Let's think on a bit more abstract level for a little while: What is to happen when time hits the set alarm time? I assume you want the alarm to sound

- As long as current time is the set alarm time,

AND

- As long as the shut-up button is not pressed

 

This can be coded simply as (sketchy, not tested!):

 

void alarm() {
    while (isAlarmTime(Tempo, minutos, horas) && (!(PINB & (1<<PB3))) {
        // Sound the alarm
    }

    // Turn off the alarm
}

No need to complicate things more than necessary!

 

- If it is not time to sound the alarm you will not get into that while-loop, and thus will not sound the alarm

- If it is time to sound the alarm, and the shut-up button is not pressed you will get into the loop and the alarm will sound

- It will continue to be caught in that loop until either the time is no longer the set-time for the alarm or the shut-up button is pressed.

 

I.e. the alarm will sound for a maximum of one minute.

 

If your requirements on how long the alarm should sound is different from that then tell us. We can not suggest code for something lacking a firm specification.

 

By the way, since we've just learned to break code out into functions and give functions descriptive names I would even go as far as this:

void alarm() {
    while (isAlarmTime(Tempo, minutos, horas) && (!shutUpButtonIsPressed())) {
        // Sound the alarm
        _delay_ms(200);
        // Turn off the alarm
        _delay_ms(200);
    }
}

which makes the alarm algorithm very clear IMO. (I'll leave it to you to design the shutUpButtonIsPressed() function. ;-)

 

Finally: You have not shown the definition of the variables horas, minutos and Tempo. I have assumed their types are int (and array of int). If they aren't int's you need to adjust to your types.

"He used to carry his guitar in a gunny sack, or sit beneath the tree by the railroad track. Oh the engineers would see him sitting in the shade, Strumming with the rhythm that the drivers made. People passing by, they would stop and say, "Oh, my, what that little country boy could play!" [Chuck Berry]

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

Last Edited: Sun. Nov 5, 2017 - 09:12 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

thanks for the help. I will test your solution in a while. In the meantime I leave here my entire code.

 

#include <avr/io.h>
#include <util/twi.h>
#include "ds1307.h"
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <avr/interrupt.h>

#define F_CPU 1000000UL									//Freq -> 1 MHz -> CLKDIV 8 ativado
#include <util/delay.h>									//Biblioteca delay.h
#include "7segment.h"									//Adiciona a biblioteca do Display de 7 Segmentos
#include "debounce.h"									//Adiciona a biblioteca do Debouncing dos botões
 
static unsigned char horas, minutos;					
#define pwm(val) 255*val/100
volatile unsigned char cnt_ovf, flag_off = 0;
volatile char Tempo[11];
volatile uint8_t buttons_down;
int16_t DISTANCIA;
unsigned int pulse = 0;
unsigned int i = 0;

/**********************************************************
*						Testes
**********************************************************/

/**********************************************************
*                   Funções Usadas 
**********************************************************/

void inic();
void digits_clock();
void digits_alarm();
void alarm();
void set_alarm();
void trigger();
void sensor();

int main(void)
{
	inic();												//Inicializações
// 	DS1307_Lock(true);									//Desbloqueia RTC
// 	Modo24h();											//Modo 24h ativo
// 	Time_Set(22,51,1);									//Acertar relógio -> este comando só é incluído na primeira vez
	
	while (1) 
    {	
		Get_Time();											//Obter tempo atual
		if( !(PINB & (1<<PB0)) )
		{	
			set_alarm();									//Mostra Alarme
		}
		else 
		{
			digits_clock();									//Mostra relógio
			alarm();
		}
	}
}

/******************************************************************
*						 Inicializações
*******************************************************************/

void inic()
{	
	/* PORTS */
	DDRA = 0xFF;															//Todos os bits como saídas -> Ordem dos Segmentos: a - f - b - e - d - dot - c - g
	DDRB = (0<<0) | (0<<1) | (0<<2) | (0<<3) | (1<<4);						//PIN's dos botões como entrada 
	DDRC = (1<<4) | (1<<5) | (1<<6) | (1<<7);								//PC5 -> Dígito 4 ; PC4 -> Dígito 1 ; PC6 -> Dígito 2 ; PC7 -> Dígito 3
	DDRD = (0<<0) | (1<<1) | (1<<2) | (1<<3) | (1<<4) | (1<<5);				//PIN do besouro como saída assim como os PINs dos motores
	
	PORTB = (1<<0) | (1<<1) | (1<<2) | (1<<3);
	
	/* SENSOR */	
	PCICR |= (1<<PCIE3);
	PCMSK3 |= (1<<PCINT24);
			
	/* TWI */
	TWSR &= ~((1<<TWPS1) | (1<<TWPS0));							//Prescaler -> 1
	TWBR = 2;													//TWI_Freq = 100 kHz -> TWBR = ((F_CPU / TWI_FREQ) - 16) / 2
	TWCR |= (1<<TWEN);											//Enable da comunicação
	
	/*	TIMER 0	*/
	TCCR0B |= (1<<CS02); 
	TIMSK0 |= 1<<TOIE0;
	
	sei();
}

/********************************************************************
*						Debounce
********************************************************************/

ISR (TIMER0_OVF_vect)
{
	debounce();
}

// Return non-zero if a button matching mask is pressed.
uint8_t button_down(uint8_t button_mask)
{
	// ATOMIC_BLOCK is needed if debounce() is called from within an ISR
	ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
	{
		// And with debounced state for a one if they match
		button_mask &= buttons_down;
		// Clear if there was a match
		buttons_down ^= button_mask;
	}
	// Return non-zero if there was a match
	return button_mask;
}

/********************************************************************
*							Sensor
********************************************************************/

// o botão encontra-se a 4,3 cm do sensor

ISR (PCINT3_vect)
{
	if (i == 0)
	{
		TCCR1B |= (1<<CS10);
		i = 1;
	}
	else
	{
		TCCR1B = 0;
		pulse = TCNT1;
		TCNT1 = 0;
		i = 0;
	}
}

void trigger()
{
	PORTB |= (1<<4);							// É enviado uma onda de ultra-som(pulso)
	_delay_us(15);								// Tempo mais que suficiente para enviar um pulso completo
	PORTB &= ~(1<<4);							// Fim do pulso
	_delay_ms(30);								// Tempo para receber onda Echo completa
	
	DISTANCIA = pulse/58;
}

void sensor()
{
	trigger();
	if (DISTANCIA < 5)
	{
		PORTD |= (1<<2) | (1<<4);
	}
	else if (DISTANCIA >= 5)
	{
		PORTD &= ~((1<<2) | (1<<4));
	}
}

/********************************************************************
*                    4 x Display de 7 Segmentos
********************************************************************/

void digits_clock()
{	
	PORTC = 0b00010000;									//Liga o dígito das unidades 
	display_num(Tempo[2]);								//Mostra o valor que estiver na variável dig4
	_delay_ms(5);
	
	PORTC = 0b10000000;										//Liga o dígito das dezenas
	display_num(Tempo[3]);									//Mostra o valor que estiver na variável dig3
	_delay_ms(5);
 	
	PORTC = 0b01000000;										//Liga o dígito das centenas
	display_num(Tempo[4]);									//Mostra o valor que estiver na variável dig2
	_delay_ms(5);
	
	PORTC = 0b00100000;										//Liga o dígito dos milhares
	display_num(Tempo[5]);									//Mostra o valor que estiver na variável dig1
	_delay_ms(5);
}

void digits_alarm()
{
	PORTC = 0b00010000;										//Liga o dígito das unidades
	display_num(minutos%10);								//Mostra o valor que estiver na variável dig4
	_delay_ms(5);
	
	PORTC = 0b10000000;										//Liga o dígito das dezenas
	display_num(minutos/10);								//Mostra o valor que estiver na variável dig3
	_delay_ms(5);
	
	PORTC = 0b01000000;										//Liga o dígito das centenas
	display_num(horas%10);									//Mostra o valor que estiver na variável dig2
	_delay_ms(5);
	
	PORTC = 0b00100000;										//Liga o dígito dos milhares
	display_num(horas/10);									//Mostra o valor que estiver na variável dig1
	_delay_ms(5);
}

void set_alarm()
{
	if (button_down(BUTTON2_MASK))
	{
		minutos ++;
		if(minutos > 59)
		{
			minutos = 0;
		}
	}
	if (button_down(BUTTON3_MASK))
	{
		horas ++;
		if(horas > 23)
		{
			horas = 0;
		}
	}
	digits_alarm();
}

/**************************************************************************
 *                                Alarme
 *************************************************************************/
void alarm()
{
	if (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10 && (PINB & (1<<PB3)))
	{
		sensor();
		
		PORTD |= (1<<5);
		_delay_ms(200);
		PORTD &= ~(1<<5);
		_delay_ms(900);
	}
	else if (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10 && (!(PINB & (1<<PB3))))
	{
		do
		{
			PORTD &= ~(1<<5);
		}
		while (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10);
	}
 	else PORTD &= ~(1<<5);
}

/**************************************************************************
*							Funções de Controlo
`*************************************************************************/ 

void DesativaTWI()
{
	TWCR &= ~(1<<TWEN);
}

void StartTWI()
{
	TWCR = (1<<TWINT)|(1<<TWSTA)|(1<<TWEN);
	while(!(TWCR&(1<<TWINT)));
}

void StopTWI()
{
	TWCR = (1<<TWINT)|(1<<TWEN)|(1<<TWSTO);
}

/**************************************************************************
*					  Função de Bloqueio/Desbloqueio
`*************************************************************************/

void DS1307_Lock(bool onoff)
{
	uint8_t unlock;
	
	if (onoff==true)
	{
		Ler_DS1307(0x00,&unlock);
		unlock&=~(1<<DS_CH);
		Escrever_DS1307(0x00,unlock);
	}
	else
	{
		Ler_DS1307(0x00,&unlock);
		unlock|=(1<<DS_CH);
		Escrever_DS1307(0x00,unlock);
	}
}

/****************************************************************
*                         Modo 24h
*****************************************************************/

void Modo24h()
{
	uint8_t mode;
	Ler_DS1307(0x02,&mode);
	mode&=~(1<<DS_24H);
	Escrever_DS1307(0x02,mode);
}

/****************************************************************
*                         Conversões
*****************************************************************/

char dec_bcd(char num)
{
	return ((num/10 * 16) + (num % 10));					//Converte número decimal para BCD
}

char bcd_dec(char num)
{
	return ((num/16 * 10) + (num % 16));					//Converte BCD para número decimal
}

/****************************************************************
*                         Escrever
*****************************************************************/

bool EscreverByte_TWI(uint8_t byte)
{
	TWDR = byte;												//O registo TWDR contém a sequência de 8 bits a transmitir
	
	TWCR = (1<<TWINT)|(1<<TWEN);								//Começo da comunicação
	while (!(TWCR&(1<<TWINT)));									//Espera pelo fim

	/*
	Verifica se na comunicação houve conclusão de um dos processos:
	SLA+W transmitido e ACK recebido || Byte de dados transmitido e ACK recebido || SLA+R transmitido e ACK recebido
	*/
	if ((TW_STATUS == TW_MT_SLA_ACK) || (TW_STATUS == TW_MT_DATA_ACK) || (TW_STATUS == TW_MR_SLA_ACK))
	{
		return true;
	}
	else
	{
		return false;
	}
}

bool Escrever_DS1307(uint8_t address, uint8_t data)
{
	bool result;
	StartTWI();
	result=EscreverByte_TWI((DS_SLA|TW_WRITE));
	if (result==false) return false;
	result=EscreverByte_TWI(address);
	if (result==false) return false;
	result=EscreverByte_TWI(data);
	if (result==false) return false;
	StopTWI();
	return true;
}

/****************************************************************
*                            Ler
*****************************************************************/

bool LerByte_TWI(uint8_t *data, bool ack)
{
	if(ack==false)
	{
		TWCR&=~(1<<TWEA);										//Se receber um NACK, diz que quer de receber dados
	}
	else
	{
		TWCR|=(1<<TWEA);										//Retorna ACK depois da receção
	}
	
	TWCR|=(1<<TWINT);											//Começo da receção de dados
	while(!(TWCR&(1<<TWINT)));									//Espera que termine
	
	/*
	Verifica se na comunicação houve conclusão de um dos processos: 
	Dados recebidos e ACK retornado || Dados recebidos e NACK retornado
	*/
	if ((TW_STATUS==TW_MR_DATA_ACK)||(TW_STATUS==TW_MR_DATA_NACK))
	{
		*data=TWDR;
		return true;
	}
	else
	{
		return false;
	}
}

bool Ler_DS1307(uint8_t address, uint8_t *data)
{
	bool result;
	StartTWI();
	result=EscreverByte_TWI((DS_SLA|TW_WRITE));
	if (result==false) return false;
	
	result=EscreverByte_TWI(address);
	if (result==false) return false;
	
	StartTWI();
	result=EscreverByte_TWI((DS_SLA|TW_READ));
	if (result==false) return false;
	
	result=LerByte_TWI(data,false);
	if (result==false) return false;
	
	StopTWI();
	
	return true;
}

/****************************************************************
*                      Acertar Horas e Minutos
*****************************************************************/

void Time_Set(uint8_t hour, uint8_t min, uint8_t sec)
{
	Escrever_DS1307(0x00,((sec/10)<<4)+(sec%10));				//seconds=20 //0-59
	Escrever_DS1307(0x01,((min/10)<<4)+(min%10));				//minute=19 //0-59
	Escrever_DS1307(0x02,((hour/10)<<4)+(hour%10));				//hour= 18 //0-23
}

/****************************************************************
*                      Obter o Tempo
*****************************************************************/

void Get_Time()
{
	uint8_t value;
	Ler_DS1307(0x00,&value);
	Tempo[0] = value & 0b00001111;
	Tempo[1] = (value & 0b01110000)>>4;
	Ler_DS1307(0x01,&value);
	Tempo[2] = value & 0b00001111;
	Tempo[3] = (value & 0b01110000)>>4;
	Ler_DS1307(0x02,&value);
	Tempo[4] = value & 0b00001111;
	Tempo[5] = (value & 0b01110000)>>4;
}

I get my clock with DS1307.

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

I tried your solution and it does the same as before, that is, it only turns off the alarm while the button is pressed :/

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

diogofvc wrote:
when I press the button PB3 it turns off the alarm but the program does not return to the normal clock
Yes, because this

		do
		{
			PORTD &= ~(1<<5);
		}
		while (Tempo[2] == minutos%10 && Tempo[3] == minutos/10 && Tempo[4] == horas%10 && Tempo[5] == horas/10);

is an endless loop.

 

Sorry, but your program flow is a mess. Typical example of programming without thinking about the design beforehand and then adding something here and there.

You should do two things:

1) look "state machine" up

2) redesign you program from scratch

 

Stefan Ernst

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

Unfortunately I do not have time for that. I was just wondering if you could help me solve this problem that seems to be simple but I can not solve it

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

diogofvc wrote:
Unfortunately I do not have time for that.
So it is something with a due date (school assignment? commercial product?). That makes it even worse.

 

diogofvc wrote:
I was just wondering if you could help me solve this problem that seems to be simple but I can not solve it
This particular problem can be solved by adding one specific statement to the loop. But that would be quick&dirty and would increase the mess. So I am quite reluctant to give you that "solution".

Stefan Ernst

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

yes, school assignment. I'm finishing my degree and I need to show the project working tomorrow. The code is only for submission at the end of the week. Then I just need it to work. Can you give me the solution?

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

I really need this to be done, please.

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

diogofvc wrote:

I really need this to be done, please.

Begging others to fix your homework gets you nowhere.

 

Sternst has provided you with the spot that has the problem.  Why don't you try and figure it out now?

 

 

JIm

If you want a career with a known path - become an undertaker. Dead people don't sue! - Kartman

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB user

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

I've been trying to do that for hours but I cant figure it out. I only come here when I cant find out for myself

I bet it's just something that's escaping me, and you guys could help with that. I'm not asking too much.

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

You've been begging for hours but yet you don't have the time to solve your problem?? 

Next time you go to the doctor, think about how he passed his exams? Would you feel confident in his expertise if you knew he fudged all his university exams and got others to do his work? How about if you were under the knife of a surgeon?

A couple of the guys here have gone to great lengths to enlighten you but yet you still beg?

Think about your problem - you sit in a loop waiting for the time to change, but how does the time change if you don't call Get_Time()?

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

The answer lies in epistemology from the  point of view of the AVR.

International Theophysical Year seems to have been forgotten..
Anyone remember the song Jukebox Band?

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

The alarm bells rang as soon as I got to:

void inic();
void digits_clock();
void digits_alarm();
void alarm();
void set_alarm();
void trigger();
void sensor();

A program with the odd function that has void return and no parameters is possibly OK but a program where almost every function has that interface (and consequently most data passing via globals) suggests someone who hasn't quite "got C" yet. I'd concentrate on learning a bit more about function inputs and outputs if I were you.