External io tcp support #169

lcgamboa · 2021-11-11T00:48:07Z

Initial release of IPC support. I only tested on Linux. After unraveling the obscure MSVC compiler error messages the code compiles in github actions. But I don't have MSVC available to test here.
The server for testing is on another branch, I don't think it's interesting to add it to the main Ripes version.

mortbopet · 2021-11-11T08:29:13Z

The server for testing is on another branch, I don't think it's interesting to add it to the main Ripes version.

I think since it seems like we're going to go with a non-Qt solution for the tcp client, i think it would be good to have proper tests of the solution so we can verify it on all platforms. I imagine a new tst_xtcpsocket.cpp test which could use this as a dummy server.

mortbopet

As i wrote above, i think having unit tests for the tcp client would be very good to ensure that this works cross-platform. A few comments on style, code cleanup and C++ norms.

src/io/VBus.h

src/io/XTcpSocket.cpp

src/io/ioexternalbus.ui

src/io/ioexternalbus.h

src/io/ioexternalbus.cpp

src/io/XTcpSocket.cpp

test/tst_xtcpsocket.cpp

mortbopet · 2021-12-12T08:56:52Z

@lcgamboa could you elaborate a little bit on what/where crashes are triggered?

mortbopet

Most of the comments are stylistic. A general comment would be to add a few more comments across the code to explain what is going on - makes reviewing, and maintaining(!) things a lot easier.

src/io/VBus.h

src/io/XTcpSocket.cpp

src/io/ioexternalbus.cpp

src/io/ioexternalbus.h

test/tst_xtcpsocket.cpp

lcgamboa · 2021-12-13T01:28:23Z

I made all the modifications you requested. Sorry for the lack of comments in the source code, it really is my fault for not being used to working in a team.
As for errors, they do not occur in normal operation.
A simple way to test is to close the connection during Ripes' No-GUI simulation.
To do this, just press the reset button on the PICSImLab board which resets the connection.
When the connection error occurs sometimes Ripes suffers a segmention fault instead of just showing the error message.

src/io/ioexternalbus.h

mortbopet · 2021-12-13T07:51:28Z

I made all the modifications you requested. Sorry for the lack of comments in the source code, it really is my fault for not being used to working in a team.

Thanks! don't worry too much about it, it's just another benefit of doing collaborative open source work that the people involved gain experience in giving, getting, and acting on feedback!

A simple way to test is to close the connection during Ripes' No-GUI simulation. To do this, just press the reset button on the PICSImLab board which resets the connection. When the connection error occurs sometimes Ripes suffers a segmention fault instead of just showing the error message.

I will try this out once i have the chance 👍. I would also recommend rebasing your branch to master since there was a bit of a nasty bug in the InstructionMatcher which caused some issues.

lcgamboa · 2021-12-13T17:19:27Z

Yes I have learned a lot working on Ripes.
As for the rebase, as my branch is based on your external_io branch, I believe you have to rebase it first in relation to the master.

mortbopet · 2021-12-13T19:30:28Z

As for the rebase, as my branch is based on your external_io branch, I believe you have to rebase it first in relation to the master.

You're right - should be done now 👍 .

src/io/XTcpSocket.cpp

mortbopet · 2022-03-04T08:44:44Z

Would you mind doing a rebase onto the current master branch? I think you've merged the master branch into this branch. By doing this, all changes in the master branch is also seen as changes in this PR, making it hard to review the code (i cannot discern your contributions from the additions in the master branch).

src/io/ioexternalbus.cpp

mortbopet · 2022-03-04T08:58:56Z

src/io/ioexternalbus.cpp

