Questions about Peter Fleury uart library

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

Hello,

 

I'm going through the 328P datasheet and want to learn about uart. As I've gone through I2C and SPI in basic aspects just to get the library running.

 

 

I've downloaded Peter Fleury uart library, after looking over and over into the library, maybe I have more than one question, but my current question is:

 

In the getc and the receive interrupt, he actually first increment the buffer index then he read or store data where I think first he should store or read them increment for the next operation.

 

So; for example, here are the most 4 important functions and ISRs for me in the Peter Fleury uart library:

 

ISR (UART0_RECEIVE_INTERRUPT)
/*************************************************************************
Function: UART Receive Complete interrupt
Purpose:  called when the UART has received a character
**************************************************************************/
{
    unsigned char tmphead;
    unsigned char data;
    unsigned char usr;
    unsigned char lastRxError;

    /* read UART status register and UART data register */
    usr  = UART0_STATUS;
    data = UART0_DATA;

    /* get FEn (Frame Error) DORn (Data OverRun) UPEn (USART Parity Error) bits */
#if defined(FE) && defined(DOR) && defined(UPE)
    lastRxError = usr & (_BV(FE)|_BV(DOR)|_BV(UPE) );
#elif defined(FE0) && defined(DOR0) && defined(UPE0)
    lastRxError = usr & (_BV(FE0)|_BV(DOR0)|_BV(UPE0) );
#elif defined(FE1) && defined(DOR1) && defined(UPE1)
    lastRxError = usr & (_BV(FE1)|_BV(DOR1)|_BV(UPE1) );
#elif defined(FE) && defined(DOR)
    lastRxError = usr & (_BV(FE)|_BV(DOR) );
#endif

    /* calculate buffer index */
    tmphead = ( UART_RxHead + 1) & UART_RX_BUFFER_MASK; <-------------------------------------------- #1

    if ( tmphead == UART_RxTail ) {
        /* error: receive buffer overflow */
        lastRxError = UART_BUFFER_OVERFLOW >> 8;
    }else{
        /* store new index */
        UART_RxHead = tmphead;
        /* store received data in buffer */
        UART_RxBuf[tmphead] = data; <---------------------------------------------------------------- receive data with the new index
    }
    UART_LastRxError |= lastRxError;
}

ISR (UART0_TRANSMIT_INTERRUPT)
/*************************************************************************
Function: UART Data Register Empty interrupt
Purpose:  called when the UART is ready to transmit the next byte
**************************************************************************/
{
    unsigned char tmptail;

    if ( UART_TxHead != UART_TxTail) {
        /* calculate and store new buffer index */
        tmptail = (UART_TxTail + 1) & UART_TX_BUFFER_MASK; <--------------------------------------------- #2
        UART_TxTail = tmptail;
        /* get one byte from buffer and write it to UART */
        UART0_DATA = UART_TxBuf[tmptail];  /* start transmission */ <------------------------------------ send data with the new index
    }else{
        /* tx buffer empty, disable UDRE interrupt */
        UART0_CONTROL &= ~_BV(UART0_UDRIE);
    }
}

/*************************************************************************
Function: uart_getc()
Purpose:  return byte from ringbuffer
Returns:  lower byte:  received byte from ringbuffer
          higher byte: last receive error
**************************************************************************/
unsigned int uart_getc(void)
{
    unsigned char tmptail;
    unsigned char data;
    unsigned char lastRxError;

    if ( UART_RxHead == UART_RxTail ) {
        return UART_NO_DATA;   /* no data available */
    }

    /* calculate buffer index */
    tmptail = (UART_RxTail + 1) & UART_RX_BUFFER_MASK; <---------------------------------------------- #3

    /* get data from receive buffer */
    data = UART_RxBuf[tmptail]; <--------------------------------------------------------------------- read data with the new index
    lastRxError = UART_LastRxError;

    /* store buffer index */
    UART_RxTail = tmptail; 

    UART_LastRxError = 0;
    return (lastRxError << 8) + data;

}/* uart_getc */

/*************************************************************************
Function: uart_putc()
Purpose:  write byte to ringbuffer for transmitting via UART
Input:    byte to be transmitted
Returns:  none
**************************************************************************/
void uart_putc(unsigned char data)
{
    unsigned char tmphead;

    tmphead  = (UART_TxHead + 1) & UART_TX_BUFFER_MASK; <--------------------------------------------- #4

    while ( tmphead == UART_TxTail ){
        ;/* wait for free space in buffer */
    }

    UART_TxBuf[tmphead] = data; <--------------------------------------------------------------------- put data in buffer for transmission
    UART_TxHead = tmphead;

    /* enable UDRE interrupt */
    UART0_CONTROL    |= _BV(UART0_UDRIE);

}/* uart_putc */

 

All the 4 functions and ISRs have increment routine first then get, put, write or receive data after.

 

For example, if the wanted data is in index 0; e.g. UART_RxBuf[0] and when I call the function for the first time, then the data is in index 0, and the function first increment the index so it's now 1.

 

So, you want UART_RxBuf[0] and you get UART_RxBuf[1] ! That's how I understood the function, can anyone explain why the author of the code first increment the index then put or receive data?

This topic has a solution.

Last Edited: Sat. May 26, 2018 - 10:42 PM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 2

Whenever you use a pointer you have the choice of pre-increment or post-increment. Which you pick doesn't really matter as long as the usage is consistent.

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

Rather than work backwards from he code, read up on fundamental data structures. The one here is a circular buffer.
https://en.m.wikipedia.org/wiki/Circular_buffer

Other data structures worth investigating are:queues,stacks,lists and trees.
Once you know the basics, you’ll be able to identify them by looking at the code -ooooh! This one’s a circular buffer!
The reasons for the increment etc will become perfectly clear.

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

clawson wrote:

Whenever you use a pointer you have the choice of pre-increment or post-increment. Which you pick doesn't really matter as long as the usage is consistent.

 

Yes, this is my point, I know the pre-increment, but my main concern is in the pre-increment I'd get UART_RxBuf[1] instead of UART_RxBuf[0] so isn't that a problem?

 

If it's a number or string them I may pick the wrong value. The first data came is is in index 0, that's my point how could I pre-increment and get data in index 1 ?

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

Kartman wrote:
Rather than work backwards from he code, read up on fundamental data structures. The one here is a circular buffer. https://en.m.wikipedia.org/wiki/... Other data structures worth investigating are:queues,stacks,lists and trees. Once you know the basics, you’ll be able to identify them by looking at the code -ooooh! This one’s a circular buffer! The reasons for the increment etc will become perfectly clear.

 

Yes I learned about circular buffers, what I need is a buffer and two counters or pointers for head and tail tracking.

Also thanks for telling me about other types of data structures.

 

For now, I'm not so focused on learning about data structures, I found this one in the code searched about it and found it's a circular buffer. I'm just curious why the author pre-increment the buffer?

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

If you understand the concept of circular buffers, then the code should be self evident. Get a pencil and paper to sketch the various possibilities.

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

Kartman wrote:
If you understand the concept of circular buffers, then the code should be self evident. Get a pencil and paper to sketch the various possibilities.

 

OK, I think I started to understand, oh how I missed this part! It doesn't matter where I start, as long as there're counters for head and tail, and put conditions for if head == tail, then that would be and overflow in case of putting new data for transmission or NO_DATA in case of getting data from the buffer to read.

 

Circular buffer - XX123XX with pointers.svg

and

Circular buffer - 6789AB5 with pointers.svg

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

in the pre-increment I'd get UART_RxBuf[1] instead of UART_RxBuf[0] so isn't that a problem?

 Why would it be a problem?  Perhaps, for the very first character, you skip the first location in the buffer.  But since the buffer is circular, it doesn't really have a beginning or an end - txbuf[0] gets filled in "last" instead of "first" (if the buffer becomes completely full.)

 

Edit: (oops  - it appears you figured that out on your own while my window was sitting idle.  Never mind.)

 

Last Edited: Mon. May 14, 2018 - 07:07 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

westfw wrote:
Perhaps, for the very first character, you skip the first location in the buffer. But since the buffer is circular, it doesn't really have a beginning or an end
+1

 

That is the point - you never access entry [N] in this buffer. You only add characters in and take characters out. Where they happen to be stored makes not difference to the user. All you hope is that it always acts as FIFO. And it will as long as the write and read pointers both employ a consistent pre or post increment. In fact for giggles you could have a circular buffer that went "backwards" so the pointers use decrements (and wraps) and not increments - from the "outside" you still would not be able to tell! So you may have written "Hello" into the buffer but it's really stored "olleH"

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

Of course I got the point, now I hope to get my code working properly :)

 

I'm developing the functions and the ISRs, really excited to apply ISRs!

 

I applied a test code for and ISR and put the code inside the ISR to blink LED with _delay_ms(50); but the behavior is strange. Instead of one ON/OFF blink, it blinks like 4 times for one received character! Why is that?

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

wolfrose wrote:
I applied a test code for and ISR and put the code inside the ISR to blink LED with _delay_ms(50);
Never delay in ISRs. A Delay_ms() is just a software counting loop - it really will hold it in the ISR (with interrupts disabled) for all that time. If you want to monitor ISR operations have it set some kind of signal flag to the loop in main() and then have that act upon the flag (where interrupts are now re-enabled).

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

50 ms is a huge time for an ISR (and, at a low speed of 9600 bauds = about 1 ms per char; this is a lowspeed, as "you" can transmit at a 115200 bauds  rate without issue).Even if I cannot explain the behavior, next behavior will be strange . (typically, receiving ISR put received data into a (circular) buffer : { array adressing, pointer (in|de) crementing and -truncating- } : this makes very few cycles, not 50 * 16 *1000 cycles , and main tries to cope with them .

 

Edited : redundant with Clawson's

Edited again : if you want to know whether reception works, you might

a) (with preexisting code) send back what was received (maybe beginning by sending always the same char, then sending a given string would be comfortable);

maybe sending back  its hexa decimal code of each received char (converted into chars) whould be interesting (sometimes PC terminals add extra chars to \n terminated strings )

