Skip to content

[Docking] Create API to enable / disable detector #5015

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.

Sign up for GitHub

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

Jump to bottom
Open
SteveMacenski opened this issue Mar 24, 2025 · 10 comments · May be fixed by #5218
Open

[Docking] Create API to enable / disable detector #5015

SteveMacenski opened this issue Mar 24, 2025 · 10 comments · May be fixed by #5218
Assignees
@ayushkumar8340
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@SteveMacenski
Copy link
Member

This would be useful so that the detector running for docking (which may be very computationally expensive, like AI) doesn't need to be always running when not in a docking situation. You can enable it when starting to dock and disable it once done - then the detector can be a task server instead of an always-on node (optionally)
@SteveMacenski SteveMacenski added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Mar 24, 2025
@ayushkumar8340
Copy link

Hi,
I am interested in working on this problem statement. Could you please assign this issue to me and provide further details?

@SteveMacenski
Copy link
Member Author

@ayushkumar8340 thanks for showing interest!

I think what we want to do here is to:

  • Update the Dock Plugin API to include a new "startDetection()" "stopDetection()" methods
  • Update documentation in the readme and nav2 website about them
  • Implement these 2 functions in the 2 plugins: Set a service name parameter to enable and disable the task server doing dock detections, with the dock type name as a request field (in case multiple dock types, to enable detections on the the current right one). If none provided (make the default empty string), then we skip and don't do anything, meaning the detector is always on

That would allow the explicit start/end of detections when they're needed for docking in case a high computation cost detector is being used (like AI)

Does that make sense? 😄

@ayushkumar8340
Copy link

Yup got it. I'll keep you posted about the updates.

@ayushkumar8340
Copy link

I had a few doubts regarding the implementation of this problem statement :

  1. As you mentioned, there should be a mechanism to turn on/off the task server doing docking detection but when i went through the code i found out that both the plugins: simple charging dock and simple non-charging dock subscribes to a topic to get the dock pose and there no such mechanism to trigger the detection node as it is a separate package.

Image
Source : https://github.com/open-navigation/opennav_docking/tree/ubr1_testing

So do you want me to add some logic inside the detection node but according to me that would not be a good and scalable solution.
Or do I need to use the std::system() function to start a process (the detector node) and then kill it using the same.

I don't think both the solutions are good enough. What do you recommend ?

  1. Does the service which i have to create will have 1 field (Just confirmation):
    a. type -> dock plugin type to enable

@mikeferguson
Copy link
Contributor

Since the image_proc nodes are lazy, if you have the plugin unsubscribe from the detected_dock_pose topic then the track_marker_node will continue to run but have pretty much zero overhead (as it will also unsubscribe from the image_raw topic)

@SteveMacenski
Copy link
Member Author

SteveMacenski commented Apr 1, 2025 •
edited
Loading

  1. This is true for the current detectors, but not all detectors are that way. Many are based on AI solutions which are heavy to run continuously and should only be enabled when required.

However, as Fergs points out, we can still subscribe and unsubscribe from that topic to automatically start/stop the detection within our new APIs (rather than calling a service) to do the same for the case of the Apriltag detector. The way I'm suggesting you implement this has service calls (if configured) to enable / disable -- but also would be good to subscribe / unsubscribe as well for those that implement lazy publishers instead! The service calls using a std_srvs/Trigger - if the service_name is set as an available option. Otherwise, just subscribe / unsubscribe

  1. Maybe actually use a std_srvs/Trigger. We can have the user set the service_name to be matched to the proper detector server interface. We should just put that into the docs.

@ayushkumar8340
Copy link

Thanks for the clarification. I'll start working now.

@SteveMacenski
Copy link
Member Author

@ayushkumar8340 any updates? :-) Its been a little while and I think this should be relatively straight forward

@bkoensgen
Copy link

hi @SteveMacenski,

since there hasn't been activity on this issue for a while, I'd be happy to take it on if thats okay. I've looked at the discussion and i understand the approach; adding service calls to enable/disable detectors while keeping the lazy subscribe/unsubscribe as fallback.

i can have a draft PR ready by tomorrow for feedback.

Thanks!

@SteveMacenski
Copy link
Member Author

Please! I appreciate it thanks!

bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue May 30, 2025
@bkoensgen
feat: implement on-demand detector control in dock plugins
6b487d4 
- Lifecycle hooks (activate/deactivate/cleanup)
- startDetection()/stopDetection() in both Charging and NonCharging docks
- Config params: detector_service_name, detector_service_timeout, subscribe_toggle
- Backward-compatible default behavior
Fixes ros-navigation#5015
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue May 31, 2025
@bkoensgen
feat: implement on-demand detector control in dock plugins
be12b95 
- Lifecycle hooks (activate/deactivate/cleanup)
- startDetection()/stopDetection() in both Charging and NonCharging docks
- Config params: detector_service_name, detector_service_timeout, subscribe_toggle
- Backward-compatible default behavior
Fixes ros-navigation#5015
@bkoensgen bkoensgen linked a pull request Jun 2, 2025 that will close this issue
Draft
7 tasks
@SteveMacenski SteveMacenski linked a pull request Jun 2, 2025 that will close this issue
feat(opennav_docking): add on-demand detector client & unit tests for dock plugins (#5015) #5218
Draft
7 tasks
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 14, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
9750cac 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  dock plugin interface.
- Implemented this logic in `SimpleChargingDock` and `SimpleNonChargingDock`
  using a std_srvs/Trigger service call and dynamic topic subscription.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  and updated the README.
- Added C++ unit tests to cover the new detector lifecycle logic.
- Stabilized Python smoke tests by fixing race conditions related to
  node activation and TF propagation.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 15, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
5bd06d3 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  docking plugin interface (`ChargingDock` and `NonChargingDock`).
- Implemented this logic in `SimpleChargingDock` and
  `SimpleNonChargingDock` using a `std_srvs/Trigger` service call and
  dynamic topic subscription to manage the detector's lifecycle.
- The `DockingServer` now ensures `stopDetectionProcess()` is called
  on all exit paths, including success, failure, and cancellation,
  to prevent dangling resources.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  to configure this behavior and updated the README documentation.
- Added comprehensive C++ unit tests to verify the new detector
  lifecycle logic, specifically covering service failure cases and
  proper cleanup on action termination.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 15, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
64ce3b3 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  docking plugin interface (`ChargingDock` and `NonChargingDock`).
- Implemented this logic in `SimpleChargingDock` and
  `SimpleNonChargingDock` using a `std_srvs/Trigger` service call and
  dynamic topic subscription to manage the detector's lifecycle.
- The `DockingServer` now ensures `stopDetectionProcess()` is called
  on all exit paths, including success, failure, and cancellation,
  to prevent dangling resources.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  to configure this behavior and updated the README documentation.
- Added comprehensive C++ unit tests to verify the new detector
  lifecycle logic, specifically covering service failure cases and
  proper cleanup on action termination.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>

-
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 15, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
c567011 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  docking plugin interface (`ChargingDock` and `NonChargingDock`).
- Implemented this logic in `SimpleChargingDock` and
  `SimpleNonChargingDock` using a `std_srvs/Trigger` service call and
  dynamic topic subscription to manage the detector's lifecycle.
- The `DockingServer` now ensures `stopDetectionProcess()` is called
  on all exit paths, including success, failure, and cancellation,
  to prevent dangling resources.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  to configure this behavior and updated the README documentation.
- Added comprehensive C++ unit tests to verify the new detector
  lifecycle logic, specifically covering service failure cases and
  proper cleanup on action termination.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>

-

updtae
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 16, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
8c41029 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  docking plugin interface (`ChargingDock` and `NonChargingDock`).
- Implemented this logic in `SimpleChargingDock` and
  `SimpleNonChargingDock` using a `std_srvs/Trigger` service call and
  dynamic topic subscription to manage the detector's lifecycle.
- The `DockingServer` now ensures `stopDetectionProcess()` is called
  on all exit paths, including success, failure, and cancellation,
  to prevent dangling resources.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  to configure this behavior and updated the README documentation.
- Added comprehensive C++ unit tests to verify the new detector
  lifecycle logic, specifically covering service failure cases and
  proper cleanup on action termination.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>

-

updtae
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 16, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
4205a29 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  docking plugin interface (`ChargingDock` and `NonChargingDock`).
- Implemented this logic in `SimpleChargingDock` and
  `SimpleNonChargingDock` using a `std_srvs/Trigger` service call and
  dynamic topic subscription to manage the detector's lifecycle.
- The `DockingServer` now ensures `stopDetectionProcess()` is called
  on all exit paths, including success, failure, and cancellation,
  to prevent dangling resources.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  to configure this behavior and updated the README documentation.
- Added comprehensive C++ unit tests to verify the new detector
  lifecycle logic, specifically covering service failure cases and
  proper cleanup on action termination.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>

-

updtae
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 16, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
4889899 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  docking plugin interface (`ChargingDock` and `NonChargingDock`).
- Implemented this logic in `SimpleChargingDock` and
  `SimpleNonChargingDock` using a `std_srvs/Trigger` service call and
  dynamic topic subscription to manage the detector's lifecycle.
- The `DockingServer` now ensures `stopDetectionProcess()` is called
  on all exit paths, including success, failure, and cancellation,
  to prevent dangling resources.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  to configure this behavior and updated the README documentation.
- Added comprehensive C++ unit tests to verify the new detector
  lifecycle logic, specifically covering service failure cases and
  proper cleanup on action termination.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>

-

updtae
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 16, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
dc2325d 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  docking plugin interface (`ChargingDock` and `NonChargingDock`).
- Implemented this logic in `SimpleChargingDock` and
  `SimpleNonChargingDock` using a `std_srvs/Trigger` service call and
  dynamic topic subscription to manage the detector's lifecycle.
- The `DockingServer` now ensures `stopDetectionProcess()` is called
  on all exit paths, including success, failure, and cancellation,
  to prevent dangling resources.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  to configure this behavior and updated the README documentation.
- Added comprehensive C++ unit tests to verify the new detector
  lifecycle logic, specifically covering service failure cases and
  proper cleanup on action termination.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>

-

updtae
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 16, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
7c5c042 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  docking plugin interface (`ChargingDock` and `NonChargingDock`).
- Implemented this logic in `SimpleChargingDock` and
  `SimpleNonChargingDock` using a `std_srvs/Trigger` service call and
  dynamic topic subscription to manage the detector's lifecycle.
- The `DockingServer` now ensures `stopDetectionProcess()` is called
  on all exit paths, including success, failure, and cancellation,
  to prevent dangling resources.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  to configure this behavior and updated the README documentation.
- Added comprehensive C++ unit tests to verify the new detector
  lifecycle logic, specifically covering service failure cases and
  proper cleanup on action termination.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>

-

updtae
bkoensgen added a commit to bkoensgen/navigation2 that referenced this issue Jun 17, 2025
@bkoensgen
feat(opennav_docking): Add dynamic lifecycle for external detectors
48771be 
This change introduces an API to dynamically enable and disable external
docking detectors, such as those based on AI, to conserve system
resources when not actively docking.

Key changes:
- Added `startDetectionProcess()` and `stopDetectionProcess()` to the
  docking plugin interface (`ChargingDock` and `NonChargingDock`).
- Implemented this logic in `SimpleChargingDock` and
  `SimpleNonChargingDock` using a `std_srvs/Trigger` service call and
  dynamic topic subscription to manage the detector's lifecycle.
- The `DockingServer` now ensures `stopDetectionProcess()` is called
  on all exit paths, including success, failure, and cancellation,
  to prevent dangling resources.
- Added new parameters (`detector_service_name`, `subscribe_toggle`, etc.)
  to configure this behavior and updated the README documentation.
- Added comprehensive C++ unit tests to verify the new detector
  lifecycle logic, specifically covering service failure cases and
  proper cleanup on action termination.

Closes ros-navigation#5015
Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>

-

updtae

Signed-off-by: Koensgen Benjamin <b.koensgen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ayushkumar8340 ayushkumar8340

Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
4 participants
@mikeferguson @SteveMacenski @ayushkumar8340 @bkoensgen