ros-controls / ros2_controllers

Generic robotic controllers to accompany ros2_control
https://control.ros.org
Apache License 2.0
321 stars 290 forks source link

Scaled jtc #1191

Open fmauch opened 4 days ago

fmauch commented 4 days ago

This is my take2 on #301.

I plan to finish this until Friday, @firesurfer feel free to poke me :-)

Currently missing

Things that will get changed, as discussed in the latest working group meeting

fmauch commented 4 days ago

Thoughts on tests I should implement:

firesurfer commented 4 days ago

hi @fmauch Great to see this being worked on again. I added some comments to the code.

Regarding the tests:

Execution with scaling factor of 0.5 takes twice as long (and does not fail, when no tolerances are set)

Makes sense

Execution with scaling < 1 and goal time tolerance leads to an early halt and failed execution

Isn't this what we patched in: https://github.com/UniversalRobots/Universal_Robots_ROS2_Driver/pull/882. As far as I can tell you also backported this patch. (And I still think the goal time should be also scaled ;) )

Execution with scaling > 1 should not fail independent of the tolerances set <-- is that true?

Same as above. From heavily using this controller in our system I have the opinion that in most use cases it makes sense (and is what a user would expect) that the goal time should also be scaled. But perhaps I understood your suggestion wrong.

fmauch commented 4 days ago

From heavily using this controller in our system I have the opinion that in most use cases it makes sense (and is what a user would expect) that the goal time should also be scaled. But perhaps I understood your suggestion wrong.

Thank you for your input. That's why I put my brain dump on that here, so we can discuss this :-) And that's also why I didn't write the documentation on that, yet.

I'll wrap my head around this a little more and try to get a better feeling for the current behavior, then come back to this.

fmauch commented 3 days ago

I've took some time to think about the controller's behavior and currently, it should be like this, right @firesurfer?

The latter is the main point we discussed in https://github.com/UniversalRobots/Universal_Robots_ROS2_Driver/pull/882. I am still fine with that approach, although I am not sure whether this covers every use case.

But with this I think I can finish the documentation and write some tests tomorrow.


Edit: Changed the third bullet point scaling 0 to 1. That was a mistake when writing this.

firesurfer commented 3 days ago

@fmauch

Execution with scaling factor of 0.5 takes twice as long.

Yes. we sample the trajectory half as fast

Execution with scaling factor of 2 should take 50% of the time.

Yes we sample the trajectory twice as fast

Execution with scaling factor of 0 should take as long as expected

I would call this paused. I have the feeling this is one of the major usecases of this controller

Independent of goal and path tolerances the action should not fail -- the goal time is scaled, as well.

In case the trajectory is within the (scaled) goal time, it should not fail -> yes. Otherwise the combination of a scaled trajectory + a set goal time tolerance would always fail. Is there a specific use case you have in mind that would not be covered by that ?

fmauch commented 3 days ago

My concern is mainly this:

If you have an application and you execute a trajectory on a robot that does speed scaling. The trajectory gets slowed down due to safety reasons, which means it finishes later than originally requested. This will slow down the whole application and no longer behave as programmed. This might be considered a fault.

One could argue that

  1. Depending follow-up actions should be triggered once their post-conditions are met (the motion action finished) and nit by time.
  2. If execution time is critical, that can be monitored as a wrapper. MoveIt! for example does that by default.

I just quickly discussed that with a colleague and he also agreed that the behavior that we currently have is probably the expected one. He was also emphasizing the second point.

So, I guess, I'll just leave that result standing. As I said, I can definitively live with it. And it's probably even better / more appropriate than "I can live with that".

fmauch commented 3 days ago

Update on the tests:

christophfroehlich commented 3 days ago
* testing is currently difficult in this repository, see [Test failures: subscription already associated with a wait set #1101](https://github.com/ros-controls/ros2_controllers/issues/1101)

In the last weeks, I compiled ros2_control+controllers on jammy+humble to run the tests.