What is wrong with this C++ class?

[Red Squirrel]

I wrote a C++ class that us used for serial I/O and another class that is a wrapper for a specific device.

I noticed that the output can randomly vary. It's like if it misses lines or something.

For example calling GETTEMP this is common:

[Feb-21-2012 02:40:56am] (4)          /dev/ttyACM0[00] SendCommand: CH1.GETTEMP
[Feb-21-2012 02:40:57am] (4)          /dev/ttyACM0[00] RecvOutput: ::
19.3750
[Feb-21-2012 02:40:58am] (4)          /dev/ttyACM0[00] SendCommand: CH2.GETTEMP
[Feb-21-2012 02:40:58am] (4)          /dev/ttyACM0[00] RecvOutput: ::
[Feb-21-2012 02:40:59am] (4)          /dev/ttyACM0[00] SendCommand: CH3.GETTEMP
[Feb-21-2012 02:40:59am] (4)          /dev/ttyACM0[00] RecvOutput: ::
0.0000
[Feb-21-2012 02:41:00am] (4)          /dev/ttyACM0[00] SendCommand: CH4.GETTEMP
[Feb-21-2012 02:41:00am] (4)          /dev/ttyACM0[00] RecvOutput: ::
[Feb-21-2012 02:41:01am] (4)          /dev/ttyACM0[00] SendCommand: CH5.GETTEMP
[Feb-21-2012 02:41:01am] (4)          /dev/ttyACM0[00] RecvOutput: ::
[Feb-21-2012 02:41:02am] (4)          /dev/ttyACM0[00] SendCommand: CH6.GETTEMP
[Feb-21-2012 02:41:02am] (4)          /dev/ttyACM0[00] RecvOutput: ::

Notice how half the time it never actually does get a number. The :: is the prompt, which is normal to get, but there has to be data after that, and often it gets skipped. I tried adding a delay, but it actually causes it to skip them every time, I don't get why it would do this.

I think my problem is I don't have a proper way to check for EOF. What is the proper way to do that?



Here are the classes:

Sorry, it's kinda long

#ifndef SerialConnector_h
#define SerialConnector_h

#include <unistd.h>
#include <fcntl.h>
#include <errno.h>

class SerialConnector
{
	private:
		string m_Path;
		int m_fd;
		bool m_conactive;
		
		void ScreenOutput(string output);				//output to screen if debug is on
	
	public:
		bool DebugMode;
		SerialConnector();
		SerialConnector(string path);
		void SetPath(string path);				//set working path (ex: /dev/tty0)
		bool Open();						//connect
		void Close();					//disconnect
		bool ConActive();					//whether connection has been lost
		string ReadLine(bool waitindef=true,bool nlonly=false);				//get all data until \r\n is hit, or only \n if arg is true
		bool WriteLine(string line, bool nlonly=false);		//writes line of data followed by \r\n or \n if 2nd arg is true
		


};


#endif




SerialConnector::SerialConnector()
{
	m_Path="";
	m_conactive=false;
	DebugMode=false;
	m_fd=0;
}

SerialConnector::SerialConnector(string path)
{
	m_Path=path;
	m_conactive=false;
	DebugMode=false;
	m_fd=0;
}


void SerialConnector::ScreenOutput(string output)
{
	if(DebugMode)LOGGER->Log(5, m_Path + ": " + output);
}




void SerialConnector::SetPath(string path)
{
	m_Path=path;
}


bool SerialConnector::Open()
{
	if(m_Path=="")
	{
		ScreenOutput("No path specified");
		return false;
	}
	
	
	m_fd=open(m_Path.c_str(), O_RDWR | O_NONBLOCK);	
	fcntl(m_fd, F_SETFL, O_NONBLOCK);
	
	
	if (m_fd == -1 )
	{
		ScreenOutput("Unable to connect");
		m_conactive=false;
		return false;
	}

	m_conactive=true;
	
	return true;
}


void SerialConnector::Close()
{
	m_conactive=false;
	close(m_fd);
}


bool SerialConnector::ConActive()
{
	return m_conactive;
}


