RSDK-4788: Add Readings to Python SDK #459

martha-johnston · 2023-10-10T19:16:46Z

fix imports (from sensor to common) and use GetReadingsRequest/Response for movement sensor and power sensor

github-actions · 2023-10-10T19:17:18Z

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	is_moving
board	gpio_pin_by_name
camera	get_image
encoder	get_position
motor	is_moving
sensor	get_readings
servo	get_position
arm	get_end_position
gantry	get_lengths
gripper	is_moving
movement_sensor	get_linear_acceleration
input_controller	get_controls
audio	get_properties
pose_tracker	get_poses
power_sensor	get_power
motion	get_pose
vision	get_classifications_from_camera

src/viam/components/power_sensor/power_sensor.py

src/viam/components/movement_sensor/movement_sensor.py

src/viam/components/movement_sensor/client.py

src/viam/components/movement_sensor/movement_sensor.py

…79d8ea1a8efdffdb670c3559e8b6b

martha-johnston · 2023-10-18T20:13:34Z

@njooma @benjirewis @clintpurser does anyone know if I should be running make buf locally and uploading that? or should those be generated through GitHub actions? seems like test aren't passing because it doesn't have the new api methods (but it should because api was merged)

…s/viam-python-sdk into rsdk-4788

benjirewis · 2023-10-19T15:13:44Z

src/viam/services/navigation/navigation.py

@@ -5,6 +5,7 @@

 from ..service_base import ServiceBase
 from . import GeoObstacle, GeoPoint, Mode, Waypoint
+from viam.proto.service.navigation import Path


@njooma here and elsewhere we were just importing Path from viam.proto.service.navigation. I see some other API types being imported from .; how does that work? We added Path (along with GetPathsRequest and GetPathsResponse) to src/viam/proto/service/navigation/__init__.py, but I don't think we were able to find Path in . here.

The . syntax is only if the class is included in the src/viam/services/navigation/__init__.py, not the .../proto/... file.

benjirewis · 2023-10-19T15:16:24Z

tests/test_motion_service.py

@@ -17,7 +17,8 @@
    "gantry": PoseInFrame(reference_frame="gantry", pose=Pose(x=2, y=3, z=4, o_x=3, o_y=4, o_z=5, theta=21)),
 }
 MOTION_CONFIGURATION = MotionConfiguration(
-    vision_services=[ResourceName(namespace="rdk", type="service", subtype="vision", name="viz1")],
+    obstacle_detectors=[ObstacleDetector(vision_service=ResourceName(namespace="rdk", type="service", subtype="vision", name="viz1"),
+                                         camera=ResourceName(namespace="rdk", type="component", subtype="camera", name="cam1"))],


I assume it's fine to just use a random camera here in the test?

Anything that can return GetObjectPointCluds for an obstacle. which cameras do.

benjirewis · 2023-10-19T15:17:08Z

src/viam/services/navigation/client.py

@@ -36,6 +39,11 @@ def __init__(self, name: str, channel: Channel):
        self.client = NavigationServiceStub(channel)
        super().__init__(name)

+    async def get_paths(self, *, timeout: Optional[float] = None) -> List[Path]:


[q] Why do we return API types to users? I thought we would convert the API type to some native Python class and return that to the user?

I want to request that follow-up API cleanup that doesn't have to do with Martha's readings change be handled in a follow-up ticket. This design does look wrong, though ) I think it should return a Python class but I don't want to make a decision given we're doing this on the fly.

It's not bad to return a protobuf message. It's actually preferred IMO. This way we don't have to create a bunch of classes that mirror the proto, nor we do we have to keep those classes updated.

@njooma that makes sense. Per recent conversation in the SDKs channel, it seems like it's just language-dependent whether the protos are "good enough" to not need native-mirrored types.

randhid · 2023-10-20T15:10:59Z

@njooma @benjirewis @clintpurser all good?

benjirewis

LGTM but probably wait for final approval from @njooma

njooma

@martha-johnston couple of Qs:

In the Flutter SDK, you changed MovementSensor so that it's no longer a subclass of Sensor. Do we want to do that here too? https://github.com/viamrobotics/viam-python-sdk/pull/459/files#diff-dae80644bfff17971abcac39a1b73496a043c30b4c86c0b72de2cc8d7869f9e2R14
The return type in the docstring should maybe be updated to reflect that anything could be returned, since it's no longer defined by the SDK
Can you also add get_readings in the ExampleMovementSensor

njooma · 2023-10-23T18:30:14Z

src/viam/services/navigation/client.py

@@ -36,6 +39,11 @@ def __init__(self, name: str, channel: Channel):
        self.client = NavigationServiceStub(channel)
        super().__init__(name)

+    async def get_paths(self, *, timeout: Optional[float] = None) -> List[Path]:


It's not bad to return a protobuf message. It's actually preferred IMO. This way we don't have to create a bunch of classes that mirror the proto, nor we do we have to keep those classes updated.

njooma · 2023-10-23T18:32:10Z

src/viam/services/navigation/navigation.py

@@ -5,6 +5,7 @@

 from ..service_base import ServiceBase
 from . import GeoObstacle, GeoPoint, Mode, Waypoint
+from viam.proto.service.navigation import Path


The . syntax is only if the class is included in the src/viam/services/navigation/__init__.py, not the .../proto/... file.

martha-johnston · 2023-10-24T15:31:07Z

@njooma I think I covered everything... I do want movement sensor to no longer be a subclass of sensor so that should be fixed now, I updated the docstring, and added get_readings to ExampleMovementSensor. also updated the nav init.py file so path could be imported along with GeoPoint, Waypoint, etc.

njooma

Almost!! One minor change. Approving it because you should be able to merge it in once you add that additional change

