[RSDK-13623] Implement golang bindings into the trajex CAPI#15
Conversation
JohnN193
left a comment
JohnN193
left a comment
There was a problem hiding this comment.
certainly easier to follow than cartographer. few small things but lgtm past that.
claude also had some nits, but they didn't seem that useful (like using Description to describe optionals & defaults)
| @@ -0,0 +1,215 @@ | |||
| //go:build !windows && !no_cgo | |||
There was a problem hiding this comment.
alittle weird calling the directory go, as that will make all the imports point to trajex/go, since things will import as go.<XXX> unless its manually changed.
mostly a nit, but maybe something like trajex/pkg/trajex would work instead? that way it still imports to trajex.<xxx>
There was a problem hiding this comment.
Yeah, claude and I went around a few times before settling on this. There is prior art for this. Basically, if we don't have go.mod at the root of the tree then it makes import naming weird for consumers. If we do have go.mod at the root of the tree, we can rename away from go and then it just becomes canonically the go tree. I had wanted to put the go sources under src/viam the same way the C++ sources are, but due to golang's pedantic insistence, that would have put src into the package name. Yuck. Overall, this seemed cleanest.
There was a problem hiding this comment.
yeah having peeps rename is fine with me
| if err := ctx.Err(); err != nil { | ||
| return err | ||
| } | ||
| inHandle := (*C.viam_trajex_tensor_map_t)(inputs.UnsafeHandle()) |
There was a problem hiding this comment.
check against nil inputs?
There was a problem hiding this comment.
Is that useful / idiomatic in go? I sort of feel like passing nil in for an object pointer means you get a panic makes sense. Happy to be told otherwise since this isn't my language.
There was a problem hiding this comment.
in the past we ran into issues with nils getting passed around in go, (some relevant tickets https://viam.atlassian.net/browse/RSDK-7743 and https://viam.atlassian.net/browse/RSDK-6070)
tbh I don't expect it to happen with trajex, but with rdk we would lean to being more defensive to avoid panics
There was a problem hiding this comment.
Cool, easy to add.
There was a problem hiding this comment.
Actually, I looked a little deeper, and the C side will error this for us, no changes required here.
| @@ -0,0 +1,201 @@ | |||
| module github.com/viamrobotics/trajex | |||
There was a problem hiding this comment.
unless we plan on swapping to viamrobotics for this repo, we prob want to change this
| module github.com/viamrobotics/trajex | |
| module github.com/viam-modules/trajex |
There was a problem hiding this comment.
Oh, good catch. I missed that claude got the repo wrong because I'm workign against a local replacement.
There was a problem hiding this comment.
yeah its not something that would have shown up till later
|
@JohnN193 - I pushed a meaningful change not based on your review. It restructures how we pick up the CAPI library dependency by making a new |
2 participants
Ignore the cgo build stuff, it is obviously local to my system. Otherwise, this is a potential shape of things. The whole stack works, the tests pass (on my machine).