Gsm response differs every time with PIC16f877a

Status
Not open for further replies.
It seems you are only seeing the final character at the end of each message. That implies you are not incrementing 'j' each time a character arrives OR you are not resetting 'J' to zero at the end of each message. Quoting myself from post #14:
I suggest you modify the code inside the while() loop so it looks for the terminator at the end of the message.
Click to expand...
Consider what happens when a message has been received, you need to do something with it and also reset 'j' back to zero or it will hit maximum value and stop storing anything else.
It will work, I have similar PIC devices here that receive similar strings to your GSM messages several times a second and have been doing so 24/7 for many years.

Brian.
 

i modified my isr like this
Code: 
_interrupt() void ISR(void)
{
    if(RCIF)
    {  
        if(RCIE == 0)
        {
            RCIE = 1;
            j = 0;
        }
        data[j] = RCREG;                                 // Read The Received Data Buffer and clears RCIF flag
        j++;
        if(data[j] > length)
            j=0;
        RCIF = 0;
    }
}

in main i written like this
Code: 
UART_str(a);
    while(RCIF == 0);
    CopyBuff();
    ClearRxBuff();
    __delay_ms(2);

void CopyBuff()
    {
    unsigned char tempcount, tempcount1;
    tempcount = tempcount1 = 0;
    for (tempcount = 0; tempcount < 63; tempcount++)
        {
        if ((data[tempcount] != 0X0D) & (data[tempcount] != 0X0A))
            {
            MyBuff[tempcount1] = data[tempcount];
            tempcount1++;
            }
        }
    }

void ClearRxBuff()
    {
    unsigned char tempcount;
    tempcount = 0;
    for (tempcount = 0; tempcount < 63; tempcount++)
        {
        data[tempcount] = 0;
        }
    RCIE = 0;
   
    }

with this i can avoid over writting

- - - Updated - - -

with the above code i can avoid the over writing but if i send more than one message(while code is debugging), the control directly going to end of the code

- - - Updated - - -

my response is like this sms is ON

\r\nOK\r\n\r\n+CMT:"+91**********",",","19/04/01,12:13:04+22"\r\nON\r\n
 
Last edited by a moderator:

I strongly recommend you flow chart this to see where you went wrong, adding lines of code to see what the effect is will never work.

Golden rule of ISR, never change the interrupt enable bits inside the ISR. Also remember how you get into the ISR in the first instance.

This is an extract of the code I use in a similar situation, the PIC in this case is a 16F1847 but the register are the same as the '877A:
Code: 
#define BUS_RXBUFFER_SIZE 61
char data[BUS_RXBUFFER_SIZE + 1];


