Camera: switch to new REQUEST_MESSAGE commands #11571
Conversation
julianoes
force-pushed
the
pr-camera-request-message
branch
from
4f46cc4
to
a532c5f
Compare
There was a problem hiding this comment.
I may be mistaken, but I believe Ardupilot would return MAV_RESULT_UNSUPPORTED which would just exit out silently here right?
There was a problem hiding this comment.
I can't have a close look now but I Randy tested it and it worked apparently. If your testing is different, then we have to address it.
There was a problem hiding this comment.
Oh nvm if it doesn't support the REQUEST_MESSAGE msgid then it will return MAV_RESULT_FAILED. At some point we should handle MAV_RESULT_UNSUPPORTED somewhere but doesn't matter for this PR.
The used commands like REQUEST_CAMERA_SETTINGS and REQUEST_CAMERA_INFORMATION are deprecated. Therefore, we should gradually try to move to the new REQUEST_MESSAGE commands. This commit does so by sending REQUEST_MESSAGE by default but falling back to the previous commands on the second try and every other retry after that.
This fixes getting the video stream.
By querying autopilot for the CAMERA_INFORMATION message, we allow an autopilot to be a proxy for a MAVLink or non-MAVLink camera. The idea is similar to a gimbal manager implemented by an autopilot.
Since we no longer request cameras to have specific camera component IDs but allow the autopilot to "be" a camera as well, we need to adjust the unit tests to account for that.
julianoes
force-pushed
the
pr-camera-request-message
branch
from
b99911e
to
8e1f4f0
Compare
|
Vehicle has built in request message with retries support: You should use that instead of building your own. |
| // are taking the response to that MAV_CMD_REQUEST_MESSAGE, we discard it and take the next | ||
| // Also, the camera manager requests MAVLINK_MSG_ID_CAMERA_INFORMATION on vehicle connection, | ||
| // so we need to ignore that as well. | ||
| while (arguments.at(2).toInt() == MAV_CMD_REQUEST_MESSAGE || arguments.at(2).toInt() == MAV_CMD_REQUEST_CAMERA_INFORMATION) { |
There was a problem hiding this comment.
This is kinda nasty and fragile. There is already the concept of creating a connect to a mock link vehicle which does not do the initial vehicle connect sequence. This is specifically for unit test problems like this. Would it be possible to hook in the gimbal request stuff into the vehicle initial connect sequence state machine so that a it only happens if that was run?
There was a problem hiding this comment.
I can try that but I will have to do a bit more reading to understand these tests. It's all a mystery to me.
There was a problem hiding this comment.
Don't worry about it. I'll clean this up after it's merged in. Will be easier for me to deal with it than have you figure out something new to you.
There was a problem hiding this comment.
Same thing the the request message stuff. I can switch that over as well.
|
Let me take a look at it. I’ll see what I can do
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Julian Oes ***@***.***>
Sent: Sunday, August 18, 2024 2:57:35 PM
To: mavlink/qgroundcontrol ***@***.***>
Cc: Donald Gagne ***@***.***>; Comment ***@***.***>
Subject: Re: [mavlink/qgroundcontrol] Camera: switch to new REQUEST_MESSAGE commands (PR #11571)
@julianoes commented on this pull request.
________________________________
In test/Vehicle/SendMavCommandWithSignallingTest.cc<#11571 (comment)>:
@@ -38,6 +38,18 @@ void SendMavCommandWithSignallingTest::_testCaseWorker(TestCase_t& testCase)
QCOMPARE(spyResult.wait(10000), true);
QList<QVariant> arguments = spyResult.takeFirst();
+
+ // Gimbal controler requests MAVLINK_MSG_ID_GIMBAL_MANAGER_INFORMATION on vehicle connection,
+ // and that messes with this test, as it receives response to that command instead. So if we
+ // are taking the response to that MAV_CMD_REQUEST_MESSAGE, we discard it and take the next
+ // Also, the camera manager requests MAVLINK_MSG_ID_CAMERA_INFORMATION on vehicle connection,
+ // so we need to ignore that as well.
+ while (arguments.at(2).toInt() == MAV_CMD_REQUEST_MESSAGE || arguments.at(2).toInt() == MAV_CMD_REQUEST_CAMERA_INFORMATION) {
I can try that but I will have to do a bit more reading to understand these tests. It's all a mystery to me.
—
Reply to this email directly, view it on GitHub<#11571 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABM2Y43SUDGHD2BYUQAYOBLZSEKE7AVCNFSM6AAAAABIQCWZQSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENBUGQYTONZWGI>.
You are receiving this because you commented.Message ID: ***@***.***>
|
|
@julianoes So in looking through these changes the main functional change is the checking of compids is a little different on the camera manager. The rest is to move to request message. Instead of merging this in and then me futzing with it. Can we instead just leave this pull here for now. And I will create a replacement pull for it. That will be easier. |
|
Ok, so I'm confused now. I just went to create a pull for this and it looks like this change is already in master: https://github.com/mavlink/qgroundcontrol/blob/master/src/Camera/QGCCameraManager.cc#L406 |
|
Didn't look at the date on this. This is super old. The changes in the pull are already in master. I'm working on cleaning up master per my above comments. @julianoes I think you can close this? |
|
Oh, I'm sorry for the confusion. I didn't look at the date either. |
Description
The used commands like REQUEST_CAMERA_SETTINGS and REQUEST_CAMERA_INFORMATION are deprecated. Therefore, we should gradually try to move to the new REQUEST_MESSAGE commands.
This commit does so by sending REQUEST_MESSAGE by default but falling back to the previous commands on the second try and every other retry after that.
Test Steps
Need to test it manually with one of my camera-manager example.
Checklist: