Nested Switch doesn't work correctly

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

Hi

 

Running AS7, C99 and using the simulator when debugging

 

I am working on a function to setup and configure the 4 USARTS of a 2560 with 1 of 9 Baud Rates, to be configurable through SW control in my finished project by passing two variables to my function eg 2, Baud_19200 for the port number and Baud Rate value

 

To facilitate this I have implemented a 4 way Switch-Case for the ports and a 9 way nested Switch-Case underneath each for the Baud rates for a value to put in UBRRnH/UBRRnL.

 

Now the problem I am getting is for what ever Baud rate value I pass to the function, this value gets acted on for all subsequent Port values.

 

Example :- Say I pass 'Function(1, Baud_9600)' meaning I want to set USART0 to 9600 Baud and return. USART0 will be set to 9600, great but then USART 1 will also be set to 9600 , then USART2 will be set 9600 and lastly USART3 will get set to 9600. Equally if I run 'Function(3, Baud_57600) USART3 will be correctly set to 57600 then USART4 will be set to 57600

I have included a section of code which is just repeated ( I cut and paste)

 

Secondary,  there must be a better way to achieve this with less code - an Array possibly suggestion will be welcome

 

 

//////////////////////////////////////////////////////////////////////////////
//							My Setup Configuration							//
//////////////////////////////////////////////////////////////////////////////

//								USART	  Definitions
//									Baud rates

#define		baud_4800				0xCF	// UBRR Value used in USART_Setup Switch Statement
#define		baud_9600				0x67	// UBRR Value USART_Setup Switch Statement
#define		baud_19200				0x33	// UBRR Value USART_Setup Switch Statement
#define		baud_28800				0x21	// UBRR Value USART_Setup Switch Statement
#define		baud_38400				0x19	// UBRR Value USART_Setup Switch Statement
#define		baud_57600				0x10	// UBRR Value USART_Setup Switch Statement
#define		baud_76800				0x0C	// UBRR Value USART_Setup Switch Statement
#define		baud_115200				0x07	// UBRR Value USART_Setup Switch Statement
#define		baud_230400				0x03	// UBRR Value USART_Setup Switch Statement

//								USART Ports 

#define		initial					0		// used once for initial port 8n1 setup
#define		USART0					1
#define		USART1					2
#define		USART2					3
#define		USART3					4

//                  Initial USARTs Setup - Asynchronous, 8 Data, Stop, no Parity
//
//#define AsyncMode						0b00000000		// 		0x00 in UCSRnC
//#define CharSize						0b00000110		//		0x06 in UCSRnC
//#define StopBit						0b00000000		//		0x00 in UCSRnC
//#define ParityMode 					0b00000000		//		0x00 in UCSRnC
//#define CharSizeH						0b0000000		//		0x00 in UCSRnB
#define MyUCSRnC						06			//      0b11001110  (from above)
#define MyUCSRnB						00			//      0b00000100  (from above)

//					Declare my Function

void Usart_Setup(uint8_t , uint8_t);

//<<<<<<<<<<<<<<<<<<< Start of MAIN >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

int main(void)

{
	Usart_Setup(0,0);				// Actual values at runtime 8n1 on all Usarts
	Usart_Setup(2,  baud_19200);	// simulated values for debugging Switch

	return 0;

}

void Usart_Setup(uint8_t Port, uint8_t Speed)
{
	switch (Port)
	{
		case 0:				// Setting up Async mode on all ports @8n1
		UCSR0C = MyUCSRnC;	// Usart 0
		UCSR0B = MyUCSRnB;
		UCSR1C = MyUCSRnC;	// Usart 1
		UCSR1B = MyUCSRnB;
		UCSR2C = MyUCSRnC;	// Usart 2
		UCSR2B = MyUCSRnB;
		UCSR3C = MyUCSRnC;	// Usart 3
		UCSR3B = MyUCSRnB;
		break;

		case 1:
		switch (Speed)
		{

			case 0xCF:
			UBRR0H =  (uint8_t) (baud_4800>>8);
			UBRR0L = baud_4800;
			break;
			case 0x67:
			UBRR0H =  (uint8_t) (baud_9600>>8);
			UBRR0L = baud_9600;
			break;
			case 0x33:
			UBRR0H =  (uint8_t) (baud_19200>>8);
			UBRR0L = baud_19200;
			break;
			case 0x21:
			UBRR0H =  (uint8_t) (baud_28800>>8);
			UBRR0L = baud_28800;
			break;
			case 0x19:
			UBRR0H =  (uint8_t) (baud_38400>>8);
			UBRR0L = baud_38400;
			break;
			case 0x10:
			UBRR0H =  (uint8_t) (baud_57600>>8);
			UBRR0L = baud_57600;
			break;
			case 0x0C:
			UBRR0H =  (uint8_t) (baud_76800>>8);
			UBRR0L = baud_76800;
			break;
			case 0x07:
			UBRR0H =  (uint8_t) (baud_230400>>8);
			UBRR0L = baud_230400;
			break;
			case 0x03F:
			UBRR0H =  (uint8_t) (baud_4800>>8);
			UBRR0L = baud_4800;
			break;
			default:
			break;
		}
			

Thnaks

Last Edited: Tue. Nov 17, 2020 - 09:51 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Docara wrote:
using the simulator when debugging

So you should be able to step through the code and see what's happening - what does that tell you?

 

The code you posted is incomplete

 

Docara wrote:
there must be a better way to achieve this with less code

For one thing, the compiler should know how to handle the 2 halves of UBRR - so you don't need to do that "manually"

 

 

I don't see the point of the inner 'switch'; eg,

			case 0xCF:                          <<< this is the value of baud_4800
			UBRR0H =  (uint8_t) (baud_4800>>8);
			UBRR0L = baud_4800;
			break;

			case 0x67:                          <<< this is the value of baud_9600
			UBRR0H =  (uint8_t) (baud_9600>>8);
			UBRR0L = baud_9600

 

 

so aren't you just doing:

		UBRR0 = Speed;

 

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. Nov 17, 2020 - 11:42 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0


Umm your code does not make much sense does it?

 

For example:

#define		baud_4800				0xCF	// UBRR Value used in USART_Setup Switch Statement
#define		baud_9600				0x67	// UBRR Value USART_Setup Switch Statement
#define		baud_19200				0x33	// UBRR Value USART_Setup Switch Statement
#define		baud_28800				0x21	// UBRR Value USART_Setup Switch Statement

then:

		switch (Speed)
		{

			case 0xCF:
			UBRR0H =  (uint8_t) (baud_4800>>8);
			UBRR0L = baud_4800;
			break;
			case 0x67:
			UBRR0H =  (uint8_t) (baud_9600>>8);
			UBRR0L = baud_9600;
			break;
			case 0x33:
			UBRR0H =  (uint8_t) (baud_19200>>8);
			UBRR0L = baud_19200;
			break;
			case 0x21:
			UBRR0H =  (uint8_t) (baud_28800>>8);
			UBRR0L = baud_28800;
			break;

but baud_4800, baud_9600 etc that you use there are 0xCF, 0x67 ect. So this code basically says:

		switch (Speed)
		{

			case 0xCF:
			UBRR0H =  (uint8_t) (0xCF>>8);
			UBRR0L = 0xCF;
			break;
			case 0x67:
			UBRR0H =  (uint8_t) (0x67>>8);
			UBRR0L = 0x67;
			break;
			case 0x33:
			UBRR0H =  (uint8_t) (0x33>>8);
			UBRR0L = 0x33;
			break;
			case 0x21:
			UBRR0H =  (uint8_t) (0x21>>8);
			UBRR0L = 0x21;
			break;
etc etc

So why not simply replace the entire switch/case with:

			UBRR0H =  (uint8_t) (Speed>>8);
			UBRR0L = Speed;

but even that is more complex than it needs to be because "UBRR0" is a 16 bit register that combines "UBRR0L" and "UBRR0H" into one. So:

			UBRR0 =  Speed;

That one line replaces the entire:

		switch (Speed)
		{

			case 0xCF:
			UBRR0H =  (uint8_t) (baud_4800>>8);
			UBRR0L = baud_4800;
			break;
			case 0x67:
			UBRR0H =  (uint8_t) (baud_9600>>8);
			UBRR0L = baud_9600;
			break;
			case 0x33:
			UBRR0H =  (uint8_t) (baud_19200>>8);
			UBRR0L = baud_19200;
			break;
			case 0x21:
			UBRR0H =  (uint8_t) (baud_28800>>8);
			UBRR0L = baud_28800;
			break;
			case 0x19:
			UBRR0H =  (uint8_t) (baud_38400>>8);
			UBRR0L = baud_38400;
			break;
			case 0x10:
			UBRR0H =  (uint8_t) (baud_57600>>8);
			UBRR0L = baud_57600;
			break;
			case 0x0C:
			UBRR0H =  (uint8_t) (baud_76800>>8);
			UBRR0L = baud_76800;
			break;
			case 0x07:
			UBRR0H =  (uint8_t) (baud_230400>>8);
			UBRR0L = baud_230400;
			break;
			case 0x03F:
			UBRR0H =  (uint8_t) (baud_4800>>8);
			UBRR0L = baud_4800;
			break;
			default:
			break;
		}

BTW you may want to rethink this anyway. Things like uart_4800 being fixed at 0xCF (207) is only valid for:

 

 

What if you want to run the AVR at 8MHz or 1Mhz or 3.6864MHz etc etc ?

 

Oh and:

#define MyUCSRnC						06			//      0b11001110  (from above)
#define MyUCSRnB						00			//      0b00000100  (from above)

You got away with it this time but do you know what happens in C if you start a number with a leading '0' and it is not then followed by either 'x' or 'b' ? If you don't know you probably want to look it up but if you had tried to use 09 you would probably have found out already !

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

I would simply calculate UBBR0 at runtime.   i.e. one statement.   Avoid the defines, switch, ...

 

Yes,  it would be wise to check the error percentage.    Error handling would complicate things.   But most apps will only permit standard baud rates.

 

David.

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

Right so many things to reply to 

 

So from the top down

 

I am stepping through the code - hence my question. This is how I have found code is running out side my Switch - Case and bypassing my 'Port' and 'Speed' values for subsequent Case statements 

 

I did wonder about entering UBRR directly - I was just following examples in the datasheet

 

I don't see the point of the inner 'switch'; eg,

I have four Port (actually 3 in use) that requires changing baud rates (ultimately via a GUI menu) this seemed the easiest way initially to gets things moving and incorporate the functionality into a function. However a light has just been switched on after your 'UBRR0=Speed' comment - clearly I over thought thanks for the input.

 

clawson - duplicate point to above-yeah fair point, and it will only be running it at 16Mhz

 

However, no one has posted about my actual question. 

 

 

 

 

 

 

 

 

 

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

Was it your intention to write the

    switch (Speed)
    {
    case 0xCF:
        UBRR0H =  (uint8_t) (baud_4800>>8);
        UBRR0L = baud_4800;
        break;
        case 0x67:
        UBRR0H =  (uint8_t) (baud_9600>>8);
        UBRR0L = baud_9600;
        break;
...

a whole 4 times ?

 

I'm going to guess that you were and that repetition should trigger a thought: -"Perhaps I should make this a function  ?"

As other freaks have indicated you only need return one value and assign it to the appropriate UBRRn resister..

 

Writing and using this function will almost certainly fix your bug.

 

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

Docara wrote:
no one has posted about my actual question

 

As noted, you didn't post the complete  code - so impossible to say what might happen in the complete code.

 

But you asked:

Docara wrote:
there must be a better way to achieve this with less code

and that has certainly been answered!

 

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

Docara wrote:

However, no one has posted about my actual question. 

 

I'm being a complete pedant but there is no question (mark) in your original post.

#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

awneil wrote:
As noted, you didn't post the complete  code - so impossible to say what might happen in the complete code.
This!

 

Give us the smallest, cut down AS7 project that still shows the issue you think you have and we can all try it in our own simulators.

 

Also before that set your build options to -Og and see if that helps (usually these things come down to over-agrresive optimisation which makes following a linear flow in the sim hard).

 

Oh and just an idea. If you want a function where you pass an "index number" to pick a given UART to be set then the word "index" in that should ring a bell! Suppose you had a structure like:

volatile uint16_t * ubrrs[] = { &UBRR0, UBRR1, &UBRR2, &UBRR3);

now is you say:

*ubrrs[2] = 0xCF;

then this will set the UBRR for UART2 (from 0..3).

 

In fact you could take this one step further:

typedef struct {
    volatile uint16_t * ubrr;
    volatile uint8_t * ucsra;
    volatile uint8_t * ucsrb;
    volatile uint8_t * ucsrc;
} uart_layout_t;

uart_layout_t uarts[] {
    { &UBBRR0, &UCSR0A, &UCSR0B, &UCSR0C },
    { &UBBRR1, &UCSR1A, &UCSR1B, &UCSR1C },
    { &UBBRR2, &UCSR2A, &UCSR2B, &UCSR2C },
    { &UBBRR3, &UCSR3A, &UCSR3B, &UCSR3C }
};

Now if you have some kind of:

uart_set_baud(int uart_num, uint16_t baud) {
    *uarts[uart_num].ubrr = baud;
}

The same function can set the baud for any of UART 0,1,2,3.

 

You don't need some dreadful great switch/case statement as sure UBRR0, UBRR1, UBRR2 and UBRR3 might all have very different addresses - but you just index into the lookup table to access the right one.

 

You are going to love Xmega (which also means AVR-0, AVR-1, AVR-Dx, etc) when you get to them as Atmel already grouped all the UART registers (and the same for all peripherals) into a struct so to address a particular UART (or whatever) you only need to know the base address of the register block.

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

clawson wrote:

 

volatile uint16_t * ubrrs[] = { &UBRR0, UBRR1, &UBRR2, &UBRR3);

 

 

thanks for explanation. i'm a newbie in both c programming and avr world. i kinda do this the same way, but haven't really noticed how much easier i could handle registers by passing pointers, and not keeping a copy of their value in structs :)

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

Each time you start over you will learn from all your mistakes and get 30% better--so why wait?  You can't beat that return on investment!

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

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

UPDATE 

 

Based on me making a passing statement about a better way about using an array and together with some comments above, the brain kicked in! I have since rewritten what I had originally produced and it now works as expected a LOT smaller and much cleaner nicer code.

 

I have set up an array holding UBRR values, I defined USARTS a value from 1 to 4, defined the Baud_Rates a value from 0 to 8 and pass this as 'Speed' . and I'm just passing port numbers and array position values (for baud Rate) to my function.

 

I have included a snippet of my codes Switch-Case  

  switch (Port)
{
  case 1:
    { UBRR0 = BaudArry[Speed];
        break;
      case 2:
        UBRR1 = BaudArry[Speed];
	break;
      case 3:
        UBRR2 = BaudArry[Speed];
        break;
      case 4:
        UBRR3 = BaudArry[Speed];
        break;
	   default:
	   break;
      }

Much better!!

 

I didn't realise that not copying over 250 lines of crappy code for you all to wade through caused so many annoyances - I apologise. But like I said

I have included a section of code which is just repeated ( I cut and paste)

other than port numbers in the registers it is all EXACTLY the same code. 

 

Anyway as I am no longer using my original code I feel this can be closed However I am more than happy to post what I had to see if the problem can be reproduced by others

Lets me know 

thanks

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

Thanks for feeding back.

 

Rather than a switch on the port number, couldn't you just use an array of Port addresses - as clawson suggested in #9 ?

 

*ubrrs[Port] = BaudArry[Speed];

 

Docara wrote:
I feel this can be closed

You do that by marking the solution - see Tip #5 in my signature, below:

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

Each time you start over you will learn from all your mistakes and get 30% better--so why wait?  You can't beat that return on investment!

 

Maybe I underestimated!!!

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

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

awneil wrote:

you just use an array of Port addresses - as clawson suggested in #9 ?

 

*ubrrs[Port] = BaudArry[Speed];

 

Just to say that (unless you pad at the start of the array) you probably want [Port -1] because the port numbers seem to be 1..4 for ports that are actually 0..3