string SerialConnector::ReadLine(bool waitindef,bool nlonly)
{
	string ret;	

	if(!m_conactive)return "";
	
	//read a single line:
	
	char buff[1]; //array (the read takes an array, not a single char)
	int bytes=0;
	
	//TODO: Add some kind of safety guard to avoid getting stuck in an infinite "no data" loop 
	
		
	while(1)
	{
		bytes= read(m_fd,buff,1);
		
		if(bytes<0)
		{
			if(errno==11)
			{
				if(waitindef)
				{
					continue; //keep trying
				}
				break; //EAGAIN: No more data			
			}
			
			ScreenOutput("Error while reading.  Errno:" + Int2Str(errno));
			return "";
		}
		else if(bytes==0)
		{
			//lost connection
			ScreenOutput("Lost connection");
			m_conactive=false;
			break; 
		}
		
		
		if(buff[0]=='\r')
		{
			if(!nlonly)
			{
				read(m_fd,buff,1);//read the following char which should be \n
				if(buff[0]=='\n')break;
			}
			else break;			
		}
		else
		{				
			ret.append(1,buff[0]);  			
		}
		
		
		if(ret.size()>1024) //hard coded limit to catch issue where device goes haywire and loops forever.  
		{
			break;
		}
		
	}
	
	return ret;
}


bool SerialConnector::WriteLine(string line,bool nlonly)
{
	if(!m_conactive)return false;
	
	string towrite = line + (nlonly? "\n" : "\r\n");
	
	return write(m_fd,towrite.c_str(),towrite.size());
}






















#ifndef uk1104Connector_h
#define uk1104Connector_h


#include <stdlib.h>
#include "SerialConnector.h"

class uk1104Connector
{
	private:
		SerialConnector * m_serialcon;
		string m_ttydevice;	//TTY device (ex: /dev/ttyACM) do not specify number, 
		int m_ttydevicesec;	//TTY sequence (Ex: ttyACM1, 2, 3 etc)
		string m_lastcommand;	//last command sent
		string m_deviceid;	//canakit device ID

	
	
	public:		
		bool DebugMode;		//enable debug		
		bool HaltAll; 		//used to halt all activity
		
		uk1104Connector(string ttydevice, string deviceid); //ex: /dev/TTYACM, 10   where deviceid is the ID assigned to the UK1104 device (not the TTY number)
		~uk1104Connector();
		
		void ScreenOutput(string output);	//output to screen if debug mode is on
		
		bool Connect(int inctries=10); //connects, if it fails, it tries another TTY number, and keeps trying up to specified number
		void Disconnect();
		
		void ClearBuffer(); 				//reads any pending data to ensure buffer is clear
		string ClearNonNum(string data); 	//clears char that is not a number or period
		bool SendCommand(string cmd);		//sends command to device
		string RecvOutput(bool clearcmdstring=false);	//get output from last command
		
		//relay control:
		bool RelOn(int rel);		//turn relay on
		bool RelOff(int rel);		//turn relay on
		
		
		//Channel control:
		bool SetChMode(int ch, int mode, int tempres=12);	//set mode (1=digital output, 2=digital input, 3=analog input*, 4=temp sensor)
		//* when setting a channel to analog all channels before it are also set analog
		
		
		//Channel get:
		
		float GetTemp(int ch); 	//get temp
		string GetRels();		//gets relay status as a string ex: 0110 means relay 2 and 3 are on.
		
		
};

#endif





uk1104Connector::uk1104Connector(string tty,string deviceid="00")
{
	m_serialcon = NULL;
	m_serialcon = new SerialConnector();
	m_ttydevice = tty;
	m_deviceid = deviceid;
	DebugMode = false;
	HaltAll = false;
	m_ttydevicesec=0;
}



uk1104Connector::~uk1104Connector()
{
	if(m_serialcon->ConActive())Disconnect();
	delete m_serialcon;
}


void uk1104Connector::ScreenOutput(string output)
{
	if(DebugMode)LOGGER->Log(4,m_ttydevice + Int2Str(m_ttydevicesec) + "[" + m_deviceid + "] " + output);
}


