Data Parsing

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

Hello All,

First of all I want to say that I am learning C (I am a beginner).
For you understand the nature/goals of the program, I am going to give a small explanation.

I am working in a telemetry project of a radio controlled airplane.

The data sent by the Rx in the model has two types, the data sent by the RX like the RSSI, the voltage at the analogue inputs (8 bits A/D), alarm levels, etc. and the data sent by the user. the RX has also a RS232 user input, and I am sending 47 bytes of data (binary format) GPS,motor battery voltage, temperature (thermistor), motor rotations per minute, atmospheric pressure (variometer), air speed, etc.

The transmitter on the ground has a RS232 output were we can pick up all the data.

The data is sent in a frame format of 11 bytes. The user format is the following:~

0x7E, 0xFD header

3th byte is the number of data bytes in the frame
4th is to discard (no data)
5th till 10th Maximo number of data bytes.
11th termination character 0x7E.

I did a C program that already parses the user data as well as the byte stuffing method explained below.

=======================================================

 Byte stuffing method:Output
Byte in frame has value 0x7E is changed into 2 bytes: 0x7D 0x5E
Byte in frame has value 0x7D is changed into 2 bytes: 0x7D 0x5D
Input:
When byte 0x7D is received, discard this byte, and the next byte is XORed with 0x20;
========================================================

Now what is my problem? I send the 47 bytes every second, and when I am going to read the UART input it can be in the middle of the of the user data, so when I store my data in a vector it can be shifted, so, I want to read/save the first frame of the user data. Also I know that the user data always starts with 0xB5, 0x62. If we look for the frame format, these 2 bytes will be in the 5th and 6th byte of the first frame. I have been fighting for almost 2 days and I can not sincronise tha data.

The program gives sporadic errors. It may work 10 or 15 seconds well and suddenly pickups the data in the middle.

