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

examples: add camera_manager #1655

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

dayjaby
Copy link
Contributor

@dayjaby dayjaby commented Dec 23, 2021

Tested with

cmake -Bbuild -H. && cmake --build build -j4 && ./build/camera_manager .

and QGroundControl version v4.2.0. On top of the default communication link to the drone, you just need to add a UDP connection to 127.0.0.1:24547 , which will make QGC show the camera control interface:
Screenshot from 2021-12-23 14-02-43

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.

Nice! Quick question: how does the client know the ip:port of the mavlinkftp server? The camera_information message says mavlinkftp:///infos/camera_info.xml. Is that specified in MAVLink somehow? Like I could imagine mavlinkftp://192.168.1.12:24547/infos/camera_info.xml...

@dayjaby
Copy link
Contributor Author

dayjaby commented Dec 23, 2021

@JonasVautherin It's just about sys ids and comp ids, not about ip addresses/ports. QGroundControl receives a HEARTBEAT from this mavlink camera manager component (with MAV_COMP_ID_CAMERA = 100). Upon receiving a heartbeat from any camera component, QGC will send a request to this component to retrieve the CAMERA_INFORMATION, which contains the cam_definition_uri. From exactly this camera component (same mavlink sysid/compid), QGC will start to retrieve the camera definition file via MAVLink FTP.

There is some inconsistency regarding "mavlinkftp" vs "mftp". In some commits I've seen mavlinkftp, but the mavlink documentation specifies "mftp" (https://mavlink.io/en/services/camera.html) and you can find that as well in QGroundControl: mavlink/qgroundcontrol@4b50ffd

Maybe it's working for me just because it still had some old data being cached, so would be good if someone else tests it (:

So basically this PR allows MAVSDK to implement the camera side of the mavlink camera protocol. There is lots of room for improvements: e.g. storing the parameters in a local file, publish all camera parameters on boot as PARAM_EXT, implement some streaming pipelines. The list is long :D

@dayjaby
Copy link
Contributor Author

dayjaby commented Dec 25, 2021

My first approach was invalid, as I first tested it against a camera_definition_uri with http, which made QGroundControl preserve that in its cache. So the mavlinkftp did not work at all, as QGroundControl never used mavlink ftp for CAMERA_INFORMATION in the first place (a PR that fixes this issue in QGC is mentioned above).

The correct URL at the moment is something like mftp://[;comp=100]infos/camera_info.xml.

@dayjaby
Copy link
Contributor Author

dayjaby commented Jan 3, 2022

Probably going to turn this into an own plugin, as this would allow to react to parameter changes quite easily:

void CameraManagerPluginImpl::init()
{
    _parent->provide_server_param_int("CAM_EV", 5);

    _parent->provide_server_param_int("CAM_ISO", 0);
    _parent->subscribe_param<uint32_t>(
        "CAM_ISO", [](uint32_t iso) { std::cout << "NEW ISO: " << iso << std::endl; }, nullptr);
}

Just not sure how to handle the different typing yet. E.g. one could end up using float for CAM_ISO or uint32 or int32 or whatever.

@dayjaby dayjaby changed the title examples: add camera_manager [WIP] examples: add camera_manager Jan 10, 2022
@julianoes julianoes changed the title [WIP] examples: add camera_manager examples: add camera_manager Jan 14, 2022
Comment on lines 15 to 18
MAVSDK::mavsdk_ftp
MAVSDK::mavsdk_mavlink_passthrough
MAVSDK::mavsdk_param_server
MAVSDK::mavsdk
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has changed in the meantime. You only need MAVSDK::mavsdk.

