Click here to Skip to main content
15,904,416 members
Please Sign up or sign in to vote.
4.00/5 (1 vote)
I'm trying to write a class to deal with receiving a GPS signal over a serial connection. For the serial talk I use the CSerial class which so far I can warmly recommend.

The idea was to create a "listener" as part of a class constantly receiving incoming chatter. Then translate it to a "GPGGAMessage struct".

There are two issues which I am aware of with my current solution:

1. Communication with the thread:
In GPSCommunication::stopListening() I just set listeningThreadEnd = true which will cause to terminate the thread after one more while-loop.
It's important to terminate the thread before serialPort.Close() is called, so I just wait a second in between.
Kind of an unsafe solution, are there suggestions for safer ways to do this, i.e. better ways to communicate with the thread?

2. Thread safety:
So the only thing I will call manually regulary is the getCurrentGPGGAMessage() function. This accesses currentGPGGAMessage which is often changed from within the thread. What is the "clean" way to make sure no writing/reading occurs simultaneously?

class GPSCommunication {
	public:
		GPSCommunication();
		bool startListening();
		bool stopListening();
		GPGGAMessage getCurrentGPGGAMessage();

	private:
		CSerial serialPort;
		GPGGAMessage currentGPGGAMessage;
		bool listeningThreadEnd;
		CWinThread* pListeningThread;

		friend UINT listeningThread(LPVOID pClassInstance);
		bool readSerial(char* message);
                void translateGPGGA(char message[100]);
};


GPSCommunication::GPSCommunication()
{
	CSerial serialPort;
}

bool GPSCommunication::startListening()
{
	
	listeningThreadEnd = false;
	std::cout << "Try to open Serial Port ..." << std::endl;
	
	serialPort.Open(_T("COM1"));

	if (serialPort.IsOpen())
	{
		std::cout << "Serial Port Open" << std::endl;
		
		serialPort.SetMask();

		pListeningThread = AfxBeginThread(listeningThread, this);
		return true;
	}
	else
	{
		std::cout << "Serial Port NOT Open" << std::endl;
		Sleep(1000);
		return false;
	}
}

bool GPSCommunication::stopListening()
{	
	listeningThreadEnd = true;
	Sleep(1000);
	serialPort.Close();
	
	return true;
}

UINT listeningThread(LPVOID pClassInstance)
{	
	GPSCommunication* pInstance = (GPSCommunication*) pClassInstance;
	
	char  message[100];

	while(!(pInstance->listeningThreadEnd))
	{
		if (pInstance->readSerial(message)) 
		{
			pInstance->translateGPGGA(message);
		}
		Sleep(50);
	}

	std::cout << "listeningThread terminates"<< std::endl;
	return 0;
}

GPGGAMessage GPSCommunication::getCurrentGPGGAMessage()
{
    return currentGPGGAMessage;
}
Posted

1. You can duplicate thread handle and use it with WaitForSingleObject() function as described here[^]. Generally (regarding other cases, when you just need to wait for some time - your listeningThread function, for example), using Sleep() is not recommended, consider using WaitForSingleObject with dummy handle (event never set user, for example) instead

2. Lock currentGPGGAMessage, obviously :). Use critical sections or mutexes to lock and unlock you shared object in any read/write operation. If possible, it would be good to rewrite your getCurrentGPGGAMessage() in the following manner: you lock currentGPGGAMessage object, for example using EnterCriticalSection, then create its copy, unlock object (LeaveCriticalSection) and return created copy as result. All operations modifying currentGPGGAMessage should also be synchronized, of course. This way you don't need to bother about your object and access to it anywhere outside your class
 
Share this answer
 
Comments
tschoni 31-May-11 22:25pm    
Regarding 1:

The link provided explains the solution well. One thing that is discussed is too is because
Thread->m_bAutoDelete = FALSE;
is used the Thread handle needs to be deleted manually.
The way I use this in a class I thought the best way to do it is using new directive when creating the thread handle in the constructor and use delete directive in the deconstructor.
This however still results in a memory leak caused by the not properly deleted thread handle, why is this?

