ardupilot icon indicating copy to clipboard operation
ardupilot copied to clipboard

AP_Camera: Load CAMERA_INFORMATION and VIDEO_STREAM_INFORMATION from Lua

Open nexton-winjeel opened this issue 1 year ago • 12 comments

This PR allows us to populate the CAMERA_INFORMATION and VIDEO_STREAM_INFORMATION MAVLink messages from Lua scripts.

Sending these messages allows a GCS to auto-configure the video stream to receive video from the payload.

Replaces https://github.com/ArduPilot/ardupilot/pull/27594.

nexton-winjeel avatar Aug 08 '24 08:08 nexton-winjeel

Binary Name      Text [B]         Data [B]     BSS (B)        Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  ---------------  -----------  -------------  ----------------------------  -------------------------
antennatracker   3516 (+0.2642%)  0 (0.0000%)  4 (+0.0015%)   3516 (+0.2638%)                                  629700
arduplane        3436 (+0.1915%)  0 (0.0000%)  4 (+0.0015%)   3436 (+0.1911%)                                  164640
blimp            3520 (+0.2602%)  0 (0.0000%)  0 (0.0000%)    3520 (+0.2598%)                                  607480
arducopter-heli  3428 (+0.1908%)  0 (0.0000%)  -4 (-0.0015%)  3428 (+0.1904%)                                  162216
arducopter       3428 (+0.1907%)  0 (0.0000%)  4 (+0.0015%)   3428 (+0.1903%)                                  161344
ardusub          3432 (+0.2158%)  0 (0.0000%)  0 (0.0000%)    3432 (+0.2155%)                                  370092
ardurover        3428 (+0.2080%)  0 (0.0000%)  -4 (-0.0015%)  3428 (+0.2077%)                                  312296

amilcarlucas avatar Aug 08 '24 09:08 amilcarlucas

If scripting has to set it in anycase why not use scripting for the whole thing? You can receive and send MAVLink messages from Lua.

IamPete1 avatar Aug 08 '24 16:08 IamPete1

@IamPete1:

If scripting has to set it in anycase why not use scripting for the whole thing? You can receive and send MAVLink messages from Lua.

Two reasons:

  1. At least for the CAMERA_INFORMATION message, the AP_Camera driver is already sending this message. I didn't want a situation where the AP_Camera driver is sending the message and the scripting is sending a conflicting message.
  2. These messages are requested rather than streamed. Is there a nice way to handle this in a script, rather than just periodically sending the message?

nexton-winjeel avatar Aug 09 '24 01:08 nexton-winjeel

@Hwurzburg:

definitely needs to be disabled in minimize include and build_options/extract_featues files added

Done.

nexton-winjeel avatar Aug 09 '24 01:08 nexton-winjeel

@Hwurzburg:

needs squash and library split

What do you mean?

  • the commits are already split per library.
  • the commits are logical changes that add one thing at a time, rather than a number of unrelated changes in a single commit.

nexton-winjeel avatar Aug 09 '24 06:08 nexton-winjeel

  1. These messages are requested rather than streamed. Is there a nice way to handle this in a script, rather than just periodically sending the message?

There is a way to do this, I actually did exactly that and was about to open a PR for it, but I also was thinking about redoing it to something like your approach here. It's easy enough to intercept a MAV_CMD_REQUEST_VIDEO_STREAM_INFORMATION, but that's being deprecated in favor of MAV_CMD_REQUEST_MESSAGE. The issue with the latter is that only one message request is cached for the lua script to grab, so I had to run the script at high speed to make sure I didn't miss a message request if the GCS sent a bunch of them all at once.

So, I think there are some advantages to your approach.

My lua script for reference video_stream.zip

robertlong13 avatar Aug 09 '24 06:08 robertlong13

only one commit per library is allowed in final pr

squash to one commit then run Tools/gittools/git-subsystems-split script and push back

Hwurzburg avatar Aug 09 '24 06:08 Hwurzburg

only one commit per library is allowed in final pr

This is definitely not true and IMO reduces clarity.

tpwrules avatar Aug 10 '24 00:08 tpwrules

@Hwurzburg, @peterbarker: Squashed.

nexton-winjeel avatar Aug 10 '24 03:08 nexton-winjeel

@peterbarker: Is FenceFloorEnabledLanding a flapping test? No code change, but it's now failing?

