-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
...
@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 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 |
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 |
e757b73
to
84a9b13
Compare
Probably going to turn this into an own plugin, as this would allow to react to parameter changes quite easily:
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. |
MAVSDK::mavsdk_ftp | ||
MAVSDK::mavsdk_mavlink_passthrough | ||
MAVSDK::mavsdk_param_server | ||
MAVSDK::mavsdk |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strncpy((char*)camera_information.vendor_name, "Foo Industries", 32); | |
strncpy((char*)camera_information.vendor_name, "Foo Industries", sizeof(camera_information.vendor_name)); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/mavsdk/plugins/ftp/ftp_impl.cpp
Outdated
@@ -36,6 +36,7 @@ FtpImpl::~FtpImpl() | |||
|
|||
void FtpImpl::init() | |||
{ | |||
LogDebug() << "register mavlink message handler"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
src/mavsdk/plugins/ftp/ftp_impl.cpp
Outdated
if (more_data) | ||
send(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (more_data) | |
send(); | |
if (more_data) { | |
send(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/mavsdk/plugins/ftp/ftp_impl.h
Outdated
@@ -216,6 +217,7 @@ class FtpImpl : public PluginImplBase { | |||
void _reset_timer(); | |||
void _stop_timer(); | |||
void _list_directory(uint32_t offset); | |||
unsigned get_size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsigned get_size(); | |
unsigned get_size() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@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:
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 ;) |
All of the existing plugins that implement such features are called |
This is awesome! |
I assume we will merge this once the #1733 is in. Or should we get it in before that? |
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? |
@dlech we only need to add subscribe/provide parameter callbacks to the .proto. For the ftp camera information file transmission: |
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. |
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: