Real time UDP packet decoder

Question

I have basic C++ lectures at a university and I don't really know how I can optimize this line of code for a real-time application (I call this function every 5 ms for UDP packet decoding).

I would like to have some tips about this code:

• How can I improve its efficiency (in time processing rather than memory? (declare variable in the UDPmanager class, use static variable, ... ?)
• I am using openFrameworks for doing this project. Would it be better to use core C++ functions?

void UDPmanager::readPacket() { 
 udpConnection.Receive(udpMessage,100000); 
 string message=udpMessage; 
 static int count = 0;
 if(message!="")
 {
 vector<string> strSensor = ofSplitString(message,"[/c]");
 vector<string> strQoS = ofSplitString(strSensor[0],"|");
 unsigned int receivedNumber = atoi(strQoS[0].c_str());
 QoS_estimator(receivedNumber,message.length(),atoi(strQoS[1].c_str()));
 if(receivedNumber<=previousPacketNumber)
 {
 sleep(1);
 return; // No need to use this packet. (= means it was duplicated in the network, < means it doesn't arrived in order)
 }
 previousPacketNumber=receivedNumber;
 strSensor.erase(strSensor.begin());
 cout << "loss : " << packetLoss << endl;
 cout << "counter : " << count++ << endl;
 cout << "receivedNumber : " << receivedNumber << endl;
 if (strSensor.size()!=sensors.size())
 {
 ofLog() << "The number of sensors changed in the system !" << endl;
 }
 for(unsigned int j =0 ; j<strSensor.size();j++)
 // For each sensors start decoding
 {
 vector<string> strData = ofSplitString(strSensor[j],"[/p]");
 for(size_t i=0; i<strData.size() ; ++i)
 {
 static vector<string> value = ofSplitString(strData[i],"|");
 if (value[0] == "[/i]") // Duplicate initialization frame
 {
 break;
 }
 if (value[0] == "POS")
 {
 sensors[j].touchpadPosition.x = (float)atoi(value[1].c_str())/10000.0;
 sensors[j].touchpadPosition.y = (float)atoi(value[2].c_str())/10000.0;
 }
 else
 {
 if(value[0] == "TOUCH")
 {
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].touch = (atoi(value[i+1].c_str()))==1;
 }
 }
 else if(value[0] == "TTHS")
 {
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].tths = atoi(value[i+1].c_str());
 }
 }
 else if(value[0] == "RTHS")
 {
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].rths = atoi(value[i+1].c_str());
 }
 }
 else if(value[0] == "FDAT")
 {
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].fdat = atoi(value[i+1].c_str());
 }
 }
 else if(value[0] == "BVAL")
 {
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].bval = atoi(value[i+1].c_str());
 }
 }
 else if(value[0] == "DIFF")
 {
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].diff = atoi(value[i+1].c_str());
 }
 }
 else {
 ofLogError() << "Unrecognized key: " << value[0];
 }
 }
 }
 }
 }
}

• AVH: Have you instrumented the code to find out where most time is being spent (i.e. stackoverflow.com/q/9608949/255803)? Which platform is this going to run on? What are the "std::cout << ..." statements doing on the realtime (embedded) platform?
• Henri Koch: @Darhuuk I will look for that thank you ! For now it is on MacOS. Those cout are for debug reasons
• AVH: The reason I asked about the platform is because it can have a large impact on how to optimize, e.g.: speed of multiplications, speed of loading from fixed address vs loading from a register-stored address + register-stored offset (compare PIC16 vs Cortex-M), ... Of course, the compiler would hopefully help you out in those areas, but it helps to know what's problematic in order to guide the compiler in the right direction.
• Maikel: Well a simple optimisation would be to use std::string_view instead of std::string when splitting message. You do this so often and you probably allocate each time memory. This doesn't look like embedded at all.
• 200_success: You say "real time", but do you really mean it? Why is there a sleep(1) inside a real-time function?

Answer 1

Welcome to CodeReview. If you are going to permanently keep your debug code around then you might want to you might want to embed it within ifdefs such as

#ifdef DEBUG
 debug code ...
#endif

Generally, here in CodeReview, we frown on still having debug code within the source code.

Your debug code could be optimized by using "\n" in the first 2 print statements, std::endl performs a flush operation on the output stream (Context switching is not good in code that needs to perform well).

Prefer std::cout Over using std;