Why is it not recommended to use Sleep();? And how to easiest create a safe dummy handle?
Timberbird 1-Jun-11 2:51am    
Is it memory leak or handle leak (number of handles used by your process increases on start/stop threads)? If latter, this is usually due to missing CloseHandle() function. To clarify things, could you please provide code for your thread object (create, start, wait, delete)?

I personally avoid Sleep() because of several limitations: actual amount of time it waits may slightly differ from the value you passed, and it makes your thread to skip current time slice no matter what (as described in MSDN ). On the other hand, I believe Sleep() requires less system resources so there are cases when its usage makes sense. A simple way to wait is to create an event (outside any loops that will wait for it, of course, to reduce resource consuming) and then call WaitForSingleObject() repeatedly:

HANDLE dummyHandle = CreateEvent(NULL, TRUE, FALSE, NULL);

while()
{
...

WaitForSingleObject(dummyHandle, AMOUNT_OF_TIME_YOU_NEED_TO_WAIT);
}

CloseHandle(dummyHandle);

This realization, by the way, comes very handy when you need to exit loop by external event, either system (caused by waitable object - thread, process, timer, console and so on) or coming from your another thread.
tschoni 1-Jun-11 3:22am    
thanks for the elaborate reply, please note that I posted a 'answer' which is actually a reply to your comment ... just to make code more readable
tschoni 31-May-11 23:55pm    
Regarding 2:

At first it was unclear to me what the difference between mutex'es and critical sections are.
A good discussion on the topic can be found here:
http://www.codeguru.com/forum/showthread.php?t=333192
Summarized: Use critical sections if resources are shared only within one process.

On how to use the critical secions: the simplest syntax (or the one that suits me most) I found in this example here - very straightforward
http://msdn.microsoft.com/en-us/library/ms686908%28v=vs.85%29.aspx
NOTE: This is a reply to Timerbirds comment, posted as 'answer' simply to make code easier to read

That's the message about the memory leak. I never dealt with handle leaks before, so I'm not sure if, if it says 'memory leak', it can still be a handle leak. Can it?

Detected memory leaks!
Dumping objects ->
{170} client block at 0x0039FC48, subtype c0, 68 bytes long.
a CWinThread object at $0039FC48, 68 bytes long

Below see how I hoped to prevent the leak.
Allocate using new directive in the class constructor.
Deleting again in the deconstructor.

GPSComm::GPSComm()
{
    CWinThread* pListeningThread = new CWinThread;
}

GPSComm::~GPSComm()
{
    delete pListeningThread;
}

bool GPSComm::startListening()
{
        listeningThreadEnd = false;
        pListeningThread = AfxBeginThread(listeningThread, this, 0, 0, 
                              CREATE_SUSPENDED, NULL);
        pListeningThread->m_bAutoDelete = FALSE;
        pListeningThread->ResumeThread();

}

C#
bool GPSComm::stopListening()
{
    listeningThreadEnd = true;
    // Wait until listeningThread properly terminates
    WaitForSingleObject( pListeningThread->m_hThread, 5000 );
    return true;
}

C#
UINT listeningThread(LPVOID pClassInstance)
{
    GPSComm* pInstance = (GPSComm*) pClassInstance;

    while(!(pInstance->listeningThreadEnd))
    {
         // do stuff
    }
    return 0;
}
 
Share this answer
 
Comments
Timberbird 1-Jun-11 4:05am    
Oh I see. Looks like the problem is in CWinThread* being overridden and lost. Let's see: you create new CWinThread object (let's call it "extra" object) in your constructor and set your pListeningThread variable to point to this object: CWinThread* pListeningThread = new CWinThread; Then, in startListening() method, you call AfxBeginThread() and override value stored in pListeningThread with pointer returned by that function. Now, there are two objects residing in memory: extra object, created in the constructor (and not used anywhere else - pointer to that object is lost by that moment) and the one created by AfxBeginThread() - let's call it "system" object. Finally, in your destructor you call delete pListeningThread;, but pListeningThread points to system objects; memory occupied by system object is released, but your extra object still stays in memory, lost and lone, until your process ends :). So the answer is simple - do not create extra object, leave the constructor empty
tschoni 1-Jun-11 4:47am    
it's as always very clear if someone explains it properly :)
thanks a lot for all your help!!!
all questions answered, everyone happy! (everyone being me)

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900