USART polled get byte with timeout - not working

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

I'm using timer0 as a timeout timer to read data from UART. The code looks as so:

uint8_t UARTGetByte(void)
{
	// Setup timer0 for timeout check
	OCR0   = 255; // ~0.03264 sec
	TIFR  |= _BV(OCF0) | _BV(TOV0);
	TCCR0 |= _BV(WGM01) | _BV(CS02) | _BV(CS01) | _BV(CS00);

	do{
		while (!(UCSR0A & _BV(RXC0)));
		return UDR0;
	}while (!(TIFR & _BV(OCF0)));
	
	return 0;
}

The problem is that after The line

TCCR0 |= _BV(WGM01) | _BV(CS02) | _BV(CS01) | _BV(CS00);

OCF0 is set (according to the debugger's I/O view, using JTAG on ATmega128). Clearing the bit also seems to have no effect. When executing, the code remains in the loop forever because I'm not sending the MCU any serial data. OCF0's value can not be seen when hovering above it with the courser, however TIFR = 3 which means it's set. How can I clear this bit so I can use the function?

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

:puzzled:
That code will never 'time-out' if a character never arrives, but is that what you really want it to do ?,
or do you actually want to exit from checking the UCSR0A when the timer expires ?

I would set TCNT0 to a known value at the start of that procedure.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
uint8_t UARTGetByte(void)
{
    // Setup timer0 for timeout check
    OCR0 = 255; // ~0.03264 sec
    TCNT0 = 0;
    TIFR |= _BV(OCF0) | _BV(TOV0);
    TCCR0 |= _BV(WGM01) | _BV(CS02) | _BV(CS01) | _BV(CS00);

    do{
        if (UCSR0A & _BV(RXC0)
            return UDR0;
    }while (!(TIFR & _BV(OCF0)));

    return 0;
}

I have an additional question. UDR0 can potentially get any value between 0 & 0xFF. Returning 0 is not really a good idea because that might be a legal character is my case so the function is meaningless. The way out that I see is to return an int16_t variable with the value of -1, for example, when the function times out. The annoying this is that I will have to cast each returned value because I'm actually waiting for uint8_t. Any elegant way around this?

Last Edited: Sun. Dec 17, 2017 - 09:23 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Your function essentially needs to leave two values to the calling code:

  1. The character read (which exhausts the value range of char so no extra information can be sent using that), and
  2. The outcome of the read (timed out or not)

 

You can do it as you sketch out above (returning something wider than a char).

 

Another possibility is to return the outcome over the return value of the function and pass out the character read through a parameter (i.e. a pointer to a char).

 

Sketchy, not tested:

 

bool UARTGetByte(char * c) {
    // Setup timer0 for timeout check
    OCR0 = 255; // ~0.03264 sec
    TCNT0 = 0;
    TIFR |= _BV(OCF0) | _BV(TOV0);
    TCCR0 |= _BV(WGM01) | _BV(CS02) | _BV(CS01) | _BV(CS00);

    do{
        if (UCSR0A & _BV(RXC0)
            *c = UDR0;
            return true;
    }while (!(TIFR & _BV(OCF0)));

    return false;
}

 

This will return true if a character actually was read, and false if the read timed out.

 

There is no absolute law, but it is often the convention to have the status as the return value, and the "actual payload" passed out over a parameter (rather than the other way around).

 

This means you can call it like this:

char c;

if (UARTGetByte(&c)) {
    // Do something with the character that was read
} else {
    // Possibly act on the time-out
}

which some prefer over

char c;


bool characterRead = UARTGetByte(&c)

if () {
    // Do something with the character that was read
} else {
    // Possibly act on the time-out
}

I won't tell my position re this fissure in the C community... ;-)

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"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]

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
bool UARTGetByte(uint8_t *byte)
{
    // Setup timer0 for timeout check (~0.03264 sec)
    TCNT0 = 0;
    TIFR = _BV(TOV0);
    TCCR0 = _BV(WGM01) | _BV(CS02) | _BV(CS01) | _BV(CS00);

    do{
        if (UCSR0A & _BV(RXC0))
        {
            *byte = UDR0;
            return true;
        }
    }while (!(TIFR & _BV(TOV0)));

    return false;
}

Thanks, this seems to be working.

Last Edited: Sun. Dec 17, 2017 - 11:37 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Slight end comment: I changed yout uint8_t to char. If the data over the UART line is "text data" then this is considered the most appropriate data type by some (including me). The int8_t and uint8_t data types are present for the cases where the data actually is integer numeric with the size of 8 bits (signed or unsigned) and calculations are to be done on the data.

 

Also, if you're not relying on any previous settings of the timer control registers then the advice is to prefer assignment (=) over or'ing in (|=). I'm sorry I didn't catch this until now - I just "nicked" your code and altered it for the discussion of passing two values out of it, but made no other alterations.

 

So, consider changing

    TIFR |= _BV(TOV0);
    TCCR0 |= _BV(WGM01) | _BV(CS02) | _BV(CS01) | _BV(CS00);

to

    TIFR = _BV(TOV0);
    TCCR0 = _BV(WGM01) | _BV(CS02) | _BV(CS01) | _BV(CS00);

or someone (perhaps you) will read the code in the future and wonder "Why is the bits or'ed in? Are there previously set bits that needs to be preserved?"

 

If the registers are all zero before this piece of code the effects will be the same of both code snippets, but it's not a matter of different behavior. It's all about being clear about intent.

 

My second snippet above essentially says "the register should have these bits set". The first snippet essentially says "in addition to any bits already set, these bits should also be set".

 

I suspect you intention is "the registers should have these bits set" and then your code should reflect this clearly, to help future readers of the code and to avoid misunderstanding and confusion.

 

HTH!

 

[EDITED: "bytes" -> "bits" in one place.]

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"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: Mon. Dec 18, 2017 - 08:16 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Updated the code, the data is not chars but numeric bytes so this is why I chose uint8_t in this case.

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

When using polled USART, I would have a function to test the presents of RXC0 and if true then go read the character...

rather then use a blocking read using wait with timeout..  This would only work if polling faster then incoming characters...

Glad you found a solution!

 

Jim

 

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

ki0bk wrote:

When using polled USART, I would have a function to test the presents of RXC0 and if true then go read the character...

rather then use a blocking read using wait with timeout..  This would only work if polling faster then incoming characters...

Glad you found a solution!

 

Jim

 

 

But isn't that what I am doing on the function in #5?

RCX0 is checked in a condition as long as the timer didn't overflow.

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

Just wanted to present another way to think about the "problem",  rather then call a function that blocks for some set period (timeout).

 

Have two functions, one tests if character has been received, kbhit(), and if so then go fetch character, getchar(). 

Using these two functions, main is free to do other things while waiting for character input.

Glad you got it working!

 

//------RS232-------------------
unsigned char kbhit(void){
//return nonzero if char waiting  polled version
unsigned char b;

  b=0;
  if(UCSRA & (1<<RXC)) b=1;
  return b;
}

//-----------------------
int getchar(void){
//polled version... 
unsigned char c;

  while(!kbhit()){}; //wait for char
  c=UDR;             //get char from usart
  return c;  
}

void main(void){
}
  init();
  while(1){
      if(kbhit()){
        c = getchar();
        // do something with char
      }
     // do something else while waiting
  }
  
}

 

Tuck that in your toolbag for later use....

 

Jim