Variable over written

The variable(PacketSize) is previously initialized to "0".

The symptoms you describe strongly indicate that this is not, in fact, the case.

Since you neglected to show how this supposed initialization is actually done, it's impossible to be any more specific than this.

By the way, the simulator runs perfectly, the target device (AT89C51ED2) in a live enviroment gives the problem.

This further supports my diagnosis. The simulator tends to initialize variables to zero even if your code doesn't.

Thanks for your response. I here with add the part where the initiation is done.

void InitVars(void)
	{
	EA = 0;
	ES = 0;
  	TR0 = 0;
	TR1 = 0;
	TR2 = 0;
	MainConveyorMotor = 0;
	check = 0;
	PacketSize = 0;
	SensorDelay = 1000;
	TransportDistance = 0x2E;
	}

unsigned char *Stringbuffer;

extern unsigned char DecodeStringData(void);
extern unsigned short calculateChecksum(unsigned char *pBuffer,int iSize);
extern void SendComm(unsigned char *Stringbuffer,unsigned char Stringlength);
extern void EncodePacket(unsigned char * pBuffer);
extern void InitVars(void);
extern void com_baudrate(void);
extern void CommandAction(void);


unsigned char xdata PusherOutput1 		_at_ 0x2000;
unsigned char xdata PusherDirection1 	_at_ 0x4000;
unsigned char xdata PusherPosInput1 	_at_ 0x8000;
unsigned char xdata DipSwitchSetting	_at_ 0xC000;
unsigned char xdata DistanceSwSetting	_at_ 0xA000; 		// hex switches

//------------- Pointers --------------------

extern unsigned char *Stringbuffer;

//------------- Bits -----------------------

//----------- Chars -------------------------

idata unsigned char PacketSize;
idata unsigned char Stringlength;

//------------ Integers ---------------------

//unsigned int count;  	  				// incremented from ticktimer for general timing
idata unsigned int SensorDelay;
idata unsigned int TransportDistance;

//------------ Arrays -----------------------


xdata unsigned char TempString[128];
xdata unsigned char ReceivedPacket[128];
xdata unsigned char TransmitPacket[128];
xdata unsigned char Data_1[24];
xdata unsigned char Data_2[24];
xdata unsigned char Command[8];

MainConveyorMotor = 0;
check = 0;
PacketSize = 0;
this seems to indicate that you have done something "funny" with startup.a51, have you?

AT89C51ED2
another possibility is that you do not set the SFR that indicates "use internal RAM" in the very beginning of startup.a51. If you are in the C is C mode and set it in the beginning of main() you will lose all initialization of internal RAM.

I know this should not affect idata, but the initialization code does strange things when it talks to "open" RAM (justifiably so).

Erik

"AT89C51ED2
another possibility is that you do not set the SFR that indicates "use internal RAM" in the very beginning of startup.a51. If you are in the C is C mode and set it in the beginning of main() you will lose all initialization of internal RAM."

This device defaults to using internal RAM from addresses 0-0x2ff, but can be configured for internal RAM up to 0x6ff.

Your code is quite strange. The Decode() function writes to SBUF, which seems odd, and it doesn't wait for TI, which may cause garbled serial output.

Your SendComm() function, or at least the part of it you have shown, doesn't seem to send anything, but sets TI. Have you got a mixture of interrupt driven and polled serial comms going on here?

You don't show us how PacketSize normally gets modified. Does this happen in an ISR?

"idata unsigned char Ctr;
for(Ctr=0; Ctr < 256; Ctr++)"

This is an infinite loop.

The SendComm function is declared reentrant. I've never used a reentrant function, but doesn't it mean locals are stored on a stack in XDATA? How does using an idata local fit in with that?

Note that the simulator does not behave the same as real code when dealing with the UART. It is quite possible to see good stuff in the serial window and see garbage on the target for the same code.

If your complete program isn't too big I think you ought to post the whole thing so we can see what's going on. Please detail any changes you have made to startup.a51, also which memory model you are using.

I'll post the full code so that things are clear. Starting with the reentrant thingy, I just did that so to test if it would help it wasn't origionally there.

static void com_isr (void) interrupt 4
	{
	static idata unsigned char		CopyCounter;
	check = 1;
	if (RI != 0)
 		{
  		RI = 0;
		rbuf [r_in] = SBUF;
		if(rbuf[r_in] == STX)
			{
			rbuf[0] = STX;
			r_in = 1;
			}
		else if (rbuf[r_in] == ETX)
			{
			r_in &= 0x7f;
			for (CopyCounter = 0; CopyCounter <= r_in; CopyCounter++)
				{
				ReceivedPacket[CopyCounter] = rbuf[CopyCounter];
				}
			PacketSize = CopyCounter;
			r_in = 0;
			}
		else
			{
			r_in++;
			}
		}
	if (TI != 0)
  		{
  		TI = 0;
  		if (t_in != t_out)
			{
    		SBUF = tbuf [t_out++];
			}
  		}
	}
