Json edits #174
Conversation
Signed-off-by: Viba Mohan <happymorning552@gmail.com>
There was a problem hiding this comment.
Pull request overview
Consolidates multiple robot-specific entrypoints into flag-driven mains and restructures camera constants into shared + robot-specific JSON, adding explicit camera/detector backend selection.
Changes:
- Replace separate
*_bot_main/unambiguous_*binaries withrobot_mainandunambiguous_mainthat select robot config via--robot. - Extend camera constants parsing/schema to include
camera_backendanddetector_backend, and use them to choose capture/detector implementations. - Add new per-robot
constants/*/camera_constants.jsonand aconstants/shared_camera_constants.json, removing the old monolithic constants file.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/unambiguous_second.cc | Removed (superseded by unambiguous_main). |
| src/unambiguous_first.cc | Removed (superseded by unambiguous_main). |
| src/unambiguous_main.cc | New unified unambiguous entrypoint with --robot camera constants selection. |
| src/second_bot_main.cc | Removed (superseded by robot_main). |
| src/main_bot_main.cc | Removed (superseded by robot_main). |
| src/robot_main.cc | New unified robot entrypoint; starts per-camera localization threads based on --robot. |
| src/test/integration_test/localization_test.cc | Updated to load robot-specific constants and select detector based on config. |
| src/test/integration_test/localization_test2.cc | Updated to load robot-specific constants and use new camera naming scheme. |
| src/localization/unambiguous_estimator.h | API updated to take a vector of camera constants (no per-camera detector enum). |
| src/localization/unambiguous_estimator.cc | Detector backend now derived from camera_constant_t.detector_backend. |
| src/camera/select_camera.cc | Added camera name resolution to handle prefixed vs stripped names. |
| src/camera/cv_camera.cc | Uses camera_backend to open UVC cameras via V4L2 when configured. |
| src/camera/camera_constants.h | Adds CameraBackend/DetectorBackend fields to camera constants. |
| src/camera/camera_constants.cc | Parses new backend fields; changes default constants path flag. |
| src/CMakeLists.txt | Switches built executables to robot_main and unambiguous_main. |
| constants/turret_bot/camera_constants.json | New turret_bot-specific camera constants. |
| constants/second_bot/camera_constants.json | New second_bot-specific camera constants. |
| constants/main_bot/camera_constants.json | New main_bot-specific camera constants. |
| constants/fiddler/camera_constants.json | New fiddler-specific camera constants. |
| constants/shared_camera_constants.json | New shared constants file (replacing monolithic constants). |
| constants/camera_constants.json | Removed old monolithic constants file. |
|
|
||
| LOG(INFO) << "Started estimators"; | ||
|
|
||
| left_thread.join(); |
There was a problem hiding this comment.
main() launches right_thread (and potentially front_thread) but only joins left_thread before returning. Any joinable std::thread destructed at scope exit will call std::terminate, so this will crash on exit. Join (or detach) right_thread and, if front_thread is engaged, join it as well before returning.
| left_thread.join(); | |
| left_thread.join(); | |
| if (right_thread.joinable()) { | |
| right_thread.join(); | |
| } | |
| if (front_thread.has_value() && front_thread->joinable()) { | |
| front_thread->join(); | |
| } |
| UnambiguousEstimator( | ||
| std::vector<std::pair<camera::camera_constant_t, Detector>>& cameras, | ||
| const std::vector<camera::camera_constant_t>& cameras, | ||
| std::optional<uint> port_start = std::nullopt, bool verbose = false, | ||
| std::optional<std::vector<std::filesystem::path>> img_dir_paths = | ||
| std::nullopt); |
There was a problem hiding this comment.
This constructor now takes std::optional<uint> for port_start. At least one caller (src/test/integration_test/localization_test2.cc) still passes std::optional<double> (via std::make_optional<double>(5801)), which should fail to compile. Update that call site to pass std::optional<uint>.
|
|
||
| // for reference, example command: | ||
| // ./build/src/test/integration_test/l2calization_test --camera_name=main_bot_right --image_folder=logs/log181/right --speed=0.5 | ||
| // ./build/src/test/integration_test/l2calization_test --robot=main_bot --camera_name=right --image_folder=logs/log181/right --speed=0.5 |
There was a problem hiding this comment.
The example command references l2calization_test, but the built target for this file is localization_test2 (see src/test/integration_test/CMakeLists.txt). This typo can mislead anyone trying to run the test binary.
| // ./build/src/test/integration_test/l2calization_test --robot=main_bot --camera_name=right --image_folder=logs/log181/right --speed=0.5 | |
| // ./build/src/test/integration_test/localization_test2 --robot=main_bot --camera_name=right --image_folder=logs/log181/right --speed=0.5 |
| "pipeline": "", | ||
| "intrinsics_path": "/bos/constants/misc/dummy_camera_intrinsics.json", | ||
| "extrinsics_path": "/bos/constants/misc/dummy_camera_extrinsics.json", | ||
| "name": "default_usb0" |
There was a problem hiding this comment.
src/test/integration_test/solver_test.cc looks up camera::GetCameraConstants().at("dummy_camera"), but this shared constants file does not define a camera named dummy_camera (the closest appears to be default_usb0). This will throw at runtime in that test unless the constants entry name is aligned (add a dummy_camera entry/alias here, or update the test to use the new name).
| "name": "default_usb0" | |
| "name": "default_usb0" | |
| }, | |
| { | |
| "pipeline": "", | |
| "intrinsics_path": "/bos/constants/misc/dummy_camera_intrinsics.json", | |
| "extrinsics_path": "/bos/constants/misc/dummy_camera_extrinsics.json", | |
| "name": "dummy_camera" |
No description provided.