Skip to content

Remodel ros2controlcli#349

Merged
bmagyar merged 35 commits intomasterfrom
remodel_ros2controlcli
Mar 30, 2021
Merged

Remodel ros2controlcli#349
bmagyar merged 35 commits intomasterfrom
remodel_ros2controlcli

Conversation

@Karsten1987
Copy link
Copy Markdown
Contributor

This currently sits on top of #310 but also addresses #341.

The current PR of @v-lopez does unfortunately not pass all its tests on OSX and I've noticed that the CLI package was in a rather wild state. This PR is also addressing their linters and refactors a lot of the node handling.
It also activates the tests of this package in the GH actions.

Victor Lopez and others added 21 commits March 23, 2021 08:07
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
fixes #320

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Copy Markdown
Contributor Author

I will still have to modify the spawner and unspawner scripts to use the python api within the controller manager to get rid of the python subprocess calls.

@bmagyar bmagyar force-pushed the remodel_ros2controlcli branch from 671bb6d to f3fd943 Compare March 28, 2021 16:07
@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented Mar 28, 2021

merged #310, rebased this one

Copy link
Copy Markdown
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start but I think this PR needs the removal of those subprocess calls too to be complete.
Thanks for adding the gh actions, good catch!

Please always elaborate on OSX issues you find as you are the only source of this info in our group. It'd be nice if you could actually open an issue with them whenever you find some and mark PRs that resolve them, that way we know they are solved for everyone.

@Karsten1987 Karsten1987 force-pushed the remodel_ros2controlcli branch from f3fd943 to 5f381cb Compare March 29, 2021 19:09
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Copy Markdown
Contributor Author

So I ended up changing the spawner scripts as well and with these changes the tests seem to work again.

I do believe there's a fine difference in the pep257 between the ros-tooling and ros-industrial CI. The source build (ros-tooling) seems to use a version which expects a semicolon ; after a doc section, whereas the binary jobs (ros-industrial) don't.
Not sure what to do here.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
add_controller_mgr_parsers(parser)

def main(self, *, args):
print("deprecated warning: Please use either 'load --state' or 'set_state'")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_state should be set_controller_state

@destogl
Copy link
Copy Markdown
Member

destogl commented Mar 31, 2021

@Karsten1987 great job! Thanks for doing this!

(I just opened a few follow-ups to clean the code even more)

add_controller_mgr_parsers(parser)

def main(self, *, args):
print("deprecated warning: Please use either 'load --state' or 'set_state'")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this in galactic+

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.

4 participants