//---------------------------------------------------
//---------------------------------------------------

void com_baudrate(void)	// using internal BRG
	{
	EA = 0;
	t_in = 0;
	t_out = 0;
	t_disabled = 1;

	BRL = 100;			//	This value sets the baudrate
	BDRCON = 0x1F;
	SM0 = 0;
	SM1 = 1;
	SM2 = 0;
	REN = 1;
	PCON |= 0x80;
	CKCON0 = 0x0B;
	PacketSize = 0;

	ES = 1;
	EA = 1;
	}



void SendComm(unsigned char * pStringbuffer,unsigned char Stringlength)
	{
	idata unsigned char Ctr;

	if (Stringlength != 0)
		{
		for(Ctr=0; Ctr<Stringlength; Ctr++)
   		{
			tbuf[t_in++] = pStringbuffer[Ctr];
			}
	 }
	 else
		{
		for(Ctr=0; Ctr < 256; Ctr++)
   		{
			if (pStringbuffer[Ctr] == 0)
				{
				break;
				}
			tbuf[t_in++] = pStringbuffer[Ctr];
			}
		}
	if( t_in != t_out)
		{
		TI = 1;
		}
	}

unsigned char DecodeStringData(void)
	{
	unsigned char i;
	unsigned short 	shReadCheckSum;
	unsigned char		chMode;
	unsigned char		chPos;
	unsigned short		shChecksum;
	chMode = 0;
	chPos = 0;

	shChecksum = calculateChecksum(&ReceivedPacket[1], PacketSize - 6);
	shReadCheckSum = 0;
	for (i = 1; i < (PacketSize - 1);i++)
		{
		if (chMode == 0)
			{
			// get command
			if ((ReceivedPacket[i] == ',') ||
		       (ReceivedPacket[i] == ';'))
				{
			    Command[chPos] = 0;
				 chPos = 0;
				 if (ReceivedPacket[i] == ',')
				 	{
				 	chMode = 1;
					}
				else
					{
				 	chMode = 3;
					}
				}
			else if (chPos < 31)
				{
				Command[chPos++] = ReceivedPacket[i];
				}
			}
		else if (chMode == 1)
			{
			// get data 1
			if ((ReceivedPacket[i] == ',') ||
		       (ReceivedPacket[i] == ';'))
				{
			    Data_1[chPos] = 0;
				 chPos = 0;
				 if (ReceivedPacket[i] == ',')
				 	{
				 	chMode = 2;
					}
				else
					{
				 	chMode = 3;
					}
				}
			else if (chPos < 31)
				{
				Data_1[chPos++] = ReceivedPacket[i];
				}
			}
		else if (chMode == 2)
			{
			// get data 2
			if (ReceivedPacket[i] == ';')
				{
			    Data_2[chPos] = 0;
				 chPos = 0;
				 chMode = 3;
				}
			else if (chPos < 31)
				{
				Data_2[chPos++] = ReceivedPacket[i];
				}
			}
		else if (chMode == 3)
			{
			// get checksum
			shReadCheckSum <<= 4;
			if ((ReceivedPacket[i] >= '0') && (ReceivedPacket[i] <= '9'))
				{
				shReadCheckSum |= ReceivedPacket[i] - '0';
				}
			else if ((ReceivedPacket[i] >= 'A') && (ReceivedPacket[i] <= 'F'))
				{
				shReadCheckSum |= ReceivedPacket[i] - 'A' + 10;
				}
			else if ((ReceivedPacket[i] >= 'a') && (ReceivedPacket[i] <= 'f'))
				{
				shReadCheckSum |= ReceivedPacket[i] - 'a' + 10;
				}
			}
		}
 	// check if we have collected all data
	if (i < (PacketSize - 1))
		{
		// error in data collection
		return 0;
		}
	// check checksum
	if (shReadCheckSum != shChecksum)
		{
		EncodePacket("STS,CKS:ERR");
		// error in checksum, do something
		return 0;
		}

	return 1;
	}



void EncodePacket(unsigned char * pBuffer)
	{
	unsigned char Counter;
	unsigned char Counter2;
	unsigned short Checksum;

	TransmitPacket[0] = STX;
	for(Counter = 0; Counter<120; Counter++)
		{
		if(pBuffer[Counter] == 0)
			{
			break;
			}
		TransmitPacket[Counter+1] = pBuffer[Counter];
		}
	Counter++;
	TransmitPacket[Counter++] = ';';
	Checksum = calculateChecksum(&TransmitPacket[1],Counter - 1);
	for(Counter2 = 0; Counter2 < 4; Counter2++)
		{
		if (((Checksum >> 12) & 0x000f) > 9)
			{
			TransmitPacket[Counter++] = ((Checksum >> 12) & 0x000f) + 'A' - 10;
			}
		else
			{
			TransmitPacket[Counter++] = ((Checksum >> 12) & 0x000f) + '0';
			}
		Checksum <<= 4;
		}
	TransmitPacket[Counter++] = ETX;
	SendComm(TransmitPacket, Counter);
	}

unsigned short calculateChecksum
    (
    unsigned char 	*pBuffer,
    unsigned char    BSize
    )
    {
    unsigned char    BByteCounter;
    unsigned char    BBitCounter;
    unsigned short   WValue;
    unsigned short	WTemp;


    WValue = 0;

    for (BByteCounter = 0; BByteCounter < BSize; BByteCounter++)
        {
        WTemp = ((unsigned char *) pBuffer)[BByteCounter];
        for (BBitCounter = 0; BBitCounter < 8; BBitCounter++)
            {
            if ((WTemp ^ WValue) & 0x0001)
                {
                WValue = (WValue >> 1) ^ 0x8408;
                }
            else
                {
                WValue >>= 1;
                }
            WTemp >>= 1;
            }
        }
    return WValue;
    }

XDATASTART      EQU     0H      ; the absolute start-address of XDATA memory
XDATALEN        EQU     6FFH

Your code is quite strange. The Decode() function writes to SBUF, which seems odd, and it doesn't wait for TI, which may cause garbled serial output.


I forgot to mention. The SBUF thingy was only in there for testing purposes. I wasn't meant to be there. I the above posting it's corrected (Sorry).
But anyway... Still the same problem. Meaning PacketSize is being overwritten somewhere.

Regards
John Garrelts

John,

There is something problematic in your code. Does this function get called spuriously ALL the time, or just sometimes? The reason I ask is this: You say that you receive "garbage" on startup. From this, you conclude that your ISR would never intentionally set the PacketSize variable to something. But look -- all that it takes for PacketSize to be set to non-zero is for that string of "garbage" to include an ASCII ETX at some point. Even assuming the data is truly random, you've got a 1/255 chance on every single character you spuriously receive.

Further, I don't see any buffer length checks in your ISR. That is, every time you get a character, you're storing the character in the "rbuf" buffer at position "r_in," but you never check to make sure that r_in is not greater than the size of your array. So... if you have a 25-position array and you receive 100 characters of garbage at startup, your routine is going to happily stomp over whatever happens to be above the buffer in memory. Perhaps try something like this:

#define MAX_PACKET_SIZE 25
unsigned char rbuf[MAX_PACKET_SIZE];

if (RI) {
   RI = 0;
   if (r_in < MAX_PACKET_SIZE) {
      rbuf[r_in] = SBUF;
      r_in++;
   } else {
     //Do something here to reset the state
     //because you've overflowed
   }
}

Hi John,

A minor thing is XDATLEN should be 0700H to cover 0x0..0x6FF, but more importantly have you initialised within AUXR the XRAM size bits XRS2, XRS1 and XRS0 to '100' to enable 1792 byte of internal XRAM within STARTUP.A51 in the 'IF XDATALEN <> 0' conditional assembly code before the MOV DPTR,#XDATASTART line?

Hope this helps,
Mark.

PacketSize (and any other global variable shared between main and an interrupt handler) should be declared volatile.

Thanks for all the suggestions. I didn't have access to the live system today. I have already implemented the changes and will let you know how it goes.

Regards
John

if, indeed, (I believe it is so) that the ISR get pushes and pops when no 'using' is stated, you STILL have the problem, just now it is something else that get overwritten.

Erik

Thanks for all the responses, The problem is solved. Drew was right, it was the volatile that needed to be added to the variables. Of course I did check all the other stuff, like adding the "using" (which I incedently had in the code before trying to find what was wrong).

Anyways thanks everyone for the backup.

Regards
John Garrelts

"Drew was right, it was the volatile that needed to be added to the variables."

Be aware that if any of these variables are larger than char then you'll need to do more than declare them volatile to ensure reliable operation.

Variables larger that char? So int and float is not protected with volatile, or are you talking about structures, arrays, etc...