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 trigger server #262

Closed
wants to merge 1 commit into from
Closed

Conversation

dlech
Copy link
Contributor

@dlech dlech commented Nov 3, 2021

This adds a new plugin for implementing subscribing to single-image MAV_CMD_IMAGE_START_CAPTURE commands. This is specifically intended to be used when Trigger interface is set to MAVLink (forward via MAV_CMD_IMAGE_START_CAPTURE) in the Camera Setup in QGroundControl.

This adds a new plugin for implementing subscribing to single-image
MAV_CMD_IMAGE_START_CAPTURE commands. This is specifically intended to
be used when *Trigger interface* is set to *MAVLink (forward via
MAV_CMD_IMAGE_START_CAPTURE)* is selected in the Camera Setup in
QGroundControl.
@dlech dlech changed the title Camera-trigger-server Camera trigger server Nov 3, 2021
@TSC21
Copy link
Member

TSC21 commented Nov 3, 2021

I believe that bringing the entire Camera microservice server side into one proto would make much more sense than just part of it.

@dlech
Copy link
Contributor Author

dlech commented Nov 3, 2021

I considered that, but it seems like there could be a use case for both, which is why I called this CameraTriggerServer rather than CameraServer. This is intended to be the software equivalent of triggering a camera via GPIO and nothing more.

@TSC21
Copy link
Member

TSC21 commented Nov 3, 2021

I considered that, but it seems like there could be a use case for both, which is why I called this CameraTriggerServer rather than CameraServer. This is intended to be the software equivalent of triggering a camera via GPIO and nothing more.

It's still part of the same microservice: https://mavlink.io/en/services/camera.html. I would see more interesting to have a camera_server plugin where I could call a get_start_capture(). The microservice doesn't really care this is going to interface with the GPIO. What it matters is that we expose an API that allows us to control cameras or receive control for cameras. Otherwise we would endup also having to break protos for video vs photo for example. And that's not the intent IMHO.

@JonasVautherin @julianoes thoughts?

@TSC21
Copy link
Member

TSC21 commented Nov 3, 2021

@dlech I would see this as a good start for a Camera Server plugin, but just adding those first commands as a start. We can then expand the proto with more functionalities.

@julianoes
Copy link
Collaborator

Thanks @dlech! I would probably lean towards a more general camera server as well. What do you think @JonasVautherin?

@dlech
Copy link
Contributor Author

dlech commented Nov 3, 2021

To fill in some more background, my use case is to have a Raspberry Pi with two cameras attached to a flight controller via serial port (i.e. TELEM2). MAVSDK will be running on the RPi and should be able to capture images when triggered by a mission started from QGroundControl.

From what I was reading in the MAVLink docs about cameras, it is written as if the camera is connected directly to the ground station. Will the flight controller automagically act as a bridge between the RPi and QGroundControl?

@TSC21
Copy link
Member

TSC21 commented Nov 3, 2021

From what I was reading in the MAVLink docs about cameras, it is written as if the camera is connected directly to the ground station. Will the flight controller automagically act as a bridge between the RPi and QGroundControl?

What do you mean with a camera being connected directly to the ground station? The camera could be connected to the RPi on the drone and then you would forward MAVLink messages either through the flight controller and its radio link, or a radio/wifi link on the RPi. With the overall Camera protocol supported then yes you can interface with the cameras through the ground station.

@dlech
Copy link
Contributor Author

dlech commented Nov 3, 2021

What do you mean with a camera being connected directly to the ground station?

The docs show GCS <--> Camera

image

@TSC21
Copy link
Member

TSC21 commented Nov 3, 2021

What do you mean with a camera being connected directly to the ground station?

The docs show GCS <--> Camera

image

Right but there will always be something in the middle forwarding those between the camera and the ground station, as I said. The diagram is correct, it just doesn't put the forwarding/routing components in the middle, because they don't interfere with the protocol in any way.

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.

Thanks a lot!

I would tend to agree with @TSC21 and @julianoes here: it would be great as a start for the camera_server plugin. To me it makes sense to have everything in one proto, being the counterpart of the camera (client) plugin 👍

@dayjaby
Copy link
Contributor

dayjaby commented Jan 14, 2022

@dlech Maybe we can join our efforts? In my "camera_manager" (or lets call it camera_server...) example PR (mavlink/MAVSDK#1655) I've done some code that's handling the camera information sharing with a GCS. This contains some information like

CAMERA_CAP_FLAGS_CAPTURE_VIDEO |
CAMERA_CAP_FLAGS_CAPTURE_IMAGE |
CAMERA_CAP_FLAGS_HAS_MODES |
CAMERA_CAP_FLAGS_CAN_CAPTURE_IMAGE_IN_VIDEO_MODE |

which is hardcoded at the moment. At least CAMERA_CAP_FLAGS_CAPTURE_IMAGE could be automatically provided, in combination with your PR, if there is a SubscribeCapture provided by the user.

@dlech
Copy link
Contributor Author

dlech commented Jan 14, 2022

Hi @dayjaby. Sure! I got sick over the holidays, so I lost momentum on my work here and haven't got back to it yet. I just pushed what I have currently to https://github.com/dlech/mavsdk/tree/camera-server if you want to compare notes.

If I'm remembering right, the point where I was at was that I realized that a camera server running on an on-board computer would have to have multiple instances of the camera server plugin running, one for each `mavsdk::System``. For example, one for the connection to the autopilot and one for the connection to the ground control station.

So my question for the rest of the MAVSDK team that I haven't asked yet is this:

Is it OK to have a shared state between two camera server plugin instances (I guess this state would be connected to the mavsdk::Mavsdk instance somehow)? This would allow MAVSDK to do much of the work of keeping track of the current state of the camera. On the one hand, this would be nice for users in that they wouldn't have to implement this themselves. On the other hand, this seems like it could get pretty tricky to keep things in a consistent state, let alone trying to cover a variety of use cases.

Or do we make the the protocol API a very thin wrapper that basically passes Mavlink requests to the user code (more like the tracking server)? This makes more work for users but also allows just about any use case one can think of.

@dayjaby
Copy link
Contributor

dayjaby commented Jan 14, 2022

@dlech is this architecture not feasible:
image
?

@dlech
Copy link
Contributor Author

dlech commented Jan 14, 2022

No, even with that setup, mavsdk.systems() will still return two separate systems and then we have to call mavsdk::CameraServer{system}; for each system. (I should not have used the word "connection" 😄)

@JonasVautherin
Copy link
Collaborator

Is it OK to have a shared state between two camera server plugin instances?

I think it is 👍. The camera_server can have some kind of std::vector<System>, which corresponds to the discovered systems that can communicate with the camera_server. The shared state is the state of the camera, makes perfect sense to me 👍.

@dlech
Copy link
Contributor Author

dlech commented Jan 15, 2022

The camera_server can have some kind of std::vector<System>

This seems like it would break the current plugin architecture where each plugin takes a single system in the constructor. That is, this doesn't seem like it would be compatible with mavsdk_server.

@JonasVautherin
Copy link
Collaborator

Sorry, I meant the application code can have some kind of std::vector<CameraServer>, each camera_server having its own System. Does that make more sense?

@dlech
Copy link
Contributor Author

dlech commented Jan 15, 2022

Yes, this was my second option where the application code has to do most of the work:

Or do we make the the protocol API a very thin wrapper that basically passes Mavlink requests to the user code (more like the tracking server)? This makes more work for users but also allows just about any use case one can think of.

@dayjaby
Copy link
Contributor

dayjaby commented Jan 29, 2022

@dlech Sorry for the late reply. I caught some virus o_o

Got some time to review your code now.

using TakePhotoCallback = std::function<void(Result, int32_t)>;

What's the exact purpose of sending a "result" to the user-provided callback function? Is the plugin itself responsible for taking the picture (and thus, knowing whether the capture succeeded or not)? I would say no. The user-provided callback function should take care of making the picture and thus report a result.

I guess to achieve something like

using TakePhotoCallback = std::function<Result(int32_t)>;

there might be some changes to the mavsdk server generators necessary :)

Even though it's debatable, for the Information message, I would prefer the namings to be as close to the mavlink camera_information namings as possible, e.g.

// Type to represent a camera information.
message Information {
    string vendor_name = 1; // Name of the camera vendor
    string model_name = 2; // Name of the camera model
    uint32 firmware_version = 3; // Version of the camera firmware
    float focal_length_mm = 4; // Focal length
    float sensor_size_h_mm = 5; // Horizontal sensor size
    float sensor_size_v_mm = 6; // Vertical sensor size
    uint32 resolution_h_px = 7; // Horizontal image resolution in pixels
    uint32 resolution_v_px = 8; // Vertical image resolution in pixels
    uint32 lens_id = 9; // Lens ID
    uint32 cam_definition_version = 10; // Camera definition version (iteration)
    string cam_definition_uri = 11; // Camera definition URI (http or mavlink ftp)
}

I will create PRs to your mavsdk forks, if that's fine for you? First of all, getting the camera information + camera parameter handling done.

@dlech
Copy link
Contributor Author

dlech commented Jan 29, 2022

What's the exact purpose of sending a "result" to the user-provided callback function?

I don't remember. But I think my existing code needs to be reworked to take into consideration #262 (comment). In this case, most of the APIs will probably have to have a result so that the mavlink reply can report the correct status if the operation succeed or failed.

Is the plugin itself responsible for taking the picture (and thus, knowing whether the capture succeeded or not)?

No, the MAVSDK plugin is responsible for translating mavlink messages to callbacks to user code. The user code is responsible for taking the picture and then notifying the MAVSDK plugin of the result.

I will create PRs to your mavsdk forks, if that's fine for you?

I should probably rebase the branches first.

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

dlech commented Feb 14, 2022

Closing in favor of #274.

@dlech dlech closed this Feb 14, 2022
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.

5 participants