Conversation
penguinland
left a comment
penguinland
left a comment
There was a problem hiding this comment.
I still don't really grok any of this module, but the code LPlausibleTM. All my feedback is either minor or optional.
| } | ||
|
|
||
| // GetPackagesOnRobot makes a Config request to app and returns the first SLAM map that it finds on the robot. | ||
| func (app *AppClient) GetPackagesOnRobot(ctx context.Context, partID string) (string, string, error) { |
There was a problem hiding this comment.
Discussion question: does this name misleadingly suggest it gets all packages, not just SLAM paths? Perhaps it's obvious what it does in the context of the cloudslam package, and the name is good as-is. I'm unsure.
I'd at least update the comment on line 67 to explain that it returns the name and version of the map it finds; otherwise it's not clear what the return values are.
There was a problem hiding this comment.
renamed to GetSLAMMapPackageOnRobot to be more clear this will only retrieve information on the SLAM map package
|
|
||
| const ( | ||
| startJobKey = "start" | ||
| updatingModeKey = "updating mode" |
There was a problem hiding this comment.
Not sure what these keys are, but localPackageKey used hyphens instead of spaces. Is it okay to have spaces in here?
There was a problem hiding this comment.
so I was using this key as a way to only return information that updating mdoe is happening, but i should probably make the formatting consistent.
since these keys are used in a map[string]interface{} I figured it was okay to have spaces, as it improved readability. When testing I did not notice any issues, but I do not know if requests & responses are handled differently for some reason.
| } | ||
| if cfg.RobotID == "" { | ||
| return []string{}, resource.NewConfigValidationFieldRequiredError(path, "robot_id") | ||
| return []string{}, resource.NewConfigValidationFieldRequiredError(path, "machine_id") |
There was a problem hiding this comment.
Not a fan of this. If we're checking cfg.RobotID, the required JSON field should be robot_id. If the JSON field is machine_id, I'd prefer to check cfg.MachineID.
There was a problem hiding this comment.
i was being lazy and only changed the json to use machine_id, rather than changing all of the code.
I can be not lazy
| } | ||
|
|
||
| // initialize completes the initiaization of the cloudslam wrapper struct. | ||
| func (svc *cloudslamWrapper) initialize() error { |
There was a problem hiding this comment.
👍 I really like that this is broken out into a helper function. Nice work!
There was a problem hiding this comment.
still don't think its too necessary, but I realized I didn't like all the extra stuff happening after I made the struct.
|
|
||
| if isUpdating { | ||
| resp[updatingModeKey] = fmt.Sprintf("slam map found on machine, starting cloudslam in updating mode. Map "+ | ||
| "Name = %v // Updating Version = %v", svc.updatingMapName, svc.updatingMapVersion) |
There was a problem hiding this comment.
If someone wants to consume this data programmatically, they need to parse the string again. What do you think about having 3 separate keys, updatingMode, updatingMapName, and updatingMapVersion or similar?
There was a problem hiding this comment.
the problem I ran into is the keys did not appear to be guaranteed to have a specific order, and having to read that information from the docommand got alittle bit annoying
| if isUpdating { | ||
| resp[updatingModeKey] = fmt.Sprintf("slam map found on machine, starting cloudslam in updating mode. Map "+ | ||
| "Name = %v // Updating Version = %v", svc.updatingMapName, svc.updatingMapVersion) | ||
| } |
There was a problem hiding this comment.
If you're not updating, this key isn't set at all. Would it make sense to have an else down here that sets it to, like, "no pre-made map found, creating a new one" or something?
There was a problem hiding this comment.
I think its okay to not have the updating mode key, as its intended to be a feature that a user adds on top of the default cloudslam behavior. Having something always return for updating mode would notify users of the feature, but it would also be kinda spammy?
if ya feel strongly I can add something
| } | ||
| resp[startJobKey] = "Starting cloudslam session, the robot should appear in ~1 minute. Job ID: " + jobID | ||
|
|
||
| } |
There was a problem hiding this comment.
I'd swap these two lines. I never like blank lines immediately after a { or immediately before a }, but having them after the } to indicate we're about to do something very different is a good choice.
There was a problem hiding this comment.
my b, just some leftover whitespace!
| @@ -321,7 +350,8 @@ func (svc *cloudslamWrapper) StopJob(ctx context.Context) (string, error) { | |||
| } | |||
|
|
|||
| // StartJob starts a cloudslam job with the requested map name. Currently assumes a set of config parameters. | |||
There was a problem hiding this comment.
Explain what the new return value is.
...explain what the pre-existing return value is, too. 😄
2 participants
this pr adds the ability to use updating mode with the cloudslam module. It works by making a
ConfigRequest to app, then parsing the packages within the config until aslam_mappackage is found. This assumes that only one map package is configured on the robot, which is a perfectly valid assumption. The feature only works if a machine part ID is configured in on the service, which allows users a workaround to disable updating mode as an option for cloudslam, if they choose to.Using the Config API might not be the best choice, as it is the API that viam-servers use to read their own configs, but functionally it does what we want and is a part of the public API that location owners have permission to use.
ticket: https://viam.atlassian.net/browse/RSDK-8438