If you are coding professionally you should get out of the habit of using the "using std;" statement. The code will more clearly define where cout and other functions are coming from (std::atoi, std::endl). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The function cout you may override within your own classes.

Profile the Code

Use profiling tools to identify where the code is spending the most time. This StackOverflow question discusses a profiling tool that works on Linux and Unix (Mac OS X). Breaking the code and following the Single Responsibility Principle may help with profiling.

Clean Up the Indentation

I know that the indentation problems I see in the code may be cut and paste errors but it would make the code easier to read if it was all indented the same way.

An example of indentation problems, most of the following lines should start in the same column.

 vector<string> strSensor = ofSplitString(message,"[/c]");
 vector<string> strQoS = ofSplitString(strSensor[0],"|");
 unsigned int receivedNumber = atoi(strQoS[0].c_str());
 QoS_estimator(receivedNumber,message.length(),atoi(strQoS[1].c_str()));
 if(receivedNumber<=previousPacketNumber)
 {
 sleep(1);
 return; // No need to use this packet. (= means it was duplicated in the network, < means it doesn't arrived in order)
 }
 previousPacketNumber=receivedNumber;

Missing Code

Reviewing the code would be easier if more of the UDPmanager Class was included, for instance the packetLoss variable is not defined within the scope of the code.

The function ofSplitString is obviously a member of the UDPmanager, but we can't see what it does and the code calls it 4 times (2 times in the loop), that function may be part of the performance issue. It should definitely be part of the optimization.

First Optimization

If the message is empty why not just return from readPacket() immediately?

 if (message=="") {
 return
 }

This would reduce the indentation in the rest of the code by one level.

Second Optimization

Reduce the number of string comparisons, in the lucky case of "POS" the code does only one string comparison, in the most unlucky case of "DIFF" the code performs 7 string comparisons.

One way to reduce the string comparisons in this code is to have another function that converts the string to an enum or integer constant. A function is not completely necessary see converting strings to enums. I suggested a separate function because the code is already too complex/long for one function, consider the Single Responsibility Principle.

If the code utilized an enum or symbolic constant, then the series of if then else if statements could be reduced to a switch/case statement:

 fieldToUpdate = convertStringToEnum(value[0]);
 switch (fieldToUpdate) {
 case POS:
 sensors[j].touchpadPosition.x = (float)atoi(value[1].c_str())/10000.0;
 sensors[j].touchpadPosition.y = (float)atoi(value[2].c_str())/10000.0;
 break;
 case TOUCH:
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].touch = (atoi(value[i+1].c_str()))==1;
 }
 break;
 case TTHS:
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].tths = atoi(value[i+1].c_str());
 }
 case RTHS:
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].rths = atoi(value[i+1].c_str());
 }
 break;
 case FDAT:
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].fdat = atoi(value[i+1].c_str());
 }
 break;
 case BVAL:
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].bval = atoi(value[i+1].c_str());
 }
 break;
 case DIFF:
 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].diff = atoi(value[i+1].c_str());
 }
 break;
 default:
 ofLogError() << "Unrecognized key: " << value[0];
 break;
 }

Magic Numbers: Use Symbolic Constants

There are 3 uses of numbers that aren't completely clear what they are, In the third line of the function 100000 is passed into udpConnection.Receive() as the second argument. In the "POS" code section the code divides by 10000.0 twice, what does this represent. Us symbolic constants that identify what the numbers actually mean. This allows you or someone else to come back to the code in 3 or more years and know exactly what is going on.

Use Iterators

I see indices used in a number of places, you might get performance enhancements by using iterators rather than indices.

Example of problem code

 for(unsigned int i=0;i<sensors[j].data.size();i++)
 {
 sensors[j].data[i].tths = atoi(value[i+1].c_str());
 }

Instead of value[i+1] use an iterator that points to the current value.

Use atof Instead of atoi In the following code it would be better to use std::atof instead of std::atoi

 if (value[0] == "POS")
 {
 sensors[j].touchpadPosition.x = (float)atoi(value[1].c_str())/10000.0;
 sensors[j].touchpadPosition.y = (float)atoi(value[2].c_str())/10000.0;
 }

Then there is no reason to use an old style "C" cast to float, prefer static_cast to old style "C" casts, see Why use static cast.

• Henri Koch: Thank you for your answer ! I will look more deeply about each of your comment to optimize my code