njooma · 2023-10-24T15:38:03Z

src/viam/services/navigation/__init__.py

@@ -1,5 +1,5 @@
 from viam.proto.common import GeoObstacle, GeoPoint
-from viam.proto.service.navigation import Mode, Waypoint
+from viam.proto.service.navigation import Mode, Waypoint, Path


This will work, but you should also add it in the __all__ list below

martha-johnston · 2023-10-24T15:43:38Z

tests/test_robot.py

@@ -60,7 +60,6 @@
    ResourceName(namespace=RESOURCE_NAMESPACE_RDK, type=RESOURCE_TYPE_COMPONENT, subtype="camera", name="camera1"),
    ResourceName(namespace=RESOURCE_NAMESPACE_RDK, type=RESOURCE_TYPE_COMPONENT, subtype="motor", name="motor1"),
    ResourceName(namespace=RESOURCE_NAMESPACE_RDK, type=RESOURCE_TYPE_COMPONENT, subtype="movement_sensor", name="movement_sensor1"),
-    ResourceName(namespace=RESOURCE_NAMESPACE_RDK, type=RESOURCE_TYPE_COMPONENT, subtype="sensor", name="movement_sensor1"),


made the __all__ change, but this was causing the tests to fail... I removed this line because they were both called "movement_sensor1" but this was of subtype "sensor", and now the tests pass. Just want to make sure this checks out @njooma

Ohh nice find. There's another spot where this should be updated, but I made a ticket for it for myself for later (not required for this)

njooma · 2023-10-24T15:56:05Z

tests/test_robot.py

@@ -60,7 +60,6 @@
    ResourceName(namespace=RESOURCE_NAMESPACE_RDK, type=RESOURCE_TYPE_COMPONENT, subtype="camera", name="camera1"),
    ResourceName(namespace=RESOURCE_NAMESPACE_RDK, type=RESOURCE_TYPE_COMPONENT, subtype="motor", name="motor1"),
    ResourceName(namespace=RESOURCE_NAMESPACE_RDK, type=RESOURCE_TYPE_COMPONENT, subtype="movement_sensor", name="movement_sensor1"),
-    ResourceName(namespace=RESOURCE_NAMESPACE_RDK, type=RESOURCE_TYPE_COMPONENT, subtype="sensor", name="movement_sensor1"),


Ohh nice find. There's another spot where this should be updated, but I made a ticket for it for myself for later (not required for this)

martha-johnston added 2 commits October 10, 2023 15:13

add GetReadings to sensor, movementsensor, powersensor

325f7dd

question

55064f5

martha-johnston requested a review from a team as a code owner October 10, 2023 19:16

martha-johnston requested review from clintpurser and benjirewis October 10, 2023 19:16

martha-johnston commented Oct 10, 2023

View reviewed changes

src/viam/components/power_sensor/power_sensor.py Show resolved Hide resolved

martha-johnston commented Oct 10, 2023

View reviewed changes

src/viam/components/movement_sensor/movement_sensor.py Show resolved Hide resolved

benjirewis requested a review from njooma October 11, 2023 14:02

benjirewis reviewed Oct 11, 2023

View reviewed changes

src/viam/components/movement_sensor/client.py Show resolved Hide resolved

src/viam/components/movement_sensor/movement_sensor.py Show resolved Hide resolved

martha-johnston and others added 4 commits October 11, 2023 16:06

fix movement and power

4f00a0c

fix tests local api

9750371

Merge branch 'viamrobotics:main' into rsdk-4788

fc83b5a

[WORKFLOW] Updating protos from viamrobotics/api, commit: 8fc476f421f…

063d5e5

…79d8ea1a8efdffdb670c3559e8b6b

martha-johnston added 2 commits October 19, 2023 09:49

formatting changes

3937a22

Merge branch 'workflow/update-protos' of ssh://github.com/viamrobotic…

c32a8e5

…s/viam-python-sdk into rsdk-4788

martha-johnston force-pushed the rsdk-4788 branch from d53203b to c32a8e5 Compare October 19, 2023 14:18

nav changes

f0a5654

martha-johnston force-pushed the rsdk-4788 branch from de8bca8 to f0a5654 Compare October 19, 2023 15:04

benjirewis reviewed Oct 19, 2023

View reviewed changes

randhid approved these changes Oct 20, 2023

View reviewed changes

randhid requested a review from benjirewis October 23, 2023 13:10

benjirewis approved these changes Oct 23, 2023

View reviewed changes

njooma requested changes Oct 23, 2023

View reviewed changes

address pr comments

1137bbc

martha-johnston requested a review from njooma October 24, 2023 15:31

njooma approved these changes Oct 24, 2023

View reviewed changes

fix test

8d17d18

martha-johnston commented Oct 24, 2023

View reviewed changes

njooma approved these changes Oct 24, 2023

View reviewed changes

martha-johnston merged commit b69b05d into viamrobotics:main Oct 24, 2023

RSDK-4788: Add Readings to Python SDK #459

RSDK-4788: Add Readings to Python SDK #459

Uh oh!

Conversation

martha-johnston commented Oct 10, 2023

Uh oh!

github-actions bot commented Oct 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martha-johnston commented Oct 18, 2023

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!

benjirewis Oct 23, 2023 • 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!

randhid commented Oct 20, 2023

Uh oh!

benjirewis left a comment

Choose a reason for hiding this comment

Uh oh!

njooma 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!

martha-johnston commented Oct 24, 2023

Uh oh!

njooma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martha-johnston Oct 24, 2023 • 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!

Uh oh!

benjirewis Oct 23, 2023 •

edited

Loading

martha-johnston Oct 24, 2023 •

edited

Loading