nexton-winjeel avatar Aug 11 '24 23:08 nexton-winjeel

Its a new test - I haven't seen it flap

andyp1per avatar Aug 12 '24 07:08 andyp1per

I can't trigger tests, so I've rebased on master again to trigger a rebuild.

nexton-winjeel avatar Aug 12 '24 09:08 nexton-winjeel

Looks like this is now overflowing the heap on the CubeOrange-EKF2 build.

nexton-winjeel avatar Sep 16 '24 03:09 nexton-winjeel

Also the AP_CAMERA_INFO_FROM_SCRIPT_ENABLED AP_MAVLINK_MSG_VIDEO_STREAM_INFORMATION_ENABLED might need to be in the scripting bindings too? You can do a depends on methods too if you want the main scripting support.

IamPete1 avatar Sep 16 '24 23:09 IamPete1

@IamPete1: I've updated the Lua bindings for the new structs and methods to depend on AP_CAMERA_INFO_FROM_SCRIPT_ENABLED.

nexton-winjeel avatar Sep 19 '24 02:09 nexton-winjeel

Rebased on master to get the changes from #28128 so CI passes.

nexton-winjeel avatar Sep 19 '24 02:09 nexton-winjeel

Binary Name      Text [B]         Data [B]     BSS (B)        Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  ---------------  -----------  -------------  ----------------------------  -------------------------
antennatracker   3512 (+0.2622%)  0 (0.0000%)  0 (0.0000%)    3512 (+0.2618%)                                  620996
arducopter       3428 (+0.1896%)  0 (0.0000%)  4 (+0.0015%)   3428 (+0.1892%)                                  150640
ardusub          3428 (+0.2142%)  0 (0.0000%)  4 (+0.0015%)   3428 (+0.2139%)                                  359756
blimp            3512 (+0.2579%)  0 (0.0000%)  0 (0.0000%)    3512 (+0.2575%)                                  598928
arduplane        3428 (+0.1899%)  0 (0.0000%)  4 (+0.0015%)   3428 (+0.1895%)                                  153320
ardurover        3432 (+0.2070%)  0 (0.0000%)  0 (0.0000%)    3432 (+0.2067%)                                  302104
arducopter-heli  3428 (+0.1897%)  0 (0.0000%)  -4 (-0.0015%)  3428 (+0.1893%)                                  151600

It's still big, but if the ifdefs work now, it should not be a problem.

amilcarlucas avatar Sep 19 '24 07:09 amilcarlucas

Running locally, I get the following:

This branch, AP_CAMERA_INFO_FROM_SCRIPT_ENABLED = 0:

Build directory: /ardupilot/build/sitl
Target          Text (B)  Data (B)  BSS (B)  Total Flash Used (B)  Free Flash (B)  External Flash Used (B)
----------------------------------------------------------------------------------------------------------
bin/arducopter   4172858    191517   222752               4364375  Not Applicable  Not Applicable

master branch:

Build directory: /ardupilot/build/sitl
Target          Text (B)  Data (B)  BSS (B)  Total Flash Used (B)  Free Flash (B)  External Flash Used (B)
----------------------------------------------------------------------------------------------------------
bin/arducopter   4172978    191453   222752               4364431  Not Applicable  Not Applicable  

Deltas between disabled and master:

Target          Text (B)  Data (B)  BSS (B)  Total Flash Used (B)  Free Flash (B)  External Flash Used (B)
----------------------------------------------------------------------------------------------------------
bin/arducopter      -120        64        0                   -56  Not Applicable  Not Applicable  

nexton-winjeel avatar Sep 20 '24 00:09 nexton-winjeel

only one commit per library is allowed in final pr

This is definitely not true and IMO reduces clarity.

then we definitely need a change to developer WIKI instructions... @peterbarker ?: "The commits of the change should be squashed (see Interactive Rebase: cleaning up commits) into one commit and then the “Tools/gittools/git-subsystems-split” script run to create a single commit for each library module affected.

Hwurzburg avatar Sep 20 '24 12:09 Hwurzburg

@Hwurzburg, I think the requirement is that individual commits should not affect more than one subsystem but multiple commits affecting a single subsystem is OK. The git-subsystems-split script is probably meant to divide up a single commit but not to consolidate commits.

rmackay9 avatar Sep 20 '24 23:09 rmackay9