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

Feature Request: CRTK-ROS bridge required interfaces #15

Closed
htp2 opened this issue Oct 28, 2021 · 7 comments
Closed

Feature Request: CRTK-ROS bridge required interfaces #15

htp2 opened this issue Oct 28, 2021 · 7 comments

Comments

@htp2
Copy link
Contributor

htp2 commented Oct 28, 2021

It would be useful to extend the crtk_cisst_ros_bridge to also allow for easy bridging of required interfaces, similar to the current one-liners for provided interfaces we have currently: (bridge_interface_provided and bridge_all_interface_provided)

I would be able to use this immediately in my work (though I can work without it just fine for now), and would be willing to help with testing and development to the extent that I would be useful. I think it would greatly increase the usability of the cisst-ros bridge for a lot of users and encourage the use of CRTK conventions, both of which are really good things from my perspective.

I understand there would be some challenges regarding handling 'optional' items in cisst required interfaces (i.e. do you try to connect to a ROS topic if optional?, Do you check to see if it exists first?, Do you need a configuration file for the user to tell you what optional ones should be included?, etc.). I'm happy to participate in any brainstorming regarding how to handle these issues.

I spoke with @adeguet1 regarding this, tagging you here for your visibility.

Thanks!

adeguet1 added a commit that referenced this issue Nov 12, 2021
* renamed class `mts_ros_crtk_bridge` to `mts_ros_crtk_bridge_provided`
* create temporary typedef for backward compatibility for `mts_ros_crtk_bridge`
* added class `mts_ros_crtk_bridge_required` to allow bridge for a required interface
* many new conversion methods for CRTK
@adeguet1
Copy link
Contributor

I pushed a bare bone version of the code to support this feature. It is not documented nor extensively tested. It also misses code to parse a JSON file to configure the bridge (will be required for dynamic creation). To be consistent, I renamed mts_ros_crtk_bridge to mts_ros_crtk_bridge_provided which is used to bridge to a provided interface. The new class is called mts_ros_crtk_bridge_required which is used to bridge to a required interface.

To populate the bridge, I can think of 3 scenarios:

  • Look at a required interface and assume everything will come from ROS. Not great for MTS_OPTIONAL features but might work for some users. This code is written but not tested yet.
  • Explicitly list the commands we want to provide. The code is written and somewhat tested using a C++ method. See https://github.com/jhu-dvrk/dvrk-ros/blob/8578cbd716d014d236370b3e01c68a09efaf2e97/dvrk_arms_from_ros/components/src/dvrk_arm_from_ros.cpp#L47. I still have to add code to populate from a configuration file.
  • Populate from ROS topics in a namespace. I'm not sure how convenient that is. It would also be dependent of the order nodes are started: if the cisst application is started too early the topics might not exist. I don't plan to implement this.

Use:

Todo:

  • Documentation
  • Maybe some missing CRTK commands
  • Add configuration file parsing
  • Port same behavior to ROS 2 CRTK bridge

@htp2, thoughts? Usable as is?

@htp2
Copy link
Contributor Author

htp2 commented Nov 14, 2021

@adeguet1 Just wanted to let you know I've seen this - thanks for adding this! I will test it as soon as I can (hopefully Mon or Tue).

@htp2
Copy link
Contributor Author

htp2 commented Nov 16, 2021

Hi Anton,

I am having some trouble getting this built on my machine. I also am using sawNDITracker. I updated to the most recent devel branch there but get this error on catkin build

/home/henry/bigss/catkin_ws/src/cisst-saw/sawNDITracker/ros/mts_ros_crtk_ndi_bridge.cpp: In member function ‘void mts_ros_crtk_ndi_bridge::bridge(const string&, const string&, double, double)’:
/home/henry/bigss/catkin_ws/src/cisst-saw/sawNDITracker/ros/mts_ros_crtk_ndi_bridge.cpp:36:5: error: ‘mts_ros_crtk’ has not been declared
     mts_ros_crtk::clean_namespace(_clean_namespace);

I thought that maybe it should be mts_ros_crtk_bridge, but I get the same error. Am I missing something?

@adeguet1
Copy link
Contributor

That's odd. I don't have access to the computer I used for this code so I can't test until tomorrow. Can you add #include <cisst_ros_crtk/mts_ros_crtk_bridge_provided.h> in: https://github.com/jhu-saw/sawNDITracker/blob/4cdae5c39927e8c9904442e45674366ce4eff03d/ros/mts_ros_crtk_ndi_bridge.h#L22 and see if that helps.

@adeguet1
Copy link
Contributor

I think the issue is now fixed. You will need to pull the latest devel branch for cisst-ros and sawNDITracker. Can you give it a try when you have some spare time?

@htp2
Copy link
Contributor Author

htp2 commented Nov 18, 2021

This did fix the issue - compiled without any problem! I'll keep you updated as I try to integrate this feature into our codebase

@htp2
Copy link
Contributor Author

htp2 commented Aug 9, 2022

Hi Anton, I know it's been a while since you worked on this or we talked about it. I had an application to test it with recently and it worked quite well. I had to make one small addition for the bridge_required to connect appropriately. Additionally, for my application, I wanted to be able to bridge servo_jv so I added that functionality in as well. Both are in a pull request #17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants