AVR Freaks

If all you want is the time, just get the time (I'm going by what you have in main- looks like only time needed). Something like-

//headers, etc.
#include <stdbool.h>          //true/false

#define TIMESIZE 7            //6chars + 0 terminate

volatile char time[TIMESIZE]; //store time HHMMSS\0
volatile bool newtime;        //signal new time available

int main(void)
{
    //init stuff

    char buf[TIMESIZE]; //to store copy of time for lcd use

    while(1){
        if( newtime == false ) continue;//no new time yet

        //there is plenty of time to copy the buffer
        //(whatever update rate gps set to- 1Hz, 2Hz, 5Hz, etc)
        //so cli/sei not really needed, but doesn't hurt either

        cli();                          //cli so isr does not modify while copy
        newtime = false;                //clear flag
        for( uint8_t i = 0; i < TIMESIZE; buf[i] = time[i] ); //copy
        sei();                          //done copy

        LcdClear();
        LcdWriteString(0, 0, buf);
    }
}

//|------------| 13ms+ @9600 baud
//$GPRMC,225446,A,4916.45,N,12311.12,W,000.5,054.7,191194,020.3,E*68

ISR(USART_RX_vect)
{
    //msg to match
    //                 01234567
    static char msg[]="$GPRMC,xxxxxx"; //assuming don't need any decimal time if its there (.000)
    //                        789012   //since its probably not of much use
    //                        012345

    static char buf[TIMESIZE]; //6 (0-5) + \0 = 7
    static uint8_t idx;        //index into msg

    char c = UART_Receive();   //get rx char

    char m = msg[idx];         //get current msg char

    if( m == 0 ){              //end of msg
        idx = 0;               //reset index
        buf[TIMESIZE-1] = 0;   //0 terminate
        for( uint8_t i = 0; i < TIMESIZE; time[i] = buf[i] ); //copy to time
        newtime = true;        //signal for main loop
        return;
    }
    if( m == 'x'){              //in the 'x's, getting time
        buf[idx-TIMESIZE] = c;  //7-12 -> 0-5, store time char
        idx++;                  //next
        if( c < '0' || c > '9' ) idx = 0; //not valid char, start over
        return;                 //done
    }
    //checking for msg match
    //if match, inc to next char, else no match start over at msd[0]
    idx = (m == c) ? idx+1 : 0;
}

The example may not be correct and may have errors, but the idea is there (I didn't try to compile it). The simple validation (checking for 0-9) will be plenty good- if you can get a valid $GPRMC, followed by 6 characters that are 0-9, there is a pretty good chance its ok so no need to be trying to figure out a checksum.

gps -> isr buf -> [time buf] -> main/tmp buf -> lcd use

that's a total of 21 bytes for buffer use.

Maybe you eventually want more than time, but by starting simple you can get a handle on problems earlier and easier. Then work your way into more complicated code as needed.

Just providing an alternate idea that may be useful, or maybe not.

When you do the ringbuffer remove, how do you know if there is a char available or not?

>I do not think this is the correct way to define things volatile......

That qualifier can be placed either before the struct keyword or after the struct closing brace and will have the same outcome. When you start dealing with pointers that have qualifiers, then it takes more thought and you have two sides of the * with different meaning. The volatile qualifier could also be moved closer to its use- where the Buffer is defined, but it doesn't matter in this case so may as well put on the struct so its not forgotten when a buffer is declared/defined.

You can move the qualifier around to check it out-

https://godbolt.org/z/5BltZj

In the isr you are inserting data into the buffer when it may be full. This buffer is not designed for that, so you need to check it first, and if its full you cannot insert the rx data. I would rather the buffer take care of itself where it simply returns a bool- insert returns true if there was room, remove returns true if there was something to return (and the argument passed is by reference). Then the buffer cannot be screwed up because you forgot to check it first. There is no need to change what you have, but that buffer code is simple enough to change if you ever wanted to (move the checks into the insert/remove and change the insert return value and argument type).

You also have an uninitialized 'char c;' which in this case you have a chance it will be an '\n' since it is not initialized, and will then never make it to the code that uses the lcd. Its just something to watch out for, and the compiler will usually complain about it when it can be a problem so listen to its warnings.

One more thing, you have a while loop that is disguised to look like it has a body but it does not- I would lose the curly braces there as it is not the body of the while loop.

None of these are your immediate problem, but they will be in the future.

If was trying to troubleshoot this, I would send the data back out to the pc to see, not the lcd where anything less than perfect leads to what you have now. You can then also send various gps sentences one at a time from the pc, let your code do its thing, and send back out to the pc. You can then deal in 'slow motion' and use a debugger if you have one, without a flood of data coming in. Once you get things squared away by using only the pc, then move over to getting data from the gps and also again send back out to the pc. One step at a time, and eventually you will have what you want.

An idea similar to this for echoing sentences back to the pc-

https://godbolt.org/z/xtuppN

this is c++ example usart code I had from another thread, and just tacked on some code in main to send gps sentences back out to pc.

The incoming test data doesn't have to be actual gps type data at first, just send something from the pc that starts with '$' and ends with \r\n, then send it back to the pc. Or whatever method you have going on, just send something back to the pc so you can see what is happening. I would be happy once I could get the gps data to echo back to the pc in real time, and have it doing that reliably for a day or so (or at least a while). Then I would move on to the next step and would most likely not have to revisit the buffer code again when my new problems show up since it was already thoroughly tested.

Kind of a long post for not saying much. I'm not sure if any of this is useful, so use if you can, ignore if you want. 

Surely this application has been done to death with Arduino?