[RSDK-8336] Add GetMappingSessionPointcloud and GetActiveMappingSessionForRobotID#3
[RSDK-8336] Add GetMappingSessionPointcloud and GetActiveMappingSessionForRobotID#3
Conversation
penguinland
left a comment
penguinland
left a comment
There was a problem hiding this comment.
LGTM! All my comments are minor things.
| SyncClient: pbDataSync.NewDataSyncServiceClient(conn), | ||
| PackageClient: pbPackage.NewPackageServiceClient(conn), | ||
| HTTPClient: &http.Client{}, | ||
| // Disable keepalives makes each request only last for a single http GET request |
There was a problem hiding this comment.
The comment says what it does, but not why it does that. This option means you need to redo the TCP handshake every time you make a new request, which might even involve redoing the TLS handshake and generating/exchanging an encryption key, if you're using HTTPS. Why go through all that effort every time?
| // GetDataFromHTTP makes a request to an http endpoint app serves, which gets redirected to GCS. | ||
| // will remove nolint in the next pr when this function gets used to retrieve pcds | ||
| // | ||
| //nolint:unused |
There was a problem hiding this comment.
👍 Thanks for maintaining the comments like this!
| mapRefreshRate = 1 / 5. // Hz | ||
| defaultLidarFreq = 5 // Hz | ||
| defaultMSFreq = 20 // Hz | ||
| mapRefreshRate = 1 / 0.2 // Seconds |
There was a problem hiding this comment.
Idle curiosity: why not make it 5 seconds? You seem to be emphasizing that this is 0.2 Hz instead of 5 seconds, which seems odd to me (but maybe makes sense in context).
Thanks for updating the units! That was an important change.
There was a problem hiding this comment.
ya know ur not wrong
| activeJob atomic.Pointer[string] | ||
| lastPose atomic.Pointer[spatialmath.Pose] | ||
| // lastPCD atomic.Pointer[string] | ||
| lastPCD atomic.Pointer[string] |
There was a problem hiding this comment.
Should it be obvious to someone working on this codebase what PCD stands for? If so, this is fine. but if not, put in a comment somewhere expanding the acronym. Not sure if it should be here or up at initPCDURL, or somewhere else.
There was a problem hiding this comment.
I wonder if this should be named lastPCDURL, to match initPCDURL. At first glance, I had expected we'd look up whatever is stored in initPCDURL and put its contents in lastPCD.
There was a problem hiding this comment.
I could change it to lastPointCloudURL to make it obvious, and I agree that it should include URL in the name
|
|
||
| func (svc *cloudslamWrapper) activeMappingSessionThread(ctx context.Context) { | ||
| for { | ||
| if !goutils.SelectContextOrWait(ctx, time.Duration(1000.*mapRefreshRate)*time.Millisecond) { |
There was a problem hiding this comment.
Mild preference to remove the period from 1000: I don't see why we need it to be a float. If it does need to be a float, mild preference to make it 1000.0 to emphasize that it's a float, since the period is easy to overlook.
| svc.lastPose.Store(&currPose) | ||
| svc.lastPCD.Store(&resp.MapUrl) | ||
| } | ||
| } |
There was a problem hiding this comment.
👍 This function is so straightforward! Thanks for making it easy to read.
susWorld
left a comment
susWorld
left a comment
There was a problem hiding this comment.
my comments are mostly code style related.
| defaultLidarFreq = 5 // Hz | ||
| defaultMSFreq = 20 // Hz | ||
| mapRefreshRate = 1 / 0.2 // Seconds |
There was a problem hiding this comment.
Where did you get these values from? Some comments explaining it would be nice
| wrapper.lastPose.Store(&initPose) | ||
| initJob := "" | ||
| wrapper.activeJob.Store(&initJob) | ||
| wrapper.lastPCD.Store(&initPCDURL) |
There was a problem hiding this comment.
newSLAM can definitely be broken down into smaller helper functions. It's too big
There was a problem hiding this comment.
I don't really like the idea of making helper functions that only get used once. At some point I may turn more of the robot/config related fields into their own struct but I do not think this is the pr to do that. If people feel strongly I can make the single-use helpers
| return wrapper, nil | ||
| } | ||
|
|
||
| func (svc *cloudslamWrapper) activeMappingSessionThread(ctx context.Context) { |
There was a problem hiding this comment.
missing function description. This applies to all functions without a description
| wrapper.lastPCD.Store(&initPCDURL) | ||
|
|
||
| // using this as a placeholder image. need to determine the right way to have the module use it | ||
| path := filepath.Clean("./test2.pcd") |
There was a problem hiding this comment.
./test2.pcd can be a global variable, that way you can simplify the next line to bytes, err := os.ReadFile(filepath.Clean(PATH_TO_FILE))
3 participants
https://viam.atlassian.net/browse/RSDK-8336
This pr adds support for viewing in progress cloudslam maps and grabbing the active mapping session on startup, if the robot already has an active cloudslam session.
additionally fixes an issue where http connections were not closing properly when stopping the module.