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

AP_Camera: Load CAMERA_INFORMATION and VIDEO_STREAM_INFORMATION from Lua #27794

Merged
merged 9 commits into from
Sep 30, 2024

Conversation

nexton-winjeel
Copy link
Contributor

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 #27594.

@amilcarlucas
Copy link
Contributor

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

Copy link
Collaborator

@Hwurzburg Hwurzburg left a comment

Choose a reason for hiding this comment

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

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

@IamPete1
Copy link
Member

IamPete1 commented Aug 8, 2024

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.

@nexton-winjeel nexton-winjeel force-pushed the upstream/lua-camera-information branch 2 times, most recently from 5164dbd to 49019d3 Compare August 9, 2024 01:17
@nexton-winjeel
Copy link
Contributor Author

@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
Copy link
Contributor Author

@Hwurzburg:

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

Done.

@nexton-winjeel nexton-winjeel force-pushed the upstream/lua-camera-information branch 3 times, most recently from e24c7df to 4513acf Compare August 9, 2024 01:53
Copy link
Collaborator

@Hwurzburg Hwurzburg left a comment

Choose a reason for hiding this comment

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

needs squash and library split

@nexton-winjeel
Copy link
Contributor Author

@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.

@robertlong13
Copy link
Collaborator

  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

@Hwurzburg
Copy link
Collaborator

Hwurzburg commented Aug 9, 2024

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

@tpwrules
Copy link
Contributor

only one commit per library is allowed in final pr

This is definitely not true and IMO reduces clarity.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

only one commit per library is allowed in final pr

That's not the case. But there are too many commits in here.

This is a problem, 'though, @nexton-winjeel
image

I usually also squash the changes to build_options.py and extract_features.py into a single commit (and often just omit the fact it touches extract_features.py) - there are two examples of that in the current commit list.

I'd also squash these two:
image

These could possibly also be squashed:
image

... they can be separated, but it's not particularly useful (and this, I think, the sort of thing that Henry might be kind of objecting to).

@nexton-winjeel
Copy link
Contributor Author

@Hwurzburg, @peterbarker: Squashed.

@nexton-winjeel
Copy link
Contributor Author

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

@andyp1per
Copy link
Collaborator

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

@nexton-winjeel
Copy link
Contributor Author

nexton-winjeel commented Aug 12, 2024

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

userdata mavlink_camera_information_t field model_name'array 32 uint8_t'skip_check read write
userdata mavlink_camera_information_t field lens_id uint8_t'skip_check read write
userdata mavlink_camera_information_t field cam_definition_uri'array 140 uint8_t'skip_check read write
userdata mavlink_camera_information_t field gimbal_device_id uint8_t'skip_check read write
Copy link
Contributor

Choose a reason for hiding this comment

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

we should bring in the XML for camera_id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tridge: Is this PR blocked on getting the MAVLink XML changes for camera_id brought in?

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

Depends should match the AP_Camera one.

@IamPete1
Copy link
Member

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.

@nexton-winjeel
Copy link
Contributor Author

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

@nexton-winjeel
Copy link
Contributor Author

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

@amilcarlucas
Copy link
Contributor

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.

@nexton-winjeel
Copy link
Contributor Author

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  

@Hwurzburg
Copy link
Collaborator

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.

@rmackay9
Copy link
Contributor

@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.

Tools/scripts/build_options.py Outdated Show resolved Hide resolved
Tools/scripts/build_options.py Outdated Show resolved Hide resolved
Tools/scripts/build_options.py Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
@IamPete1 IamPete1 dismissed their stale review September 30, 2024 23:18

Scripting dependencies fixed.

@rmackay9 rmackay9 merged commit 2989d62 into ArduPilot:master Sep 30, 2024
96 checks passed
@nexton-winjeel nexton-winjeel deleted the upstream/lua-camera-information branch September 30, 2024 23:23
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.