Waiting for Signal using local Eventloop using multiple QThreads

kain

Hello,

I am writing an application which communicates to other modules via QSerialPort. This communication is in an ordinary Request=>Response style.
This communication is done on a single thread/object which has a Slot for sending and a Signal for receiving data.

I want to use that Signal/Slot in a synchronous way and am using a local QEventLoop to wait for that 'received' signal:

template<typename T1, typename T2> signed short sendReceive(unsigned char sendCommand, unsigned char destAddress, T1 sendItem = nullptr, T2 recvItem = nullptr) {
	// We will wait for the answer or timeout. For that, use EventLoop
	QTimer timer;
	timer.setSingleShot(true);
	QEventLoop loop;
	signed short ret;
	QMetaObject::Connection * const connection = new QMetaObject::Connection;
	*connection = connect(mmcom.get(), &MultiMasterCommunicator::dataReceived,
			[&ret, &recvItem, sendCommand, connection, this] (unsigned char command, QByteArray recvBuffer) {
		if (command == sendCommand) {
			if (recvItem != nullptr)
				memcpy(recvItem, recvBuffer.data(),recvBuffer.length());
			//*recvItem = *(reinterpret_cast<T2>(recvBuffer.data()));
			ret = PR0142PERIPHCOMERROR_NO_ERROR;
		} else {
			ret = PR0142PERIPHCOMERROR_UNDEFINED_COMMAND_RESPONSE;
		}
		QObject::disconnect(*connection);
		delete connection;
		emit dataReceived();
	});
	connect(this, SIGNAL(dataReceived()), &loop, SLOT(quit()));
	connect(&timer, SIGNAL(timeout()), &loop, SLOT(quit()));
	send (sendCommand, destAddress, sendItem);
	qDebug() << "Send Command " << sendCommand << " from Thread " << QThread::currentThreadId();
	timer.start(CONFIG_RECEIVE_TIMEOUT);
	loop.exec();
	// TODO: Handle Timeouts
	qDebug() << "RECV Command " << sendCommand << " from Thread " << QThread::currentThreadId();;
	return ret;
}

The above function is called from different slots like the following:

signed short PCom::GetFullDeviceInfo(FULL_DEVICE_INFO *pstrFullDeviceInfo, unsigned char ucAddress)
{
    return sendReceive(COMMAND_GET_FULL_DEVICE_INFO, Address, nullptr, pstrFullDeviceInfo);
}

All these slots belong to a single QObject in a seperate Thread. The slots are invoked via BlockingQueuedConnections from different threads.
However, this does not work as I expect: This solution works well if only one thread is invoking those slots but I get SIGVAULTS if multiple threads are invoking those slots.
From the debug-info above I would expect that after each 'Send Command' the corresponding "RECV Command" would follow, but instead I get this:

Send Command  16  from Thread  0x5e3c
RECV Command  16  from Thread  0x5e3c
Send Command  16  from Thread  0x5e3c
RECV Command  16  from Thread  0x5e3c
Send Command  16  from Thread  0x5e3c
Send Command  16  from Thread  0x5e3c
RECV Command  16  from Thread  0x5e3c
RECV Command  16  from Thread  0x5e3c
Send Command  17  from Thread  0x5e3c
Send Command  16  from Thread  0x5e3c
RECV Command  16  from Thread  0x5e3c
RECV Command  17  from Thread  0x5e3c
Send Command  16  from Thread  0x5e3c
Send Command  16  from Thread  0x5e3c
RECV Command  16  from Thread  0x5e3c
Send Command  16  from Thread  0x5e3c
RECV Command  16  from Thread  0x5e3c
RECV Command  16  from Thread  0x5e3c

As you can see, there are multiple 'Send' following each other and multiple "RECV" too. Does anyone know what the mistake is here and at best a possible solution?

aha_1980

Hi @kain,

First I would like you to over-think the architecture.

Serial ports are slow, so having a blocking function to send a request/waiting for an answer will lead to bad performance. Therefore I cannot help you in this direction.

The better approach is to give a command to the object that manages the serial port, and receive an signal when the answer is ready. That way can be used without or with threads with very good performance. If you give the command as well as the answer by signals, this is automatically thread-safe.

Regards

kain

Hey @aha_1980 ,

thank you for your reply.
I am aware of that architecture and that synchronous calls are some kind of bad practice.
But: I need that mechanism that always only one object/worker is communicating on that SerialBus, i.e. that no one sends a second command before the answer of the first command is completely received. So I would use some synchronization-mechanism via Mutex anyway. Since I hide that synchronous calls in a seperate thread on a lower level I dont really care for the performance here.
My problem here is that the needed blocking-mechanism does not work as I would expect it. Since the above function is only called from one thread I would expect that 'Send' and 'Receive' would only appear in pairs (Send->Receive->Send->Receive..), but that is not the case.

If I would try to implement this in an asynchronous manner I would only shift that problem to a higher level of abstraction.

If that is not possible I would be happy for some advise for how to refactor that code. Right now I am already using signals for sending and receiving data:

template<typename T1> void send(unsigned char command, unsigned char destAddress, T1 sendItem = nullptr) {
        auto sendBuffer = QByteArray::fromRawData(reinterpret_cast<const char*>(&sendItem), sizeof(T1));
        emit sendCommand(command, destAddress, sendBuffer);
    }

So the sending and receiving is done in another QObject located on another thread.

Best Regards

fcarney

I took a look at QSerialPort. It seems like it might be possible to do polling to wait for the data to come back. Is that a possibility or do you need to make it event based for some reason?

dheerendra

@kain
According to your logic it should work like send->recv->send->recv. Mismatch is only possible if they are executed from different threads. But thread is also same. This may not be issue.

>emit sendCommand(command, destAddress, sendBuffer);

My suspect is the above send. Can you tell me where is the connect for above signal ? I have strong feeling is that slot for the above signal is in different threads. Are you using the BlockedQueuedConnection for the above statement ?

aha_1980

@kain said in Waiting for Signal using local Eventloop using multiple QThreads:

I am aware of that architecture and that synchronous calls are some kind of bad practice.

If I would try to implement this in an asynchronous manner I would only shift that problem to a higher level of abstraction.

No, that is not true. It simple depends on the architecture of your program. Qt provides asynchronous classes for exactly that kind of problem. There are a lot of network or serial port examples that show how to communicate that way.

But: I need that mechanism that always only one object/worker is communicating on that SerialBus, i.e. that no one sends a second command before the answer of the first command is completely received.

Fair point. But as the communication class is aware of that, all you need is a input FIFO that fills with commands and send the commands when the serial port is free to do so.
When the answer is in, you give a signal containing the answer and continue sending the next command.

So I would use some synchronization-mechanism via Mutex anyway.

If you use signals&slots, no.

Since I hide that synchronous calls in a seperate thread on a lower level I dont really care for the performance here.

If it is really like that, why don't you use the waitForBytesWritten and waitForReadyRead functions? They will simplify your code a lot as you don't need the local event loop anymore.

Regards

kain

Hey,

first of all thank for the comments so far.
@dheerendra
This is how I connect my sendcommand to the object that handles the SerialBus communication:

connect(this, &PeripherieCommunicator::sendCommand, this->mmcom.get(), &MultiMasterCommunicator::send, Qt::ConnectionType::BlockingQueuedConnection);

It seems to me that when I use the QEventLoop for blocking the parent object returns to it own EventLoop and thus acceptinga new command resulting in a second "Send" before finishing the first "Receive". Is that possible?

@aha_1980 :
You are correct but to be honest I do not know how I would change the architecture to use only that non-blocking mechanisms. For example at some high layer I got some logic that move an axis using a motor, like (PSEUDO-CODE)

int MoveAxisToPos(long pos);

This would call some code which would do something like:

int EnableAxis(bool someParam);
int StartMovingOfAxis();
do 
{
  auto finished = ReadAxisState();
} while (>0)
if (finished < 0) emit error();
else emit success();

and each of these commands would result in some communication on that QSerialBus and that QSerialBus is going to be called from multiple threads (each of them controlling some device on that SerialBus). So I would connect and disconnect my signals/slots all the time. Also for each command I would need to create a seperate (lamba?) function that handles the answer of the underlying layer. With many commands that would result in some nested functions or using some kind of stateMachine (like QStateMachine), so every Command would be a seperate StateMachine. This for me seems to a big coding-overhead and that is why I chose to use that blocking/synchronous approach like above.

aha_1980

Hi @kain,

Are you talking about QSerialBus or QSerialPort? That are two different things!

and that QSerialBus is going to be called from multiple threads

Aha! In you last post, you said that only one thread is involved?! How does that fit together? And do you really need multiple threads? Why?

So I would connect and disconnect my signals/slots all the time.

No!

This for me seems to a big coding-overhead and that is why I chose to use that blocking/synchronous approach like above.

I'm pretty sure you just think too complicated. Because what you do now is big overhead.

Probably it's better to first explain in words what exactly has to be done.

Regards

PS: Also note that in your last example you mixing Signals&Slots with blocking operations and that very likely to not work.

dheerendra

I feel your program does not work the way you wanted. Look at the following
facts.

sendReceive(..) method is called by - Thread 0x5e3c
->connect(mmcom.get(), &MultiMasterCommunicator::dataReceived
Above connect statement indicates that slot needs to be executed. This slots needs to be executed in the context of Thread 0x5e3c only.
You start the event loop here. So your Thread - 0x5e3c blocked here. It can exit only
a. if the timeout or
b. datareceived() signal of current object. This will never happen becz signal datareceived() should come from execution for lamda slot by Thread-0x5e3c.
->connect(this, &PeripherieCommunicator::sendCommand, this->mmcom.get(), &MultiMasterCommunicator::send
This connect indicates that this->mmcom.get() object returned by this method is owned by another thread. Here again you would like to block yourself till the serial data collector finishes the work.
I feel most the time your event loop exits becz of timeout rather than the signal dataRecved(..)

It seems to me that when I use the QEventLoop for blocking the parent object returns to it own EventLoop

It will be different eventloop for this purpose. It is not mixed with main event loop.

Issue here is that thread which is blocked due to eventloop,

may receive another sendRequest(..).
Then it receives this dataReceived(..) signal may serial data collector thread.

The moment Thread-0x5e3c is free because of Timeout, main eventloop starts the work. It picksup the sendRequest which is first event. This results in one more send.

In general I felt logic/flows mixed up with so many blockings. Current logic blocks the same thread multiple times. Your idea is that Request to Serial Data Collection should go in sequence & get the response in serial mode.

It can be simplified lot better with simple architecture. Signal/Slots provide simple mechanism to achieve this across the thread.

dheerendra

Another thing I forgot add is that your send method blocked before exec itself. Event loop starts only after data reading. This makes eventloop dummy. It does not serve any purpose.

dheerendra

What happened to this issue ? Are you able to solve the issue ?

kain

I reworked my Class that handles my QSerialPort to just be synchronous using "waitReadyRead), etc since I blocked in the calling class anyway.
Then I changed my SendReceive-function to just that synchronous functions, that way I get rid of my local Eventloop.
So far this solution works for me as expected.

J.Hilk

@kain
QAbstractSocket, that also inherits from QIODevice has this note in the waitForReadyRead documentation

Note: This function may fail randomly on Windows. Consider using the event loop and the readyRead() signal if your software will run on Windows.

granted QSerialPort is not an QAbstractSocket, so I'm nearly 100 % sure it won't effect you, but just in case you run into unexpected timeouts, keep that in mind ;)

Also don't forget to change the topic to sovled, if your problem/answer is actually solved.