+                    QJsonObject osymbols = desc.object().value(QString("symbols")).toObject();
+
+                    m_regDescs.clear();
+                    for (QJsonObject::iterator i = osymbols.begin(); i != osymbols.end(); i++) {


Does range notation not work for a QJsonObject?

for(auto& it : osymbols) ...

I couldn't use it and I didn't find any example that uses it.
The range notation (using const auto&) returns an object of type QJsonValueRef which I was unable to convert to use.

src/io/ioexternalbus.cpp

src/io/ioexternalbus.h

…variable/MSVC_VERSION.html#variable:MSVC_VERSION

lcgamboa · 2023-05-01T17:28:15Z

I added support for passing simulation time to PICSimLab to synchronize simulators. Is there an easy way to be able to use the instruction counter value instead of the constants I added for testing?

https://github.com/lcgamboa/Ripes/blob/c3a4e5ca0031db5711a7cbded3e7378d6fb25320/src/io/ioexternalbus.cpp#L54

mortbopet · 2023-05-05T09:41:25Z

@lcgamboa the processor interface contains a hook to get the current cycle count, which can be used through the processor handler.

lcgamboa · 2023-05-06T02:31:11Z

@mortbopet I ended up using the wall time instead of the instructions counter, it got better result since Ripes doesn't use a fixed simulation frequency.

I followed your guidance and tried to use QTcpSocket instead of the XTcpSocket class I created. But I keep having deadlock problems because of Qt's sockets implementation and the ioRead and ioWrite implementation in Ripes.
Running one cycle at a time works fine.
Running in animation mode with an interval greater than 150ms works without problems, with smaller intervals occasionally deadlock occurs as several threads try to use the ioRead and ioWrite methods simultaneously.
In no-gui fast mode it works until you try to pause the simulation. When the pause is triggered several threads try to use the ioRead method at the same time and deadlock always occurs.

As it is necessary to send a command via TCP and wait for the response within the ioRead and ioWrite methods, the only possible way I found to wait is to use a QEventLoop.
To solve the problem, it would be necessary to manage at a higher level so that there was no access to the ioRead and ioWrite methods by several threads simultaneously or change the form of request of the ioRead and ioWrite methods to generate a trigger signal and wait for another signal response.
I don't have enough Ripes knowledge to make that kind of change and not whether the work would pay off. Maybe using XTcpSocket isn't such a bad idea for now.

If you want to take a look at the code using QTcpSocket it's in this branch https://github.com/lcgamboa/Ripes/blob/QTtcpSocket/src/io/ioexternalbus.cpp .

mortbopet · 2023-05-06T07:52:19Z

But I keep having deadlock problems because of Qt's sockets implementation and the ioRead and ioWrite implementation in Ripes.

Could you explain this a bit more? I'm assuming that the control flow of a simulation is:

code executes in ripes
an MMIO-mapped access is [read | written] to the external bus
this feeds into ioWrite/ioRead functions
This creates a tcp message which eventually will return - the Ripes simulation is stalled in the ioWrite/ioRead command while this is going on.
The TCP request finishes, and the simulation progresses.

Where does the deadlock come into play? where are the multiple threads coming from/doing, which tries to access the bus concurrently?

lcgamboa · 2023-05-06T12:59:19Z

Apparently the problem is between step 4 and 5, the Ripes simulation is not always stalled.

Running step by step everything works, several threads access the ioRead and ioWrite methods.

ioRead  start threadID= QThread(0x555556124120)  simtime=  1683376517233706956
ioRead  stop  threadID= QThread(0x555556124120)  simtime=  1683376517233706956
ioRead  start threadID= QThreadPoolThread(0x5555572a6d90, name = "Thread (pooled)")  simtime=  1683376532943357397
ioRead  stop  threadID= QThreadPoolThread(0x5555572a6d90, name = "Thread (pooled)")  simtime=  1683376532943357397
ioRead  start threadID= QThreadPoolThread(0x55555675eb30, name = "Thread (pooled)")  simtime=  1683376533556835921
ioRead  stop  threadID= QThreadPoolThread(0x55555675eb30, name = "Thread (pooled)")  simtime=  1683376533556835921
ioRead  start threadID= QThreadPoolThread(0x5555572b7ab0, name = "Thread (pooled)")  simtime=  1683376535504702982
ioRead  stop  threadID= QThreadPoolThread(0x5555572b7ab0, name = "Thread (pooled)")  simtime=  1683376535504702982
ioRead  start threadID= QThread(0x555556124120)  simtime=  1683376535561063213
ioRead  stop  threadID= QThread(0x555556124120)  simtime=  1683376535561063213

In animation mode apparently the main thread tries to use ioWrite before the pooled thread finishes ioRead:

ioRead  start threadID= QThread(0x555556124120)  simtime=  1683376663470320350
ioRead  stop  threadID= QThread(0x555556124120)  simtime=  1683376663470320350
ioRead  start threadID= QThreadPoolThread(0x5555572ac290, name = "Thread (pooled)")  simtime=  1683376665338831463
ioRead  stop  threadID= QThreadPoolThread(0x5555572ac290, name = "Thread (pooled)")  simtime=  1683376665338831463
ioRead  start threadID= QThreadPoolThread(0x55555725f150, name = "Thread (pooled)")  simtime=  1683376665373060131
ioRead  stop  threadID= QThreadPoolThread(0x55555725f150, name = "Thread (pooled)")  simtime=  1683376665373060131
ioRead  start threadID= QThreadPoolThread(0x5555571802a0, name = "Thread (pooled)")  simtime=  1683376665438149355
ioRead  stop  threadID= QThreadPoolThread(0x5555571802a0, name = "Thread (pooled)")  simtime=  1683376665438149355
ioRead  start threadID= QThread(0x555556124120)  simtime=  1683376665472576233
ioRead  start threadID= QThreadPoolThread(0x555556f71830, name = "Thread (pooled)")  simtime=  1683376665478719220
ioRead  stop  threadID= QThread(0x555556124120)  simtime=  1683376665472576233
ioRead  start threadID= QThread(0x555556124120)  simtime=  1683376665487467718
ioWrite start threadID= QThreadPoolThread(0x555556f71830, name = "Thread (pooled)")  simtime=  1683376701053311357
"Unknown error"
ioRead  stop  threadID= QThread(0x555556124120)  simtime=  1683376665487467718
QIODevice::write (QTcpSocket): device not open

In fast simulation mode, when the pause is triggered apparently the main thread tries to access the ioRead several times.

ioWrite start threadID= QThreadPoolThread(0x55eef4307fe0, name = "Thread (pooled)")  simtime=  1683376957597663202
ioWrite stop  threadID= QThreadPoolThread(0x55eef4307fe0, name = "Thread (pooled)")  simtime=  1683376957597663202
ioWrite start threadID= QThreadPoolThread(0x55eef4307fe0, name = "Thread (pooled)")  simtime=  1683376957598503058
ioWrite stop  threadID= QThreadPoolThread(0x55eef4307fe0, name = "Thread (pooled)")  simtime=  1683376957598503058
ioRead  start threadID= QThreadPoolThread(0x55eef4307fe0, name = "Thread (pooled)")  simtime=  1683376957598720687
ioRead  start threadID= QThread(0x55eef328c120)  simtime=  1683376967598299545
ioRead  start threadID= QThread(0x55eef328c120)  simtime=  1683376967682604900
ioRead  stop  threadID= QThread(0x55eef328c120)  simtime=  1683376967682604900
ioRead  start threadID= QThread(0x55eef328c120)  simtime=  1683376967732737434
ioRead  stop  threadID= QThread(0x55eef328c120)  simtime=  1683376967732737434
ioRead  start threadID= QThread(0x55eef328c120)  simtime=  1683376967762770341
ioRead  stop  threadID= QThread(0x55eef328c120)  simtime=  1683376967762770341
ioRead  start threadID= QThread(0x55eef328c120)  simtime=  1683376967779625643
ioRead  stop  threadID= QThread(0x55eef328c120)  simtime=  1683376967779625643
ioRead  stop  threadID= QThread(0x55eef328c120)  simtime=  1683376967598299545
Falha de segmentação

To wait for the Socket's response, it is necessary to use a local QEventLoop to process the signals and events, I believe that this is why the simulation does not stalled.

mortbopet reviewed Nov 11, 2021

View reviewed changes

mortbopet reviewed Nov 12, 2021

View reviewed changes

src/io/XTcpSocket.cpp Outdated Show resolved Hide resolved

test/tst_xtcpsocket.cpp Outdated Show resolved Hide resolved

lcgamboa requested a review from mortbopet December 11, 2021 00:07

mortbopet reviewed Dec 12, 2021

View reviewed changes

mortbopet reviewed Dec 13, 2021

View reviewed changes

src/io/ioexternalbus.h Outdated Show resolved Hide resolved

mortbopet force-pushed the external_io branch from daad80f to 09b8475 Compare December 13, 2021 19:30

lcgamboa force-pushed the external_io_tcp_support branch from 1d5d11b to 6e48ef1 Compare December 14, 2021 00:59

mortbopet reviewed Dec 18, 2021

View reviewed changes

src/io/XTcpSocket.cpp Show resolved Hide resolved

lcgamboa force-pushed the external_io_tcp_support branch from 6e48ef1 to db91e99 Compare February 7, 2022 18:34

mortbopet requested changes Mar 4, 2022

View reviewed changes

mortbopet and others added 13 commits April 30, 2023 20:46

Initial commit for external bus peripheral

fc7ebc0

initial tcp support

9ae8279

fix windows library in cmakelists.txt

62aa1bc

fix windows library in cmakelists.txt

046e222

fix windows library in cmakelists.txt

0727ebe

fix windows library in cmakelists.txt

014ff6f

fix windows library in cmakelists.txt

2457d7f

fix windows library in cmakelists.txt

205ef81

unused header removed

d18d54c

unused header removed

e6f9ecd

cast added

81d270f

cast added

8583516

use closesocket to MSVC work

ee2590c

lcgamboa and others added 14 commits April 30, 2023 20:47

change char * to QByteArray

24d1e3e

new command VB_QUIT added

9971614

finalize socket

1ac633b

write one full message instead send cmd + payload

1d3ed44

verify server json errors

da838f9

code cleanup and C++ norms

946457d

Go back to _MSC_VER instead MSVC https://cmake.org/cmake/help/latest/…

a08c67a

…variable/MSVC_VERSION.html#variable:MSVC_VERSION

go back with clilen int for MSVC and unsigned int for gcc

38a4b30

variables renamed

e317cfa

cmakelist updated

ae30603

Initial commit for external bus peripheral

5f61e94

update protocol to support simulation time

3e0d103

fix rebase to master

20af59e

fix QMutex crash

6983ca2

lcgamboa force-pushed the external_io_tcp_support branch from 7c2c6b9 to 6983ca2 Compare May 1, 2023 13:14

update ELFIO dependency

244e798

lcgamboa changed the base branch from external_io to master May 1, 2023 16:07

fix mortbopet requests

c3a4e5c

lcgamboa marked this pull request as draft May 1, 2023 17:14

lcgamboa added 5 commits May 1, 2023 14:41

fix clang-format

5059bc3

fix int to QString conversion on Qt6

16b5357

Fix Qt6 QString conversions

11ab37f

fix TCHAR to QString conversion

44114c7

fix TCHAR to QString conversion

809a611

Use current wall time to synchronize simulators

860247c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External io tcp support #169

External io tcp support #169

lcgamboa commented Nov 11, 2021

mortbopet commented Nov 11, 2021

mortbopet left a comment

mortbopet commented Dec 12, 2021

mortbopet left a comment

lcgamboa commented Dec 13, 2021

mortbopet commented Dec 13, 2021

lcgamboa commented Dec 13, 2021

mortbopet commented Dec 13, 2021

mortbopet commented Mar 4, 2022

mortbopet Mar 4, 2022

lcgamboa May 1, 2023

lcgamboa commented May 1, 2023

mortbopet commented May 5, 2023

lcgamboa commented May 6, 2023

mortbopet commented May 6, 2023

lcgamboa commented May 6, 2023

External io tcp support #169

Are you sure you want to change the base?

External io tcp support #169

Conversation

lcgamboa commented Nov 11, 2021

mortbopet commented Nov 11, 2021

mortbopet left a comment

Choose a reason for hiding this comment

mortbopet commented Dec 12, 2021

mortbopet left a comment

Choose a reason for hiding this comment

lcgamboa commented Dec 13, 2021

mortbopet commented Dec 13, 2021

lcgamboa commented Dec 13, 2021

mortbopet commented Dec 13, 2021

mortbopet commented Mar 4, 2022

mortbopet Mar 4, 2022

Choose a reason for hiding this comment

lcgamboa May 1, 2023

Choose a reason for hiding this comment

lcgamboa commented May 1, 2023

mortbopet commented May 5, 2023

lcgamboa commented May 6, 2023

mortbopet commented May 6, 2023

lcgamboa commented May 6, 2023