-
Notifications
You must be signed in to change notification settings - Fork 44
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.
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
Use ref-counting in Switchboard (patch memory leak) #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't go through the whole thing in detail, but left a few comments. One high level feedback that I have is that this PR should update all of the switchboard and plugin documentation (writing a new plugin, etc.).
Thread safety looks good (only skimmed through it). However, I do miss the "proof of thread safety" comment blocks that you used to have.
Didn't get a chance to review memory safety though, so that's something other reviewers should focus on.
One high level question about the thread structure: as many threads as synchronous topics, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some old unresolved conversations from a previous review, but I think you are addressing these later.
- New 'plugin_id_t' type definition - Fixed Switchboard logging - Added pre-sleep feature for debugging with gdb
- Updated inheritance in 'common/data_format.hpp' - Updated signatures in 'common/plugin.hpp' - Moved 'configs/ci.yaml' plugin order to match 'configs/native.yaml' - New preintegration handler in 'gtsam_integrator/plugin.cpp' - Reformatted 'gtsam_integrator/plugin.cpp' - Changed assertion to RAC in 'runtime/sqlite_record_logger.hpp' - Updated branch and symlinks in 'scripts/install_gtsam.sh'
… variable, debugging flags
- Removed '-fsanitize' from 'common/common.mk' - Fixed nullptr check in 'offload_data/plugin.cpp' - Added 'maybe_unused' to constexpr in 'runtime/main.cpp' - Fixed compilation for 'zed/src/main.cpp' Co-authored by Samuel Grayson <sam@samgrayson.me>
- Added comments to 'zed/CMakeLists.txxt' - Changed most topic `put` calls to use in-place calls to `allocate` - Removed 'get' alias for 'get_ro' in 'common/switchboard.hpp' - Updated 'put' example in 'common/switchboard.hpp' - Fixed version tags in 'configs/*' - Added back 'groud_truth_slam' to 'configs/native.yaml' - Added/moved 'fast_pose_sample_time' in 'gldemo/plugin.cpp' - Added back '_m_first_time' to 'gldemo.plugin.cpp' - Added in-line construction for PimObject in 'gtsam_integrator/plugin.cpp' - Added comment/todo for each integrator's 'last_imu_offset' - Removed '_m_imu_integrator' from 'offline_imu_cam/plugin.cpp' - Fixed '_m_ground_truth_offset' usage in 'pose_lookup/plugin.cpp' - Removed member names from allocations in 'pose_prediction/plugin.cpp' - Removed '_m_imu_integrator' from 'realsense/plugin.cpp' - Updated calls to 'put' in 'realsense/plugin.cpp' - Added pre-sleep usage comment in 'runtime/main.cpp' - Added back '_p_should_skip' in 'timewarp_gl/plugin.cpp'
- Added missing 'kimera_path' to 'configs/headless.yaml' - Updated versions/integration for 'configs/monada.yaml' - Updated 'time_type' usage in 'gldemo/plugin.cpp' - Fixed calls to 'abort' in 'pose_lookup' - Removed named arguments in 'pose_lookup/plugin.cpp' - Removed unused 'frame' in 'timewarp_gl/plugin.cpp'
5ca32f6
to
9e36825
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there.
@@ -17,7 +19,7 @@ SET(EXECUTABLE_OUTPUT_PATH ".") | |||
|
|||
find_package(ZED 3 REQUIRED) | |||
find_package(OpenCV REQUIRED) | |||
find_package(CUDA ${ZED_CUDA_VERSION} EXACT REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're in a gray area here. I'm willing to let this be if CUDA 11.2 has been properly tested with ZED 3.4.2 (which complains during install if you don't have the exact version).
common/switchboard.hpp
Outdated
* Thread-safe | ||
* - Caveat: | ||
* | ||
* This (circular) queue based solution may race if >= N writers attempt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't have to be N writers. Just N writes.
- Typo: 'destructible' in 'common/switchboard.hpp' - Updated description of 'event' in 'common/switchboard.hpp' - Removed unused '_m_imu_cam' in 'rk4_integrator/plugin.cpp' - Removed unneeded assertion in 'timewarp_gl/plugin.cpp'
common/tests/test_switchboard.hpp
for verification).Don't require events to inherit fromswitchboard::event
.