Brian/deep object detection #26
Conversation
…config issues and removed other launch files TODO: Allow dynamic batch sizes. Optimize!
Edwardius
left a comment
Edwardius
left a comment
There was a problem hiding this comment.
Good work! Some comments to address, most of which should remove more code than it adds.
I'm seeing a number of recovery mechanisms that I'm not fond of. Silently failing is very dangerous, and stops developers and users from making sure that the code is running exactly as they want it to.
deep_object_detection/CMakeLists.txt
Outdated
| if(BUILD_TESTING) | ||
| find_package(ament_cmake_gtest REQUIRED) | ||
| if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/test/test_postprocessors.cpp) | ||
| ament_add_gtest(test_postprocessors test/test_postprocessors.cpp) |
There was a problem hiding this comment.
these should be using deep_test library
| #### Step 1: Launch Camera Sync Node | ||
|
|
||
| First, launch the camera sync node to synchronize your camera feeds: | ||
|
|
||
| ```bash | ||
| ros2 launch camera_sync multi_camera_sync.launch.yaml | ||
| ``` | ||
|
|
||
| Or with custom configuration using a parameter file: | ||
|
|
||
| Create a config file `camera_sync_config.yaml`: | ||
| ```yaml | ||
| multi_camera_sync: | ||
| ros__parameters: | ||
| camera_topics: | ||
| - "/CAM_FRONT/image_rect_compressed" | ||
| - "/CAM_FRONT_LEFT/image_rect_compressed" | ||
| - "/CAM_FRONT_RIGHT/image_rect_compressed" | ||
| use_compressed: true | ||
| sync_tolerance_ms: 33.0 | ||
| ``` | ||
| Then launch with: | ||
| ```bash | ||
| ros2 launch camera_sync multi_camera_sync.launch.yaml \ | ||
| --ros-args --params-file camera_sync_config.yaml | ||
| ``` | ||
|
|
||
| Alternatively, modify the default config file at: | ||
| `$(find-pkg-share camera_sync)/config/multi_camera_sync_params.yaml` | ||
|
|
There was a problem hiding this comment.
multi_camera_sync and this node should be able to be added into a single component container to allow for zero copy message passing. If that already works, maybe make note of it here?
| description="Detections output topic override", | ||
| ) | ||
| provider_arg = DeclareLaunchArgument( | ||
| "preferred_provider", |
There was a problem hiding this comment.
provider should be explicit, if one fails, the node should just fail, no fall back needed
| if (!executor_) { | ||
| throw std::runtime_error("No backend initialized; dropping batch"); | ||
| } |
There was a problem hiding this comment.
if no backend is initialized, node should just fail to initialize no?
There was a problem hiding this comment.
i have a hunch that this code is the result of a badly designed execution provider backend plugin that I made. Please check if this code should actually just be part of the backend execution plugin (also deep_object_detection should not have any onnx-specific implementation details in it, it should agnostic to the backend it uses.)
There was a problem hiding this comment.
question you should be asking is: if i changed the backend plugin i used, would this node still run?
things that are supported:
tested with yolov8m.onnx and camera_sync, publishing at ~30hz with tensorrt ep