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

add new camera server #274

Closed
wants to merge 4 commits into from
Closed

add new camera server #274

wants to merge 4 commits into from

Conversation

dlech
Copy link
Contributor

@dlech dlech commented Feb 14, 2022

This adds new camera server plugin protocol with a few basic commands for device discovery and still image capture.

It works like this:

  1. New camera service instance is created and attached to system.
  2. The Subscribe* methods are called to register callbacks. These will define the camera capability flags. If a callback is registered, then the capability flag is set, otherwise the flag is not set.
  3. The Set* methods are called to set the initial state.
  4. The SetInformation method is called to "activate" the plugin. This method must be called after all of the above. Also, all of the above steps must be done right away after creating the mavsdk instance, otherwise systems might think that there is no camera.
  5. To handle taking pictures, the SubscribeTakePhoto method is called. When this method is called back, the implementer must call the SetInProgress with true method right away. Then the picture is taken and the implementer calls SetInProgress with false. Finally, the implementer must call the RespondTakePhoto method.

This adds new camera server plugin protocol with a few basic commands
for device discovery and still image capture.

It works like this:

1. New camera service instance is created and attached to system.
2. The SetInformation method must be called right away, otherwise
   systems will think that there is no camera.
3. To handle taking pictures, the SubscribeTakePhoto method is called.
   When this method is called back, the implementer must call the
   SetInProgress with `true` method right away. Then the picture is
   taken and the implementer calls SetInProgress with `false`. Finally,
   the implementer must call the PublishPhoto method.
@dlech
Copy link
Contributor Author

dlech commented Feb 14, 2022

This is a revision of #262 based on the feedback there. Cc: @dayjaby.

@dlech
Copy link
Contributor Author

dlech commented Feb 14, 2022

The bigger design pattern that can be inferred here is that for any state, there is a SetXyz method where the user code pushes the current state to the plugin. The plugin stores this state and will use it to automatically respond to request messages without calling back to user code. So future methods would be SetCameraSettings, SetVideoStreamInformation, SetVideoStreamStatus, SetStorageInformation, etc. Some of these can be made more granular to only change certain aspects of a setting instead of corresponding directly to a mavlink message. For example, the SetInProgress method only updates the image_status field of the CAMERA_CAPTURE_STATUS message.

For messages received from an autopilot/GCS that require the camera to actually do something and not just return the current state, there are SubscribeXyz methods. These are things like taking a picture, changing the mode, formatting the storage, etc. For each SubscirbeXyz method, there will be a corresponding RespondXyz method similar to how the tracking_server plugin works. The case of taking pictures is a bit different since there isn't a single response, so it has a PublishPhoto method instead of a RespondTakePhoto method. However, now that I am writing about it and thinking about the implementation, this may not actually be a special case after all, in which case I would rename the method to match the proposed pattern.

@dlech dlech mentioned this pull request Feb 14, 2022
@dlech
Copy link
Contributor Author

dlech commented Feb 15, 2022

However, now that I am writing about it and thinking about the implementation, this may not actually be a special case after all, in which case I would rename the method to match the proposed pattern.

I've updated the proposed protocol to reflect this. The idea is that from the point of view of the plugin user, the subscribe function will always look like a single image capture message. The plugin internals will automatically call back the subscribe method multiple times for a single mavlink message with a time interval.

@dlech dlech changed the title WIP: add new camera server add new camera server Feb 15, 2022
@dayjaby
Copy link
Contributor

dayjaby commented Feb 17, 2022

Some additions on top of your .proto that I had in mind can be found at https://github.com/dayjaby/MAVSDK-Proto/commits/camera-server:

  1. Add image/video/survey camera modes:
    // Subscribe to camera mode commands
    rpc SubscribeSetCameraModeImage(SubscribeSetCameraModeImageRequest) returns(stream SetCameraModeImageResponse) {
        option (mavsdk.options.async_type) = ASYNC;
    }
    rpc SubscribeSetCameraModeVideo(SubscribeSetCameraModeVideoRequest) returns(stream SetCameraModeVideoResponse) {
        option (mavsdk.options.async_type) = ASYNC;
    }
    rpc SubscribeSetCameraModeSurvey(SubscribeSetCameraModeSurveyRequest) returns(stream SetCameraModeSurveyResponse) {
        option (mavsdk.options.async_type) = ASYNC;
    }

