Can you comment on this Serialport code?

[cmisip]

Arduino : start bit, ID, data (2 bytes)

uint16_t buff[2];
uint8_t MESSAGE_START=0x7f;


uint16_t send(uint8_t x){ 
  do {
    Serial.write(x);  //ID of data to request
    while (Serial.available() < 4);
    
    if (Serial.available()) {       
       
       int num=Serial.readBytes(reinterpret_cast<char*>(buff),4);
    }
  } while (( (buff[0] >> 8) != x ) || ((buff[0] &0xff) != MESSAGE_START )) ; // little end is first byte of uint16_t
  //delay(100);
  
return Decode(buff[1]);

}

On the PC side: start bit, ID, data (2 bytes)

...
uint8_t data = serialPort1->ReadByte();  //Read x sent by arduino above

cli::array<Byte>^ mybytes = { 0x7f, data & 0xff, *SimRec[data].cindex & 0xff, *SimRec[data].cindex >> 8 };

					serialPort1->Write(mybytes, 0, 4);
...

Any errors, possible trouble, optimizations?

Thanks,
Chris

[salem c]

> while (Serial.available() < 4);
What happens if the PC goes away?
Do you just halt here forever?

> if (Serial.available())
The previous line has already asserted that there are available bytes.

> serialPort1->Write(mybytes, 0, 4);
Does this guarantee to send 4 bytes, or can it return with some partial success in case the transmit buffer has become full?

Traditional serial lines had no built-in error detection and recovery (this you had to do yourself).
Though I'm guessing you're doing serial over USB, so that might fare better.

[cmisip]

I realize now that I have a deadlock.

If arduino sends x and the PC app has not started yet, Arduino will wait for bytes, but PC will never send them because it missed x.

Does this guarantee to send 4 bytes, or can it return with some partial success in case the transmit buffer has become full?

I've run it for hours and it seems that PC is always sending 4 bytes. However, I think it is best to plan for this issue. I don't really want to put a stop byte cause that will mean I have 5 bytes to transmit. I think what I will do and please let me know if it would be practical, is that I will as a check of data integrity, make sure the first byte and last byte both add to 255. The first byte will be the ID byte that the arduino sends. The middle 2 bytes will be the uint16_t data.

Thanks,
Chris

[jonnin]

the port should be buffered by the OS. I think if you fire it up and send stuff it just sits there, so the PC would see the x if nothing else is reading the port. You can verify that easily enough, though its safer to wait T time and if nothing comes along resend the X every so often. My memory here is about gone. Ymodem batch? Kermit? Its a blur.

practical just depends on what you need to do. If you are going to bother with something like that at all, consider the CRC type ideas where the extra bytes can actually reconstruct the data if there was a transmit error maybe (eg 16 bit int bytes 1 and 2 xored together may work, but that is off the cuff). Does the header really mean anything or is it always the same? 50% of your traffic is not data if the header is meaningless.

[cmisip]

Can you elaborate on this XOR method or CRC? It would be awesome if I could reconstruct the 2 data bytes from the other 2 data bytes that I am using also for start and stop and ID.

Of the 4 bytes I am sending, the start byte is meaningless. The second byte is ID which I use on the arduino to see if I got the proper response byte (Arduino sends x, y, z and should get x,x1,x2 and y,y1,y2 etc. That way I can tell if the response is for x request or y request, etc. ). I also just realized that my start byte is within the domain of values that I might expect as data.

Thanks,
Chris

Thanks

[dhayden]

while (Serial.available() < 4); Put a sleep in there to avoid burning up CPU cycles and turning your Arduino into a space heater. The amount to sleep depends on the application and baud rate, but you might consider the time required to transmit 1 byte.

[salem c]

Unless you're really hammering the bandwidth, I'd suggest using formatted printable strings.

Arduino -> PC
REQ=x\n

PC -> Arduino
RSP=x,value\n

You can easily debug your Arduino just by connecting to some terminal emulator. You can see requests and type responses directly.

Using whatever "serial spy" or "usb spy" diagnostic s/w, seeing printable strings is going to be much more meaningful than cryptic bytes.

The \n makes a handy reference point for re-establishing synchronisation in case of message corruption / loss.

It's easy to extent.

[jonnin]

Can you elaborate on this XOR method or CRC?

CRC you can look up, its involved and probably a sledgehammer here (but, it was the idea you wanted anyway).

xor (and, to an extent, addition) are redundant.
a^b = r
then
r^b = a
and
r^a = b
pretty much like
a+b = r then
r-b = a and r-a = r

so if your last byte is a^b (the 2 bytes of the 16 bit value then the following are true)
a^b is the last byte, so if they match, the packet was undamaged
r^b != a then r, a, or b is wrong. if you know which, you can reconstruct the data:
if r and a are right, you can get b out
if r and b are right, you can get a out
if a and b are right, r is irrelevant anyway

you can fool with all these ideas if you want. Most of this stuff I barely recall how to do it, but you could, back when data went over phone lines, either add a lot of crap to detect and repair or a little crap to detect and request resending to deal with ancient communications like phone lines that had noise that damaged data back in the day. I wouldn't here. Send the 2 byte value twice if you want 4 bytes, if its not the same discard and request resend or discard and take the next one depending on the system. Don't overthink it, but the old school approaches were kinda cool :)

[cmisip]

Unless you're really hammering the bandwidth, I'd suggest using formatted printable strings.

I decided against string because I can send the same info with less bytes using uint16_t.

you can fool with all these ideas if you want.

It's an interesting topic. I was just curious about it. It seemed magical. However for my purpose it is really not necessary because I request data probably about 3 dozen times a minute. If the first request yields an error, the next should be fine. So far, I have not encountered any missing data in the stream. The biggest problem was the deadlock I mentioned. Sometimes I start the PC app and then no serial port data comes in. Sometimes it starts away immediately.

Anyway, I think I might have fixed it. Here is the revised serialport code. Let me know if the logic is in error or there are some improvements needed. There is no blocking "while (Serial.available() < 4);" now so there is no need for a delay I think.



//PC

cli::array<Byte>^ mybytes = { data, SimRecs[data]->get_data() & 0xff, SimRecs[data]->get_data() >> 8 , 255-data};
					serialPort1->Write(mybytes, 0, 4);

//ARDUINO

uint8_t buff[4];
uint16_t send(uint8_t x){ //WORKS with uint16_t buff[4]
  do { 
    do {
       int count=0;  

       //Start sending if the receive buffer is empty  
       if (Serial.available() == 0) {
         Serial.write(x);         
       }

       //Wait for a response but quit after a few tries in case the PC app is not running
       do {
         delay(1);
         count++;
       }while ((Serial.available() ==0) || (count < 20));

       //If we quit, check if it is because there is data in the receive buffer, then read
       if (Serial.available()) 
          buff[0]=Serial.read();

   } while (buff[0] != x ) ;  //Repeat until we get 
  
   //We are here because we found x, read 3 bytes from the serial buffer
     int num=Serial.readBytes((reinterpret_cast<char*>(buff))+1,3);
  
  } while ( (buff[0] != x ) && (buff[3]  != (255-x) )) ; // Repeat the entire process until we confirm receipt of x and 255-x
  
  //Got here because we got a complete data sequence : x databyte1 databyte2 255-x
  uint16_t out=0;
  
  memcpy(&out, (reinterpret_cast<char*>(buff))+1,2);
  return Decode(out );

}

Do you guys see any deadlock in this code?

Thanks,
Chris

[jonnin]

However for my purpose it is really not necessary because I request data probably about 3 dozen times a minute.

Yes, I agree. And it seemed extra magical in the late 80s as kids started to connect to each other with their modems.

I don't see anything else wrong with it logically.
the memcpy looks wrong. Not logically, but using it there. its copying 2 bytes. use casting to do an integer assign (16 bit) from a to b, which is cheaper. memcpy hides a loop and is not efficient for anything < 8 bytes.

[coder777]

Do you guys see any deadlock in this code?

You have a deadlock on line 22 when the counterpart does not reply with x.

So you send the first byte and wait till it's returned. If it is not replied you will not continue (I.e. deadlock). When you receive the byte you send you read three more bytes. If that doesn't happen and the last byte isn't 255-x you send that x again...

The PC code does not reflect this. So how does it work at all?

I would suggest a start and and end byte. When both match you can be relatively sure that the data is correct.

[cmisip]

I was just thinking about that on the way to work today. How about this?