bool uk1104Connector::Connect(int inctries)
{
	if(m_serialcon==NULL || HaltAll)return false;
	
	int ttynum=0;
	
	//try to find an open serial port
	//TODO: Ensure we are connecting to a uk1104 device, and that the ID is the one specified
	
	for(;ttynum<inctries;ttynum++)
	{
		m_serialcon->DebugMode=DebugMode;
		m_serialcon->SetPath(m_ttydevice + Int2Str(ttynum));
		m_ttydevicesec=ttynum;
	
		if(m_serialcon->Open())
		{			
			ScreenOutput("Connected to port, verifying device ID...");
			//cout<<"[connected]"<<flush; //debug
			//we have connected, now ensure ID is correct:
			
			//cout<<"if1"<<endl; //debug
			
			SendCommand(""); 		//get initial prompt
			SendCommand("about");	//get device id
			
			//cout<<"if2"<<endl; //debug
						
			//cout<<"[commandssent]"<<flush; //debug
			
			bool validdevice=false;
			
			//catch data till we see ID: 
			for(int i=0;i<7;i++)
			{
				//cout<<"for1"<<endl; //debug
				
				//cout<<"[loopstart]"<<flush; //debug
				string aboutdata = RecvOutput(false);
				//cout<<"[loopdata: "<<aboutdata<<"]"<<flush; //debug
				
				//cout<<"for2"<<endl;//debug
				
				int pos = aboutdata.find("ID:");
				
				if(pos!=string::npos && aboutdata.size()>=pos+6)
				{
					//cout<<"[found]"<<flush; //debug
					string id = aboutdata.substr(pos+4,2);
					
					ScreenOutput("Connected to device ID " + id);
					
					if(id==m_deviceid)validdevice=true;
					
					break;
				}
				
				//cout<<"[loopend]"<<flush; //debug
			}
			
			ClearBuffer();
			
			if(!validdevice)
			{
				m_serialcon->Close();
				continue;
			}
			
			return true;
		}
	}
	
	return false;
}




void uk1104Connector::Disconnect()
{
	ScreenOutput("Disconnected");
	m_serialcon->Close();
}


void uk1104Connector::ClearBuffer()
{
	if(HaltAll || !m_serialcon->ConActive())return;
	
	//cout<<"clearbuff1"<<endl; //debug
	do
	{
		//usleep(500);
	}while(m_serialcon->ReadLine(false,false)!="");
	//cout<<"clearbuff2"<<endl; //debug
	
}


string uk1104Connector::ClearNonNum(string data)
{
	string ret;
	
	for(int i=0;i<data.size();i++)
	{
		switch(data[i])
		{
			case 0x30: ret.append(1,data[i]);break;
			case 0x31: ret.append(1,data[i]);break;
			case 0x32: ret.append(1,data[i]);break;
			case 0x33: ret.append(1,data[i]);break;
			case 0x34: ret.append(1,data[i]);break;
			case 0x35: ret.append(1,data[i]);break;
			case 0x36: ret.append(1,data[i]);break;
			case 0x37: ret.append(1,data[i]);break;
			case 0x38: ret.append(1,data[i]);break;
			case 0x39: ret.append(1,data[i]);break;
			case 0x2E: ret.append(1,data[i]);break;
		}
	}


	return ret;
}


bool uk1104Connector::SendCommand(string cmd)
{
	if(HaltAll || !m_serialcon->ConActive())return false;
	
	sleep(1);
	
	m_serialcon->DebugMode=DebugMode;
	
	ClearBuffer();
	
	m_lastcommand=cmd;

	bool ret=true;
	
	ScreenOutput("SendCommand: " + cmd);
	
	ret=m_serialcon->WriteLine(cmd);
	
	
	//lost con, try to reconnect...
	if(!m_serialcon->ConActive())
	{
		ScreenOutput("Lost connection, attempting to reconnect...");
		if(!Connect())return false;
		
		ret=m_serialcon->WriteLine(cmd);  //try again...
	}
	

	return ret;
}



string uk1104Connector::RecvOutput(bool clearcmdstring)
{
	if(!m_serialcon->ConActive() || HaltAll)return "";
	
	
	m_serialcon->DebugMode=DebugMode;
	
	
	//cout<<"recvout1"<<endl; //debug
	
	
	
	string output = m_serialcon->ReadLine(); //get value
	
	
	//device may be messing up, need to try to shut the relays off and exit as gracefully as possible

	if(output.find("ERROR")!=string::npos)
	{
		ScreenOutput("FATAL possible runaway error occured.  Attempting to shut off all relays and bailing out...");
		
		Disconnect();
		
		//use system to shut relays to bypass everything having to do with this class	
		system(("echo 'rels.off' >" + m_ttydevice + Int2Str(m_ttydevicesec)).c_str());		
		
		return "";
	}
	
	
	//cout<<"recvout2"<<endl; //debug
	
	if(clearcmdstring) //since command string is outputted by default, get rid of it
	{
		//cout<<"OUT: ["<<output<<"]"<<endl; //debug
		
		
		int pos = output.find(m_lastcommand);
		int size = m_lastcommand.size();
		
		if(pos!=string::npos && (pos+size)<=output.size())
		{		
			output.erase(pos,size);	
		}
		
		//cout<<"AFT: ["<<output<<"]"<<endl; //debug
	}
	
	ScreenOutput("RecvOutput: " + output);
	
	return output;
}


bool uk1104Connector::RelOn(int rel)
{
	if(HaltAll || !m_serialcon->ConActive())return false;
	if(rel<1)rel=1;
	else if(rel>4)rel=4;	

	return SendCommand("REL" + Int2Str(rel) + ".ON");
}


bool uk1104Connector::RelOff(int rel)
{
	if(HaltAll || !m_serialcon->ConActive())return false;
	if(rel<1)rel=1;
	else if(rel>4)rel=4;	

	return SendCommand("REL" + Int2Str(rel) + ".OFF");
}




bool uk1104Connector::SetChMode(int ch, int mode,int tempres)
{
	if(HaltAll || !m_serialcon->ConActive())return false;
	if(ch<1)ch=1;
	else if(ch>6)ch=6;	
	if(tempres<9)tempres=9;
	else if(tempres>12)tempres=12;
	
	
	bool ret=true;

	ret = SendCommand("CH" + Int2Str(ch) + ".SETMODE("+Int2Str(mode)+")");
	

	
	if(ret && mode==4) //set temp precision to max
	{
		ret = SendCommand("CH" + Int2Str(ch) + ".SETTEMPRES("+Int2Str(tempres)+")");		
	}
	


	return ret;
}




float uk1104Connector::GetTemp(int ch)
{
	if(HaltAll || !m_serialcon->ConActive())return -999;
	if(ch<1)ch=1;
	if(ch>6)ch=6;
	
	
	int ret=0;
	
	string tempstr;
	
	
	SendCommand("CH" + Int2Str(ch) + ".GETTEMP");
	
	tempstr = RecvOutput(true);
	
	
	//cout<<"preval:["<<tempstr<<"]"<<endl;//debug
	
	
	tempstr=ClearNonNum(tempstr);
	

	//cout<<"val:["<<tempstr<<"]"<<endl; //debug
	
	return atof(tempstr.c_str());
}




string uk1104Connector::GetRels()
{
	if(HaltAll || !m_serialcon->ConActive())return "";
	
	
	
	
	string tempstr;	
	
	if(!SendCommand("rels.get"))return "";
	
	tempstr = RecvOutput(true);
	
	string ret;

	for(int i=0;i<tempstr.size();i++)
	{
		if(tempstr[i]=='0')ret.append(1,'0');
		else if(tempstr[i]=='1')ret.append(1,'1');
	}
	
	
	
	return ret;
}

 

[Ken g6]

Tl;dr, but...

if you printf("print\rthis"), \r will return you to the beginning of the line without making a newline. So you'll get:

thist

Could that be related to your problem?

 

[Red Squirrel]

I use \r\n everywhere so don't think it would be that. Though I'm also wondering, should I maybe be using \n only?

 

[degibson]

I use \r\n everywhere so don't think it would be that. Though I'm also wondering, should I maybe be using \n only?

If you're writing C++, use std::endl. Otherwise, \n only, or gtfo.

 

[Red Squirrel]

Actually I was not sure if I could use endl for serial connection, so that will work? What is that equivalent to, is it just \n?

 

[AyashiKaibutsu]

Actually I was not sure if I could use endl for serial connection, so that will work? What is that equivalent to, is it just \n?

I think it depends on the machine it's complied on whether it's \n or \r\n, but I do very little in C++ so not sure.

 

[Red Squirrel]

Wouldn't it be more specific to what I'm communicating with? Serial vs file, OS? etc. For example in windows a file it's \r\n but in Linux it's \n. What is typical for serial? Or does it actually depend on the device that I'm connecting to? \r\n seems to be working for this device. When I use an app like picocom what does it send when I press enter?

Though, I don't think the issue I'm running into has to do with the return sequence. 99% of the time it works, it's 1% where it just randomly screws up. I made a "fix" where if it does not get the value, it tries again. So far it seems to work and does not seem to mess up twice in a row. Though this does feel like a big hack job....

I have a feeling my issue is with waiting for EOF. Maybe I'm doing it wrong.

 

[degibson]

Wouldn't it be more specific to what I'm communicating with? Serial vs file, OS? etc. For example in windows a file it's \r\n but in Linux it's \n. What is typical for serial? Or does it actually depend on the device that I'm connecting to? \r\n seems to be working for this device. When I use an app like picocom what does it send when I press enter?

We were talking about printf(). Some random device may do some random thing... you never know.

Though, I don't think the issue I'm running into has to do with the return sequence. 99% of the time it works, it's 1% where it just randomly screws up. I made a "fix" where if it does not get the value, it tries again. So far it seems to work and does not seem to mess up twice in a row. Though this does feel like a big hack job....

I haven't looked through your code, but it sounds racey to me. Look into how you're handling EAGAIN and see if it makes sense.

I have a feeling my issue is with waiting for EOF. Maybe I'm doing it wrong.

You shouldn't be getting EOFs at all on a serial device, unless maybe you're disconnecting the device or it's sending EOF characters. That would probably show up as a read() failure, or not at all.

 

[Red Squirrel]

What is the best way to look for EOF then? Basically how do I know when to stop reading data before sending another command? do I need to just know exactly what the output is going to be and wait for it? That's sorta what I'm doing now but it seems to randomly vary and it throws the program off.

 

[Mark R]

What is the best way to look for EOF then? Basically how do I know when to stop reading data before sending another command? do I need to just know exactly what the output is going to be and wait for it? That's sorta what I'm doing now but it seems to randomly vary and it throws the program off.

You can't look for EOF, as there isn't such a thing. Code runs much faster than serial ports, so just having an empty buffer doesn't mean that there is no more data to come.

The only real way to do it is to look for something that marks the end of the data (e.g. a return/newline) and once you see it, then stop.

Then, to make sure that you are in sync, you could add a delay, and then use a piece of code to make sure the read buffer is empty (draining it if necessary), before sending each command.

I'm just wondering if your processing of the output (to remove the echoed command) is causing problems. You send the command with a CRLF after it. This gets echoed back, but you stop reading after a CRLF. This could be why you don't pick up the output.

I would see what your raw unfiltered data from the serial port looks like.

 

[Red Squirrel]

You know what, it's funny because I had second guessed I should be calling readline twice, but since it worked with doing it once I never really looked back. I'm trying to do it twice now to see if it helps. Still does not explain the random nature of it though. Like one day it only errored out once (where it does not get a result) today it did it about 6 times... every day is random. (running app 24/7).

I am now doing it twice and I'll see what happens. Because I'm using non blocking mode if there's no data in the queue it wont stay stuck, so at least there's that. Better to grab "too much" than not enough.

 

[Red Squirrel]

I have another issue now, I figured I would use the same thread as I already posted the code.

The issue may be partially related to the board itself that I'm communicating with, but thing is, the issue does not happen when I use picocom, so it's really weird.

The issue is, after a server reboot, or if the device is disconnected a few times, the SerialConnector::ReadLine(bool waitindef,bool nlonly) will get stuck in some weird loop where the device is spitting out a crap ton of random text data. Basically in it's specific language, but all broken up. It is caught by the hard coded limit of 1024 chars.

The uk1104Connector::RecvOutput() function will then detect that state as when the device starts outputting lot of junk, the word error always shows up a bunch of times. Basically it almost acts as if the operator is just inputting lot of random data so the device is not understanding the commands and outputting that it's an error.

When the device falls into this state where it just acts all weird, it still works fine in picocom. But if I do "echo rels.get > /dev/ttyACM0" (where rels.get is a command) then do "cat /dev/ttyACM0" it will also do the same thing.

The thing that gets me though is that picocom seems to be safe from this behavior. I'm sifting through it's code to try to figure out how they did the serial communication. Wondering if anything in my code pops out as being wrong? Maybe I'm not retrieving data properly? Is my way maybe sending some kind of control character which then makes it get stuck in that loop? I'm not explicitly sending anything, but maybe in the back end one of the functions/options I'm using is causing it? Just wondering.

 

[Red Squirrel]

Ok I think I may have figured something out. I still don't fully understand what is going on, but after a couple server reboots it seems to be good. I changed my connect function as so:

bool SerialConnector::Open()
{
	if(m_Path=="")
	{
		ScreenOutput("No path specified");
		return false;
	}


	
	//init stuff
	
	struct termios tio;
	//struct termios stdio;
	//int tty_fd;
	//fd_set rdset;


	//memset(&stdio,0,sizeof(stdio));
	//stdio.c_iflag=0;
	//stdio.c_oflag=0;
	//stdio.c_cflag=0;
	//stdio.c_lflag=0;
	//stdio.c_cc[VMIN]=1;
	//stdio.c_cc[VTIME]=0;
	//tcsetattr(STDOUT_FILENO,TCSANOW,&stdio);
	//tcsetattr(STDOUT_FILENO,TCSAFLUSH,&stdio);
	//fcntl(STDIN_FILENO, F_SETFL, O_NONBLOCK);       // make the reads non-blocking	


	memset(&tio,0,sizeof(tio));
	tio.c_iflag=0;
	tio.c_oflag=0;
	tio.c_cflag=CS8|CREAD|CLOCAL;           // 8n1, see termios.h for more information
	tio.c_lflag=0;
	tio.c_cc[VMIN]=1;
	tio.c_cc[VTIME]=5;


	//connect
	m_fd=open(m_Path.c_str(), O_RDWR | O_NONBLOCK);	
	
	
	
    cfsetospeed(&tio,B115200);            // 115200 baud
    cfsetispeed(&tio,B115200);            // 115200 baud	

	//this is required, it seems that it only needs to be called once per server reboot. It sets some kind of global thing that prevents the serial device (in my test: uk1104 from canakit) to go insane and spam junk
	tcsetattr(m_fd,TCSANOW,&tio);
	
	if (m_fd == -1 )
	{
		ScreenOutput("Unable to connect");
		m_conactive=false;
		return false;
	}

	m_conactive=true;
	
	return true;
}

The key code is

	memset(&tio,0,sizeof(tio));
	tio.c_iflag=0;
	tio.c_oflag=0;
	tio.c_cflag=CS8|CREAD|CLOCAL;           // 8n1, see termios.h for more information
	tio.c_lflag=0;
	tio.c_cc[VMIN]=1;
	tio.c_cc[VTIME]=5;



	
    cfsetospeed(&tio,B115200);            // 115200 baud
    cfsetispeed(&tio,B115200);            // 115200 baud

It seems when this code gets executed by any program, it fixes the problem for that session. I grabbed this from an online example and when I compiled it stand alone, my program magically started to work. So I threw it into my compile function and voila, it works! Wow, so glad I got this figured out, it was really starting to stress me.

It still leaves me lot of unanswered questions, but at this point I'm just glad it works... running a test run of my app now after a couple reboots and so far so good.