RSDK-7532 - Adding getProperties to vision service #3925

kharijarrett · 2024-05-08T18:51:00Z

No description provided.

github-actions · 2024-05-08T18:51:17Z

Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!

component	function
base	IsMoving
board	GPIOPinByName
camera	Properties
encoder	Properties
motor	IsMoving
sensor	Readings
servo	Position
arm	EndPosition
audio	MediaProperties
gantry	Lengths
gripper	IsMoving
input_controller	Controls
movement_sensor	LinearAcceleration
power_sensor	Power
pose_tracker	Poses
motion	GetPose
vision	ClassificationsFromCamera

bhaney

The biggest change is to add an returned error to the properties, because if we are on purpose making the Properties struct something that could potentially grow in the future, you'll want to avoid the possibility of having to add an error return in the future

Also, add tests! Or rather, you can add GetProperties checks to our existing tests. You can check to see if some of our classifiers or detectors return the expected properties. Be sure to add tests to the client and server code as well

bhaney · 2024-05-09T17:32:56Z

services/vision/client.go

+
+	ext, err := protoutils.StructToStructPb(extra)
+	if err != nil {
+		return nil


You want to return the error

bhaney · 2024-05-09T17:33:09Z

services/vision/client.go

+		Extra: ext,
+	})
+	if err != nil {
+		return nil


Return the error

bhaney · 2024-05-09T17:38:31Z

services/vision/client.go

@@ -242,6 +242,26 @@ func protoToObjects(pco []*commonpb.PointCloudObject) ([]*vision.Object, error)
 	return objects, nil
 }

+func (c *client) GetProperties(ctx context.Context, extra map[string]interface{}) *Properties {


The method should also return an error

bhaney · 2024-05-09T17:53:18Z

services/vision/vision.go

@@ -128,6 +128,8 @@ type Service interface {

 	// GetObjectPointClouds returns a list of 3D point cloud objects and metadata from the latest 3D camera image using a specified segmenter.
 	GetObjectPointClouds(ctx context.Context, cameraName string, extra map[string]interface{}) ([]*viz.Object, error)
+	// properties
+	GetProperties(ctx context.Context, extra map[string]interface{}) *Properties


I think you'll want to return an error as well -- in the interest of future proofing, what if you add something to the properties that can easily return an error? You would then have to introduce a breaking change to add the error output

bhaney · 2024-05-09T17:56:21Z

services/vision/vision.go

+func (vm *vizModel) GetProperties(ctx context.Context, extra map[string]interface{}) *Properties {
+	_, span := trace.StartSpan(ctx, "service::vision::GetProperties::"+vm.Named.Name().String())
+	defer span.End()
+
+	return &vm.properties


change to return an error

bhaney · 2024-05-09T17:56:43Z

testutils/inject/vision_service.go

@@ -28,6 +28,7 @@ type VisionService struct {
 		n int, extra map[string]interface{}) (classification.Classifications, error)
 	// segmentation functions
 	GetObjectPointCloudsFunc func(ctx context.Context, cameraName string, extra map[string]interface{}) ([]*viz.Object, error)
+	GetPropertiesFunc        func(ctx context.Context, extra map[string]interface{}) *vision.Properties


change to return an error

bhaney

Thanks for adding error and some tests, but you'll still want to add tests for GetProperties to client_test.go and server_test.go. You'll want to see that the properties are transferred over GRPC successfully

bhaney

Nice! LGTM

adding getProperties to vision service

c463854

viambot added the safe to test This pull request is marked safe to test from a trusted zone label May 8, 2024

link to real api repo

d31de85