Cleaned up launch file example_1/rrbot.launch.py

[philipchurchley]

Addresses #950 by cleaning up a single launch file: example_1/rrbot.launch.py. This file has been improved to use a declarative programming style, as described by @emersonknapp at ROSCon, which can be viewed in the livestream at 6:10. A couple of local variables are needed for the Event Handlers, but all other unnecessary local variables are removed.

This change is a draft PR for feedback before rolling out changes to other launch files in other examples.

[christophfroehlich]

Thanks for picking this. Some comments, overall looks fine!

[emersonknapp]

I'm happy to see this! Let me know if i can help :) Added one small comment that might feel stylistically a little more comfortable.

We can not go down the rabbit hole here if you don't want to but I'll bring it up:

Why do we need those event handlers to enforce ordering need to be there? Does the spawner not work if started in parallel? Is there a bug, design choice, or true technical limitation that means you couldn't just start the two spawners and rviz all at once and have everything work properly?

I'm not a direct user of ros2_controls so I'm missing a lot of context, but have noticed that most of the event handler usage I'm running into is related to ros2_controls use, so I'm curious to learn why it's commonly used that way and if there's anything we could do to simplify the launch usage around it.

[christophfroehlich]

Let's not pollute this good-first-issue, I'll open an issue to discuss this ;)

[christophfroehlich]

This is great. Do you want to mark this as ready for review, or do you want to rollout the changes to all the other launch files first?

Btw: There is a python module for xacro, which makes single calls to xacro a bit more readable. But it lacks the support of substitutions being passed as xacro arguments, that's why I'd leave it as it is now.

[philipchurchley]

Thanks for your feedback!

Do you want to mark this as ready for review, or do you want to rollout the changes to all the other launch files first?

Up to you, I would be happy with either. I can go ahead and start working on the other launch files.

[christophfroehlich]

Let's finish and merge if for example_1 first. I have merged an old PR reverting some changes. To fix the conflicts, just apply the outcome of the discussion with #961: It seems that there is no need for any EventHandler.

[philipchurchley]

Sounds good. To confirm, I'm pushing another commit which removes the EventHandlers, as what was described in the discussion. The conflicts seem outdated and are unrelated to EventHandlers.

[mergify]

This pull request is in conflict. Could you fix it @philipchurchley?

[christophfroehlich]

Sounds good. To confirm, I'm pushing another commit which removes the EventHandlers, as what was described in the discussion. The conflicts seem outdated and are unrelated to EventHandlers.

Exactly. But you have to merge the master branch into yours to fix the conflicts.

[christophfroehlich]

Perfect, isn't it ready for review (and merge) now?

[christophfroehlich]

Thanks. Simpler and -32 lines ;)

[philipchurchley]

Thanks! For future launch files, how do you recommend I deal with EventHandlers? I'm guessing for some launch files they might still be necessary, but for others such as this one, might not be.

[christophfroehlich]

I don't see where it is really necessary. For the chained controllers we just have to spawn them within the same instance I guess. Let's try to avoid them at all