Skip to content
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

Closed
wants to merge 37 commits into from
Closed

Conversation

tsender
Copy link
Contributor

@tsender tsender commented Nov 13, 2022

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 the rosbridge_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).

@defunctzombie
Copy link
Contributor

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.

@tomkimsour
Copy link

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.

@JKaniarz
Copy link

@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.

@tsender
Copy link
Contributor Author

tsender commented Apr 19, 2023

@JKaniarz To clarify, you're saying that the TCP handler should NOT start if bson_only_mode = False for the reason you mentioned? Also, my foxy branch is more up-to-date than my ros2 branch, in case you want to see the other changes I made.

@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.

@defunctzombie
Copy link
Contributor

@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.

@JKaniarz
Copy link

@JKaniarz To clarify, you're saying that the TCP handler should NOT start if bson_only_mode = False for the reason you mentioned? Also, my foxy branch is more up-to-date than my ros2 branch, in case you want to see the other changes I made.

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.

@tsender
Copy link
Contributor Author

tsender commented Apr 21, 2023

@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.

@JKaniarz
Copy link

@tsender

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.

@mvccogo
Copy link

mvccogo commented Aug 4, 2023

Bumping. Having this would be nice because the most used rosbridge C++ client implementation supports only the TCP protocol.

@tsender
Copy link
Contributor Author

tsender commented Aug 7, 2023

@mvccogo, you are welcome to fork my ros2 branch and take the time to address the linting and other errors. I simply never had the time to fix all those silly linting errors.

@JKaniarz
Copy link

JKaniarz commented Aug 8, 2023

Bumping. Having this would be nice because the most used rosbridge C++ client implementation supports only the TCP protocol.

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

@mvccogo
Copy link

mvccogo commented Aug 8, 2023

@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.

@tsender
Copy link
Contributor Author

tsender commented Aug 8, 2023

@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.

Copy link

github-actions bot commented Apr 2, 2024

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.

@github-actions github-actions bot added the stale label Apr 2, 2024
@github-actions github-actions bot closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants