initial module by JohnN193 · Pull Request #1 · viam-modules/cloudslam-wrapper

JohnN193 · 2024-07-16T20:13:41Z

adding the module piece by piece. hopefully this does not go poorly. first is everything but the actual code

this first pr is only for standing up the repo, setting up the module that implements an unimplemented slam service and creates clients to talk to app, as well as some stuff for building & linting the code. ideally just look at the go code in module.go and the cloudslam/ for this pr, but let me know if ya got questions for other parts of the code and I can try to answer!

https://viam.atlassian.net/browse/RSDK-8334

penguinland

LGTM! All my feedback is either minor or not actionable.

penguinland · 2024-07-17T16:33:17Z

@@ -0,0 +1,56 @@
+#!/bin/bash
+


Mild preference for a comment at the top of the file explaining what this is for. Why do we need to "find the real linker," as though there's a fake linker, or we can't find any linker at all, or something like that?

tbh this file is ripped straight from rdk, and i needed it to fix som glibc issue i was having when trying to build the binary

So, I get that you just copied this from elsewhere, but that doesn't answer the question of what it does or why it's important. Any info about that would be appreciated in the file. and if the comment in the file is just "this is copied from the RDK because glibc doesn't compile without it. We don't know why it works, but it apparently does," that's better than nothing.

thought i was explicitly using this in the Makefile still, but it looks like I removed that part of the code at some point. the script was used to generate the correct build flags after i added the rimage package to the repo. Going to remove it for now in the hopes that i don't need it again later.

because I don't have mastery over the build systems in rdk I don't want to make any claims that I don't know, and I do not believe its worth interviewing the people that do have that mastery to add comments that they chose to not include.

penguinland · 2024-07-17T17:13:51Z

+
+// NewDataSyncClientFronConn creates a new DataSyncClient.
+func NewDataSyncClientFronConn(conn rpc.ClientConn) pbDataSync.DataSyncServiceClient {
+	c := pbDataSync.NewDataSyncServiceClient(conn)


I guess these functions are fine, but they seem like they're just an extra redirect around a different function. Wouldn't it be both less code and run faster to just make an alias?

NewCloudSLAMClientFromConn := pbCloudSLAM.NewCloudSLAMServiceClient NewPackageClientFromConn := pbPackage.NewPackageServiceClient NewDataSyncClientFromConn := pbDataSync.NewDataSyncServiceClient

removed functions entirely

(The functions are still here. I wonder if you have unpushed changes on your local machine.)

penguinland · 2024-07-17T17:18:05Z

+	packageClient pbPackage.PackageServiceClient
+	syncClient    pbDataSync.DataSyncServiceClient
+
+	activeBackgroundWorkers sync.WaitGroup


Mild preference to use an rdk/utils.StoppableWorkers, which replaces this line and lets you get rid of lines 58 and 59.

tru i did this before u actually showed me stoppableworkers

so I still use the cancelCtx for my clients to app, so I do not want to get rid of those, switching to stoppableWorkers for future changes otherwise

You can get the context out of a StoppableWorkers by calling workers.Context(). but I don't feel strongly. Leave as-is if it's too much effort to change. I do believe you implemented it correctly in here.

penguinland

This all still LGTM. All my feedback is minor stuff.

penguinland · 2024-07-18T15:59:17Z

+	return c
+}
+
+// getDataFromHTTP makes a request to an http endpoint app serves, which gets redirected to GCS.


...ohh, I've been parsing this sentence wrong the whole time! Perhaps it says that this function makes a request to an http endpoint which is served by the program in the app repo! I'd put in a word like "which" or "that" between "endpoint" and "app" to make this clearer. I was sure that it was talking about some kind of "endpoint app," and couldn't figure out what the "serves" was doing on the end of that.

penguinland · 2024-07-18T16:06:18Z

+)
+
+// CreateCloudSLAMClient creates a new grpc cloud configured to communicate with the robot service based on the cloud config given.
+func (svc *cloudslamWrapper) CreateCloudSLAMClient() error {


Discussion question: it's confusing to define the cloudslamWrapper struct in one file and define methods on it in another file, because looking at cloudslam.go you'd never expect this function to exist, and looking here you can't easily see what svc contains. However, it's also confusing to put it into one file because very long files are hard to keep in your head all at once. Which is easier to work with? I tentatively lean towards one giant file, but can see arguments for both sides.

edit: if the file is too long, we could split Config out to a separate file. Not sure how helpful that would be, though, since the Config stuff is not very long.

I personally liked splitting them up into separate files that "own" different functionalities. In the past it was annoying to manage all of the cloudslam utilities when they were in the same file, especially when I go to add package creation(which is where the sync/package clients come into play). Ideally I would make my own struct for each file, but since everything was part of the same package I figured it was okay to use the same struct.

penguinland · 2024-07-18T16:07:08Z

+	}
+
+	dialOpts := make([]rpc.DialOption, 0, 2)
+	// Only add credentials when secret is set.


Comment doesn't match code: we're adding credentials no matter what. Was there supposed to be an if statement here?

leftover from old client code, removing

penguinland · 2024-07-18T16:08:39Z

+		return err
+	}
+
+	dialOpts := make([]rpc.DialOption, 0, 2)


Why does this start with capacity 2? I think we add at most 1 item to it.

penguinland · 2024-07-18T16:09:10Z

+		}),
+	)
+
+	conn, err := rpc.DialDirectGRPC(svc.cancelCtx, u.Host, svc.logger.AsZap(), dialOpts...)


dialOpts is an array of length exactly 1. Why bother with the array at all? This seems like it would be easier if dialOpts were just an rpc.DialOption without the array.

leftover from an old client code. switched over.

penguinland · 2024-07-18T16:09:48Z

+
+// NewDataSyncClientFronConn creates a new DataSyncClient.
+func NewDataSyncClientFronConn(conn rpc.ClientConn) pbDataSync.DataSyncServiceClient {
+	c := pbDataSync.NewDataSyncServiceClient(conn)


(The functions are still here. I wonder if you have unpushed changes on your local machine.)

penguinland · 2024-07-18T16:12:49Z

+		}),
+	)
+
+	conn, err := rpc.DialDirectGRPC(svc.cancelCtx, u.Host, svc.logger.AsZap(), dialOpts...)


If you replace svc.cancelCtx with svc.workers.Context(), you can get rid of cancelCtx and cancelFunc entirely. However, this assumes that svc.workers is non-nil when we get to here, and I don't think that's currently true. Maybe leave as-is.

yeah the workers are made last, and the clients need to exist before I can start the thread the workers will use.

penguinland · 2024-07-18T16:15:47Z

+	VIAMVersion    string // optional cloudslam setting, describes which viam-server appimage to use(stable/latest/pr/pinned)
+	SLAMVersion    string // optional cloudslam setting, describes which cartographer appimage to use(stable/latest/pr/pinned)
+	MSFreq         float64
+	LidarFreq      float64


All these fields are public mutable state. Mild preference to make them private, before someone else starts using them. I doubt it makes a difference in this repo, but it's a good habit to get into. Public mutable state makes everything harder to reason about!

JohnN193 added 2 commits July 16, 2024 16:12

initial module without code

f0a85af

intitial cloudslam module

4a95181

JohnN193 requested review from penguinland and susWorld July 16, 2024 21:34

penguinland approved these changes Jul 17, 2024

View reviewed changes

address feedback, add comments to struct

c6ca851

JohnN193 requested a review from mariapatni July 17, 2024 18:21

JohnN193 added 3 commits July 17, 2024 16:06

lint

4514c19

rm unused file

f093a0a

remove unused line

bf089b1

penguinland approved these changes Jul 18, 2024

View reviewed changes

address feedback

2bbc58b

JohnN193 merged commit 5cb0ad5 into main Jul 23, 2024

JohnN193 deleted the init-module branch July 23, 2024 20:30

Conversation

JohnN193 commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

penguinland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnN193 Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

penguinland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

penguinland Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JohnN193 commented Jul 16, 2024 •

edited

Loading

JohnN193 Jul 17, 2024 •

edited

Loading

penguinland Jul 18, 2024 •

edited

Loading