Your help is appreciated. I may have something wrong with the sequence of "if else, else if" the program is the following (the parts of code as comments // is the way I think it should be, but, doesn't work):

Thanks in advance.
Regards,
Manuel


void USART()
{
 uint8_t sig[] = { 0x7E, 0xFD};
 
 	data_count=0;
	header_flag=0;

 	while (data_count<=44)
	{

	//init_frame_count:
	  header_flag=0;
	  stuff_byte=0;
	  frame_data=1;
	  uint8_t read_sig = 0;
	

	 while  (frame_data>0)
		{ 

   			while (!(UCSR0A & (1<<RXC0))) {} 
  		  	received_byte = UDR0;
	
	
			  if (read_sig < 2)
        		read_sig = received_byte == sig[read_sig] ? read_sig + 1:received_byte == sig[0];
			  else
				{
					switch (read_sig)
							{
							case 2:
								    {
									frame_data=received_byte;
									read_sig++;
									break;
									}

							case 3:
								{
								read_sig++;
								break;
								}

						default:

						if  (header_flag == 2)
						     {  
						     goto save_data;
							  }
							  else
								if((received_byte == 0xB5) && (header_flag==0) && (read_sig==4))
								  {
								 read_sig++;
								 frame_data--;
								 header_flag=1;
								 break;
								  }
								 else
								 if ((received_byte == 0x62) && (header_flag==1) && (read_sig==5))
								 {
								 frame_data--;
								 header_flag=2;
								 break;
								 }
								//else
								 //{
								// goto init_frame_count;
								// }		
									
						save_data:

							if (received_byte == 0x7D)
								{
								stuff_byte=1;
								}
								else

								if ((stuff_byte==1) && ((received_byte == 0x5D) || (received_byte == 0x5E)))
								    {
								    received_byte^=0x20;
									GPS_data[data_count++]=received_byte;
									frame_data--;
									}
									else
									{GPS_data[data_count++]=received_byte;
									frame_data--;
									}
									 

						
							
							}
				}	
			}
		}
}



/**************************************************************
 Routine to print the data vector*/
void Print()
 {
  uint8_t i;

   for (i=0; i<=44; i=i+1)

	 {
	 	while (!(UCSR0A & (1<<UDRE0))) {} 

	 	  UDR0= GPS_data[i];
	 }
  }     



/**************************************************************
*/




/* main -- Program starts here */
int main(void)
{	

initialize_usart();


	for(;;)

	{


		USART();

		Print();

//	 position_calc();


		

	}	
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
{
goto save_data;
}

Well, you are a beginner you say. And yet, you use the dreaded command "goto". Where exactly did you get that advice from? There is a reason why the goto command is not used in common code. That reason is that it will cause you a lot of problems if you don't REALLY know what you are doing. For starters, it makes your code harder to read and track.

case 2:
  {
   frame_data=received_byte;
   read_sig++;
   break;
  } 

You don't need brackets here (although they don't do any harm)

   if (read_sig < 2)
              read_sig = received_byte == sig[read_sig] ? read_sig + 1:received_byte == sig[0]; 

What do you want to do here, exactly? Since you are a beginner, why are you trying to write complicated code? There is a golden principle called KISS. That stands for "Keep It Simple, Stupid" (not personal for you, it's the principle and we all (have to/should) follow it).

-Pantelis

Professor of Applied Murphology, University of W.T.F.Justhappened.

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

Maybe try adding delays to account for the transmission time.

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

I think you want a finite state machine to decode your data. You have so many flags that it makes it difficult to understand what your code will do in all circumstances. Where do you reset the stuff_byte flag? How to write a finite state machine? I described a method in a recent post by cakemonster regarding a mega8 and tachometer. I think once you master this, the code will be much simpler

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

Thanks for your inputs.

As I said I am starting to learn C. I also learned (I think) assembly by my self. The reason I want to learn C, is to do math. calculations easier, otherwise I rather prefer assembly.

Regarding the "go to", I know that is sacrilege, and I know also that I would get criticism about it, but, I do not have knowledge enough to turn around it, with my grow in knowledge I will avoid it.

 if (read_sig < 2) 
              read_sig = received_byte == sig[read_sig] ? read_sig + 1:received_byte == sig[0]; 

This is the Conditional operator. I leaned it in a book "condition? expression1: expresion2" With this code I am trying to serach for a begining of a frame, 0x7E, 0x7D.

The "stuff_byte" flag is reset after

while (data_count<=44)

Now, that I got all the critics about the way I wrote this kind of C code, Is there some one that is willing to help me??

Thanks and regards,
Manuel

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

Manuel,

please don't view criticism just a polite way to tell you that what you did is nothing but crap. In fact, criticism, at least when expressed the way they did above, actually *is* a form of help you're looking for.

Next, maybe you managed to learn assembler somehow. Maybe you also manage to learn C somehow. But from what you've written so far, i got the impression that you solely managed to use the right syntax to get beyond any compiler/assembler errors. But what you're still missing is learning how to actually program. Programming is much more than knowing the syntactical elements of a language. Programming starts much earlier. Describe your problem. Design a way to solve the problem. Structure the several tasks within that design. Whatever. Finally start coding.

If you don't see a way to avoid a goto in your programs, this fact is the best prove for my statement above. And your use of the conditional operator is something i've never seen before. While maybe correct, at least as a beginner try to use clear short statements even if it might result in 4 lines instead of the one complex line you've posted above.

Einstein was right: "Two things are unlimited: the universe and the human stupidity. But i'm not quite sure about the former..."

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

Feedback about your code (what you call criticism) IS help. When you hear it from someone more experienced, learn to accept it. If you don't like it, study hard to become better than the one criticizing you.

Quote:
I do not have knowledge enough to turn around it, with my grow in knowledge I will avoid it
If you don't have enough knowledge to go around it, you don't have enough knowledge to use it. It does you more harm than good.

I know what a conditional operator is and how it works. The point is, do you? Can you write me the title and the page of that book that describes this use of the operator? Because I have searched the net and I couldn't find an example similar to yours. I think one problem is there, but since it is a rare case, one of the c gurus here is probably more qualified to answer.

One more thing, where is your "default" case ending? Before or after "the save_data:" block?

-Pantelis

Professor of Applied Murphology, University of W.T.F.Justhappened.

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

Manuel, i suggested a technique for you to solve your problem. Your protocol looks very similar to slip.

Think about it- you wrote the code and can't get it working, how do you expect anyone else to do any better? There's no comments in the code and is poorly structured, you don't fix that code, you throw it away. Over 10 years ago my boss wrote something similar but worse and it worked- sort of. It had a pattern sensitivity that showed up occasionally. i wsted over a week testing and trying to fix the code. In frustration, i rewrote the code in a couple of hours as a state machine. Not only was it simpler, it worked and could be proven to work. So please don't take our comments as being harsh as we really are trying to help. Tough love maybe?

You want to read 'Code Complete' if you are serious about improving your coding skills.

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

pnp wrote:
I know what a conditional operator is and how it works. The point is, do you? Can you write me the title and the page of that book that describes this use of the operator? Because I have searched the net and I couldn't find an example similar to yours.
Stop picking at that line. It is ok and does what it is intended for.

Stefan Ernst

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

Quote:
Stop picking at that line. It is ok and does what it is intended for.

Stefan, if that code was working or not, was never the question. This code

#include "stdio.h"
#define _i int
#define _v void
#define _c char
#define _f float
#define _d double
#define _p *
#define o(_C_) putchar((char)_C_);
#define pf(_S_,_E_) printf(_S_,_E_);
#define ps(_S_) puts(_S_);
#define s(_S_) _S_;
#define m(_S_) int main() { _S_ return 0; }
#define f(_T_,_N_,_P_,_S_) _T_ _N_ _P_ {_S_}
#define c(_E_,_1_,_2_) if(_E_){_1_}else{_2_}
#define w(_E_,_S_) while(_E_){_S_}
#define v(_T_,_N_) _T_ _N_;
#define a(_N_,_E_) _N_=_E_;
#define b(_0_,_1_,_2_) ((_1_)_0_(_2_))
#define u(_0_,_1_) (_0_(_1_))
f(_i,S_,(_i _X_,_i _Y_),s(return b(+,_X_,_Y_)))f(_v,H_,(),o(0x48)o(0x65)o(0x6C)o(0x6C)o(0x6F)o(0x20)o(0x57)o(0x6F)o(0x72)o(0x6C)o(0x64)o(0x21))m(s(H_())o(S_(0x5,0x5)))

(shamelessly copied from the net)

does work perfectly as well, but would you write in that style?

Einstein was right: "Two things are unlimited: the universe and the human stupidity. But i'm not quite sure about the former..."

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

DO1THL wrote:
Quote:
Stop picking at that line. It is ok and does what it is intended for.

Stefan, if that code was working or not, was never the question.

It was for Pantelis (pnp). He alleged that the OP hasn't understood the conditional operator and that "one problem is there".

Stefan Ernst

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

Using a FSM you get something like this: (not tested of course, just an rough example)

ch=get_char_from_uart();

switch(state)
{
 // Look for first signature byte
 case 0:if (ch==0x7F) 
         { 
         state=1; 
         } 
        break;

 // Second byte of signature?
 case 1:if (ch==0xFD) 
         { 
         state=2; 
         } 
        else 
         { 
         state=0; 
         } 
        break;

 // Receive frame data
 case 2:if (ch==0x7E) // Stuffing?
         { 
         state=3; 
         } 
        else
         {
         frame[i++]=ch; 
         }
        break; 

 // Destuff
 case 3:ch^=0x20;
        frame[i++]=ch;
        
        // End of frame?
        if (ch==0x7E) 
         {
         handle_frame_data();
         state=0;
         }
        else
         {
        state=2; 
         }
        break;

 default:state=0;
         break;
 }

The byte stuffing and frame delimiters might cause ambiguous situations. Is this protocol a given, or did you invent it yourself?

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

sternst wrote:
It was for Pantelis (pnp). He alleged that the OP hasn't understood the conditional operator and that "one problem is there".
True, it was an issue, but I also wrote:
Quote:
I think one problem is there, but since it is a rare case, one of the c gurus here is probably more qualified to answer.
I researched the net (admittedly only 20mins) and all the examples and literature I was able to find mentioned numbers or variables in the x2:x3 part of x1?x2:x3, and not expressions of any kind. One site mentioned that it is actually wrong to do that, and that is why I asked the OP for reference. Thanks for making it clear for me, I will create some code to work on it, for future reference.

-Pantelis

Professor of Applied Murphology, University of W.T.F.Justhappened.

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

This is probably way too late to be helpful, but here's kind of an "outline-form" treatment of how I'd approach the OP's problem.

The telemetry receiver's AVR uses a simple interrupt service routine to copy every received character into a circular buffer. Here, the ISE is the "producer" of data for the circular buffer.

A non-interrupt task removes (consumes) the raw received characters from the circular buffer and undoes any "byte stuffing" it encounters to recreate the stream of "plaintext" data from the aircraft

These plaintext bytes are then handed to a process that operates in one of two ways, depending on whether it is "in synch" with a dataframe or not.

If it is not in synch, the process checks for the "start of frame" (SOF) marker. Plaintext bytes not equal to SOF are discarded. When SOF is found, the process switches to the "in synch with dataframe" state by setting a "dataframe offset" index value to zero.

Each plaintext byte for which the process *is* "in synch" (probably indicated by "dataframe offset < DATAFRAME_LENGTH") is stored in a "received dataframe" buffer, at the dataframe offset location, which is then incremented.

When the dataframe becomes "full", consult the fields of that just-received frame in whatever way you like, and switch the process back to "not in synch".

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

Hello all,

Thanks to you all for your inputs and advise. I admit that I do not know C. What I learn is from reading a book and as DO1THL said, write the code trying to avoid errors and follow book examples. About discussion above, I am not here for arguing, but, to hear your advice.

Jayjay1974, the code is specified by the product maker.

Quote:

 Byte stuffing method:Output
Byte in frame has value 0x7E is changed into 2 bytes: 0x7D 0x5E
Byte in frame has value 0x7D is changed into 2 bytes: 0x7D 0x5D
Input:
When byte 0x7D is received, discard this byte, and the next byte is XORed with 0x20;

I am going to write your code and see how it works.

The system sends a set of frames with one interval of about 30ms. A typical frame (not user frame) looks like this:
7E FE 47 5E 4F A4 00 00 00 00 7E

A user frame looks like this:
7E FD 04 1B B5 62 01 05 00 01 7E
This frame has 4 bytes of data (3th byte gives the number of data bytes) the data bytes are B5 62 01 05. The number of bytes by frame may vary from 1 to 6.
Considering there is always data coming out of the RS232 port at 9600 baud rate, (about 850uS every byte.If we consider 1.5 clock per instruction with the MCU running at 16MHz we can process about 9000 instructions until the buffer is full again. The user data is send every second and takes about 40ms, my approach to the problem was the following:

Go look for the user data, process and save it in a vector.
After do all the calculations for convert the binary data from GPS, calculate the battery voltage using the A/D data, etc. Display it and go back looking for a new set of user data.

Problems:
I have to synchronize the data, this means that I have to look for the beginning of the user data and start process it.
I have to look to the byte stuff and correct it before save into vector.

The code I did to look for the frame and to the byte stuffing is below. Do you think I should keep the code or make a well structured like the jayjay1974 example?

Thanks for your advise.
regards,
Manuel


 void USART()
{
 uint8_t sig[] = { 0x7E, 0xFD}; // User header frame
 
 	data_count=0;

	while (data_count<=46)  // 46 bytes of user data
	{

	  stuff_byte=0;
	  frame_data=1;
	  uint8_t read_sig = 0;
	

	 while  (frame_data>0)
		{ 

   			while (!(UCSR0A & (1<<RXC0))) {} 
  		  	received_byte = UDR0;
	
// look for the user frame header 0x7E, 0x7D
	
			  if (read_sig < 2)
        		read_sig = received_byte == sig[read_sig] ? read_sig + 1:received_byte == sig[0];
			  else
				{
					switch (read_sig)
							{
							case 2:
								    {
									frame_data=received_byte; // Get the number of bytes in the frame
									read_sig++; 
									break;
									}

							case 3:
								{
								read_sig++; //discard the 4th byte 

								break;
								}

						default:

// looks for byte stuff (0x7D) and corrects it

							if (received_byte == 0x7D)
								{
								stuff_byte=1;
								}
								else

								if ((stuff_byte==1) && ((received_byte == 0x5D) || (received_byte == 0x5E)))
								    {
								    received_byte^=0x20;
									GPS_data[data_count++]=received_byte;
									frame_data--;
									}
									else
									{GPS_data[data_count++]=received_byte;
									frame_data--;
									}
									 

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

jayjay's style is exactly what I'm talking about. Add some code to test for packet length and you're done. The state machine is so much easier to write, understand and debug - why would you do it any other way? Be it in assembler, C, Pascal or whatever.

Try to avoid lines line this:
read_sig = received_byte == sig[read_sig] ? read_sig + 1:received_byte == sig[0];

we've got assignment, equality and a trinary operator - whilst it may be legal, ensuring it is correct and does what is intended is a mental challenge. MISRA C suggests avoiding the use of this operator. If we have to refer to the C standard to decipher the line, then it is poorly coded. Similarly with precedence of operators. Simple code has simple defects.
Again, read 'Code Complete' and get a copy of MISRA C for the full explanation.

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

Hello all,

I tried to follow the example of jayjay, but I do not know why it doesn't work.
I do not used the length of the frame to control the flow because is varies depending of the number of 0x7E and 0x7D that appears within the data (Byte staffing). I used the number of data bytes because it considers 0x7D, 0x5E as one byte.

I also used a the UART routine to immediately read the data and send it out, and the data is there.
Considering that I couldn't parsing any data, I also verify the second read character if it is a 0x7E because of data stream sequence, example: 7E FD 04 1B B5 62 01 05 00 01 7E, 7E FD 04 1B B5 62 01 05 00 01 7E.

We have 2 0x7E, if we consider the last 0x7E of the frame as 1st byte, when it goes read the 2nd byte it also reads another 0x7E (no match) and goes back to "case 0" reading 0xFD, that isn't also the 1st character, so it will aborts. Due to this I also test in "case 1" if it is 0x7E and if yes I consider it the 1st character.

Any way, even with this changes it doesn't work. I think that as the times passes I think I do not know anything about C.

The code is below:

Please let me know your comments.

thanks and regards,
Manuel


/** 
**********************PROGRAMA FUNCIONA************************
**
**test the Telemetry channel for wrong data transmission
**
**
**
**
**
**
**
*/
#include 
#include 


// CONSTANTS
#define F_CPU 16000000UL
#define USART_BAUDRATE_0 9600 
#define BAUD_PRESCALE_0 (((F_CPU / (USART_BAUDRATE_0 * 16UL))) - 1)


static uint8_t telemetry_data[47];
volatile uint8_t received_byte;
volatile uint8_t data_count = 0;
volatile int8_t number_data_bytes=0;








void initialize_usart() 
{
						// Set the USART for a baudrate of 9600
UBRR0L=BAUD_PRESCALE_0;   //Load the lower 8 bits of baude rate value into low byte 
UBRR0H=(BAUD_PRESCALE_0>>8); // load the upper 8 bits of the baude rate value
UCSR0B =(1<<TXEN0)|(1<<RXEN0); 							// turn the tranmition on
UCSR0C =(0<<USBS0)|(3<<UCSZ00);  //Set 8 Bits data 1 stop bit
}


/****************************************************************
* The USART1 reveives the data from a HUB . The data is in 
*binary format to be more compact. The data is to be send down
*using the RX link to the TX at a baudrate of 9600.*/


 void USART()
{
 
 	data_count=0;
	number_data_bytes=1;
	uint8_t state = 0;

 	while (data_count<=44)
	{


	 while  (number_data_bytes>0)
		{ 

   			while (!(UCSR0A & (1<<RXC0))) {} 
  		  	received_byte = UDR0;

			//while (!(UCSR0A & (1<<UDRE0))) {} 
		 	//  UDR0= received_byte;
	

//ch=get_char_from_uart(); 

switch(state) 
{ 
 // Look for first signature byte 
 case 0:if (received_byte==0x7E) 
         { 
         state=1; 
         } 
        break; 

 // Second byte of signature?
 //test also if can be the first
 
 case 1:if (received_byte==0xFD) 
         { 
         state=2; 
         } 
        else 
         { 
         state=0; 
         } 
		 if (received_byte==0x7E)
		 {
		 state=1;
		 }
		 else
		 {
		 state=0;
		 }
        break;
		
 // Get the number os data bytes in the frame
 case 2:
         {
	     number_data_bytes = received_byte;
		 state=3;
		 }
  // Discard the 4th byte
 case 3:
         state=4;
	     break;
// Test for begining of user data package 0xB5, 0x62

 case 4:
         if (received_byte == 0xB5)
	     {
		 number_data_bytes--;
		 state=5;
		 } 
		 else
		 {
		 state=0;
		 }
		 break;

 case 5:
 		 if (received_byte==0x62)
		 {
		  number_data_bytes--;
		  state=6;
		  }
		  else
		  {
		   state=0;
		   }
		  break;

/* Byte_stuffing: if is find a data byte with the value 0x7E or 
   0x7D these are replaced respectively by 0x7D, 0x5E and
   0x7D, 0x5D. 0x7D must be discarded and the following byte
   (0x5E or 0x5D) is ORED with 0x20.*/ 

 case 6:
 	if (received_byte==0x7E) // Stuffing? 
         { 
         state=7; 
         } 
        else 
         { 
         telemetry_data[data_count++]=received_byte;
		 number_data_bytes--; 
         } 
        break; 

 // byte stuffing correction
 
 case 7: received_byte^=0x20; 
         telemetry_data[data_count++]=received_byte;
	 number_data_bytes--;
	 state=6;
	 break;
     
 
 default:state=0; 
         break; 
 	} 
	}
   }
}

/**************************************************************
 Routine to print the data vector*/
void Print()
 {
  uint8_t i;

   for (i=0; i<=44; i=i+1)

	 {
	 	while (!(UCSR0A & (1<<UDRE0))) {} 

	 	  UDR0= telemetry_data[i];
	 }
  }     



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

That seems unnecessarily complicated. How about something like

void stuffchar(char c);
static int i, stuff;
switch (i++) {
case 0:   //first sync byte
  if (c!=0x7e) i=0;
  break;
case 1:   //second sync byte
  if (c!=0x7d) i=0;
  break;
case 2:  //we are synchronized
  len=c; //get the length
  if (len>10) i=0; //bad length, desync
  break;
case 3:  //toss this byte
  break;
default:
  if (i>length) break;
  if (c==0x7d) stuff=0x20;
  else {
    buf[i-4]=c ^ stuff;
	stuff = 0;
  }
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
I researched the net (admittedly only 20mins) and all the examples and literature I was able to find mentioned numbers or variables in the x2:x3 part of x1?x2:x3, and not expressions of any kind.
Then you looked in the wrong place. Even K&R describes the operator as:
expr1 ? expr2 : expr3
Quote:
One site mentioned that it is actually wrong to do that
Don't believe everything that you read. The problem that I have with the line is that the expressions used don't make any sense. If the conditional is true, read_sig is incremented by 1, if it is false, read_sig is set to 0 or 1 depending on the result of another conditional. If the result is the one desired, it is certainly not clear as to why it is correct.

Regards,
Steve A.

The Board helps those that help themselves.

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

Koshchi, thanks for your comments. I am going to take your advise in consideration.

dak664, thanks for your help. I am going to test your example and let you know the result.

Thanks and regards,
Manuel

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

Koshchi wrote:
The problem that I have with the line is that the expressions used don't make any sense. If the conditional is true, read_sig is incremented by 1, if it is false, read_sig is set to 0 or 1 depending on the result of another conditional. If the result is the one desired, it is certainly not clear as to why it is correct.
Come on, it isn't that complicated.
 uint8_t sig[] = { 0x7E, 0xFD};
 ...
           if (read_sig < 2)
              read_sig = received_byte == sig[read_sig] ? read_sig + 1:received_byte == sig[0]; 

read_sig indicates which byte is expected next. 2 means both bytes were detected.

if  received_byte == sig[read_sig]

if the received byte matches the expected one

then  read_sig = read_sig + 1

then expect the next byte (or we are ready)

else  read_sig = received_byte == sig[0]

else reset the signature searching,
and which byte we expect next (first or second) depends on whether the received byte already matches the first or not.

And just to make clear:
I think too that a clean state machine is the better solution. I just wanted to clarify that the original code is not as faulty as indicated by some people. Not that the OP thinks he has written total nonsense.

Stefan Ernst

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

Quote:

Code:
else read_sig = received_byte == sig[0]
else reset the signature searching,

By setting the (used as an index) read_sig to true/false? That part confuses me a bit. But it could be tricky C? If it doesn't match sig[0] then read_sig will be 0/false and you are looking for the first byte of the signature; else it will be 1/true and because the first byte matched you are looking for the second byte? Cute, but certainly not KISS.

Lee

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Case 6 has a defect methinks. The way i debug these is to output the incoming character and the state via a printf, that way you can see what the state machine is doing in certain circumstances. Another way is to use the simulator and step through the code. You seem to have the basics of C down pat but your problem wasnt with the language you used but rather the program logic you employed. Would the same logic in asm have made a difference?

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

Hello all.

Can you now give a opinion about the new code ( apart from the controversial Conditional Operand) that I am going solve next.

I wanted make it step by step to be more secure.

(The program is working correctly).

Your comments are appreciated.

Thanks and regards,
Manuel


/** 
**********************PROGRAMA FUNCIONA************************
**
**test the Telemetry channel for wrong data  transmission
**
**
**
**
**
**
**
*/
#include 
#include 
#include 


// CONSTANTS
#define F_CPU 16000000UL
#define USART_BAUDRATE_0 9600 
#define BAUD_PRESCALE_0 (((F_CPU / (USART_BAUDRATE_0 * 16UL))) - 1)


static uint8_t GPS_data[47];//
volatile uint8_t received_byte;
static uint8_t received_test[12];
volatile uint8_t data_count = 0;
volatile int8_t frame_data=0;
volatile int8_t stuff_byte=0;
volatile uint8_t frame_count=0;
volatile uint8_t header_flag=0;
volatile uint8_t sync_frame=0;
volatile uint8_t read_sig = 0;



void initialize_usart() 
{
						// Set the USART for a baudrate of 38400
UBRR0L=BAUD_PRESCALE_0;   //Load the lower 8 bits of baude rate value into low byte 
UBRR0H=(BAUD_PRESCALE_0>>8); // load the upper 8 bits of the baude rate value
UCSR0B =(1<<TXEN0)|(1<<RXEN0); 							// turn the tranmition on
UCSR0C =(0<<USBS0)|(3<<UCSZ00);  //Set 8 Bits data 1 stop bit
}


/****************************************************************
* The USART1 reveives the data from the GPS. The data is in 
*binary format to be more compact. The data is to be send down
*using the RX link to the TX at a baudrate of 1200.*/


 void USART()
{
 uint8_t sig[] = { 0x7E, 0xFD};
 
 	data_count=0;
	sync_frame=0;

 	while (data_count<=44)
	{

	
	  frame_data=1;
	  uint8_t read_sig = 0;
	

	 while  (frame_data>0)
		{ 

   			while (!(UCSR0A & (1<<RXC0))) {} 
  		  	received_byte = UDR0;
	
			// Look for first signature 0x7E, 0x7D
			  if (read_sig < 2)
        		read_sig = received_byte == sig[read_sig] ? read_sig + 1:received_byte == sig[0];
			  else
				{
					switch (read_sig)
							{

							//save the number od data bytes in the frame
							case 2:
								{
								frame_data=received_byte;
								read_sig++;
								}
								break;
							// Discard this byte and set syncronize to first data frama was done
							case 3: if (sync_frame==1)
								{
								read_sig = 6;
								}
								else
								{
								read_sig++;
								}
								break;
							//Test 1st syncronized byte
							case 4:if ((received_byte == (0xB5)) && (sync_frame == 0))
								{
								read_sig=5;
								frame_data--;
								}
								else
								{
								read_sig=0;
								}
								break;
							//Test 2nd syncronized byte 
							case 5: if ((received_byte == (0x62)) && (sync_frame == 0))
							    {
								read_sig=6;
								frame_data--;
								sync_frame=1;
								}
								else
								{
								read_sig=0;
								}
								break;
							// Test for byte stuffing 0x7D or 0x7E if not byte stuffing save data	
							case 6: if ((received_byte == (0x7D)) || (received_byte == (0x7E)))
								{
								read_sig=7;
								}
								else
								{
								GPS_data[data_count++]=received_byte;
								frame_data--;
								}
								break;
							//Convert 2nd byte stuffing, XOR with 0x20 and save the data
							case 7: if ((received_byte == (0x5E)) || (received_byte == (0x5D)))
							 	{
								received_byte ^= 0x20; 
								GPS_data[data_count++]=received_byte;
								frame_data--;
								read_sig=6;
								}

					default:
							break;

							}    
					}
				}
			}
	}

/**************************************************************
 Routine to print the data vector*/
void Print()
 {
  uint8_t i;

   for (i=0; i<=44; i=i+1)

	 {
	 	while (!(UCSR0A & (1<<UDRE0))) {} 

	 	  UDR0= GPS_data[i];
	 }
  }     

/**************************************************************
 main -- Program starts here */
int main(void)
{	

initialize_usart();


	for(;;)

	{

		USART();

		Print();

//	 position_calc();

	}	

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

I haven't read - just scanned over it but I can see from the level of indentation already that it's too complex.

( http://en.wikipedia.org/wiki/Cyc... )

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

Cliff, I'm glad i'm not the only one who finds this bit of code difficult to decipher. Manuel, did you not understand what we were telling you? Simple code is simple to write and debug. Spend some time with the simulator to debug your code. We've given you a number of suggestions, but you've ignored the crucial ones.

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

Quote:

Spend some time with the simulator to debug your code.

I'd start with a pad of paper and a pencil and design it first before you even start to type source. Good code just falls out of good design. Do not be attempted to let software "grow organically" where you implement something - that bit works then you think "next I also need it to do X" so now you bolt in some additional code to do "X" and so on. Before long you have a bowl full of spaghetti.

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

Clawson, Kartman,

Thanks for your opinions/comments. I am going to try make it simpler.

Thanks and regards,

Manuel

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

One more remarks about complicated expressions like 'expr1 ? expr2 : expr3':

In ancient times such constructs could improve performance because they were modeled with certain machine instructions in mind. So using it could make your program run faster and be smaller.

Today you are using an optimizing compiler. The compile will optimize all code, independent of how you write it. So you can code the same thing using 'expr1 ? expr2 : expr3' or a corresponding 'if-then-else' construct and the resulting machine code will be very similar.

So, as a good programmer you will choose to write your programs as clearly as possible. There is no point anymore to try so make a program faster by using obscure constructs, the compilers made that obsolete. But you'll make an effort to make your program readable. A good exercise is to pick up an old program of oneself a couple of months/years after writing it an see if you still can understand it.

Of course, if you are interested in those fancy constructs, have a look at the 'obfuscated C' contests, where people try to write a c program in as little text as possible. One typical goal is to write a program where the program text very small, like 4kBytes. But this is a sport, not a way to write useful programs.

Markus