Implement component metadata protocol #2272

bkueng · 2024-04-05T12:59:27Z

This implements the MAVLink component metadata protocol.
Details:

Allows users to retrieve the JSON metadata. The idea is to then build on top of that to handle the specific component types (e.g. parameters or events)
Implementation for both client + server (with system test)
File caching based on CRC
Support for mftp + http (if curl is enabled)
translation is not implemented

Client example usage:

auto component_metadata = ComponentMetadata{system.value()};
component_metadata.register_notification_callback([] (ComponentMetadata::MetadataType type, uint32_t compid, const std::string& json_data) {
	std::cout << "Got metadata: type: " << (int)type << " compid: " << compid << std::endl;
});

TODO:

To support https, curl with openssl needs to be enabled (I tested this already)
For XZ decompression, the xz-embedded submodule is added. liblzma could be used instead.
Are plugins required to stop ongoing mftp/http downloads when PluginImplBase::disable is called?

Requires PX4/PX4-Autopilot#22980

Tested with PX4 (SITL + Pixhawk, mftp + https links)

julianoes

Amazing! Thanks so much for the split commits, makes the review much easier.

I left a bunch of comments. I might do a more in-depth review once I try out some of the pieces.

What's missing is the proto pull request. I want to make sure I understand the user interface.

src/mavsdk/core/locked_queue.h

src/mavsdk/core/mavlink_ftp_client.h

src/mavsdk/core/mavlink_ftp_client.cpp

julianoes · 2024-04-05T19:27:57Z

.gitmodules

+[submodule "src/third_party/xz-embedded/submodule"]
+	path = src/third_party/xz-embedded/submodule
+	url = https://github.com/tukaani-project/xz-embedded.git


I'd prefer to get the dependency in the same way like all others (except gtest):

https://github.com/mavlink/MAVSDK/blob/main/third_party/CMakeLists.txt
E.g. https://github.com/mavlink/MAVSDK/blob/main/third_party/jsoncpp/CMakeLists.txt

Which library is up to you.

This ^. Do not add dependencies as submodules.

Alright, I changed to the system lib liblzma in 5175884

src/mavsdk/core/fs_utils.cpp

src/mavsdk/plugins/component_metadata_server/component_metadata_server_impl.cpp

julianoes · 2024-04-05T19:45:48Z

src/mavsdk/plugins/component_metadata_server/component_metadata_server_impl.cpp

+    }
+    _tmp_path = *tmp_option;
+
+    LogDebug() << "Storing metadata under " << _tmp_path;


By the way, I would probably add a _debugging bool like other plugins have it which can be enabled using export MAVSDK_COMPONENT_METADATA_DEBUGGING=1, and then you can make it quite verbose in that case.

Done in a725299

julianoes · 2024-04-05T19:48:21Z

src/system_tests/component_metadata.cpp

+    for (int i = 0; i < 100 && (!received_events || !received_parameters); ++i) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(100));
+    }


You didn't want to do this one with std::future and std::promise?

I thought it's not worth the extra complexity given it's a test and we need to wait for multiple things.

Right, I wonder what the canonical way is to wait for two futures.

julianoes · 2024-04-05T19:49:22Z

src/mavsdk/core/mavlink_ftp_client.cpp

+                        start_timer(3.0);
+                        LogDebug() << "No session available, retrying...";


That makes sense, nice.

julianoes · 2024-04-05T19:49:49Z

src/mavsdk/core/mavlink_ftp_client.cpp

 {
    _system_impl.unregister_timeout_handler(_timeout_cookie);
    _system_impl.register_timeout_handler(
-        [this]() { timeout(); }, _system_impl.timeout_s(), &_timeout_cookie);
+        [this]() { timeout(); }, duration_s.value_or(_system_impl.timeout_s()), &_timeout_cookie);


Oh, I didn't know value_or, that's nice.

bkueng · 2024-04-11T16:23:27Z

Thanks for the review @julianoes! Here's the proto changes: mavlink/MAVSDK-Proto#338
I'll amend the commits after things settle.

bkueng · 2024-04-25T13:14:43Z

CI is passing now, except for the proto submodule not being updated yet.
I also had to update the re2 dependency (c1883f4). I'm not sure why that problem shows up here, but not on main.

julianoes

Fine to be merged once we have proto merged!

julianoes · 2024-04-26T01:25:06Z

src/mavsdk/core/fs_utils.h

@@ -0,0 +1,20 @@
+#pragma once


We should probably merge this file with https://github.com/mavlink/MAVSDK/blob/main/src/system_tests/fs_helpers.h but it can happen after this PR.

Otherwise there might still be data in the buffer. All other places already close the stream.

This version (5.4.5) is not affected by the recently discovered backdoor (https://tukaani.org/xz-backdoor/)

… an ongoing transfer

instead of calling the callback directly. Otherwise if the callback tries to trigger a new request, it would block indefinitely when trying to add a work item to _work_queue, which is already locked by this: LockedQueue<Work>::Guard work_queue_guard(_work_queue);

To include this commit: google/re2@9ebe4a2 Which should fix the CI error: CMake Error at /home/runner/work/MAVSDK/MAVSDK/build/release/third_party/install/lib/cmake/re2/re2Config.cmake:15 (message): File or directory /include referenced by variable re2_INCLUDE_DIR does not exist !

Brings component metadata plugins. Signed-off-by: Julian Oes <julian@oes.ch>

sonarcloud · 2024-04-26T01:37:39Z

Quality Gate failed

Failed conditions
2 Security Hotspots
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

julianoes · 2024-04-26T02:25:44Z

Thanks a lot @bkueng!

bkueng force-pushed the component_metadata branch from e6061f7 to f4800cf Compare April 5, 2024 13:47

julianoes self-requested a review April 5, 2024 19:18

julianoes reviewed Apr 5, 2024

View reviewed changes

bkueng mentioned this pull request Apr 11, 2024

add component_metadata and component_metadata_server mavlink/MAVSDK-Proto#338

Merged

bkueng force-pushed the component_metadata branch from 2116187 to 78400f5 Compare April 17, 2024 13:47

bkueng force-pushed the component_metadata branch 3 times, most recently from 8cd8a6e to c1883f4 Compare April 25, 2024 11:46

julianoes previously approved these changes Apr 26, 2024

View reviewed changes

julianoes dismissed their stale review via 075cb7d April 26, 2024 01:27

bkueng and others added 15 commits April 26, 2024 13:28

mavlink_ftp_client: allow specifying the target component id per request

c5e85d6

fix mavlink_ftp_client: close ofstream before calling the callback

1979c80

Otherwise there might still be data in the buffer. All other places already close the stream.

core: add fs utils to get cache & temp directories

37c1145

component_metadata: add file cache class

b06c3f5

core: add MAV_COMP_ID_ONBOARD_COMPUTER to component_name

1222df7

core: add liblzma dependency

150c670

This version (5.4.5) is not affected by the recently discovered backdoor (https://tukaani.org/xz-backdoor/)

component_metadata: add protocol implementation

6fb1257

component_metadata: add server protocol implementation

248acc0

system_tests: add test for component_metadata

1b06734

mavlink_ftp_client: retry download if the other component already has…

3abc88f

… an ongoing transfer

component_metadata: update proto files

8177a3c

component_information: remove old generated files

32f1466

proto: update submodule

a13aa62

Brings component metadata plugins. Signed-off-by: Julian Oes <julian@oes.ch>

julianoes force-pushed the component_metadata branch from 075cb7d to a13aa62 Compare April 26, 2024 01:29

julianoes merged commit d68204a into mavlink:main Apr 26, 2024
27 of 28 checks passed

bkueng deleted the component_metadata branch April 26, 2024 06:28

bkueng mentioned this pull request Jun 12, 2024

Events plugin #2338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement component metadata protocol #2272

Implement component metadata protocol #2272

bkueng commented Apr 5, 2024

julianoes left a comment

julianoes Apr 5, 2024

JonasVautherin Apr 6, 2024

bkueng Apr 11, 2024

julianoes Apr 5, 2024

bkueng Apr 11, 2024

julianoes Apr 11, 2024

julianoes Apr 5, 2024

bkueng Apr 11, 2024

julianoes Apr 11, 2024

julianoes Apr 5, 2024

julianoes Apr 5, 2024

bkueng commented Apr 11, 2024

bkueng commented Apr 25, 2024

julianoes left a comment

julianoes Apr 26, 2024

sonarcloud bot commented Apr 26, 2024

julianoes commented Apr 26, 2024

		start_timer(3.0);
		LogDebug() << "No session available, retrying...";

Implement component metadata protocol #2272

Implement component metadata protocol #2272

Conversation

bkueng commented Apr 5, 2024

julianoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkueng commented Apr 11, 2024

bkueng commented Apr 25, 2024

julianoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 26, 2024

Quality Gate failed

julianoes commented Apr 26, 2024