TCP server implementation #46
Conversation
In general I think this is a really good solution and I think the event-based tcp is a good approach. I have tested that it works with the ScriptSender, it works very well and it solves #45.
As you mention I also think that it should be used inside the ReverseInterface. I could look into implementing it in the reverse interface if needed? |
I went ahead and implemented it, but you could test it a bit more, if you'd like. Currently, stopping the ROS driver with CTRL-C results in a RTDE pipeline producer overflow that I still need to hunt down |
I will test that it works with the reverse interface as well.
It is related to the pipeline running in the RTDEClient stopping the pipeline, when the object is destroyed, should fix that issue. Below example could be a solution: RTDEClient::~RTDEClient()
{
pipeline_.stop();
}
|
Yep, that does the trick :-) |
|
@urmahp I'm done with implementing this ATM, please go ahead and test it as much as you can :-) Before merging, I'd like to also refactor the tcp_client a bit more so it is more understandable and introduce unit tests for the server and client. Let me know, if you'd like to participate in one of these. |
I will be happy to test it.
I can work on some unit test for the server and client, while testing it. |
|
I have tested that it works with both the ROS driver and the Isaac driver and overall it is working very well. However I see one issue once in a while, it is not occuring every time, only once in a while. Unfortunately I haven't been able to track down the issue completely, but I believe it might have something to do with that the disconnect callback functions isn't called, because the server maybe is in a specific state at the point of disconnect. The error message |
|
I'll try to look into it tonight. |
|
Hm, I once hit I finally found a way to reproduce this:
Open questions as for now:
|
|
Thank you very much @fmauch for looking into the issue and fixing it. I apologize that I haven't time to look into the unit test yet, but I can look into it today if needed. What did you have in mind with the unit test? |
Nothing to apologize. Additionally, the current keepalive implementation is not really satisfying. If we connect to the reverse interface, it will be happy as long as we don't disconnect. We should check whether we actually receive a keepalive signal. We should get a keepalive signal (basically any message currently) from the robot before we send the next message. I'd like to implement this using a unit test that should trigger a disconnect event when the keepalive signal is missing. @urmahp would you like to implement this? |
|
I will start developing some unit test to the server now, I will create a pr to your branch once I have some test. I will look into the keepalive implementation as well. |
|
@urmahp I'll have some time to work on this today and tomorrow. If you didn't start with the keepalive implementation, I'll start doing that. |
I haven't started the keepalive implementation. I have added a PR to your branch on some unit test for the tcp server. |
|
I've created a draft implementation of the keepalive handling here. Let's keep discussions on this there. |
|
@urmahp If you could take another review over the PR that would be really nice. |
|
I will test how it is working and I will also look into the sigpipe error which made one of the tcp server test fail on this build. |
I think, I found the error: When running the test standalone with output in rare cases (I needed to run it up to 2500 times to trigger) I get this failure: I think, that this is the problematic code block: void TCPServer::handleDisconnect(const int fd)
{
URCL_LOG_DEBUG("%d disconnected.", fd);
close(fd);
if (disconnect_callback_)
{
disconnect_callback_(fd);
}
FD_CLR(fd, &masterfds_);
[...]
}Calling the I'll leave the test looping over night and it should not have failed tomorrow morning. |
|
95000 successful tests later I would say that this was indeed the problem. |
Nice thanks for solving the issue. I have tested that it works, and it is working very well 😃 I believe these three variable declaration could be deleted as they aren't used anymore in the UrDriver. I think that when merging this line should be removed from the ROS driver, else it would log a warning every control cycle, because the ROS driver is still using the script from the resource folder in the ROS driver. |
It was me introducing the issue, anyway ;-) Thanks for writing the tests that showed that :-)
Addressed in 587a584
Addressed in UniversalRobots/Universal_Robots_ROS_Driver#346 |
This implements an event-based tcp server for easier use. It is decoupled from the TCPSocket class to remove one layer of complexity. Also, the event-based approach allows interrupting this server as it won't be blocking inside an accept or recv call if not applicable.
|
I cleaned up the branch, from my side this is ready to merge. |
I agree. |
This implements an event-based tcp server for easier use. This could be a solution for #45
In this commit I only implemented it for the ScriptSender, but it could also
be used (probably with modifications) inside the ReverseInterface.