-
Notifications
You must be signed in to change notification settings - Fork 526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding Back TCP Handler to ROS2 #824
Conversation
Other maintainers should weigh in on this with their perspective. My perspective is that such a change is outside the scope of the project and maintenance bandwidth. I think the goals of rosbridge are to provide connectivity between a ROS environment and browser-like environments. Browsers do not have raw TCP socket access and must use a more limited available set of socket connection technologies (i.e. websockets). Additionally, I view this as duplicative of what you can already do with the websocket rosbridge. Websockets run on TCP and there are numerous websocket client libraries in all languages. You can write a program in any language to connect to the existing websocket server and receive messages and these messages would be sent over TCP. While using raw TCP sockets does cut out a layer of protocol - it also avoids us having to define TCP specific framing and messaging in the spec which is currently focused on websocket specific concepts. |
In #639 they also mention few project relying on the legacy package that uses tcp. I think in order for them to be able to upgrade their package and keep using rosbridge. It is a good idea to actually adding this. |
@tsender It would be worthwhile to have the TCP handler fail to start when not in BSON mode. There's no easy way to divide a byte stream into JSON without parsing the JSON. The code in protocol.py allegedly handles this by counting braces, but it fails on braces in strings. Some JSON parsers have an option to handle concatenated JSON, but not all of the parsers supported by rosbridge_suite can do it. JSON over TCP just isn't viable without major changes. It's best to fail gracefully. |
@JKaniarz To clarify, you're saying that the TCP handler should NOT start if @defunctzombie While I understand your concern, I do think it would still be useful to have the TCP handler in ROS2. It's simply another useful communication method. For one thing, if I understand your comment correctly, it seems like you would prefer to eliminate the TCP handler that already exists in the ROS1 branch. All I did was bring it back (with minor changes to work with the latest libraries and changes in Python 3) since someone initially took the time to create it for ROS1. I also kept the exact same structure of the message handling that you guys had. So, I honestly do not understand what "maintenance upkeep" is too taxing, since you already support this in ROS1. Second, I have had problems working with the rosbridge_suite websocket in my past testing: #718 and #720 And third, the only use case I have for this repo is with the ROSIntegration plugin for Unreal Engine, and that plugin was only configured to connect to rosbridge_suite via the TCP handler. It was just easier for me to patch the TCP handler than to reconfigure ROSIntegration to properly work with the websocket support that Unreal provided, And in my initial testing, I ran into the above problems. But, at the end of the day, this is your repo. If you still choose not to have the TCP handler back, then I can still move forward with my own work, since I have a fork with the changes I need. |
@tsender Thanks for the additional background. I would generally describe this repo as a community maintained repo. I am personally not actively maintaining this repo nor am I aware of anyone who is actively maintaining this repo that would step up to accept and want to maintain the proposed changes. I'm leaving the PR open for others to learn and if an existing maintainer does want to champion it I will leave it to them. |
The choices is between "mostly works but some strings cause an unrecoverable desynchronization" and "refuse to run in JSON mode". I am recommending the latter. Also, I did some testing and this PR will not run after merging. I submitted a PR to your repo (which should automatically update this PR) to get it running again. |
Updated tcp_handler interface to match latest (but didn't implement anything new)
…ed TCPServer to ThreadedTCPServer
@JKaniarz I fixed all the merge conflicts and got everything running. Both my foxy and ros2 branches should match now and have no conflicts with the main branch. I also addressed the issue you mentioned where multiple clients couldn't connect to the TCP server (I had to change it back to a ThreadedTCPServer). So far, I have not noticed any issues and everything works as expected. Though I do not understand what is causing the failures in the CI pipeline. |
The trunk is failing the checks so you may have inherited it. There’s still some linter errors, though. |
Bumping. Having this would be nice because the most used rosbridge C++ client implementation supports only the TCP protocol. |
@mvccogo, you are welcome to fork my |
If you’re referring to “ROSIntegration” for UE4, they accepted my PR for websocket support. Just set protocol to ws in your game instance. If you’re using rosbridge2cpp it shouldn’t be too hard to integrate easywsclient. https://github.com/dhbaird/easywsclient |
@JKaniarz Not using Unreal, but yes, ideally I could ditch TCP and implement it using easywsclient (usage seems easy, will take a look at it later). It's just that I needed something working ASAP, and this fork seemed like a good solution. @tsender Upon forking I had to fix some node parameters that were set as integers instead of floats, not sure if you have this error. Also, for some reason, the rosbridge_websocket file that should call rosbridge_websocket.py is returning Exec error (file format). To fix it locally I just moved the code to rosbridge_websocket and kept the !#/usr/bin/env python3 line at the beginning. |
@mvccogo I actually do not use the default launch files, so I wouldn't know of any problems. I originally just copied the launch file from the ros1 branch and thought it looked ok. I currently use my own python3 launch files in the ROS2 workspace I use. |
This PR has been marked as stale because there has been no activity in the past 6 months. Please add a comment to keep it open. |
Public API Changes
Just adding back the TCP handler with the exact same launch file as before
Description
This PR is long overdue. A while ago #639 suggested the removal of the TCP/UDP handlers in ROS2 since no one took the time to port them over properly. A few people had attempted to bring back the TCP handler in their own forks but never made any PRs to this repo. I copied those attempts and fixed any outstanding bugs.
While I do not use the latest versions of ROS2 (like galactic and humble), I have tested these changes on my own fork for eloquent and have not had any issues. I made sure to have the new
tcp_handler.py
file closely match any changes recently made to the corresponding file in ROS1. And therosbridge_tcp
script also closely matches the ROS1 version, except with a few minor changes needed to make it run properly.I should add that I only use rosbridge with BSON. I do not use this repo with JSON, and I do not have any way of testing this code with JSON (but given that pretty much most of my additions have been copied from the ROS1 version, I don't think there would an issue).
This PR only adds back the TCP handler (someone else can port over the UDP handler if they need it).