Each mode has its own subscribe function as the camera capabilities get automatically constructed depending on which camera mode subscriptions exist.

  1. Zoom and focus support
    // Subscribe to camera zoom commands
    rpc SubscribeSetCameraZoom(SubscribeSetCameraZoomRequest) returns(stream SetCameraZoomResponse) {
        option (mavsdk.options.async_type) = ASYNC;
    }

    // Subscribe to camera focus commands
    rpc SubscribeSetCameraFocus(SubscribeSetCameraFocusRequest) returns(stream SetCameraFocusResponse) {
        option (mavsdk.options.async_type) = ASYNC;
    }

The ZoomRequest contains the minimal and maximal focal length, so it's able to handle both SET_CAMERA_ZOOM with the zoom values being in focal length or as a percentage.

  1. Handling camera parameters (float/int):
    rpc SubscribeParamInt(ProvideParamIntRequest) returns(stream ProvideParamIntResponse) { option (mavsdk.options.async_type) = ASYNC; }
    rpc SubscribeParamFloat(ProvideParamFloatRequest) returns(stream ProvideParamFloatResponse) { option (mavsdk.options.async_type) = ASYNC; }

Handles stuff like CAM_ISO, CAM_EV, or any other parameter specified in the camera definition xml.

@dlech
Copy link
Contributor Author

dlech commented Feb 17, 2022

The subscription to feature flag idea is interesting. It took me a while to understand what you meant but now think I like it. 👍 It means that we would need an additional rule that all subscriptions need to be made before calling SetInformation. I like that better than having to specify the flags and subscribe separately.

Also, as I described above, each Subscribe method needs a corresponding Respond method so that the user code can send an appropriate response when it has handled the action. It isn't safe for the plugin to assume that all of these operations were successful and respond automatically. Also it could take some time, e.g. to actually change the mode, and the response should be delayed until after the mode has actually physically been changed in the camera.

Additionally, we will need methods like SetCameraZoom to tell the plugin what the current zoom level is so that it can automatically respond to, for example, MAV_CMD_REQUEST_MESSAGE. It could be possible that the actual zoom doesn't match the requested zoom. And we also need a way to know the initial zoom state.

@dlech
Copy link
Contributor Author

dlech commented Feb 17, 2022

I think there should only be one SubscribeSetMode method since this will correspond to the CAMERA_CAP_FLAGS_HAS_MODES flag. The CAMERA_CAP_FLAGS_CAPTURE_IMAGE flag is determined by SubcribeTakePhoto, etc.

// Sets the camera information. This must be called as soon as the camera server is created.
rpc SetInformation(SetInformationRequest) returns(SetInformationResponse) { option (mavsdk.options.async_type) = SYNC; }
// Sets image capture in progress status flags. This should be set to true when the camera is busy taking a photo and false when it is done.
rpc SetInProgress(SetInProgressRequest) returns(SetInProgressResponse) { option (mavsdk.options.async_type) = SYNC; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should rename it to "SetImageCaptureInProgress" as that's less ambigious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably pick either TakePhoto or ImageCapture and use that everywhere to keeps things consistent. I initially chose the name TakePhoto to match camera.proto, so this would be SetTakePhotoInProgress to match SubscribeTakePhoto.

@julianoes
Copy link
Collaborator

@dlech how should we proceed here? There seem to be some good discussion but I'm not sure if they have been applied or if you plan on changing this PR? I'm sorry I was busy for a while and only just returning to all these PRs.

@dlech
Copy link
Contributor Author

dlech commented Apr 7, 2022

how should we proceed here?

I think I would like to solve mavlink/MAVSDK#1700 first. It would make creating and consuming an API much easier.

@julianoes
Copy link
Collaborator

Oops, I guess I merged your commits as part of #283, closing this. Let's follow up in new PRs.

@julianoes julianoes closed this May 6, 2022
@dlech dlech deleted the camera-server branch May 6, 2022 03:20
@dlech
Copy link
Contributor Author

dlech commented May 6, 2022

Ha, OK.

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.

3 participants