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.

Sign up for GitHub

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

Jump to bottom

camera: adding subscription APIs #426

Merged
merged 9 commits into from
Jun 20, 2018
Merged

camera: adding subscription APIs #426

merged 9 commits into from
Jun 20, 2018

Conversation

julianoes
Copy link
Collaborator

@julianoes julianoes commented Jun 13, 2018 •
edited
Loading

This PR adds missing camera subscription APIs:

  • camera mode
  • video stream info
  • capture info
  • camera status
  • current setting
  • possible settings

Connects to #334.

@julianoes julianoes requested a review from JonasVautherin June 13, 2018 11:08
@julianoes julianoes force-pushed the camera-subscriptions branch from e773e75 to 6d354ef Compare June 13, 2018 11:21
JonasVautherin
JonasVautherin reviewed Jun 13, 2018
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Looks like good progress!

Is it right to say that the camera mode is advertised by the camera, but the video stream info is not, so the plugin will have to poll it while a callback is registered?

plugins/camera/camera_impl.cpp Outdated
void CameraImpl::subscribe_video_stream_info(
const Camera::subscribe_video_stream_info_callback_t callback)
{
// FIXME: This will only work if you also send a request to get the video stream info,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean it should call get_video_stream_info repeatedly when a callback is registered, right? How difficult is it to implement?

Because the frontend should not have to poll at all, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, we just have to do a call_every, not that hard 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just haven't ever used the streaming API and there are no tests, so I didn't bother.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done but untested.

@julianoes
Copy link
Collaborator Author

Is it right to say that the camera mode is advertised by the camera

I think right now it is not advertised as such. It's just when you set it you also receive the feedback and use that. We potentially could add polling the mode as well in order to see changes made by e.g. QGroundControl.

@julianoes julianoes force-pushed the camera-subscriptions branch from 4f30e63 to b929346 Compare June 13, 2018 15:46
@JonasVautherin
Copy link
Collaborator

We potentially could add polling the mode as well in order to see changes made by e.g. QGroundControl.

But if QGC sets the mode, won't the camera advertise it? Anyway it's fine for now.

@julianoes
Copy link
Collaborator Author

But if QGC sets the mode, won't the camera advertise it? Anyway it's fine for now.

It could/should advertise it using: http://mavlink.org/messages/common#CAMERA_SETTINGS. We probably need to add that to E90.

julianoes added 6 commits June 14, 2018 07:50
@julianoes
camera: added subscribe_mode method
9dad1ac 
This adds the possibility to subscribe to the camera mode.
@julianoes
camera: added subscribe_video_stream_info
717dd3c 
This adds an API to subscribe to the video stream info, however, this is
currently not really implemented and tested.
@julianoes
camera: renamed capture_info_async
2984304 
The name subscribe_capture_info matches the other subscription methods.
@julianoes
camera: added subscribe_status
cb87c74 
This adds the method to subscribe to the camera status and also adds
polling for the status at 1 Hz.
@julianoes
camera: minor comment fixes
6302d2a
@julianoes
camera: add async video stream info, poll info
5f435ba 
This adds an async getter for the video stream info and uses it in the
sync version using std::future/promise.

Also, this adds polling to the video stream info.

Note that there are currently no integration tests for this, so this is
untested.
@julianoes julianoes force-pushed the camera-subscriptions branch from b929346 to 6453c1f Compare June 14, 2018 07:26
@julianoes
camera: added subscribe_current_options
e57f488 
This adds a subscription method for the current options. It should
trigger whenever a setting is updated.
@julianoes julianoes force-pushed the camera-subscriptions branch from 6453c1f to e57f488 Compare June 14, 2018 07:31
@julianoes
camera: added get_possible_options
10740db 
This adds an API to subscribe to all possible options that could
currently be selected.
@julianoes
Copy link
Collaborator Author

@JonasVautherin this should be complete but I'm not happy with the naming yet.

Currently it's all options but we need to distinguish between a "selected option" (a setting) and a "selectable option" (an option).

@julianoes
Copy link
Collaborator Author

julianoes commented Jun 14, 2018 •
edited
Loading

Currently we have:

typedef std::function<void(const std::vector<std::pair<std::string, std::string>>)>
    subscribe_current_options_callback_t;

typedef std::function<void(const std::vector<std::pair<std::string, std::vector<std::string>>>)>
    subscribe_possible_options_callback_t;

We could refactor that to:

struct Option {
    std::string option_id;
};

struct Setting {
    std::string setting_id;
    Option option;
};

struct SettingList {
    std::string setting_id;
    std::vector<Option> options;
};

typedef std::function<void(const std::vector<Setting>)>
    subscribe_current_settings_callback_t;

typedef std::function<void(const std::vector<SettingList>)>
    subscribe_possible_settings_callback_t;

@JonasVautherin what do you think?

@JonasVautherin
Copy link
Collaborator

I like your proposition. I would suggest SettingOptions instead of SettingList, though :-).

@julianoes
Copy link
Collaborator Author

Ok, I'll refactor it.

@julianoes
camera: refactored API to use Setting and Option
e19e548 
This should make the API easier by using structs as types. Some of the
getters and setters could still be improved by using the struct types
instead of the std::strings.
@julianoes julianoes force-pushed the camera-subscriptions branch from efbb427 to e19e548 Compare June 14, 2018 19:59
@JonasVautherin JonasVautherin merged commit 6a36ad3 into develop Jun 20, 2018
@JonasVautherin JonasVautherin deleted the camera-subscriptions branch June 20, 2018 09:10
rt-2pm2 pushed a commit to rt-2pm2/DronecodeSDK that referenced this pull request Nov 27, 2018
@JonasVautherin
Merge pull request mavlink#426 from dronecore/camera-subscriptions
1e5aad8 
camera: adding subscription APIs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonasVautherin JonasVautherin JonasVautherin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@julianoes @JonasVautherin