uint16_t send(uint8_t x){ //WORKS with uint16_t buff[4]
  do { 
    int count2=0;
    do {
       int count=0;  

       //Start sending if the receive buffer is empty  
       if (Serial.available() == 0) {
         Serial.write(x);         
       }

       //Wait for a response but quit after a few tries in case the PC app is not running
       do {
         delay(1);
         count++;
       }while ((Serial.available() ==0) || (count < 5));

       //If we quit, check if it is because there is data in the receive buffer, then read
       if (Serial.available()) 
          buff[0]=Serial.read();
       else { //No data? let's wait a few cycles, perhaps shipping is delayed
          count2++;
       }

       if (count2>5) { //Waited long enough, need to reorder x
          Serial.write(x);
          delay(1);
       }
   } while (buff[0] != x ) ;  //Repeat until we get 
  
   //We are here because we found x, read 3 bytes from the serial buffer
     int num=Serial.readBytes((reinterpret_cast<char*>(buff))+1,3);
  
  } while ( (buff[0] != x ) && (buff[3]  != (255-x) )) ; // Repeat the entire process until we confirm receipt of x and 255-x
  
  //Got here because we got a complete data sequence : x databyte1 databyte2 255-x
 

  
  //memcpy(&out, (reinterpret_cast<char*>(buff))+1,2);
  return Decode(*(uint16_t*)(buff+1) );

}

And it seemed extra magical in the late 80s

Reminds me of the days when my parents bought us a IBM PC XT 8088 with 4.77 Mhz speed and 640k ram. It had a turbo button. The memory would count all the way to 640k and beep, if I remember correctly.
Anyway I remember something about CRC check failed. Data error. "Abort, Retry, Ignore". Then we have to retype our entire thesis in Wordstar. Those floppies weren't reliable. I still can't forget 362496.

Thanks,
Chris

[coder777]

You still have an infinite loop when the pc doesn't answer. So you may limit the two outer loops as well.

Why don't you just send the whole buffer and then wait for the confirmation?

[cmisip]

Here's another version without the infinite loop:

uint16_t send(uint8_t x){ //WORKS with uint16_t buff[4]
  int wait_max=5; //search up to 5 bytes befor resending
  int wait_after=15; // search another 15 bytes after resending x
  int count2=0;
  uint8_t buff[4]={0};
  do {      
    do {
       int count=0;  

       //Start sending if the receive buffer is empty  
       if (Serial.available() == 0) {
         Serial.write(x);         
       }

       //Wait for a response but quit after a few tries in case the PC app is not running
       do {
         delay(1);
         count++;
         if (count>=5) break;
       }while (Serial.available() == 0);

       //If we quit, check if it is because there is data in the receive buffer, then read
       if (Serial.available()) 
          buff[0]=Serial.read();
       if (buff[0] != x) {
          
          count2++;
          //delay(1);
       }

       //Waited long enough, need to reorder x 
       //IMPORTANT: This might have to be increased if there are many requests in a loop
       //For example, if the main loop is requesting 5 variables each 4 bytes long, you
       //might need to step over 5x4=20 bytes before you get x in the stream
       if (count2==wait_max) { //how many bytes to check before resending x
          Serial.write(x);
          delay(1);
       }
       
       if (count2>=wait_max+wait_after) break; //how many bytes to check total
   } while (buff[0] != x ) ;  //Repeat until we get 

   
   //We are here because we found x, read 3 bytes from the serial buffer
   if (count2<=(wait_max+wait_after))
     int num=Serial.readBytes((reinterpret_cast<char*>(buff))+1,3);
  
  if (count2>(wait_max+wait_after)) break;
  } while ((buff[0] != x ) && (buff[3]  != (255-x) )) ; // Repeat the entire process until we confirm receipt of x and 255-x
  
  //Got here because we got a complete data sequence : x databyte1 databyte2 255-x OR we tried for wait_max * 10 cycles
    return Decode(*(uint16_t*)(buff+1) );
  
}

This is much better now. I got rid of the code on the PC side that sends zero data: id databyte1=0 databyte2=0 255-id. This is possible because the arduino code is now non blocking. This conserves the serial bandwidth.

I realize that I must step over a particular number of bytes before I can find x if it is in a stream. So I must keep track of how many variable requests are in a stream. I need to implement some kind of global variable that tracks how many variable requests are in the main arduino loop. What would be the best container that would have minimal performance impact and accept only unique entries of request_ID ( a uint8_t) and will have a .size() function?

Why don't you just send the whole buffer and then wait for the confirmation?

I am not sure what you mean?

Thanks,
Chris

[jonnin]

maybe: https://www.cplusplus.com/reference/unordered_set/unordered_set/
does that do what you need? Not sure, it only allows one copy of data but it may take data to mean 'everything in the object' not by one field. What are you trying to do with the unique entries?

[cmisip]

On line 23 the arduino is looking for x and if it can't find it, it increments count2.

If there are 5 data requests in a loop, then if x was received by the PC app, x should be found in the next 5x4 = 20 bytes. If x is not found by the time 20 bytes are read, it is probably because the PC app did not get the request ( because it was starting up probably ). So after reading 20 bytes and not finding x, I have to request x again. I should now find it within the next 20 bytes. It seems that I have to keep looking a certain number of bytes to find x and give up after. The number of bytes to check seems dependent on the number of 4 byte data requests in succession. So if i can track the number of data requests in an arduino loop() method, I could estimate, how long to keep searching for x in the stream before giving up and requesting it again. Does that logic seem sound? I've been at this for a while and I may not be thinking clearly.

So each call to send(uint8_t) should save the unique request id and the size will tell me how many bytes to scan.

This might be ovethinking it though. I could just quit after trying the next few bytes. send() returns a zero. However, in the next loop, there will be a request for x and there will be a data for x.

Thanks,
Chris

[cmisip]

Well, something horrible has happened.

It works fine if I need to fish out one data on the Arduino. When I request multiple data, It's giving me that and other values as well. It's as if it sees multiple combinations of data enclosed by ID and 255-id like that combination is not unique enough. I am going to have to rethink my strategy.

Chris

[jonnin]

does it choke when the data portion happens to contain the start/end flag values byte-wise?

[cmisip]

If I check for the end flag 255-x, it doesn't seem to work. So I tried removing that and going to an earlier version. Here is what works so far: ( I am only giving the PC values less than 50)

PC code

private: System::Void serialPort1_DataReceived(System::Object^ sender, System::IO::Ports::SerialDataReceivedEventArgs^ e) {
	System::IO::Ports::SerialPort^ sp = (System::IO::Ports::SerialPort^)sender;

	serialPort1->ReadTimeout = 200;
	serialPort1->WriteTimeout = 200;

	

	if (serialPort1->BytesToRead > 0) { //if there is data in the buffer


		uint8_t data = serialPort1->ReadByte();
		cout << "Received data " << (unsigned)data << endl;


		if (data >= 250) { //SimReserved 

		}
		else if ((data >= 200) && (data < 250)) { //SimVoice

		}
		else if ((data >= 100) && (data < 200)) { //SimEvent

		}
		else if (data < 50) { //SimVar
			
			if (data < SimRecs.size()) {
			
				if (!SimRecs[data]->active) {
					SimRecs[data]->active = true;


					resubscribe++;

				}
				
				

				if (SimRecs[data]->get_data()) {

					cli::array<Byte>^ mybytes = { data, SimRecs[data]->get_data() & 0xff, SimRecs[data]->get_data() >> 8 , 255-data};
					serialPort1->Write(mybytes, 0, 4);

					cout << "index " << (unsigned)data << endl;
					
					cout << "double " << (unsigned)SimRecs[data]->get_double() << endl;
					cout << "uint16_t " << (unsigned)SimRecs[data]->get_data() << endl; 
					 
					
				}
				else {
					cli::array<Byte>^ mybytes = { data, 0, 0, 255 - data };
					serialPort1->Write(mybytes, 0, 4);
				}

			}
			else {
				cout << "HIgh" << endl;
				cli::array<Byte>^ mybytes = { data, 0 , 0, 255-data };
				serialPort1->Write(mybytes, 0, 4);

		  }

			cout << "RES " << resubscribe << endl;


		}
	}


	
}

ARDUINO CODE

uint16_t sendn(uint8_t x){ //WORKS with uint16_t buff[2]
  uint8_t buff[4]={0};
  int count=0;
  do {
       if (((count % 4) == 0) || (count == 0)) {
       Serial.write(x);
       }
       count++; 
        while (Serial.available() < 1) {
                delay(1);
        }
    
    

      buff[0]=Serial.read();
    
    if (buff[0] == x ) {
      while (Serial.available() < 3)
         delay(1);
      Serial.readBytes((&buff[1]),3);   
    }     

  } while  (buff[0] != x ) ;
  //&& (buff[3] != (255-x) )) ; // little end is first byte of uint16_t

return Decode((uint16_t) ( buff[1]+(buff[2]<<8) ) );

}

[coder777]

I am not sure what you mean?

I thought you send the start/end byte to the pc as well. But ok you don't.

If I check for the end flag 255-x, it doesn't seem to work.

So what's wrong with it? Is there debug output possible?