feat: add start/stop_tlog_recording() to Mavsdk#2870
Open
bansiesta wants to merge 24 commits intomavlink:mainfrom
Open
feat: add start/stop_tlog_recording() to Mavsdk#2870bansiesta wants to merge 24 commits intomavlink:mainfrom
bansiesta wants to merge 24 commits intomavlink:mainfrom
Conversation
julianoes
requested changes
Apr 24, 2026
Collaborator
julianoes
left a comment
julianoes
left a comment
There was a problem hiding this comment.
See comments. And also, we might want to add an example to show this in action.
|
|
||
| // Raw MAVLink wire packet. | ||
| std::array<uint8_t, MAVLINK_MAX_PACKET_LEN> wire{}; | ||
| const uint16_t wire_len = mavlink_msg_to_send_buffer(wire.data(), &message); |
Collaborator
There was a problem hiding this comment.
Does this touch a mavlink channel and status? If so, we have to check out a separate channel.
|
|
||
| #include <array> | ||
| #include <atomic> | ||
| #include <chrono> |
Collaborator
There was a problem hiding this comment.
Includes should be added to the cpp file unless they are required in the hpp.
Ryanf55
reviewed
Apr 24, 2026
bansiesta
added a commit
to bansiesta/MAVSDK
that referenced
this pull request
Apr 26, 2026
Add an explicit destructor to the TlogFile struct so the file is always flushed and closed deterministically when the struct is destroyed, even if stop_tlog_recording() is not called explicitly. The MavsdkImpl destructor already calls stop_tlog_recording(), so normal teardown is unaffected. This is belt-and-suspenders for any exceptional path where the unique_ptr is reset without going through stop_tlog_recording() first. Addresses review feedback from julianoes and Ryanf55 on mavlink#2870. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
julianoes
requested changes
Apr 26, 2026
Comment on lines
+37
to
+48
| // Definition of the opaque tlog-file wrapper forward-declared in mavsdk_impl.hpp. | ||
| // Keeps <fstream> out of the public/impl header. | ||
| struct TlogFile { | ||
| std::ofstream stream; | ||
| ~TlogFile() | ||
| { | ||
| if (stream.is_open()) { | ||
| stream.flush(); | ||
| stream.close(); | ||
| } | ||
| } | ||
| }; |
Collaborator
There was a problem hiding this comment.
It's fine to have this in mavsdk_impl.hpp and add the includes that we need. It is already an impl. All I meant is that if we have the choice whether to put an include in .hpp or .cpp we should put it in .cpp unless required in the interface.
julianoes
previously approved these changes
Apr 27, 2026
Ryanf55
suggested changes
Apr 27, 2026
Contributor
Ryanf55
left a comment
Ryanf55
left a comment
There was a problem hiding this comment.
It would be good to add an example to demonstrate usage of logging, especially given this is designed to be thread safe, it might be worth writing an example that can interleave two streams of data and demonstrate no deadlocks/segfaults.
|
|
||
| class RawConnection; // Forward declaration | ||
|
|
||
| // Defined in mavsdk_impl.cpp to avoid pulling <fstream> into this header. |
Contributor
There was a problem hiding this comment.
Suggested change
| // Defined in mavsdk_impl.cpp to avoid pulling <fstream> into this header. |
Avoid commenting things that are self explained. Forward declarations are general good coding practice and don't require comments explaining.
| std::array<uint8_t, 8> ts{}; | ||
| uint64_t v = now_us; | ||
| for (int i = 7; i >= 0; --i) { | ||
| ts[static_cast<size_t>(i)] = static_cast<uint8_t>(v & 0xFFu); |
Contributor
There was a problem hiding this comment.
Suggested change
| ts[static_cast<size_t>(i)] = static_cast<uint8_t>(v & 0xFFu); | |
| ts.at(static_cast<size_t>(i)) = static_cast<uint8_t>(v & 0xFFu); |
Per the C++ Core Guidelines.
Code can call the .at() member function on each class, which will result in an std::out_of_range exception being thrown. Alternatively, code can call the at() free function, which will result in fail-fast (or a customized action) on a bounds violation.
This has minimal overhead, so we should prefer bounds checked access.
| static_cast<uint64_t>(std::chrono::duration_cast<std::chrono::microseconds>( | ||
| std::chrono::system_clock::now().time_since_epoch()) | ||
| .count()); | ||
| std::array<uint8_t, 8> ts{}; |
Contributor
There was a problem hiding this comment.
This 8 is repeated a few times. Define it as a const or constexpr constant local for this function.
| _intercept_outgoing_messages_callback = callback; | ||
| } | ||
|
|
||
| bool MavsdkImpl::start_tlog_recording(const std::string& path) |
Contributor
There was a problem hiding this comment.
Should we consider using for file IO paths rather than a std::string? It seems that MAVSDK is already adopting filesystem across the codebase.
Collaborator
There was a problem hiding this comment.
We only expose std::string to the public interface and not std::filesystem, pretty sure.
Contributor
There was a problem hiding this comment.
That could be a good V4 task for your AI friend.
Collaborator
There was a problem hiding this comment.
No, that's on purpose. I want to keep the API types minimal because I think it makes it easier for wrapper code. Also, it means an application linking to the library doesn't transitively depend on std::filesystem, and std::filesystem stays internal only.
| * @param path Output file path (e.g. "flight.tlog"). | ||
| * @return true if the file was opened successfully, false otherwise. | ||
| */ | ||
| bool start_tlog_recording(const std::string& path); |
Contributor
There was a problem hiding this comment.
Suggested change
| bool start_tlog_recording(const std::string& path); | |
| [[nodiscard]] bool start_tlog_recording(const std::string& path); |
Ignoring errors from this would likely be a bug. We should recommend at an API level to handle errors.
Collaborator
There was a problem hiding this comment.
Makes sense, happy to add that in. We are not using it yet in the public API but I think for v4 (which this is) we might as well!
Comment on lines
+37
to
+38
| // TlogFile is forward-declared in mavsdk_impl.hpp; defined here so that | ||
| // <fstream> is not pulled into the header. |
Contributor
There was a problem hiding this comment.
Suggested change
| // TlogFile is forward-declared in mavsdk_impl.hpp; defined here so that | |
| // <fstream> is not pulled into the header. |
Collaborator
There was a problem hiding this comment.
Yep yep, AI documenting its traces. Remove please.
| _tlog_file->stream.close(); | ||
| } | ||
| _tlog_file = std::make_unique<TlogFile>(); | ||
| _tlog_file->stream.open(path, std::ios::binary | std::ios::trunc); |
Contributor
There was a problem hiding this comment.
Is _tlog_file guaranteed to be non-null here?
Edit: Yes: https://isocpp.org/wiki/faq/freestore-mgmt#new-never-returns-null
| // checkout via mavlink_get_channel_status() is required here. | ||
| std::array<uint8_t, MAVLINK_MAX_PACKET_LEN> wire{}; | ||
| const uint16_t wire_len = mavlink_msg_to_send_buffer(wire.data(), &message); | ||
| _tlog_file->stream.write(reinterpret_cast<const char*>(wire.data()), wire_len); |
Contributor
There was a problem hiding this comment.
According to https://en.cppreference.com/cpp/io/basic_ostream/write, if write fails, badbit is set
This does not handle write errors. They are silently ignored.
Can we bring these errors up at least in console errors? Otherwise, you silently may not have a working tlog file.
Contributor
|
I tried this with this example code. The tlog file is zero bytes even though messages are printed in terminal. CMakeLists.txt cmake_minimum_required(VERSION 3.10.2)
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
project(write_tlog)
add_executable(write_tlog
write_tlog.cpp
)
find_package(MAVSDK REQUIRED)
target_link_libraries(write_tlog
MAVSDK::mavsdk
)
if(NOT MSVC)
add_compile_options(write_tlog PRIVATE -Wall -Wextra)
else()
add_compile_options(write_tlog PRIVATE -W2)
endif()write_tlog.cpp //
// Simple example to demonstrate how to write a TLOG using MAVSDK.
//
#include <mavsdk/mavsdk.hpp>
#include <chrono>
#include <cstdint>
#include <iostream>
#include <future>
#include <csignal>
using namespace mavsdk;
// Global atomic flag to indicate if the program should exit
std::atomic<bool> should_exit{false};
// Signal handler for Ctrl+C
void signal_handler(int signal)
{
if (signal == SIGINT) {
std::cout << "\nCtrl+C pressed. Exiting..." << std::endl;
should_exit.store(true);
}
}
void usage(const std::string& bin_name)
{
std::cerr << "Usage : " << bin_name << " <connection_url> <index> <on|off>\n"
<< "Connection URL format should be :\n"
<< " For TCP server: tcpin://<our_ip>:<port>\n"
<< " For TCP client: tcpout://<remote_ip>:<port>\n"
<< " For UDP server: udpin://<our_ip>:<port>\n"
<< " For UDP client: udpout://<remote_ip>:<port>\n"
<< " For Serial : serial://</path/to/serial/dev>:<baudrate>]\n"
<< "For example, to connect to the simulator use URL: udpin://0.0.0.0:14540\n"
<< "Index is the relay instance number, 0 or greater.\n";
}
int main(int argc, char** argv)
{
if (argc != 3) {
usage(argv[0]);
return 1;
}
const std::string connection_url = argv[1];
const std::string tlog_filename = argv[2];
Mavsdk mavsdk{Mavsdk::Configuration{ComponentType::CompanionComputer}};
const ConnectionResult connection_result = mavsdk.add_any_connection(connection_url);
if (connection_result != ConnectionResult::Success) {
std::cerr << "Connection failed: " << connection_result << '\n';
return 1;
}
std::cout << "Waiting to discover system...\n";
auto prom = std::promise<std::shared_ptr<System>>{};
auto fut = prom.get_future();
// We wait for new systems to be discovered, once we find one that has an
// autopilot, we decide to use it.
Mavsdk::NewSystemHandle handle = mavsdk.subscribe_on_new_system([&mavsdk, &prom, &handle]() {
auto system = mavsdk.systems().back();
prom.set_value(system);
});
// We usually receive heartbeats at 1Hz, therefore we should find a
// system after around 3 seconds max, surely.
if (fut.wait_for(std::chrono::seconds(3)) == std::future_status::timeout) {
std::cerr << "No autopilot found, exiting.\n";
return 1;
}
// Get discovered system now.
auto system = fut.get();
// Start Logging.
std::cout << "Writing tlog to '" << tlog_filename << "' ...\n";
mavsdk.start_tlog_recording(tlog_filename);
// Main loop
std::cout << "Logging to TLOG. Press Ctrl+C to exit..." << std::endl;
while (!should_exit.load()) {
std::this_thread::sleep_for(std::chrono::milliseconds(500));
}
std::cout << "Stopping writing to TLOG." << std::endl;
mavsdk.stop_tlog_recording();
return 0;
}Terminal 1, ardupilot Terminal 2, mavsdk/cpp Terminal Output - notice the ctrl+C might not be handled correctly, I don't see |
Collaborator
|
@Ryanf55 I'll leave this to you and @bansiesta to iterate and get working. |
Collaborator
|
@Ryanf55 is this now working for you? |
julianoes
reviewed
Apr 28, 2026
|
|
||
| TEST_F(Curl, DownloadText_HTTP_Success) | ||
| { | ||
| GTEST_SKIP() << "S3 bucket no longer exists, skipping flaky external test"; |
Collaborator
There was a problem hiding this comment.
Hang on. Can we use a different bucket?
Contributor
There was a problem hiding this comment.
@bansiesta Create a separate PR for handling the missing S3 bucket problem. It has nothing to do with tlogs.
Adds two methods to the Mavsdk class: bool start_tlog_recording(const std::string& path); void stop_tlog_recording(); Recording captures all incoming MAVLink traffic across the entire Mavsdk instance (all connected systems and connections) rather than per-system, matching the behaviour of QGC and MAVProxy which put multiple sysids in a single tlog. Each record in the file is the standard tlog format: [8 bytes] big-endian microsecond Unix timestamp [N bytes] raw MAVLink wire packet The hook sits in process_message() before the intercept/drop callback so that all received traffic is captured. Writes are serialised with a dedicated mutex so the I/O thread and any concurrent stop call are safe. Closes mavlink#2142 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move <array> and <chrono> includes from mavsdk_impl.hpp to the .cpp (they are not required in the header) - Call stop_tlog_recording() in ~MavsdkImpl() so the file is always flushed/closed on destruction, even if the caller forgets - Add comment clarifying that mavlink_msg_to_send_buffer() only serialises an already-finalised message and does not touch any MAVLink channel status, so no channel checkout is needed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add start_tlog_recording() and stop_tlog_recording() to the API reference docs, and update the docstring to include MAVProxy alongside Mission Planner and pymavlink as per review feedback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ader Address Julian's review feedback: includes that are not required in the header should live in the implementation file. Switch _tlog_file from a plain std::ofstream member (which forced <fstream> into the header) to a std::unique_ptr<std::ofstream>, using only <iosfwd> in the header. The full <fstream> include is now in mavsdk_impl.cpp where it belongs.
Per review feedback: libmav_receiver.hpp, mavlink_address.hpp, mavlink_command_receiver.hpp, and mav/BufferParser.h were included in mavsdk_impl.hpp but are only needed by the translation unit. Moved them to mavsdk_impl.cpp. The existing forward-declaration block in the header is sufficient for the unique_ptr<BufferParser> member. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace std::unique_ptr<std::ofstream> member with an opaque TlogFile struct (forward-declared in header, defined in cpp) so that <iosfwd> no longer needs to appear in mavsdk_impl.hpp - Remove "QGroundControl," from the start_tlog_recording() docstring as suggested by Ryan - The destructor already calls stop_tlog_recording() and the channel- checkout comment was already present; no changes needed there Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The header was updated in the previous commit to remove "QGroundControl," from the start_tlog_recording() docstring, but the generated API reference doc was not updated. Sync it so the docs check CI passes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Clarify in doc comments that recording is automatically stopped on Mavsdk destruction (RAII), so explicit stop_tlog_recording() is optional - Expand channel-checkout comment in receive path to directly answer reviewer question: mavlink_msg_to_send_buffer() does not touch any mavlink_channel_t state, no checkout needed - Note RAII cleanup in Mavsdk destructor doc comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
….hpp Both are already pulled in transitively via mavsdk.hpp. Per review feedback, headers should not include what is already available through their own includes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add an explicit destructor to the TlogFile struct so the file is always flushed and closed deterministically when the struct is destroyed, even if stop_tlog_recording() is not called explicitly. The MavsdkImpl destructor already calls stop_tlog_recording(), so normal teardown is unaffected. This is belt-and-suspenders for any exceptional path where the unique_ptr is reset without going through stop_tlog_recording() first. Addresses review feedback from julianoes and Ryanf55 on mavlink#2870. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Per review feedback, mavsdk_impl.hpp is already an implementation header so it is fine to define TlogFile there with the needed <fstream> include, rather than keeping an opaque forward declaration and defining the struct in the .cpp file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move TlogFile struct and <fstream> include from mavsdk_impl.hpp to mavsdk_impl.cpp; keep only a forward declaration in the header so no tlog-related headers leak into the public translation units. - Destructor already calls stop_tlog_recording() after io_thread joins (added in the original PR); no further change needed there. - mavlink_msg_to_send_buffer() comment already explains it only copies pre-serialised bytes and does not touch channel status, so no dedicated channel checkout is required. - "MAVProxy" was already present in the start_tlog_recording() doc comment; no change needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
julianoes clarified that including <fstream> and defining TlogFile directly in mavsdk_impl.hpp is fine — it is already an impl header, not a public interface, so the usual preference for keeping includes in .cpp does not apply here. Remove the unnecessary forward-declaration pattern and define TlogFile inline in the header as it was originally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move the TlogFile struct from mavsdk_impl.hpp into mavsdk_impl.cpp so that <fstream> is not pulled into the header. A forward declaration in mavsdk_impl.hpp is sufficient because MavsdkImpl's destructor (which calls unique_ptr's deleter) is defined in the .cpp where TlogFile is complete. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The tlog recording block existed only in process_message() (the legacy mavlink_message_t path) but was absent from process_libmav_message(), which is the path used by all UDP/TCP/serial connections. Any call to start_tlog_recording() therefore produced a zero-byte file. Fix: - Add raw_bytes (std::vector<uint8_t>) to Mavsdk::MavlinkMessage so the wire bytes can travel with the message through the call chain. - In LibmavReceiver::parse_libmav_message_from_buffer(), populate raw_bytes from the finalized mav::Message via data()/finalizedSize(). - In MavsdkImpl::process_libmav_message(), mirror the tlog-write block that already existed in process_message(), writing the 8-byte BE timestamp followed by the raw wire bytes. Also add examples/write_tlog/ — a minimal working example that correctly registers SIGINT/SIGTERM handlers so the file is always flushed and closed on exit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ks, error handling) - Add [[nodiscard]] to start_tlog_recording() as suggested - Use .at() for bounds-checked timestamp byte access in both write paths - Define magic constant 8 as constexpr timestamp_bytes - Log and stop recording on stream write failure instead of silently ignoring - Remove self-explanatory comments on forward declarations Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Log an error and stop recording if a stream write fails (badbit set), so silent data loss is visible rather than producing a corrupt/empty file. - Move start_tlog_recording() before the system discovery wait in the write_tlog example so the initial handshake messages are captured too. - Also call stop_tlog_recording() on the early-exit timeout path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove the five-line explanatory block above mavlink_msg_to_send_buffer() that over-documented implementation details (julianoes: "AI documenting its traces. Remove please."), and remove the self-explanatory forward declaration comment in mavsdk_impl.hpp (Ryanf55 suggestion).
…vlink_direct
Remove explanatory comment blocks that over-document self-evident code,
per review feedback from julianoes ("AI documenting its traces").
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bansiesta
force-pushed
the
feature/tlog-api
branch
from
f128571 to
7f78a1d
Compare
These changes (renaming CurlTest class, curl_wrapper.h include) are unrelated to the tlog recording feature and should not be part of this PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous "revert" commit accidentally changed #include "curl_wrapper.hpp" to #include "curl_wrapper.h" (which does not exist) and also introduced class and test name changes inconsistent with upstream. Upstream renamed headers from .h to .hpp in mavlink#2844; this restores curl_test.cpp to match that change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.