Classical circular buffer serial code - not working..??

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

I am implementing ( actually porting a working PIC code ) MODBUS RTU slave on Mega88 . I am using the Imagecraft compiler ICC7 , with all optimisations enabled .
The code is attached alongwith . The query is sent as : 0x01 , 0x03 , 0x0b , 0xb7 , 0x00 , 0x02 , CRC1 and CRC2.
What is happening is when a typical MODBUS query # 3 is sent , we are not proceeding beyond the dbgflag value of 2 . dbgflag is incidently used by me to tell me where my code is reaching ( or not reaching ) . ( Pls. do not ask why I am not using JTAG , that is another problem that my JTAG is not working with Mega88...)
Now , when I comment the line marked in the code boldly as "PROBLEM LINE"... the dbgflag value reaches 5 , as it should .
The question is : What is wrong here , which i am not able to see ? The supposedly problematic line is actually needed , and just cannot be removed.
Is my ISR correctly implemented ? Is the circular buffer OK ?
Please help ASAP , as after 5 days of debugging with only an LCD , I am ready to break a few things ! :cry:

Attachment(s): 

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

trinity_parag wrote:
that is another problem that my JTAG is not working with Mega88...

That's actually the problem I'd concentrate on fixing. I take it you are talking about either an Atmel JTAGICEmkII or an Atmel Dragon as they are the only two devices you can do OCD on a Mega88 with as they are the only interfaces to support DebugWire.

Cliff

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

I would recode that mess of if else and incorrectly indendented code. Use a switch statement. Get rid of all those if/elses - where there is probably a mistake in the nesting.

As is, the code
if(!stx_received)

followed by a bunch of checks for non-zero value of that same variable is terrible form, bound to break one's brain.

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

Is my ISR correctly implemented ? Is the circular buffer OK ?

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

this...
while(read_ptr!=write_ptr)

has a race condition since the multibyte variable access isn't atomic with the ISR.

This of course will be a rare timing situation.

but your main problem is likely a logic error obfuscated by the poorly structured, uncommented code.

if the buffer size is < 256 you can use a one-byte array index.

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

Is 'meter_add' initialized to some value anywhere? If not, it will have value 0x00. In the sequence you show us, after the 0x00 a 0x02 is received. The comparison with 0x03 will always be false then.
I suppose meter_add should have value 0x01. The next byte received is the command value 0x03. Everything should really work then.

Regards,
Jörg.

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

Thanks all for the responses .
Meter address is initialised initally ( read from the EEPROM , as last saved ). While testing , it was 0x01. The first byte of a valid MODBUS query is the meter address , followed by the 0x03 indicating the query type. Only if the meter address match is followed by a 0X03 do we want to even take in the next six bytes and compare the CRC . If not , we simply want to go back to checking for the next meter address match .

Quote:
while(read_ptr!=write_ptr) has a race condition since the multibyte variable access isn't atomic with the ISR.

I do not quite get it . Even if relevant variables have been declared volatile ? Could you please explain a bit more ?
In any case , the problem is persistent , and not once in a while , as would be the case of non atomic race away..?
Is my ISR OK ?
The circular buffer is <256 bytes , and we have tried :
a. instead if pointers , using character indexes into the buffer e.g.
ISR :
cir_buffer[write_count++]=UDR;
if (write_count==250) write_count=0;
// and in the main loop...
while(read_count != write_count){
com_char=cir_buffer[read_count++];
if (read_count==250) read_count=0;
// and so on further..

we have also tried replacing all the if-elses you found repugnunt with a switch-case... still no go..
}

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

Could this be a compiler issue ?

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

trinity_parag wrote:

Quote:
while(read_ptr!=write_ptr) has a race condition since the multibyte variable access isn't atomic with the ISR.

I do not quite get it . Even if relevant variables have been declared volatile ? Could you please explain a bit more ?

For those 16 bit pointers then the AVR has to hold them in two registers. To access read_ptr and put it into R16/R17 it might do:

LDS R16,read_pntr
LDS R17,read_pntr+1

Suppose an interrupt occurs between those two instructions and during that interrupt the contents of read_pntr happen to roll over from 0xFF to 0x100. R16 already has 0xFF in it, on return from interrupt R17 will then pick up 0x01 so R16:R17 now hold 0x1FF and not 0x100 as they should. You need to do the test in such a way that you can wrap cli() and sei()s around it to avoid this.

Cliff

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

Here is the implementation of a queue with the character buffer, rear, and front pointer stored in a single structure. Feel free to use it. Define MAX_BUF_SIZE accordingly. Also, be sure to follow the advice of stevech and clawson regarding atomic access if you are accessing the queue from an ISR.

Tom

/*************************************************************
 *  queue.h
 *************************************************************
 */

#ifndef __QUEUE_H_
#define __QUEUE_H_

#include 

#ifndef TRUE
#define TRUE 1
#endif

#ifndef FALSE
#define FALSE 0
#endif

#define MAX_BUF_SIZE 32

typedef struct queue_s {
     uint8_t buffer[MAX_BUF_SIZE + 1];
     uint8_t rear;
     uint8_t front;
} queue_t;

int byteQueueIsEmpty(queue_t *p);
int byteQueueIsFull(queue_t *p);
void byteQueueInit(queue_t *p);
int byteQueueInsert(queue_t *p, uint8_t byte);
uint8_t byteQueueRemove(queue_t *p);
uint8_t byteQueueFront(queue_t *p);
int byteQueueDelete(queue_t *p);

#endif
/*************************************************************
 *  queue.c
 *************************************************************
 */

#include "queue.h"

/*************************************************************
 * byteQueueIsEmpty()
 *
 * Return TRUE if the specified queue is empty.
 *
 *************************************************************
 */
int byteQueueIsEmpty(queue_t *p)
{
     return (p->rear + 1) % (MAX_BUF_SIZE + 1) == p->front;
}

/*************************************************************
 * byteQueueIsFull()
 *
 * Return TRUE if the specified queue is full.
 *
 *************************************************************
 */
int byteQueueIsFull(queue_t *p)
{
     return (p->rear + 2) % (MAX_BUF_SIZE + 1) == p->front;
}

/*************************************************************
 * byteQueueInit()
 *
 * Initialize the specified queue.
 *
 *************************************************************
 */
void byteQueueInit(queue_t *p)
{
     p->front = 1;
     p->rear = 0;
}

/*************************************************************
 * byteQueueInsert()
 *
 * Insert a character into the rear of the specified queue.
 *
 *************************************************************
 */
int byteQueueInsert(queue_t *p, uint8_t byte)
{
     if ((p->rear + 2) % (MAX_BUF_SIZE + 1) == p->front)
          return FALSE;

     p->rear = (p->rear + 1) % (MAX_BUF_SIZE + 1);
     p->buffer[p->rear] = byte;
     return TRUE;
}

/*************************************************************
 * byteQueueRemove()
 *
 * Remove and return a character from the front of the
 * specified queue.  Be sure to check if queue is empty
 * before calling.
 *
 *************************************************************
 */
uint8_t byteQueueRemove(queue_t *p)
{
     uint8_t byte;

     byte = p->buffer[p->front];
     p->front = (p->front + 1) % (MAX_BUF_SIZE + 1);
     return byte;
}

/*************************************************************
 * byteQueueFront()
 *
 * Return the the character at the front of the specified
 * queue.  Be sure to check if queue is empty before calling.
 *
 *************************************************************
 */
uint8_t byteQueueFront(queue_t *p)
{
     return p->buffer[p->front];
}

/*************************************************************
 * byteQueueDelete()
 *
 * Delete the character at the front of the specified queue.
 *
 *************************************************************
 */
int byteQueueDelete(queue_t *p)
{
     if ((p->rear + 1) % (MAX_BUF_SIZE + 1) == p->front)
          return FALSE;

     p->front = (p->front + 1) % (MAX_BUF_SIZE + 1);
     return TRUE;
}
/*************************************************************
 *  example.c
 *
 *  Compiled with gcc and verified in cygwin
 *
 *************************************************************
 */
#include 
#include "queue.h"

int main(void)
{
     int i;
     queue_t myQueue;   /* Declare the queue */

     /* Initialize the queue
      */
     byteQueueInit(&myQueue);

     /* Put bytes in queue until full
      */
     i = 0;
     while (!byteQueueIsFull(&myQueue)) {
          printf("Inserting: %d\n", i);
          byteQueueInsert(&myQueue, i++);
     }

     /* Get the byte at the front of the queue
      * without removing it.
      */
     i = byteQueueFront(&myQueue);
     printf("At front is: %d\n", i);

     /* Remove one byte at a time from the queue
      * until it is empty.
      */
     while (!byteQueueIsEmpty(&myQueue)) {
          i = byteQueueRemove(&myQueue);
          printf("Removed: %d\n", i);
     }
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

OK, so now I have looked over your code. I had to re-structure main() in order to see what was happening.

I also took the liberty of replacing your "circular buffer" with calls to the queue functions I posted earlier. You can assume that interrupts are disabled/enabled in the queue functions where necessary to ensure that the uart receive ISR is not accessing the buffer while it is being modified.

After doing this, I can't see a problem.

Get yourself a book on C or access some of the many sites on the web that have C programming information. Try to work on your programming style/skills.

static volatile queue_t rx_queue;

void main(void)
{
     init_avr();
     Init_I2C();
     init_lcd();
     init_variables();

     byteQueueInit(&rx_queue);

     dbgflag=0;

     while (1) {

          /* Process the data from the rx_queue as we receive it.
           * Build up a transmit string in tx_buffer.
           */
          while (!byteQueueIsEmpty()) {

               /* The rx_queue is not empty, retrieve a received
                * byte.
                */
               comm_char = byteQueueRemove(&rx_queue);

               switch (stx_received) {
               case 0:
                    if (comm_char == meter_add) {
                         stx_received = 1;
                         tx_buffer[0] = meter_add;
                         frame_cnt = 2;
                         dbgflag = 1;
                    }
                    break;

               case 1:
                    dbgflag = 2;

                    if (comm_char == 3) {
                         stx_received = 2;
                         tx_buffer[1] = comm_char;
                         dbgflag = 3;
                    } else {
                         /* Look for meter_add again? */
                         stx_received = 0;
                    }
                    break;

               case 2:
                    tx_buffer[frame_cnt++] = comm_char;
                    if (frame_cnt == 8) {
                         dbgflag = 4;
                         communication();
                    }
                    break;

               default:
                    /* stx_received is not what we expected!
                     * How should this be handled?
                     *
                     * For now, loop here if this happens.
                     */
                    while (1);
                    break;
               }
          }



     }
}

Here is the new uart receive routine using the queue functions.

#pragma interrupt_handler uart0_rx_isr:19
void uart0_rx_isr(void)
{
     /* Hey! We received a byte.
      */
     if (!byteQueueIsFull(&rx_queue)) {
          /* Queue the rx data if the rx_queue is not full
           */
          byteQueueInsert(&rx_queue, UDR0);
     } else {
          /* Hmmm, rx_queue is full. How should this
           * be handled?
           */
          while(1);
     }
}

Good luck!

Tom

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

I donated a ring-buffered serial I/O set of code and demo in the AVRfreaks project area. There are several others there too, rather than rolling your own...
https://www.avrfreaks.net/index.p...

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

I use iccavr v6.... try this... turn on stack checking.... include ... sprinkle some _StackCheck() calls down in the loops... use this _StackOverflowed function that will override the default function that just jumps to zero... not useful

void _StackOverflowed(char n){
  cprintf("stack overflow! %d",n);
  while(1){}; //hang up during debug
}

How much ram left? Enough for a stack? 256 bytes?

Imagecraft compiler user

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

Thanks zoom and bobgardner .
I will try both and revert .