Skip to content

added in seperate topic for contious updates - #20

Open
redto0 wants to merge 1 commit into
iscumd:ros2from
redto0:continous_drive_mode_set
Open

added in seperate topic for contious updates#20
redto0 wants to merge 1 commit into
iscumd:ros2from
redto0:continous_drive_mode_set

Conversation

@redto0

@redto0 redto0 commented Jul 9, 2026

Copy link
Copy Markdown

No description provided.

@redto0

redto0 commented Jul 21, 2026

Copy link
Copy Markdown
Author

@andyblarblar can you review and merge if your happy with this?

@andyblarblar

Copy link
Copy Markdown
Member

Are kill states actually readable yet? Seems fine otherwise

@andyblarblar

Copy link
Copy Markdown
Member

Also by ros convention services are probably a better fit for setting a persistent state

@redto0

redto0 commented Jul 21, 2026

Copy link
Copy Markdown
Author

@andyblarblar
To my knowledge, they aren't .😭 I left it in in case that becomes a thing later. It shouldn't affect the code if it isnt being used, its just there if it that happens later as an extension of the lock out toggle.

tbh I didnt look into using a service for this. What would that entail?

@MarkPopkov

Copy link
Copy Markdown

Also by ros convention services are probably a better fit for setting a persistent state

I disagree on using services, to quote the ROS2 Docs:

In ROS 2, services are expected to return quickly, as the client is generally waiting on the result. Services should never be used for longer running processes, in particular processes that might need to be preempted for exceptional situations.

It recommends actions for longer-running things, something we probably should look into

PR looks fine to me though

@MarkPopkov MarkPopkov left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM, will test as soon as I can

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.

3 participants