b) if you like LEDs, a global volatile variable should be set in the ISR; if this variable is set,  main does what you want (put a LED on): if this variable is not set, main should put the LED off;

main should always reset the global volatile variable.

Last Edited: Mon. May 14, 2018 - 04:57 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 2

Sounds like someone needs to read my tutorials on ‘the traps when using interrupts’ and ‘multi-tasking’

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

dbrion0606 wrote:

50 ms is a huge time for an ISR (and, at a low speed of 9600 bauds = about 1 ms per char; this is a lowspeed, as "you" can transmit at a 115200 bauds  rate without issue).

9600 is ok for now as I'm testing the code.

 

Quote:

(typically, receiving ISR put received data into a (circular) buffer : { array adressing, pointer (in|de) crementing and -truncating- }

All these parts are clear, but why "truncating" is important, isn't that a loss in the data, maybe you may loose important data.

 

Quote:

a) (with preexisting code) send back what was received (maybe beginning by sending always the same char, then sending a given string would be comfortable);

maybe sending back  its hexa decimal code of each received char (converted into chars) whould be interesting (sometimes PC terminals add extra chars to \n terminated strings )

I applied these basic functions and got the data on the Arduino IDE serial monitor. Now I want to get into ISRs.

 

void uart_tx(uint8_t data)
{
   //Wait for empty transmit buffer
   while ( !( UCSR0A & (1<<UDRE0)) );
   //Put data into buffer, sends the data
   UDR0 = data;
}

uint8_t uart_rx(void)
{
   /* Wait for data to be received */
   while ( !(UCSR0A & (1<<RXC0)) );
   /* Get and return received data from buffer */
   return UDR0;
}

void tx_str(uint8_t *str, uint8_t str_len)
{
   while(*str)
       tx(*str++);
}	

 

Quote:

b) if you like LEDs, a global volatile variable should be set in the ISR; if this variable is set,  main does what you want (put a LED on): if this variable is not set, main should put the LED off;

main should always reset the global volatile variable.

 

OK, but what does the ISR exactly do? Didn't understand so much.

 

Kartman wrote:
Sounds like someone needs to read my tutorials on ‘the traps when using interrupts’ and ‘multi-tasking’

 

Yes, indeed :)

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

If I were you I'd stick with synchronous transmission - just implement asynchronous reception first. Usually when you send you don't mind waiting for "hello" or whatever to head off down the line but it's the reception you cannot plan for as you don't know when the other end might start sending you characters and you want the code to respond instantly when it does.

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

I have a question about a line in:

void uart_putc(unsigned char data)

Last function in the main post

 

    while ( tmphead == UART_TxTail ){
        ;/* wait for free space in buffer */
    }

OK, so this while is looping forever until there's a free space.

 

My question, does while loop blocks for functions while the condition is true?

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

wolfrose wrote:
My question, does while loop blocks for functions while the condition is true?
while() loops in C always loop while the condition is true. That is the designed intention of while().

 

In the above code you have two pointers into the wring buffer. He calls them "head" and "tail". Personally I would usually tend to call them the "write" and "read" pointers. The condition his code is guarding against is then the place to write has caught up with the place to read - that implies the buffer is full. So it has to wait until the transmission logic (which is happening in the TX interrupt) has taken one or more characters from the buffer and sent them off down the line. This will cause the "read" pointer to move on so it will no longer be at the location where this code want to write the next one you are trying to putc(). So the while loop simply waits until "tail" has moved, then it will be safe to add in the character that the user has just asked to send.

 

BTW another way to do "ring buffers" (aka FIFOs) such as are being used here is with both the read/write pointers but also a count of how many characters are in the buffer waiting to be used. Then the check becomes the much more obvious:

while(countUsed == BUFFER_CAPACITY);

this then waits until one or more characters are sent. The countUsed is decremented and it is then no longer equal to CAPACITY (ie "full buffer").

 

BTW as long as you set the buffers to sensible size it's hopefully the case that you don't often spend too much time in "while buffer is full" waiting loops as the buffer should seldom be full anyway.

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

My question, does while loop blocks for functions while the condition is true?

 

As it is very important to send, every other operation (except interrupts, which can clear the buffer if they are it is a sending intrrupt) is suspended.

Edited : this works if sending/transmitting interrupt is enabled (Clawson advises not to use transmitting interrupt , as one is free to choose when one transmits) and empty (at least, make it less full)  the transmitting buffer.

 

"OK, but what does the ISR exactly do? Didn't understand so much."

 

The ISR either:

puts the received char into a buffer, checks and updates the buffer pointer(s) (maybe 20 cycles).

or

allows main to trigger a LED  -maybe 5 cycles- (with a volatile byte/uint8_t variable -Clawson's hint- main checks -and resets- when -s-he/it has time) 

or

play with a state machine, according to the value of the received char (but main can play with it)

or

echo back the character (but it is ugly : main can do it; convert char ....)

 

In both cases, ISR begins by saving registers (eats cycles) and ends by restoring registers (eat other cycles). -maybe 100 cycles-

 Then if you call a function from the ISR, it saves/restores registers, too -unless it is inlined- -many cycles-

 

If this function is a delay, it eats ... more than 500 000 cycles... for 50 ms  (hence people are terrified) ... and ISR are disabled (at 9600 bauds, you will miss 80% of the characters a PC sends continuously)

 

I agree with you I should reread Kartmans tutorials ... but tutorial section is very difficult to browse...

If you want to play with UARTs, abcminiusers tutorial is/are -a series of tuttorials-  interesting, too (explains how to have a interrupt less software, and how to make it evolve to an ISR driven software)

 

 

 

Last Edited: Tue. May 15, 2018 - 09:48 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dbrion0606 wrote:
"OK, but what does the ISR exactly do? Didn't understand so much."   The interrupts either: puts the received char into a buffer, checks and updates the buffer pointer(s) (maybe 20 cycycles). or allows main to trigger a LED  -maybe 5 cycles- (with a volatile byte/uint8_t variable -Clawson hint- main checks -and resets- when -s-he/it has time)  or play with a state machine, according to the value of the received char (but main can play with it) or echo back the character (but it is ugly : main can do it; convert char ....)

 

By my question I meant the plane B you suggested in the earlier reply about setting an LED ON/OFF with interrupt and the main. I didn't understand the actual procedures of your plane, what is the procedure you suggested? What does and ISR do and what's the main do, also what's the benefit of defining the variable as volatile in the main?

 

Thank you for support,

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

Your enterrups allows main to handle LED ( volatile uint8_t )

enableLED = 1; then returns. (or fills a buffer)

main

checks for enableLED

if ( enableLED == 1) {

  // lights leds

  // delays50ms

} else {// clear LED;}

enableLED = 0; // this will blink led for at least 50 ms... blinks cannot be consistent with the number of received chars

 

 

"what's the benefit of defining the variable as volatile in the main?"

 

Whell enbleLED should be defined in the main, but shared by main and ISR  (else, how can main know a char has been received?) and optimiser should not wipe it out (hence volatile), leading to very weird and most unpleasant, unexpected  results (one verifies it works with a pencil and paper, and optimiser removes one variable...).

 

Edited :

I bet Fleury's variables
UART_RxHead, UART_LastRxError,  UART_TxHead and UART_TxTail
are declared as volatile and are global (defined before main and ISR)

 

 

Last Edited: Tue. May 15, 2018 - 10:09 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have another question!

 

Is it important to declare a tmp variable in each function and ISR?

 

For example, in Peter Fleury, the programmer is writing the code as the following:

 

tmptail = (UART_RxTail + 1) & UART_RX_BUFFER_MASK;

 

What if I write it like this:

 

UART_RxTail = (UART_RxTail + 1) & UART_RX_BUFFER_MASK;

 

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

dbrion0606 wrote:

Your enterrups allows main to handle LED ( volatile uint8_t )

enableLED = 1; then returns. (or fills a buffer)

main

checks for enableLED

if ( enableLED == 1) {

  // lights leds

  // delays50ms

} else {// clear LED;}

enableLED = 0; // this will blink led for at least 50 ms... blinks cannot be consistent with the number of received chars

 

 

OK, I understand now, you mean I include the LED SET/RESET with the receiving byte ISR. Yes, that's really smart :)

 

Thank you,

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

wolfrose wrote:
Is it important to declare a tmp variable in each function and ISR?
The reason he does that is that even while the uart_putc() (or getc) is operating there may be some UART interrupts "in the background". So the position of Head/Tail could be changed even as the function is operating. So he takes a "snapshot" of their position before then going on to use it. In fact he comes close to a concept you will need to know about later: atomicity. If the Head/Tail indexes to the array had been any wider than "char" then he would have needed "atomic protection" to read them to ensure that they didn't get update right in the middle of the variable being read. But his code is making use of a feature of single byte variables that the single opcode that will read the value cannot, itself be interrupted. So no atomic protection is required. But as you get further into the use of interrupts this is an area you are going to need to know about. If the interrupt code might access anything "in the foreground" and that item is a type that is larger than 1 byte wide then you will need to give the access atomic protection. The C compiler comes with this: https://www.nongnu.org/avr-libc/... for that very reason.

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

" I understand now, you mean I include the LED SET/RESET with the receiving byte ISR. "

ISR just allows main to set LED (by setting a shared variable)

main checks this shared variable when it has time (has millions of cycles to do : is very busy), if allowed  needed, sets the LED for 50 ms, then clears it, and, anyway and always, clears the same shared variable (it would be smarter if interrupts were disabled during the clearing of the shared variable, then reenabled  -maybe 5 cycles and 1 byte- : in this case, is not relevant-harmless anyway-  but seems a good habit : edited this habit is consistent with Clawson hint of atomicity).

Last Edited: Tue. May 15, 2018 - 10:36 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You do need the temporary variable. Without it, you'd be changing UART_RxTail before reading the member of the array, meaning that the other side of the logic could overwrite the character with a new one. The relationship between head and tail has to be preserved, so you should never change UART_RxTail until after you've read the character you want out of the array.

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

the_real_seebs wrote:
you'd be changing UART_RxTail before reading the member of the array,

Actually, in the code the programmer, increments the indexes before reading or writing to the buffers, but the members here told me since it's a ring buffer it doesn't matter where you start or stop. And also with this understanding I think it should be OK as long as the position of incrementing is the same for writing or reading in all the functions and ISRs.

 

But, I didn't understand the complete procedure you explained to me, could you explain more, also notice in the main post the increments in all the functions and ISRs are before the action of reading and writing to buffers.

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

Suggest you dry run a "thought experiment" or, in this case a paper and pencil experiment. Draw out a "buffer" on paper and head and tail pointers (easily rubbed out and redrawn with an eraser) and consider all the scenarios of foreground calls to getc/putc and then asynchronous interrupts occurring at any point around them and watch what happens to the head/tail pointers.

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

clawson wrote:
Suggest you dry run a "thought experiment"

Turn your though experiment into a programming experiment.

Write (copy) a simple command line program for a circular buffer, and print it's contents on 1 line, together with the head / tail pointers (array indices are simpler for humans)

Write some code to add to and read from the buffer, and watch carefully what the Head and tail does.

Often the buffer is defined as "empty" if Head & Tail are the same.

In other implementations the buffer is full if Head & Tail are the same.

Some implemantations always leave some room in the buffer empty, others can use each byte for storage.

The amount of data in the buffer is usually calculated from the relative distance of Head & Tail.

 

There are a few difficult situations to carefully handle if the write code can be interrupted by the read code, or vise versa.

This requires carefull management of pre or post increment.

For example, what will be the result (calc items in buf) if that function is called halfway during a write to (or read from) the buffer?

What happens if you try to write to a full buffer?

What happens if you try to read from an empty buffer?

 

It is quite task to get all these nitty bits in the right place.

Paul van der Hoeven.
Bunch of old projects with AVR's:
http://www.hoevendesign.com

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

For ring buffering I'd be tempted to use this:

 

http://www.fourwalledcubicle.com...

 

Sure you can use Fleury and his own buffering but if you do that you don't actually learn much (just learn how to use someone else's library code) whereas if you only "buy in" the buffering itself you could make your own implementation of all the reset of the UART code/interrupts around it and learn more in the process.

 

I suppose the ultimate is not to use anyone else's code at all. Write it all from scratch, including the ring buffering and then you learn (and keep control) of every part of the solution.

 

Personally I like Dean's lightweight buffering because it is so light weight (and hence pretty easy to understand/follow) and he makes the sensible choice (in my opinion) of adding a count to the buffer. That way you aren't faced with the quandry of when head=tail of whether it is saying totally full or totally empty.

 

PS and yes Dean's code is nothing but a .h file - there is no .c/.cpp, it's all in that 40 line header

Last Edited: Wed. May 16, 2018 - 09:00 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

"Write (copy) a simple command line program for a circular buffer,"

Well, does OP write command line programs on a PC (he should, but...)

Is it simple to have a command line program being interrupted and sharing variables with the interrupting thread on a PC (I am afraid it is very complicated).

Simplest solution is to figure out (I used my head; a sheet of paper, pencil and eraser might be best) what happens if an interrupt (needing 

UART_RxTail

to know about overflows ) occurs in this piece of code

 /* calculate buffer index */
    tmptail = ( UARTRxTail + 1) & UART_RX_BUFFER_MASK; <---------------------------------------------- #3

    /* get data from receive buffer */
    data = UART_RxBuf[tmptail]; <----------

or between

UART_RxTail ++; // for simplicity, I forgot the mask, which makes things wotse
// here, interrupts can wrongly detect an overflow (leading to spurious char loss)
data= UART_Rxuf[UART_RxTail];

 

 

Last Edited: Wed. May 16, 2018 - 10:18 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

For ring buffering I'd be tempted to use this:

 

http://www.fourwalledcubicle.com...

 

Sure you can use Fleury and his own buffering but if you do that you don't actually learn much (just learn how to use someone else's library code) whereas if you only "buy in" the buffering itself you could make your own implementation of all the reset of the UART code/interrupts around it and learn more in the process.

 

I suppose the ultimate is not to use anyone else's code at all. Write it all from scratch, including the ring buffering and then you learn (and keep control) of every part of the solution.

 

Personally I like Dean's lightweight buffering because it is so light weight (and hence pretty easy to understand/follow) and he makes the sensible choice (in my opinion) of adding a count to the buffer. That way you aren't faced with the quandry of when head=tail of whether it is saying totally full or totally empty.

 

PS and yes Dean's code is nothing but a .h file - there is no .c/.cpp, it's all in that 40 line header

 

Absolutely you're right. In the last couple days, I tried to copy Peter Fleury's library, and it didn't work, it seems I have a problem with the UDRIE0! I don't know when I upload the code the TX LED is just ON all the time, RX ISR I think is working well.

 

Then, I started a new library and wrote the code without ring buffering.

 

This is my code, of course it's not working. But what the problems here?

 

#include <avr/io.h>
#include <Arduino.h>
#include "uart_new.h"
uint8_t rx_buf[32],tx_buf[32];
uint8_t rx_p,tx_p;
uint8_t rx_errors,tx_sBuf,rx_sBuf;

void uart_init_new(uint16_t ubrr)
{
    rx_p=0,tx_p=0;
    UBRR0H = (uint8_t)(ubrr>>8);
    UBRR0L = (uint8_t)ubrr;
    UCSR0B = (1<<RXEN0)|(1<<TXEN0)|(1<<RXCIE0); //Enable Rx and Tx & Receive interrupt
    UCSR0C = (1<<USBS0)|(3<<UCSZ00);            //Set frame format: 8data, 2stop bit
}

uint16_t get_char(void)
{
    if (rx_p == 0)
        return NO_DAT;
    else
        return rx_buf[rx_p--];
}

void put_char(uint8_t data)
{
    if (tx_p < 32)
        tx_buf[tx_p++] = data;
    UCSR0B |= (1<<UDRIE0);
}

void put_s(uint8_t *str)
{
    while (*str)
        put_char(*str++);
}

ISR(USART_RX_vect)
{
    if (rx_p < 32)
        rx_buf[rx_p++]=UDR0;
    else
        rx_errors = Buf_OF >> 8;
}

ISR(USART_UDRE_vect)
{
    if (tx_p == 0)
        UCSR0B &= ~(1<<UDRIE0);
    else
        UDR0 = tx_buf[tx_p--];
}

 

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

Where's the rest of the code? You're falling for the trap of thinking the error is in the code you don't understand.

 

Have you checked the basic usart functions work - ie without interrupts? This would rule out hardware and basic initialisation problems.

 

I note you've not read the tutorials I pointed you towards. This may have helped you avoid issues like not declaring shared variables 'volatile'

Last Edited: Sun. May 20, 2018 - 12:35 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1
        tx_buf[tx_p++] = data;
...
        UDR0 = tx_buf[tx_p--];

So it is a LIFO (or Stack).

Since you are dealing with characters and strings I somehow doubt that that was your intention.

Stefan Ernst

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

Kartman wrote:

Where's the rest of the code? You're falling for the trap of thinking the error is in the code you don't understand.

 

Have you checked the basic usart functions work - ie without interrupts? This would rule out hardware and basic initialisation problems.

 

I note you've not read the tutorials I pointed you towards. This may have helped you avoid issues like not declared shared variables 'volatile'

 

Yes I copied them from the 328P datasheet, they worked well, but I faced a problem with the buffers, as I read them in the main function as for loops, in the following:

 

#include <avr/io.h>
#include <Arduino.h>
#include "uart_basic.h"

void uart_init_basic(uint8_t baud_rate)
{
    /*Set baud rate */
    UBRR0H = (uint8_t)(baud_rate>>8);
    UBRR0L = (uint8_t)baud_rate;
    UCSR0B = (1<<RXEN0)|(1<<TXEN0);
    //Set frame format: 8data, 2stop bit
    UCSR0C = (1<<USBS0)|(3<<UCSZ00);
}

void uart_tx_basic(uint8_t data)
{
   //Wait for empty transmit buffer
   while ( !( UCSR0A & (1<<UDRE0)) );
   //Put data into buffer, sends the data
   UDR0 = data;
}

uint8_t uart_rx_basic(void)
{
   /* Wait for data to be received */
   while ( !(UCSR0A & (1<<RXC0)) );
   /* Get and return received data from buffer */
   return UDR0;
}

void tx_str_basic(uint8_t *str, uint8_t str_len)
{
   while(*str)
       uart_tx_basic(*str++);
}

 

And this is my application code in Arduino IDE:

 

#include "uart_basic.h"

#define rx_bs 10
uint8_t rx_buffer[rx_bs];
void setup() {
  uart_init_basic(103);

}

void loop() {
  for (uint8_t k=0;k<rx_bs;k++)
  {
    rx_buffer[k]=uart_rx_basic();
    k++;
  }

  for (uint8_t i=0;i<rx_bs;i++)
  uart_tx_basic(rx_buffer[i]);
  uart_tx_basic('\n');
  
}

I applied this code with my Bluetooth module, and I sent and received data on my Android phone with S2 Terminal for Bluetooth.

 

Then I moved to ISRs and it's not so difficult, I just have to manage the sent and received that and most importantly manage the buffers.

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

sternst wrote:

        tx_buf[tx_p++] = data;
...
        UDR0 = tx_buf[tx_p--];

So it is a LIFO (or Stack).

Since you are dealing with characters and strings I somehow doubt that that was your intention.

 

WOW, I really missed that! You're right I'm not implementing a FIFO system.

 

Seems that how important are ring buffers, now I know that! Thanks fir the help.

 

 

How, about this modification? I added buffers' counters to count the incoming or outgoing data.

 

#include <avr/io.h>
#include <Arduino.h>
#include "uart_new.h"

static uint8_t rx_buf[32],tx_buf[32];
static uint8_t rx_p,tx_p,rx_cnt,tx_cnt;
static uint8_t rx_errors,tx_sBuf,rx_sBuf;

void uart_init_new(uint16_t ubrr)
{
    rx_p=0,tx_p=0,rx_cnt=0,tx_cnt=0;
    UBRR0H = (uint8_t)(ubrr>>8);
    UBRR0L = (uint8_t)ubrr;
    UCSR0B = (1<<RXEN0)|(1<<TXEN0)|(1<<RXCIE0); //Enable Rx and Tx & Receive interrupt
    UCSR0C = (1<<USBS0)|(3<<UCSZ00);            //Set frame format: 8data, 2stop bit
}

uint16_t get_char(void)
{
    if (rx_cnt == 0)
        return NO_DAT;
    else
    {
        if (rx_p < rx_cnt)
        return rx_buf[rx_p++];
    }
}

void put_char(uint8_t data)
{
    if (tx_cnt < 32)                        // start count the input data
        tx_buf[tx_cnt++] = data;            // 1st input location tx_cnt
    else
        return Buf_OF;

    UCSR0B |= (1<<UDRIE0);
}

void put_s(uint8_t *str)
{
    while (*str)
        put_char(*str++);
}

ISR(USART_RX_vect)
{
    if (rx_cnt < 32)                        // start count the received data
        rx_buf[rx_cnt++]=UDR0;              // 1st output location rx_cnt
    else
        rx_errors = Buf_OF >> 8;
}

ISR(USART_UDRE_vect)
{
    if (tx_cnt == 0)                        // haven't put any data
        UCSR0B &= ~(1<<UDRIE0);
    else
    {
        if (tx_p < tx_cnt)                  // check if pointer < counted data
        UDR0 = tx_buf[tx_p++];
    }
}

 

Last Edited: Sun. May 20, 2018 - 05:53 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 2

What does your code do after you’ve sent 32 chars? You’ve got no mechanism to reset tx_p and tx_cnt. In terms of a design choice, if you want a general solution, then the circular buffer is the one. However, if you want to send/receive blocks of data like you would if you were doing a protocol like modbus, then the simple buffer approach would be the choice.

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

You can have two kinds of counters :

the number of chars that are avalaible in the RX / Tx buffer (is limited) : this gives an info about whether a buffer is empty/full.

 

the number of chars "you" recieved / transmitted since power on (should be large : uint16_t or uint32_t might be interesting.

 

Anyway, you should be aware that the way you declare these counters do not protect them from optimisation (as they are not declared as volatile, optimiser migh decide opertaions on these variables are useless in µISR, say, making code .... faster.

 

If you want to know how many chars have been sent since power on, code updating or reading this number should be protected:

if this counter is increased in an ISR, suppose you aklready received 255 chars; if main copies this number with and begins with LSB,  mains LSB will become ...255. If an ISR increases this counter, before MSB is copied, MSB will become 1 (and LSB 0, but it is already copied) .... and main will "think" 0x01FF == 511 chars have been received. Things are likely to be worse if complex calculations (instead of copies) occur...

 

Such an horrible behavior seldom occurs and is difficult to detect (fastest way is to figure out what will happen if a ISR is triggered ... at each line of main, with one's brain, or with a pencil, a sheet of paper and an eraser) ...

 

Solution is to make code accessing shared variables uninterruptible/unsplittable (in greek atomic)  : avr's ISR already are (hope I do not make a mistake) ; remain main thread... Hence atomic blocks.... (edited : reread post 23)

 

Last Edited: Tue. May 22, 2018 - 09:50 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
What does your code do after you’ve sent 32 chars? You’ve got no mechanism to reset tx_p and tx_cnt. In terms of a design choice, if you want a general solution, then the circular buffer is the one. However, if you want to send/receive blocks of data like you would if you were doing a protocol like modbus, then the simple buffer approach would be the choice.

 

OK, the code now works as I didn't yet change the variables to volatile, I want to observe the effect of volatile on the system, but the problem now is that after receiving and displaying the received chars, the put_char function keeps transmitting the last received char.

 

This is my latest code:

 

#include <avr/io.h>
#include <Arduino.h>
#include "uart_new.h"

/*******************************************
If you want to prevent reading a string
backwards then you need FIFO implementation
*******************************************/

static uint8_t rx_buf[32],tx_buf[32];
static uint8_t rx_p,tx_p,rx_cnt,tx_cnt;
static uint8_t rx_errors,tx_sBuf,rx_sBuf;

void uart_init_new(uint16_t ubrr)
{
    rx_p=0,tx_p=0,rx_cnt=0,tx_cnt=0;
    UBRR0H = (uint8_t)(ubrr>>8);
    UBRR0L = (uint8_t)ubrr;
    UCSR0B = (1<<RXEN0)|(1<<TXEN0)|(1<<RXCIE0); //Enable Rx and Tx & Receive interrupt
    UCSR0C = (1<<USBS0)|(3<<UCSZ00);            //Set frame format: 8data, 2stop bit
}

uint16_t get_char(void)
{
    if (rx_cnt == 0)
        return NO_DAT;
    else if (rx_p < rx_cnt)
        return rx_buf[rx_p++];
}

void put_char(uint8_t data)
{
    if (tx_cnt < 32)                        // start count the input data
        tx_buf[tx_cnt++] = data;            // 1st input location tx_cnt
    UCSR0B |= (1<<UDRIE0);
}

void put_s(uint8_t *str)
{
    while (*str)
        put_char(*str++);
}

ISR(USART_RX_vect)
{
    if (rx_cnt < 32)                        // start count the received data
        rx_buf[rx_cnt++]=UDR0;              // 1st output location rx_cnt
    else
        rx_cnt = 0;                         // when the counter reached max value, reset it
}                                           // => just overwrite everything, maybe not very optimized way

ISR(USART_UDRE_vect)
{
    if (tx_p < tx_cnt)                      // check if pointer < counted data
        UDR0 = tx_buf[tx_p++];
        else
        {
            tx_p = 0;                       // when the pointer reach max value, reset it
            tx_cnt= 0;                      // 1. when i comment this line, it send all tx_buf content but doesn't send new input chars
                                            // 2. when enabled it sends only one char but capable of receiving new chars
            UCSR0B &= ~(1<<UDRIE0);         // disable empty data register interrupt
        }
}

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

I mentioned earlier why your method was a bad choice. I’ll repeat it to refresh your memory - use a circular buffer if you want to do single characters. Use a linear buffer if you want to send a block of characters. Why? Work it out on paper.

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

dbrion0606 wrote:

You can have two kinds of counters :

the number of chars that are avalaible in the RX / Tx buffer (is limited) : this gives an info about whether a buffer is empty/full.

 

the number of chars "you" recieved / transmitted since power on (should be large : uint16_t or uint32_t might be interesting.

 

Anyway, you should be aware that the way you declare these counters do not protect them from optimisation (as they are not declared as volatile, optimiser migh decide opertaions on these variables are useless in µISR, say, making code .... faster.

 

If you want to know how many chars have been sent since power on, code updating or reading this number should be protected:

if this counter is increased in an ISR, suppose you aklready received 255 chars; if main copies this number with and begins with LSB,  mains LSB will become ...255. If an ISR increases this counter, before MSB is copied, MSB will become 1 (and LSB 0, but it is already copied) .... and main will "think" 0x01FF == 511 chars have been received. Things are likely to be worse if complex calculations (instead of copies) occur...

 

Such an horrible behavior seldom occurs and is difficult to detect (fastest way is to figure out what will happen if a ISR is triggered ... at each line of main, with one's brain, or with a pencil, a sheet of paper and an eraser) ...

 

Solution is to make code accessing shared variables uninterruptible/unsplittable (in greek atomic)  : avr's ISR already are (hope I do not make a mistake) ; remain main thread... Hence atomic blocks.... (edited : reread post 23)

 

 

I think that what I'm doing, maybe not perfectly. I know the ring buffer is an excellent solution, but here I want to do my best to improve the linear method I implemented, check my last code, there are counters and reset routines but I have now a little problem that the microcontroller is locking on sending the last char.

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

OK, solved the problem in linear implementation, I just had to reset the counter and pointer in get_char function.

 

#include <avr/io.h>
#include <Arduino.h>
#include "uart_new.h"

static volatile uint8_t rx_buf[32],tx_buf[32];
static volatile uint8_t rx_p,tx_p,rx_cnt,tx_cnt;
static volatile uint8_t rx_errors,tx_sBuf,rx_sBuf;

void uart_init_new(uint16_t ubrr)
{
    rx_p=0,tx_p=0,rx_cnt=0,tx_cnt=0;
    UBRR0H = (uint8_t)(ubrr>>8);
    UBRR0L = (uint8_t)ubrr;
    UCSR0B = (1<<RXEN0)|(1<<TXEN0)|(1<<RXCIE0); //Enable Rx and Tx & Receive interrupt
    UCSR0C = (1<<USBS0)|(3<<UCSZ00);            //Set frame format: 8data, 2stop bit
}

uint16_t get_char(void)
{
    if (rx_cnt == 0)
        return NO_DATA;
    else if (rx_p < rx_cnt)
        return rx_buf[rx_p++];
        else
        {
            rx_cnt = 0;         <<=============== here is the addition which solved the problem :)
            rx_p = 0;           <<=============== and here
        }
}

ISR(USART_RX_vect)
{
    if (rx_cnt < 32)                        // start count the received data
        rx_buf[rx_cnt++]=UDR0;              // 1st output location rx_cnt
    else
    {
        rx_cnt = 0; // when the counter reached max value, reset it, overwrite everything
        rx_p = 0;
    }
}

void put_char(uint8_t data)
{
    if (tx_cnt < 32)                        // start count the input data
    {
        tx_buf[tx_cnt++] = data;            // 1st input location tx_cnt
    }

    UCSR0B |= (1<<UDRIE0);
}

ISR(USART_UDRE_vect)
{
    if (tx_p < tx_cnt)                      // check if pointer < counted data
        UDR0 = tx_buf[tx_p++];
        else
        {
            tx_p = 0;                       // when the pointer reach max value, reset it
            tx_cnt= 0;                      // also reset counter for new transmission
            UCSR0B &= ~(1<<UDRIE0);         // disable empty data register interrupt
        }
}

void put_s(uint8_t *str)
{
    while (*str)
        put_char(*str++);
}

But it needs some improvement and tweaks, because it lags and not transfer all the chars correctly!

 

Thank you all,

Last Edited: Fri. May 25, 2018 - 12:51 PM