void __interrupt() ISR(void)
{
	if(PIR1bits.RCIF)
	{
		PIR1 &= ~(1 << RCIF);			//clear UART interrupt flag
		buf = RCREG;					//save the received character
		
		if(buf == '\r)			        //check for end of packet
		{
			PacketReceived = 1;         // terminator was found
			*data[] = 0;                // see my note after the code
			j = 0;
		}
		
		else if(j < BUS_RXBUFFER_SIZE)	//check for space to store byte
		{
			data[j++] = buf;
			data[j] = 0;         		//add terminator
		}
	}
}


/*************************************************************************/
void main(void)
{
	while(1)
	{
		if(PacketReceived)
		{
			PacketReceived = 0;		//clear the flag
			
            // your code to process the string goes here
            // data[] contains the string.

			*data[] = 0;
			j = 0;
		}
		
	}
}

I have edited out much of my code and changed the variable names to match yours. This is how it works:
The ISR is entered every time a character is received by the USART, in my case it is at 38K Bauds but that doesn't change anything except the USART configuration. The character is compared to '\r' which would indicate the end of a packet (GSM string). If it matches, meaning the end of the string has been found, it sets the 'PacketReceived' variable. It also sets the pointer to the saved data back to the beginning and clears the first byte of the string, in my code, the first byte is a module ID number so you may want to remove the line "*data[] = 0;" from the ISR if necessary. If the character was not '/r' it means the end of the string has not yet been found so it first checks to see if there is space to store it and if space exists it puts it in the data[] array. It then also moves the pointer (j) to the next storage space and adds a zero there. This is so the string always has a zero at its end, zero being the 'C' string terminator value. It is also why the size of the data array is made one space bigger than the maximum string size, it is to make space for the terminator at the end.

In the main() code, the 'PacketReceived' variable is checked. If it contains '1' it means there should be a string in the data[] array so you can deal with it as you wish. Before finishing, the pointer (j) is moved back to the beginning of the array and the first byte of the array is set to zero so it starts empty again.

The full program is 636 lines of code so what I show is highly edited. For your information, The PIC sits on a long serial link (over 100m) and extracts data when it's module ID is detected to set an on-screen clock and overlays messages on a security CCTV screen. So as well as handling the serial data, it handles timer interrupts to keep the clock running, the time setting routines, message routines and drives the video insertion IC.

Brian.
 
Golden rule of ISR, never change the interrupt enable bits inside the ISR
Click to expand...

A commonly used practice in this sense when one is not totally sure on how things happen during interrupt events is to add an internal counter to check for reentrancy of interruptions, in case there is an unavoidable process within the ISR that is not as fast as desired, such as:

Code: 
ISR Interrupt_x_Handler()
{
iCounterInterruptX++;
// Do required stuffs
iCounterInterruptX--;
}

In this case, when checked in Main(), the global variable above must always have the value 'zero'.
 
betwixt, i change my isr like this

Code C - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16

__interrupt() void ISR(void)
{
    if(RCIF)
    {  
        if(RxSTatus == 0)
        {
           RxSTatus = 1;
            j = 0;
        }
        data[j] = RCREG;                                 // Read The Received Data Buffer and clears RCIF flag
        j++;
        if(data[j] > 61)
            j=0;
        RCIF = 0;
    }
}

now its working fine

- - - Updated - - -

i have one more problem, when i send sms as , the led LOAD1 ON is turning on , response is like this

"\r\n+CMT:"+91mobileno","","19/04/04,09:47:37+22"\r\nLOAD1 ON\r\n\\0\\0​

but if again i send sms LOAD1 OFF its not turning off the led, response is

"""","date,time+22"\r\nLOAD1 OFF\r\ntime+22"\r\nLOAD1 ON\r\n\r\n+CMT:"+91mobileno","​

its over writing the the new sms into previous message, the response should be

"\r\n+CMT:"+91mobileno","","19/04/04,09:47:37+22"\r\nLOAD1 ON\r\n\\0\\0​

how to clear the previous message please help me
 

Coming late to this thread but your lines in the ISR:
Code: 
if(data[j] > 61)
j=0;
don't seem right to me.
I think you are checking to see if 'j' (the index into the 'data' array) has reached the end of the array (which is defined to be 61 items long. However what you are actually doing is to compare the next unused element of 'data' to 61. As you are receiving ASCII text, that effectively means you are checking to see if any character after "=" (which excludes the digits but includes all of the alpha characters) is the ASCII table has been left in the buffer by some previous operation.
What I suspect you want is"
Code: 
if(j >= 61)
j = 0;
This checks the index value itself. Also I believe that 'data' is defined to be 61 elements long. That means the valid index values are from 0 up to 60. When j is 60 and so you have written to the last element in the array, the subsequent increment of 'j' will make it 61. Your test will NOT reset 'j' at that point but will allow 'j' to be used as an index value the next time the ISR is called. In THAT case, 'j' will be 61 and so index beyond the end of the array which will cause some undefined action (depending on what is in the data memory the compiler has placed immediately after the 'data' array).
Susan
 

thanks for your reply susan, i made that changes,

i want more suggestions please once go through my code and let me know the corrections
Code: 
#include <xc.h>
#include<string.h>
#define _XTAL_FREQ 20000000
char a[]="AT\r"; 
char echo[]="ATE0\r";
char sim[]="AT+CPIN?\r";                                 // To Check sim status
char mode_text[]="AT+CMGF=1\r";                          // to set text mode
char message_rec[]="AT+CNMI=1,2,0,0,0\r";             // use to set recipient number and mesg
char buf,data[62];
char MyBuff[62];
char RxSTatus = 0;
unsigned int i,j;  

void UART_Init(int baudRate)
{    
    BRGH=0;
    SYNC=0;
    TXSTA = 0X20;                                      // Asynchronous mode, 8-bit data & enable transmitter
    RCSTA = 0X90;                                      // Enable Serial Port and 8-bit continuous receive           
    SPBRG = 31;                                        // baud rate @20MHz Clock
}

void UART_TxChar(char ch)
{   
    while(TXIF == 0);                                   // Wait till the transmitter register becomes empty
    TXIF = 0;                                           // Clear transmitter flag
    TXREG = ch;                                         // load the char to be transmitted into transmit reg
}    

void UART_str(char *ch)                                 // to write data continously
{
    for(i=0;ch[i]!=0;i++)
    {
        UART_TxChar(ch[i]); 
    }
}

__interrupt() void ISR(void)
{
    if(RCIF)
    {  
        if(RxSTatus == 0)
        {
           RxSTatus = 1;
            j = 0;
        }
        data[j] = RCREG;                                 // Read The Received Data Buffer and clears RCIF flag
        j++;
        if(data[j ]> 60)
            j=0;
        RCIF = 0;
    }
}

void CopyBuff()
{
    unsigned char tempcount, tempcount1;
    tempcount = tempcount1 = 0;
    for (tempcount = 0; tempcount < 61; tempcount++)
        {
        if ((data[tempcount] != 0X0D) & (data[tempcount] != 0X0A) & (data[tempcount] != '\0'))
            {
            MyBuff[tempcount1] = data[tempcount];
            tempcount1++;
            }
        }
    }

void ClearRxBuff()
    {
    unsigned char tempcount;
    tempcount = 0;
    for (tempcount = 0; tempcount < 61; tempcount++)
        {
        data[tempcount] = 0;
        }
    RxSTatus = 0;
   
    }
void main(void)
{
    char res,res1,res2,res3,res4,res5,res6,res7,res8,res9,res10,res11,res12,res13,res14,res15;
    char ch;
    TRISD = 0X00;
    PORTD = 0X00;
    TRISC = 0x80;                                       // Configure Rx pin(rc7) as input and Tx(rc6) as output 
    RCIE = 1;                                           // UART RecIeving Interrupt Enable Bit
    PEIE = 1;                                           // Peripherals Interrupt Enable Bit
    GIE = 1;                                            // Global Interrupt Enable Bit
    UART_Init(9600);                                    // Initialize the UART module with 9600 baud rate
    
    UART_str(a);
    while(RCIF == 0);
    CopyBuff();
    ClearRxBuff();
    __delay_ms(2);
   
    
    UART_str(echo);
    while(RCIF == 0);
    CopyBuff();
    ClearRxBuff();
    __delay_ms(2);
    
    UART_str(sim);
     while(RCIF == 0);
    CopyBuff();
    ClearRxBuff();
    __delay_ms(2);
    
    UART_str(mode_text);
      while(RCIF == 0);
    CopyBuff();
    ClearRxBuff();
    __delay_ms(2);
    
    UART_str(message_rec);
    while(RCIF == 0);
    CopyBuff();
    ClearRxBuff();
    __delay_ms(2);
    
      
    while(1)
    {
    res = strstr(data,"LOAD1 ON");
    if(res)
    {
        RD0 = 1;
    } 
    res1 = strstr(data,"LOAD1 OFF");
    if(res)
    {
        RD0 = 0;
    }
  
    }
}
i need to control 8 leds with sms one by one</string.h></xc.h>
 
Last edited by a moderator:

I'm glad it works for you but I think you will run into problems later. It seems far more complicated than it needs to be.

Brian.
 

yes you are right betwixt, im facing the problem now it self. this is my interrupt
Code: 
__interrupt() void ISR(void)
{
    if(RCIF)
    {  
        if(RxSTatus == 0)
        {
           RxSTatus = 1;
            j = 0;
        }
        data[j] = RCREG;                                 // Read The Received Data Buffer and clears RCIF flag
        j++;
        if(j > 60)
            j=0;
        RCIF = 0;
    }
}

my code is running fine sometimes, but sometimes its behaving strange,

- - - Updated - - -

i need your help please to correct my code,
 
Last edited by a moderator:

Please comply with Andre's request for code tags around sections of program otherwise the forum software removes all the formatting.

The reason for your problem is that you keep storing characters until there are 61 in the data[] array, what you need to do is look for something in the message you can use as a terminator and use that instead. In the code I posted, it looks for '\r' but it could be '\n' or a zero at the end of the message, you need to see your GSM module specification to see which yours uses.

There is still a need to check the message length so there is no chance of 'j' becoming so big that 'data[j]' doesn't overwrite whatever is stored after it but under normal use, I doubt you will receive messages as long as 60 characters anyway. If you do not do that check and a long string of characters arrives it will crash or corrupt the program.

Incidentally, there is an error in the last code you posted:
Code: 
while(1)
    {
    res = strstr(data,"LOAD1 ON");
    if(res)
    {
        RD0 = 1;
    } 
    res1 = strstr(data,"LOAD1 OFF");
    if(res)
    {
        RD0 = 0;
    }
  
    }
I think the last 'if' should refer to 'res1' not 'res'. There are better and smaller ways to compare lots of strings to see if they match a set pattern.

Brian.
 

The UARTs in some of these Microchip MCUs can be tricky beats and you really do need to follow the data sheet properly when initialising them. In the UART_Init function, you set the baud rate (SPBRG) last - however this is the *first* register you should set. Also while not always wrong, I get a bit nervous of the code when configuration registers are loaded all at once with 'magic' numbers - my preference is to refer to the individual bit fields and set them with values that make sense (even if they are numbers, they can be explained individually in comments) and then the important bits such as TXEN and RXEN can be set after everything else is set up correctly.
The data sheet clearly states that TXIF "...cannot be cleared in software. It will reset only when new data is loaded into the TXREG register."
In 'CopyBuff', you transfer characters from the 'data' array to the 'MyBuff' array but skip out any '\r', '\n' or '\0' characters. That means that the string that ends up in MyBuff will NOT have a null terminator (or any standard terminator character) but, if the new message is shorter than the previous message in 'data' will contain the trailing characters of the previous message. Also 'MyBuff' will not have any indication of how long the active string is. If at some later stage you try to use the 'strstr' function on 'MyBuff' (instead of 'data') then it will keep on going until it randomly finds a null character somewhere in your data memory.
I think your character reception logic is a bit off in that you send characters using the 'UART_str' function. You immediately test the RCIF bit, presumably checking that there is a complete string that has been received.
However, when a character is being received, the RCIF bit will be 0 until the last part has been processed and the new value moved to the RXREG. At that time, then hardware will set the RXIF bit which, when the current instruction has been completed, will trigger an interrupt.
The ISR reads the character which, as you comment states, also clears the RCIF bit - the fact you try to clear this read-only bit later is again a waste of time. Therefore by the time the ISR completes, the RCIF bit will be clear. Therefore, when your main line code resumes, it will get stuck in the 'while' loop unless, by some chance, the test of that bit is the actual instruction that is being executed when the RCIF bit is set. In this case, the test will show the bit set, but the ISR will then be called which saves the status bits. When the ISR returns, the status bits will be restored to show that the bit is set (which the ISR will have cleared) and you will exit the loop.
As you can see, if you exit the loop then it is only because you randomly happen to test the RCIF just after a character has been received but before the ISR has been triggered.
You need to test for the end of the message within the ISR and set a flag for the main code to show that the completed message has arrived.
When you have a complete message, you call 'CopyBuff' to transfer the string from 'data' to 'MyBuff' which is good. You then call 'ClearBuff' which clears out 'data' (and also gets around some of the string length issues I mention above - I suspect accidentally).
I'm not sure the purpose of waiting for 2mSec but at least it does not do any harm
You repeat this sequence several times before you enter the main loop.
In the main loop, all you do is to test the contents of 'data' for one of a couple of known strings. This assumes that the UART is continuing to receive strings and processing them via the ISR. OF course the ISR will be triggered at random times throughout the execution of the main loop and can be altering 'data' even while the 'strstr' function is trying to perform its test (even though it has been interrupted).
It would be much better to follow the process you used above - get the ISR to tell you when a complete message has been received; copy that to another buffer and process that buffer contents.
Finally, any variable that is altered within an ISR but tested for in the main code should be declared as 'volatile' so the compiler will not try to optimise the reading of the variable value.
Susan
 



hi betwixt, thanks for your time here are my changes with my isr and strstr functions (silly doubt how to use code tags)

Code C - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26

__interrupt() void ISR(void)
{
    if(RCIF)
    {  
        if(RxSTatus == 0)
        {
           RxSTatus = 1;
            j = 0;
        }
        buf = RCREG;                                 // Read The Received Data Buffer and clears RCIF flag
        data[j] = buf;
        j++;
        if(buf == '\n')
            j=0;
         }
 
 if(res)
    {
        RD0 = 1;
        
    } 
    res1 = strstr(data,"LOAD1 OFF");
    if(res1)
    {
        RD0 = 0;
    }


- - - Updated - - -


in this case im transferring upto 61 characters from 'data' to 'MyBuff' , 
Code C - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13

void CopyBuff()
{
    unsigned char tempcount, tempcount1;
    tempcount = tempcount1 = 0;
    for (tempcount = 0; tempcount < 61; tempcount++)
        {
        if ((data[tempcount] != 0X0D) & (data[tempcount] != 0X0A) & (data[tempcount] != '\0'))
            {
            MyBuff[tempcount1] = data[tempcount];
            tempcount1++;
            }
        }
    }

how to change this fucnction,should i change the condition as

if ((data[tempcount] != 0X0D) & (data[tempcount] != 0X0A))​
 
Last edited by a moderator:

Susan and I are approximately 12 hours apart in time zones so I will reply, I hope she doesn't mind.

Firstly, to add code tags, you copy your code to the message window (as you have been doing) then immediately before it, type [ C O D E ] without the spaces and immediately after it type [ / C O D E ] again without the spaces. Note the '/' added inside the brackets to turn code mode off again. I can't show it here without spaces or it will treat this message as code!

The problem with the buffer length is as both of us have explained - the length of 61 characters is just the maximum size you have allocated or storage, what you should aim to do is start filling the storage from the first location at the start of each message and add a terminating zero to the end of the message. Then when you use your Copybuff() routine you can take out the 'for' loop and just keep copying from the first location until a zero is found. You can do that very easily with a routine like:
Code: 
tempcount=0;
while(data[tempcount]) 
{
    MyBuff[tempcount] = data[tempcount];
    tempcount++;   
}
Note that the while() will keep the loop running until it sees data[tempcount] equals zero. There are alternative string copy commands that will do the same thing.

That termination zero is very important. In 'C' a text string normally ends with a zero. The zero is not a printable ASCII character so it is used as an "end of text" terminator. Most of the 'C' string commands recognize it, for example if you search for a character in a string it will stop when it finds a zero rather than running on forever or until it finds a match. If you look back to my code in post #23 you will see that after storing a character I move the storage pointer to the next place and add a zero there. It ensures that no matter how many characters are stored, a zero is always added to the end.

Brian.
 


thank you so much,
 

hi betwixt, now im facing another problem with gsm, the gsm module some times echoing back at commands
 

The command "ATE0" should turn local echo off. I see you send it near the start of initialization but it doesn't seem to be accepted.
Are you checking the module returns "OK" before sending the next command?

Brian.
 

yes, module responding with "OK",before sending the nxt command

- - - Updated - - -

i'm totally in confusion state what is happening with my code. i tried with ATE0 as a first command,then AT&W it some what stopping the echoing back of the command, but again i'm strucking with my previous problem i.e over writing the response of next command
 
Last edited:

What is the max length of the received SMS with SMS headers? Does the whole received SMS data fits in your buffer? Your PIC has less RAM.
 

the maximum length is upto 56 characters
 

What is the use of CopyBuff() function? Why not use strcpy() function as you are not parsing any data from the received data?

Another way is to disable UART interrupt after "\r\nOK\r\n" is received and then test it "LOADx ON" or "LOADx OFF" is received and control the appropriate Leds or Relays and then reenable the UART interrupt.
 

Status
Not open for further replies.

Similar threads

Menu
Log in
Register
Cookies are required to use this site. You must accept them to continue using the site. Learn more…