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

tesseract_rviz port again #27

Closed
wants to merge 12 commits into from

Conversation

jdlangs
Copy link
Contributor

@jdlangs jdlangs commented May 4, 2022

Based off @mpowelson's work in #11 with updates to match tesseract_ros changes since this ros2-dev branch was formed.

This provides working environment state and trajectory monitor displays. Some remaining known issues:

  • rviz crashes when either of the display is removed. It seems to be because a slot is called on an already deleted QObject (the VisualizationWidget) but I haven't been able to figure out why.
  • When playing a received trajectory it was very jerky for me, with the robot jumping between states that are quite far apart. Don't have any ideas right now what's happening.

@Levi-Armstrong @marip8 if you guys want to take a look at all that would be great. Given the size of the changes though, we may just want to merge straight away and continue debugging.

@jdlangs
Copy link
Contributor Author

jdlangs commented May 4, 2022

Forgot to mention puzzle_piece_example.launch in tesseract_ros_examples can be used a standalone example to test out the plugins.

@Levi-Armstrong
Copy link
Contributor

I am not sure if you want to wait, but all of the rviz objects are about to be replaced with Tesseract specific components see this PR.

@Levi-Armstrong
Copy link
Contributor

I am about finished with the contact monitor and then will only have the manipulator plugin to replace.

@jdlangs
Copy link
Contributor Author

jdlangs commented May 4, 2022

It would be nice to sync but this branch is pretty far behind on tesseract, using 0.7 right now. I will take a look at the redesign but I'm guessing we don't have the bandwidth to update everything right now.

@Levi-Armstrong
Copy link
Contributor

It would be nice to sync but this branch is pretty far behind on tesseract, using 0.7 right now. I will take a look at the redesign but I'm guessing we don't have the bandwidth to update everything right now.

Not a lot has changed between then mostly bug fixes and minor tweaks.

@dpiet
Copy link

dpiet commented May 12, 2022

@jdlangs I came across your fork and PR and just wanted to relay that I'm also observing jerky behavior on the trajectory visualization, both in your puzzle_piece_example and a setup we're working on. Any further thoughts on what might be causing it?

@jdlangs
Copy link
Contributor Author

jdlangs commented May 13, 2022

@jdlangs I came across your fork and PR and just wanted to relay that I'm also observing jerky behavior on the trajectory visualization, both in your puzzle_piece_example and a setup we're working on. Any further thoughts on what might be causing it?

No, I haven't dug into it at all yet. If you investigate further and have any ideas please do share.

@jdlangs
Copy link
Contributor Author

jdlangs commented May 13, 2022

@Levi-Armstrong @marip8 it would be nice to get CI back up on this repository. Is there any hope of leveraging the tesseract docker images or do you think we should set it up to build them from source in a ROS2 image for now?

@marip8
Copy link
Contributor

marip8 commented May 13, 2022

I think we'll need to revise the tesseract CI process to build from a ROS-independent docker image using colcon before we could realistically support a ROS2 build here. The current tesseract packages are built using catkin so the workspaces would need to be cleaned and rebuilt anyway. So you would only be saving yourself the time of downloading tesseract and installing dependencies (which isn't trivial) if you tried to use the existing tesseract Docker images

@jdlangs
Copy link
Contributor Author

jdlangs commented May 13, 2022

We can move any CI discussion to #28 now

@Levi-Armstrong
Copy link
Contributor

@jdlangs I noticed that the startMonitoringEnvironment is not implemented along with other supporting functionality. Is there an issue with implementing this in ros2?

@jdlangs
Copy link
Contributor Author

jdlangs commented May 23, 2022

Sort of. Having to deal will service calls and subscriptions means having to worry about how the callbacks will get run when that wasn't a concern in ROS1. It's more a question of what the design should be and I don't feel like we've settled on a best practice so I just punted on that functionality.

The main tension is should a ROS library defer all callback management to whoever runs an executor on the node? If so, to make things happen concurrently you need multiple callback groups and then the user needs to know to execute with multiple threads. If not, the library could create it's own internal nodes/executors but that increases complexity and makes debugging more difficult. Also, the user needs to know multiple nodes exist in a process and avoid setting the name attribute in a launch file because it will set that name on all nodes and cause issues.

Any thoughts or opinions on this would be very welcome.

@gavanderhoorn
Copy link

Not use ROS 2? 🤡 😉

@jdlangs
Copy link
Contributor Author

jdlangs commented May 23, 2022

This inspired me to post on the discourse meme thread:

@Levi-Armstrong
Copy link
Contributor

Sort of. Having to deal will service calls and subscriptions means having to worry about how the callbacks will get run when that wasn't a concern in ROS1. It's more a question of what the design should be and I don't feel like we've settled on a best practice so I just punted on that functionality.

The main tension is should a ROS library defer all callback management to whoever runs an executor on the node? If so, to make things happen concurrently you need multiple callback groups and then the user needs to know to execute with multiple threads. If not, the library could create it's own internal nodes/executors but that increases complexity and makes debugging more difficult. Also, the user needs to know multiple nodes exist in a process and avoid setting the name attribute in a launch file because it will set that name on all nodes and cause issues.

Any thoughts or opinions on this would be very welcome.

I came across this while researching. Is this a valid approach here?

@jdlangs
Copy link
Contributor Author

jdlangs commented May 24, 2022

I came across this while researching. Is this a valid approach here?

That example gets the callback to run by calling rclcpp::spin_some which isn't an option here because someone probably already has the node in an executor. It would have to maintain an internal separate node/callback group and execute it separately to make it work.

I guess there's a third design option too which is to disallow any synchronous behavior of callbacks and the service response would have to run outside of the scope of the client calling it. Then the class user doesn't need to know anything about how to execute the node, but the code rearrangement might get kind of hideous.

@Levi-Armstrong
Copy link
Contributor

The main tension is should a ROS library defer all callback management to whoever runs an executor on the node? If so, to make things happen concurrently you need multiple callback groups and then the user needs to know to execute with multiple threads.

I assume this would not work in the case of rviz where it loads a plugin which contains the environment monitor. This would require you build rviz from source and set up the multi threaded executor. Is this correct?

@Levi-Armstrong
Copy link
Contributor

@jdlangs If I understand correctly the issue is that from within a subscriber callback we are then calling a service call which is the issues. What if it is change such that the subscriber callback sets a local member variable to indicate that an update is required. Then have a ros timer with a callback which checks this variable and performs the service call to get the changes. I am assuming a timer callback is not subjected to the same limitation. What do you think?

@jdlangs
Copy link
Contributor Author

jdlangs commented May 25, 2022

Good point about rviz, it looks like they do use a single threaded executor

I am assuming a timer callback is not subjected to the same limitation.

Timer callbacks are indeed also processed by the executor. The most "proper" solution then probably has to be the async one then where the service response runs as its own callback. The separate internal node would be simpler to implement though.

@Levi-Armstrong
Copy link
Contributor

The most "proper" solution then probably has to be the async one then where the service response runs as its own callback.

Is there an example somewhere on how this should be done?

@jdlangs
Copy link
Contributor Author

jdlangs commented May 25, 2022

There's a toy example in the main ros2 examples repo

@marrts marrts mentioned this pull request Aug 5, 2022
@jdlangs
Copy link
Contributor Author

jdlangs commented Aug 12, 2022

Superseded by #33

@jdlangs jdlangs closed this Aug 12, 2022
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

Successfully merging this pull request may close these issues.

6 participants