RSDK-4522 RSDK-3601 Add complex module example #150

benjirewis · 2023-09-19T20:29:24Z

Adds a complex module example (based on the Python and Go implementations). Validates that dependencies exist in the ModuleService.

benjirewis

This complex module still doesn't work: the gRPC services for gizmo and summation don't seem to get added to the generated pool. I'm looking into why that is, but I thought it might be good to get some review on everything else in the meantime (I don't think much will change).

src/viam/examples/modules/complex/CMakeLists.txt

src/viam/examples/modules/complex/base/impl.cpp

src/viam/examples/modules/complex/base/impl.hpp

src/viam/examples/modules/complex/gizmo/impl.cpp

src/viam/examples/modules/complex/proto/buf.gen.yaml

src/viam/examples/modules/simple/main.cpp

stuqdog

nice, looking pretty good! thanks for putting this together.

src/viam/examples/modules/complex/summation/api.cpp

src/viam/examples/modules/complex/README.md

src/viam/examples/modules/complex/base/impl.cpp

src/viam/examples/modules/complex/summation/api.cpp

src/viam/examples/modules/complex/summation/api.hpp

src/viam/examples/modules/complex/summation/impl.cpp

cheukt · 2023-09-21T15:58:28Z

can you add links out to the example module repos like in #152?

acmorrow

I'm not jumping into the review of the new complex module and client, but please lmk if you would like me to.

src/viam/examples/modules/complex/CMakeLists.txt

src/viam/examples/modules/complex/proto/buf.gen.yaml

src/viam/examples/modules/complex/CMakeLists.txt

src/viam/sdk/module/service.cpp

acmorrow · 2023-09-22T14:42:57Z

@benjirewis - I know there was some discussion about making the example modules standalone projects embedded within this repo, much like the example projects are today. I'd suggest for that to happen in another ticket / review rather than this one.

benjirewis · 2023-09-22T15:53:48Z

@cheukt added a reference similar to the ones you added in #152 for the complex module (thanks for adding those other ones).

@acmorrow filed https://viam.atlassian.net/browse/RSDK-5131. I won't do that work as part of this PR.

acmorrow · 2023-09-25T19:52:39Z

@benjirewis - Do you think it might be possible to pull the fix for https://viam.atlassian.net/browse/RSDK-4275 out into its own PR, which I would immediately LGTM, in the interest of getting that fix committed? It's just the two try/catch things around construct_resource but it fixes a big ergonomic headache when working with modules implemented in C++, since the status quo is that if you make any mistake in the module configuration you get failure but no actionable details.

benjirewis · 2023-09-25T20:18:57Z

@acmorrow yeah I can do that right now. Apologies for the delay on this PR, lots of small obstacles 😮‍💨 . #154 .

benjirewis

I still need to write test_module.cpp, but gRPC service descriptors are now working properly. I'll move this out of draft, and it's ready for another review.

src/viam/examples/modules/complex/main.cpp

acmorrow

This LGTM for the build system changes for the complex module example, and for the other SDK changes outside the examples. I'll leave the specifics of the complex example implementation to others.

src/viam/sdk/common/utils.cpp

benjirewis · 2023-10-02T16:24:23Z

@stuqdog Adding tests is not super trivial (having some issues linking the generated protobuf files into the existing test suite architecture). If you don't mind I'm gonna kick out that work into another ticket/PR (https://viam.atlassian.net/browse/RSDK-5226). Per @cheukt's suggestion, I tested the custom base, gizmo and summation with the Python complex module client, and everything seems to be working.

stuqdog

Thanks for the work on this! A couple minor things, but in general this is looking great!

stuqdog · 2023-10-02T18:20:03Z

src/viam/examples/modules/complex/CMakeLists.txt

+  file(CHMOD ${BUF_COMMAND} PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE)
+endif()
+
+# Buf only goes back to 1.41.1 for generation against their "custom remote"


(minor) I suspect this comment was copy pasted from the top level CMakeLists.txt? I think it's a bit out of date at this point. Buf no longer supports remote generation and so we no longer make distinctions between "before 1.41.1" and "between 1.41.1 and 1.51".

Ah thanks for that background; I did indeed copy this from the top level CMakeLists.txt. I've removed the comment, but are you saying that I don't even need two different buf.yaml files, as remote generation is not a thing anymore?

No, you still need two forms. In the past there were three required forms, and this comment was sort of trying to explain why. But we are now down to two forms. So, just remove the comment.

Eventually, we should DRY this logic with src/viam/api/CMakeLists.txt by moving buf-based proto generation support into a tool. That tool should be installed as part of the SDK, so that clients of the SDK can make use of it for their own proto generation needs. That'd be more or less forced by making the modules standalone projects anyway.

Nice; copied your comment to https://viam.atlassian.net/browse/RSDK-5131.

src/viam/examples/modules/complex/base/impl.cpp

src/viam/examples/modules/complex/client.cpp

src/viam/examples/modules/complex/gizmo/api.hpp

src/viam/examples/modules/complex/gizmo/impl.cpp

stuqdog

lgtm!

src/viam/examples/modules/complex/README.md

cheukt

LGTM

benjirewis commented Sep 19, 2023

View reviewed changes

benjirewis requested review from acmorrow, stuqdog and cheukt September 19, 2023 20:55

stuqdog requested changes Sep 20, 2023

View reviewed changes

acmorrow requested changes Sep 21, 2023

View reviewed changes

benjirewis changed the title ~~RSDK-4522 RSDK-4275 RSDK-3601 Add complex module example~~ RSDK-4522 RSDK-3601 Add complex module example Sep 25, 2023

benjirewis force-pushed the complex-module branch from 709a6a7 to f5ab178 Compare September 28, 2023 16:52

benjirewis commented Sep 28, 2023

View reviewed changes

src/viam/examples/modules/complex/main.cpp Show resolved Hide resolved

benjirewis requested a review from acmorrow September 28, 2023 20:41

benjirewis marked this pull request as ready for review September 28, 2023 20:41

benjirewis requested a review from a team as a code owner September 28, 2023 20:41

acmorrow approved these changes Sep 29, 2023

View reviewed changes

src/viam/sdk/common/utils.cpp Outdated Show resolved Hide resolved

src/viam/sdk/common/utils.cpp Outdated Show resolved Hide resolved

benjirewis requested a review from stuqdog October 2, 2023 16:25

stuqdog requested changes Oct 2, 2023

View reviewed changes

benjirewis requested a review from stuqdog October 3, 2023 16:44

stuqdog approved these changes Oct 3, 2023

View reviewed changes

cheukt reviewed Oct 3, 2023

View reviewed changes

src/viam/examples/modules/complex/README.md Show resolved Hide resolved

benjirewis force-pushed the complex-module branch from 1dc4370 to 969ebdb Compare October 3, 2023 17:57

benjirewis requested a review from cheukt October 3, 2023 17:58

cheukt approved these changes Oct 3, 2023

View reviewed changes

benjirewis added 3 commits October 3, 2023 16:02

initial

fc9e724

vaguely working MyBase

99b0675

fix mybase

602e16a

benjirewis added 21 commits October 3, 2023 16:02

fix mybase again sigh

34b5067

RSDK-3601 Validate modular resource dependencies in the ModuleService

093ff2c

gizmo initial

95663c9

attempt at gizmo streaming

f0c653f

managed buf and missing imports

bc71ad1

summation initial

2199046

add client

727a426

cleanup

1955f10

typos

f6446cb

ethan and drew comments

fefe46b

add link to example module repos

261f499

summation api include order

64bb40f

no sources in add_executable

ae67fc5

use cmake for buf generate

f2a107b

compilation errors

d81a6cc

another compilation fix attempt

0d14c6e

include generated files, do not copy to source

3facefb

register api from within main instead of statically

7937a3f

remove utils conversion functions

95aa6fc

ethan comments

c56eff0

more info about proto generation

2712255

benjirewis force-pushed the complex-module branch from 969ebdb to 2712255 Compare October 3, 2023 20:07

benjirewis merged commit 10f6b74 into viamrobotics:main Oct 3, 2023
2 checks passed

benjirewis deleted the complex-module branch October 3, 2023 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-4522 RSDK-3601 Add complex module example #150

RSDK-4522 RSDK-3601 Add complex module example #150

benjirewis commented Sep 19, 2023 •

edited

Loading

benjirewis left a comment •

edited

Loading

stuqdog left a comment

cheukt commented Sep 21, 2023

acmorrow left a comment

acmorrow commented Sep 22, 2023

benjirewis commented Sep 22, 2023

acmorrow commented Sep 25, 2023

benjirewis commented Sep 25, 2023 •

edited

Loading

benjirewis left a comment

acmorrow left a comment

benjirewis commented Oct 2, 2023 •

edited

Loading

stuqdog left a comment

stuqdog Oct 2, 2023

benjirewis Oct 3, 2023

acmorrow Oct 3, 2023

benjirewis Oct 3, 2023

stuqdog left a comment

cheukt left a comment

RSDK-4522 RSDK-3601 Add complex module example #150

RSDK-4522 RSDK-3601 Add complex module example #150

Conversation

benjirewis commented Sep 19, 2023 • edited Loading

benjirewis left a comment • edited Loading

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

cheukt commented Sep 21, 2023

acmorrow left a comment

Choose a reason for hiding this comment

acmorrow commented Sep 22, 2023

benjirewis commented Sep 22, 2023

acmorrow commented Sep 25, 2023

benjirewis commented Sep 25, 2023 • edited Loading

benjirewis left a comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

benjirewis commented Oct 2, 2023 • edited Loading

stuqdog left a comment

Choose a reason for hiding this comment

stuqdog Oct 2, 2023

Choose a reason for hiding this comment

benjirewis Oct 3, 2023

Choose a reason for hiding this comment

acmorrow Oct 3, 2023

Choose a reason for hiding this comment

benjirewis Oct 3, 2023

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

cheukt left a comment

Choose a reason for hiding this comment

benjirewis commented Sep 19, 2023 •

edited

Loading

benjirewis left a comment •

edited

Loading

benjirewis commented Sep 25, 2023 •

edited

Loading

benjirewis commented Oct 2, 2023 •

edited

Loading