Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

camera: change API to support multiple cameras #352

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

julianoes
Copy link
Collaborator

This changes the API to support more than one camera within one camera plugin instance.

This will enable multiple cameras in language wrappers instead of just C++ as it is now.

/*
* Prepare the camera plugin (e.g. download the camera definition, etc).
*/
rpc Prepare(PrepareRequest) returns(PrepareResponse) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this being removed? The idea was to be able to download the camera definition in advance, because it may be slow/timeout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My plan is to have it initialize automatically when it discovers a camera.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... IIRC we've had issues in the past where is was being slow and either blocking stuff or producing a lot of timeouts. Then it was lazily initialized, but it caused issues because the very first call to a camera function was sometimes super slow (again, timeouts...).

Making it explicit has a few advantages:

  • If you don't use the camera at all, you don't download the camera definition
  • You can download the camera definition before you start using the camera (because usually you know in advance that you will use it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With C++ you would actually not even instantiate the plugin but I can see that for mavsdk_server it's a different story.

Beat added file caching for MAVSDK, so if we use that to cache the FTP file, it should be fairly quick actually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your call 👍. I just wanted to mention why I added this function (in my experience the camera definition has always been the root of tons of camera problems)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I hear you. Let me try and see how it goes.

protos/camera/camera.proto Show resolved Hide resolved
This changes the API to support more than one camera within one camera
plugin instance.

This will enable multiple cameras in language wrappers instead of just
C++ as it is now.

We also rename the status to storage because that's what it really is.
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.

2 participants