mavsdk.set_configuration(configuration);
ConnectionResult connection_result = mavsdk.add_any_connection("udp://:24547");
if (connection_result != ConnectionResult::Success) {
std::cerr << "Error setting up Mavlink FTP server.\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
std::cerr << "Error setting up Mavlink FTP server.\n";
std::cerr << "Error setting up Mavlink camera manager.\n";

case MAV_CMD_REQUEST_CAMERA_INFORMATION:
mavlink_camera_information_t camera_information;
camera_information.time_boot_ms = 0;
strncpy((char*)camera_information.vendor_name, "Foo Industries", 32);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
strncpy((char*)camera_information.vendor_name, "Foo Industries", 32);
strncpy((char*)camera_information.vendor_name, "Foo Industries", sizeof(camera_information.vendor_name));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense. Thanks!

mavlink_camera_information_t camera_information;
camera_information.time_boot_ms = 0;
strncpy((char*)camera_information.vendor_name, "Foo Industries", 32);
strncpy((char*)camera_information.model_name, "T100", 32);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
strncpy((char*)camera_information.model_name, "T100", 32);
strncpy((char*)camera_information.model_name, "T100", sizeof(camera_information.model_name));

CAMERA_CAP_FLAGS_CAN_CAPTURE_VIDEO_IN_IMAGE_MODE |
CAMERA_CAP_FLAGS_HAS_BASIC_ZOOM | CAMERA_CAP_FLAGS_HAS_BASIC_FOCUS |
CAMERA_CAP_FLAGS_HAS_VIDEO_STREAM;
camera_information.cam_definition_version = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this starts at 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On mavlink.io there is no specification on that one. As its a uint16_t field, 0 as a starter makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you're right, ok.

@@ -36,6 +36,7 @@ FtpImpl::~FtpImpl()

void FtpImpl::init()
{
LogDebug() << "register mavlink message handler";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
LogDebug() << "register mavlink message handler";

<< (int)payload->size << " offset: " << (int)payload->offset << " seq: " <<
payload->seq_number;
*/
LogDebug() << "ftp - opc: " << (int)payload->opcode << " size: " << (int)payload->size
Copy link
Collaborator

Choose a reason for hiding this comment

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

debugging...

payload->size = 1;
uint8_t* pData = &payload->data[0];
*pData =
error_code; // Straight reference to data[0] is causing bogus gcc array subscript error
Copy link
Collaborator

Choose a reason for hiding this comment

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

What?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole mavsdk ftp plugin seems to be based on the PX4 implementation. So I based the ftp burst operation also on the PX4 implementation (this bogus comment comes from PX4: https://github.com/PX4/PX4-Autopilot/blob/f8c2ee73db3654a34976291e5ac94c437e27d4a7/src/modules/mavlink/mavlink_ftp.cpp#L1126)

The implementation here is not complete yet; I transmit the entire file within one burst. Appearently, in PX4, they burst out up to 35000 bytes at once, but it's not specified yet: https://mavlink.io/en/services/ftp.html#burst-read-file

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aaaah, good point. We should do that...

Comment on lines 1403 to 1404
if (more_data)
send();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (more_data)
send();
if (more_data) {
send();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -216,6 +217,7 @@ class FtpImpl : public PluginImplBase {
void _reset_timer();
void _stop_timer();
void _list_directory(uint32_t offset);
unsigned get_size();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
unsigned get_size();
unsigned get_size() const;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@dayjaby
Copy link
Contributor Author

dayjaby commented Jan 14, 2022

@julianoes applied your suggestions.

However, I still don't feel comfortable having this just as an example. I think it's doing a lot of similar stuff as your component_information plugin; and eventually we will need something similar for VIDEO_STREAMING_INFORMATION as well.

I think of something along the lines of:

  1. We implement a camera_manager plugin
  • Users can use that plugin to provide callback functions for the different camera parameters, to forward parameter changes to their non-mavlink cameras
  1. We implement a video_stream_manager plugin
  • Users provide callback functions to establish a video stream, for example via GStreamer

Any common functionality among the camera_manager, video_stream_manager, component_information and ftp plugin, we should put into src/mavsdk/core. D'accord?

If you have a better word than "manager", let me know ;)

@dlech
Copy link
Collaborator

dlech commented Jan 14, 2022

If you have a better word than "manager", let me know ;)

All of the existing plugins that implement such features are called xyz_server.

@Jaeyoung-Lim
Copy link
Contributor

This is awesome!

@julianoes
Copy link
Collaborator

I assume we will merge this once the #1733 is in. Or should we get it in before that?

@dlech
Copy link
Collaborator

dlech commented Apr 13, 2022

I don't think we want to merge it as-is, but rather reuse the ideas on top of the new changes. Don't we want a MAVSDK-Proto API for this too?

@dayjaby
Copy link
Contributor Author

dayjaby commented Apr 14, 2022

@dlech we only need to add subscribe/provide parameter callbacks to the .proto.

For the ftp camera information file transmission:
I think @julianoes moved the FTP stuff out of a plugin into something that we can include in the camera server implementation already. Just the FTP burst needs some update.

@julianoes
Copy link
Collaborator

There has been some progress in #2088 in the same direction. I think once that other PR is merged, we can take a diff and see what is missing from this PR and pull it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants