RSDK-3323 - addSensorReading refactor #102
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
viambot
added
safe to test
…od' of https://github.com/kim-mishra/viam-cartographer into RSDK-3323-refactor-slam-service-add-sensor-reading-method
viambot
added
safe to test
kkufieta
previously requested changes
May 26, 2023
viambot
added
safe to test
viambot
added
safe to test
EshaMaharishi
approved these changes
May 26, 2023
Comment on lines
260
to
261
| cartographer::mapping::TrajectoryBuilderInterface *trajectory_builder; | ||
| int trajectory_id; |
There was a problem hiding this comment.
@kkufieta , do we need to access trajectory_builder and trajectory_id under map_builder_mutex?
If so @kim-mishra could we move this to just after L244, so that it's grouped with things that need to be accessed under map_builder_mutex?
There was a problem hiding this comment.
Yes, trajectory_builder should be protected by map_builder_mutex. It makes sense to do the same for trajectory_id even though it's probably not strictly necessary.
There was a problem hiding this comment.
Can we please move this to CPU bound initialization time (i.e. point 5)?
nicksanford
reviewed
May 30, 2023
| @@ -252,6 +256,9 @@ class SLAMServiceImpl final : public SLAMService::Service { | |||
| // started. | |||
| std::string latest_pointcloud_map; | |||
| // --- | |||
|
|
|||
| cartographer::mapping::TrajectoryBuilderInterface *trajectory_builder; | |||
There was a problem hiding this comment.
- Why does this need to be global?
- Where is this pointer initialized?
- Is it possible for
AddSensorReadingto be called beforeProcessDataAndStartSavingMapsis called & causes memory corruption due to this pointer pointing at invalid memory? - When do we free the object this pointer points to?
- If we really need a pointer on an object, could we construct this at CPU-bound initialization time (that way we are guaranteed it will always be a valid pointer) & have this be a smart pointer?
There was a problem hiding this comment.
- I believe we put this definition here so that it could be accessed within
AddSensorReadingso that we did not have to pass it in and change the function signature - I think it is initialized here within
map_builder.SetTrajectoryBuilder() - I think that would be a problem yes, we talked about moving it into the initialization function @kkufieta is working on as a separate PR
- I think that would happen in the terminate function this ticket refers to
- I am a little confused on what you mean here - would putting it in Kats init function be what you are talking aout?
Comment on lines
260
to
261
| cartographer::mapping::TrajectoryBuilderInterface *trajectory_builder; | ||
| int trajectory_id; |
There was a problem hiding this comment.
Can we please move this to CPU bound initialization time (i.e. point 5)?
| @@ -741,6 +744,8 @@ void SLAMServiceImpl::ProcessDataAndStartSavingMaps(double data_start_time) { | |||
| if (!use_live_data) { | |||
| // We still want to optimize the map in localization mode, but we do not | |||
| // need to update the backup of the map | |||
| cartographer::transform::Rigid3d tmp_global_pose; | |||
There was a problem hiding this comment.
- Why did we need to move this inside the loop?
- Is there a risk of us leaking memory from creating new
tmp_global_poses every iteration of the while loop without ever freeing the one from the previous loop?
There was a problem hiding this comment.
- Because we wanted to keep the function signature simple and not have to pass this var into
AddSensorReadingwe wanted to only initialize it if it were going to be used, although now I am getting a little confused why it is better for this to be in the loop. @kkufieta would you mind explaining this again Based on this doc I think we actually will need to delete this if we continue with this route thanks for pointing that outI don't think so, I think memory leaks only happen when you use dynamic memory (i.e.newormalloc)
There was a problem hiding this comment.
This is happening outside the loop. This code is only executed in offline mode after we've processed all the data.
viambot
added
safe to test
…od' of https://github.com/kim-mishra/viam-cartographer into RSDK-3323-refactor-slam-service-add-sensor-reading-method
viambot
added
safe to test
viambot
added
safe to test
…od' of https://github.com/kim-mishra/viam-cartographer into RSDK-3323-refactor-slam-service-add-sensor-reading-method
viambot
added
safe to test
viambot
added
safe to test
kkufieta
dismissed
their stale review
In order to remove myself as a reviewer, I have to dismiss my review.
nicksanford
reviewed
Jun 8, 2023
| @@ -647,10 +647,34 @@ void SLAMServiceImpl::SaveMapWithTimestamp() { | |||
| } | |||
| } | |||
|
|
|||
| bool SLAMServiceImpl::AddSensorReading( | |||
There was a problem hiding this comment.
NOTE: In the implementation which makes it into CartoFacade you will need to detect the first time AddSensorReading has been called & set the start time on the map builder to be the time that the first lidar reading was taken.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Move parts of code in
ProcessDataAndStartSavingMapsthat are not file syste dependent into new method calledAddSensorReadingTesting