Skip to content

Conversation

@penguinland
Copy link
Contributor

This turned out to be two problems combined! 1) update the SDK to use a more recent version of the protobufs in the API repo, and then 2) remove the Status RPC from the boards. The PR isn't quite done: I'm unsure how this should interact with #230, which is where the compiled version of the updated protos is. but the rest I think is ready for a look.

Updating the protobufs was harder than I expected: viamrobotics/api#470 (unrelated to boards) added a new type that had not been used previously, which meant that getting it to work required updating CMakeLists.txt to include the Google libraries for the new type.

Changes to boards:

  • get_status doesn't exist any more. You can get the standard status from a component the same way you would for a motor, but there isn't a special board-specific one any more.
  • In the component status you can get, analog and digital interrupt values are stored as ints, not wrappers around ints. No need to unwrap.
  • get_analog_names and get_digital_interrupt_names can't easily be implemented any more. They don't exist in the Flutter and TypeScript SDKs, and are hopefully being removed from the Go and Python SDKs soon (they have a purpose on the server side, but no obvious use on the client side, so probably shouldn't be in these SDKs). So, I removed them.
  • Similarly, I removed get_analog_readers and get_digital_interrupts, as they're not present in the Go SDK and would be very hard to implement without the old status.

Everything compiles against a recent version of the API repo, but not against the protobufs copied into this repo itself. How should that be accomplished?

@penguinland penguinland requested a review from a team as a code owner May 3, 2024 14:35
@penguinland penguinland requested review from njooma and stuqdog and removed request for a team May 3, 2024 14:35
Copy link
Contributor

@martha-johnston martha-johnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as always, thanks for leaving such a detailed description, it was super helpful

Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing only build stuff here, I'll leave the actual SDK change to other reviewers, though since they are almost entirely removals it basically looks fine to me.

Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me! It seems we're pulling in a lot of new code though, is it possible to filter some of that down?

Copy link
Contributor Author

@penguinland penguinland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take another look: the extra build dependencies are pruned down to just:

  • app/mltraining/v1/ml_training.pb.* (dependency added in viamrobotics/api#470), and
  • google/rpc/status.pb.* mentioned in ml_training.pb.*

I'm still unsure how this PR should interact with #230, to get the latest .proto files over here. Any advice?

@penguinland
Copy link
Contributor Author

penguinland commented May 7, 2024

Github's CI cannot build this because it requires a more recent version of the compiled .proto files from the api repo than what's currently in src/viam/api/. What's the procedure for updating that? I still believe the correct versions might be in #230, but that PR without this one would be just as problematic as this PR without that one.

@penguinland
Copy link
Contributor Author

I talked to Ethan about how to get the updated protos: he says I should rebase this branch off of #230, then change this PR to pull into that branch instead of main. When we're happy here, we merge this into #230, and then merge that PR into main. Changes will be coming soon...

@penguinland penguinland changed the base branch from main to workflow/update-protos May 8, 2024 17:01
@penguinland
Copy link
Contributor Author

Okay, I've rebased and changed the target branch. Let's see how this looks...

@github-actions github-actions bot force-pushed the workflow/update-protos branch from 16e4193 to ea086e0 Compare May 9, 2024 15:27
@penguinland
Copy link
Contributor Author

Rebased to get rid of merge conflicts. If the CI checks pass, I'm going to merge this...

@penguinland penguinland merged commit cdafd50 into viamrobotics:workflow/update-protos May 9, 2024
@penguinland penguinland deleted the update_status branch May 9, 2024 16:39
@penguinland penguinland restored the update_status branch May 9, 2024 19:39
stuqdog pushed a commit that referenced this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants