GOTO, help me avoid it!

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

I do search the forums for a bit before starting a new thread, but I couldn't find anything like this.

I blame the way I went about coding this, what with the ISR and how I handle incoming commands. It works just fine, but it makes for a very long and repetitive main statement. An operating system of sorts may help me, but I don't want to do that just yet, this program is nearing completion believe it or not. (have to navigate backwards in menus and print out port status/ADC values)

Pardon the length, I've seen longer posted here ;D

/*************************************************************************/

#include 
#include 
#include 
#include 
#include 
#include "menu_driver.h"
#include "menu.h"


#define F_CPU 		2000000UL
#define USB 		USARTC0
#define BAUDRATE 	9600
#define BAUDSET 	0x0C
#define BUFFLEN		20

#define SENDMENU(_array)	 clearScreen(); cursorHome(); sendMenu(&_array); newLine()


volatile uint8_t command[10];

USART_data_t USART_data;

volatile uint8_t bufCnt;
char U;
char buffer[BUFFLEN];
volatile bool bFlag = false;

void uartSetup()
{
*SNIP*
}

void clearScreen(void){ sendMenu(&clrscreen); }

void cursorHome(void){ sendMenu(&cursorhome); }

void newLine(void){ sendMenu(&newline); }

void sendMenu(uint8_t * menu)
{
	uint16_t menuLength = 0;

	while(pgm_read_byte(&menu[menuLength]) != 0x00)
	{
		USART_TXBuffer_PutByte(&USART_data, pgm_read_byte(&menu[menuLength]));
		menuLength++;
	}
	return;
}

int main(void)
{	
	uartSetup();

	_delay_ms(10);
	sendMenu(&mainmenu);

	PMIC.CTRL |= 0x07;

	asm("sei");

	while (1)					//MAIN MENU****----++++----****
	{
		while (bFlag == false)	//No command yet			 
		{
		}

		while (bFlag == true)	//Command has been received
		{
			PORTE.OUTCLR = PIN0_bm;

			bFlag = false;
			uint8_t i=0;
			
			char tmp[bufCnt];

			strncpy(tmp, command, bufCnt);

			bufCnt=0;		//RESET CMD BUFFER, OUR CMD IS IN tmp[] NOW

			if (strncmp(tmp, "1", sizeof(tmp)) == 0) { SENDMENU(one); goto menu1; }
			else if (strncmp(tmp, "2", sizeof(tmp)) == 0) { SENDMENU(two); goto menu2; }			
		}
	}


	while (1)					//MENU 1****----++++----****
	{
		menu1:

		while (bFlag == false)	//No command yet			 
		{
		}

		while (bFlag == true)	//Command has been received
		{
			PORTE.OUTCLR = PIN0_bm;

			bFlag = false;
			uint8_t i=0;
			
			char tmp[bufCnt];

			strncpy(tmp, command, bufCnt);

			bufCnt=0;		//RESET CMD BUFFER, OUR CMD IS IN tmp[] NOW

			if (strncmp(tmp, "1", sizeof(tmp)) == 0) { SENDMENU(one_one); }
			else if (strncmp(tmp, "2", sizeof(tmp)) == 0) { SENDMENU(one_two); }

			
		}
	}


	while (1)					//MENU 2****----++++----****
	{
		menu2:

		while (bFlag == false)	//No command yet			 
		{
		}

		while (bFlag == true)	//Command has been received
		{
			PORTE.OUTCLR = PIN0_bm;

			bFlag = false;
			uint8_t i=0;
			
			char tmp[bufCnt];

			strncpy(tmp, command, bufCnt);

			bufCnt=0;		//RESET CMD BUFFER, OUR CMD IS IN tmp[] NOW

			if (strncmp(tmp, "1", sizeof(tmp)) == 0) { SENDMENU(one); }
			else if (strncmp(tmp, "2", sizeof(tmp)) == 0) { SENDMENU(two); }
		}
	}
}

ISR(USARTC0_DRE_vect)
{
	USART_DataRegEmpty(&USART_data);
}

ISR(USARTC0_RXC_vect)
{
	PORTE.OUTSET = PIN0_bm; 
	//USART_RXComplete(&USART_data);
	
	U = USB.DATA;			// move the RXed character from UDR register to U variable 
	if (U == 0x0D)			// carriage return? if yes end of line and compare for cmd_buff 
	{  
		bFlag = true;           
		command[bufCnt] = U;
	}    

	else
	{   
		bFlag = false;
		command[bufCnt] = U;			//copy RXd character to the buffer array 
		bufCnt++;					// increase the char counter for the buff array 
		if (bufCnt >= BUFFLEN) {bufCnt=0;}
	} 

}

The compiler also reports an warning of

Quote:
../AGEMX.c:58: warning: passing argument 1 of 'sendMenu' from incompatible pointer type

It works... but warnings are there for a reason, I'm not exactly sure what to do (I came up with that mangled function myself)

So I ask: Suggestions on how to rewrite without goto statements? & How can I alleviate the incompatible poitner type problem?
xmega128a1 on latest winavr in avrstudio4

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

Use a state machine.

Stealing Proteus doesn't make you an engineer.

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

We can't know what line 58 is since you snipped.
It's likely a result of defining a string (array of char) and then doing &stringname: stringname is already a pointer so &stringname is a pointer to pointer to char, char **p.

You're running the state machine on the program counter, which is why you find yourself using goto for state change. If you have a professor to satisfy, invent an explicit state variable, put your three states in a big switch, and the switch in a for ( ; ; ) loop.

There's nothing inherently evil about unstructured jumps (goto) - that's the underlying mechanism when you do structured exception handling. In that case you may even jump from somewhere in one function so somewhere several levels up the call tree, and you don't hear _that_ touted as evil. Which is strange.

/Kasper

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

Thank you for your replies, the code I snipped was:

	PORTE.DIRSET = PIN0_bm;
	PORTC.OUTSET = PIN3_bm;
	PORTC.DIRSET = PIN3_bm;
	PORTC.DIRCLR = PIN2_bm;

	USART_InterruptDriver_Initialize(&USART_data, &USB, USART_DREINTLVL_MED_gc);


	USART_Format_Set(&USB, USART_CHSIZE_8BIT_gc, USART_PMODE_DISABLED_gc, false);
	USARTC0.BAUDCTRLA = BAUDSET;
	USARTC0.CTRLA = USART_RXCINTLVL_MED_gc | USART_TXCINTLVL_OFF_gc | USART_DREINTLVL_MED_gc;
	USARTC0.CTRLB = USART_RXEN_bm | USART_TXEN_bm;

I chose not to use the USART_data receive buffer and instead used the command[] to catch input (because at the time I didn't know PMIC had to enable interrupts)

Were you talking about the sendMenu(); function when you were talking of pointers? In menu.h I have the strings stored as const uint8_t in PROGMEM, It works but I'll digest what you posted about **

No, no professor to satify, that may be part of the problem... I don't think I will have any courses resembling computer science aside from intro to programming for engineers, this is only a hobby (for now)

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

KKP wrote:
There's nothing inherently evil about unstructured jumps (goto) - that's the underlying mechanism when you do structured exception handling. In that case you may even jump from somewhere in one function so somewhere several levels up the call tree, and you don't hear _that_ touted as evil. Which is strange.
Structured exception handling is like using goto in a manner similar to an ejection seat on an exploding fighter plane and that is a perfectly reasonable thing to do. Using the ejection seat to exit a safely landed aircraft, doesn't show much knowledge of how the craft was designed.

There is no evil to goto, but it is a tool that is only rarely needed and that folks coming from BASIC seem to think makes good programming sense in many cases where there are far better option, such as a case statement for the OP.

IMHO, and this will sound elitist, it shouldn't be used by novices or even moderately experienced folks using C. IMHO such folks have plenty of alternative tools that are much easier follow than the simple expedient of goto. IMHO, it should be reserved for only for the more advanced folks who already understand all the other choices and need to build something like structured exception handling. IIRC I've only used it once in a deeply nested bit of code that had a minor bug that I needed to correct the night before the due date. As soon as I had time, I rewrote it to something that I wouldn't mind someone else reading and didn't need the goto.

I would advise the OP that anytime he thinks a goto is a good idea in C, he is probably missing something important and should post a question here to find out what he is missing.

But anyway this is just my opinion and all the arguements are well fought turf and anything that can be said on the topic has been well said decades ago and is a mere Google search away.

Smiley

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

smileymicros wrote:
I would advise the OP that anytime he thinks a goto is a good idea in C, he is probably missing something important and should post a question here to find out what he is missing.

Precisely! A state machine of sorts is just what I needed. The gotos left me feeling empowered, and made me want to jump from main() into functions and all over the place. I now have a typedef with my menus, and I think that the while(1) loops back around (not having anything to do) much more quickly now.

I bet I can optimize this further, even though timing isn't a constraint. The objective of all of this is learning, which I fell I am getting plenty of!

Boy would I sure hate to code this in ASM with my present knowledge :P The pointers are still yelling at me though >.>

I changed:

#define SENDMENU(_array)	 clearScreen(); cursorHome(); sendMenu(&_array); newLine()

to

#define SENDMENU(_array)	 clearScreen(); cursorHome(); sendMenu(_array); newLine()

Now the warning tells me that I am discarding qualifiers from the pointer terget type.

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

syntroniks wrote:
Now the warning tells me that I am discarding qualifiers from the pointer terget type.
sendMenu takes a pointer to "uint8_t" and you call it with a pointer either to "const uint8_t" or to "volatile uint8_t". I assume the first because you read the target from flash. So change the function parameter to "const uint8_t * menu".

Stefan Ernst

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

Your #define is also not a common usage and an ordinary function would do as well and not hide anything unecessarily. I strongly suggest you read Kernighan and Ritchie or any other introductory book or website on C programming before trying to get too fancy. Just keep it simple and readable. Look at what other folks are doing and follow that lead, before spending too much time trying to invent your own way.

Smiley

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

For the record, here is the structure of a regular state machine with an explicit state variable:

enum
{
	State_A = 0
	State_B = 1
	State_C = 2
	State_D = 3
} States;

uint8_t CurrentState = State_A;

int main(void)
{
	for (;;)
	{
		// Each iteration the correct handler code for the current state is run
		switch (CurrentState)
		{
			case State_A:
				// State A Processing Here
			
				if (SomethingHappens)
				{
					// Change to a different state
					CurrentState = State_B;
				}
				
				break;
			case State_B:
				// State B Processing Here
			
				break;
			case State_C:
				// State C Processing Here

				break;
			case State_D:
				// State D Processing Here
				
				break;
		}
	}
}

Note that you don't always have to progress to the next state -- you can jump to any state you like. This is a much more structured way of ordering your code.

I've used this method in my USB stack for the Host Mode state machine, as well as in my "Simple Simon" project. On the flipside, I've used goto in my first and only time in the C language inside the same project to save on FLASH space, in a large function where exception handling was needed.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

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

In addition to Dean's excellent state variable example, I shall add the state procedure example.

This approach is a little more complicated but is commonly used when 1) the processing for each state is complicated and 2) there are a lot of states to handle. The idea is to encapsulate the processing for each state in a function and then use a function pointer to determine which function to call. The example:

typedef void (*StateFunc) ( void );

stateFunc CurrentState;

void StateA ( void )
{
	... state A processing
	if ( transition )
	{
		CurrentState = StateB;
	}
}

void StateB ( void )
{
	... state B processing
	if ( transition )
	{
		CurrentState = StateC;
	}
	else
	{
		CurrentState = StateA;
	}
}

void StateC ( void )
{
	... state C processing
	if ( transition )
	{
		CurrentState = StateD;
	}
	else
	{
		CurrentState = StateA;
	}
}

void StateD ( void )
{
	... state D processing
	if ( transition )
	{
		CurrentState = StateA;
	}
	{
		CurrentState = StateC;
	}
}

int main ( void )
{
	/* Initialize the current state */
	CurrentState = StateA;
	
	/* Process forever */
	while (1)
	{
		CurrentState();
	}
}

This is the simplest case -- each state function could accept an incoming value (perhaps a captured keyboard click) or more. The function can also return a value, for decision processing at the main level.

The advantages to this approach are:

- The decision time (the time to decipher the state and start executing the procedure) is identical for all states

- Each state processing is encapsulated, making the code easier to maintain.

We have several "old timers" from the hard disk drive industry in our company; this approach is standard in HDD servo code.

One quick warning: On the ATMega2560/2561, this approach has a "gotcha" in the function pointer code. Read my post on FreeRTOS for ATmega2560/1 for more about it.

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

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

smileymicros wrote:
There is no evil to goto, but it is a tool that is only rarely needed and that folks coming from BASIC seem to think makes good programming sense in many cases where there are far better option, such as a case statement for the OP.

I don't think we, where it matters, disagree on the usage. My annoyance with the anti-goto-evangelism is that it teaches people _not_ to stop and think when their clear and understandable code turns into unsolvable spaghetti in the pursuit of avoiding one goto. (I agree that this thread is not an example of any such need)

/Kasper

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

Well regardless, I really appreciate the help I have received. My whole program looks in structure a lot like what Dean just posted here, and it is easier on the eyes to say the least.

I actually managed to get my hands on a copy of K&R, and it answered a whole heap of questions, also helped my ** pointer deal out. Basically I could choose either "&whatever[0]" or "whatever". Not &whatever.

Once again, I appreciate the time you all have taken answering, without these states, I wouldn't be able to expand much into anything useful. (now I'm running at 16 times the clock speed - 6 times the baud rate getting ready to mess with a 64k eeprom when I have >100k flash memory left :-D)

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

Quote:

KKP wrote:
There's nothing inherently evil about unstructured jumps (goto)

LOL--guess we have to go back to the seminal discussion, and then debate whether "evil" and "harmful" are synonyms.
http://en.wikipedia.org/wiki/Con...

Quote:

In that case you may even jump from somewhere in one function so somewhere several levels up the call tree, and you don't hear _that_ touted as evil.

Well, I guess I would on the surface. If I'm "crashing out" and am going to do a full restart/reset then I guess anything goes. But in many many years of C programming I'm not yet smart enough to handle longjmp().

In practice, I may end up with a goto or a few in maybe 1 of 5 apps. Probably less. Dyed-in-the-wool GCC users may have a few more, given the utility of the "indirect goto" extension. But that ain't real C.

Lee

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

25 years and over a million lines of C/C++ written, I've used a goto once or twice; the one instance I recall clearly involved the last step of optimization that added another 5% of throughput to some time critical code.

As for goto'ing somewhere outside of the current function, that is just asking for trouble. Like its cousin exception handling, that would be a sign of profound programmer laziness, and as in other areas of life, the consequences are predictable and unpleasant.

C: i = "told you so";