Open athackst opened 4 years ago
The PR seems to undo changes recently merged as part of #234?
(Also please sign off the commits to pass DCO.)
Working on the DCO.
And yes, I tried those changes, but they didn't work for me :woman_shrugging:
DCO complete.
It would probably be good to add a unit test to better test the services. I was able to test it with my bot, but I'm unaware of a ROS package that has service name differences that's already a part of the ROS ecosystem that also has unit tests.
Alternatively we could add one in this package, but that would mean it would need to be compiled with ROS1 and ROS2 (and be different) and this package is already somewhat tricky to setup.
Any other changes requested on this?
Any other changes requested on this?
My previous comment still applies:
The PR seems to undo changes recently merged as part of #234?
"I tried those changes, but they didn't work for me" isn't a satisfactory answer for reverting it in this PR.
Here's a detailed console output of the current eloquent branch which appears to be incomplete in actually mapping services of different names in a rosbridge.
And a corresponding dockerfile to reproduce:
To be fair, my intention isn't to "revert a PR" -- it's to make service mapping work.
I have now provided an example of the output I was seeing with the current state of the code, as well as a dockerfile for you to reproduce it on your end. I don't believe I'm doing something wrong in my set up. If I have, please let me know what I did wrong?
I was able to successfully map services after I made changes to the code base, which I'm sharing here, since I thought it might be valuable to others.
Ping @nuclearsandwich.
Any update on this?
Updated rosbridge to handle services with rule-based name matching.
Motivating use: ROS2 has more strict rules on variable name format in services than ROS1, requiring a name change between ROS1 and ROS2 variables.
This PR fixes:
Tested: On the AWS Deepracer using this dockerfile
This change is