From c7ab23f38c29711b04dfff4537d90566d20db969 Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Tue, 14 May 2024 14:16:56 +1200 Subject: [PATCH 1/3] core: Don't create empty system I don't believe it is still required to create these fake systems. That's because we send out heartbeats from MavsdkImpl or rather ServerComponentImpl, and not from within SystemImpl any longer which makes a lot more sense anyway. This commit removes creating the fake system. Signed-off-by: Julian Oes --- src/mavsdk/core/mavsdk_impl.cpp | 19 +------------------ src/mavsdk/core/system_impl.cpp | 6 +----- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/src/mavsdk/core/mavsdk_impl.cpp b/src/mavsdk/core/mavsdk_impl.cpp index 34fddfbd92..caae028004 100644 --- a/src/mavsdk/core/mavsdk_impl.cpp +++ b/src/mavsdk/core/mavsdk_impl.cpp @@ -372,19 +372,6 @@ void MavsdkImpl::receive_message(mavlink_message_t& message, Connection* connect std::lock_guard lock(_systems_mutex); - // The only situation where we create a system with sysid 0 is when we initialize the connection - // to the remote. - if (_systems.size() == 1 && _systems[0].first == 0) { - LogDebug() << "New: System ID: " << static_cast(message.sysid) - << " Comp ID: " << static_cast(message.compid); - _systems[0].first = message.sysid; - _systems[0].second->system_impl()->set_system_id(message.sysid); - - // Even though the fake system was already discovered, we can now - // send a notification, now that it seems to really actually exist. - notify_on_discover(); - } - bool found_system = false; for (auto& system : _systems) { if (system.first == message.sysid) { @@ -559,12 +546,8 @@ std::pair MavsdkImpl::setup_udp_remo new_conn->add_remote(remote_ip, remote_port); auto handle = add_connection(new_conn); std::lock_guard lock(_systems_mutex); - if (_systems.empty()) { - make_system_with_component(0, 0); - } - // With a UDP remote, we need to initiate the connection by sending - // heartbeats. + // With a UDP remote, we need to initiate the connection by sending heartbeats. auto new_configuration = get_configuration(); new_configuration.set_always_send_heartbeats(true); set_configuration(new_configuration); diff --git a/src/mavsdk/core/system_impl.cpp b/src/mavsdk/core/system_impl.cpp index e923d164a8..eedef99b7d 100644 --- a/src/mavsdk/core/system_impl.cpp +++ b/src/mavsdk/core/system_impl.cpp @@ -541,11 +541,7 @@ void SystemImpl::set_connected() _connected = true; - // System with sysid 0 is a bit special: it is a placeholder for a connection initiated - // by MAVSDK. As such, it should not be advertised as a newly discovered system. - if (static_cast(get_system_id()) != 0) { - _mavsdk_impl.notify_on_discover(); - } + _mavsdk_impl.notify_on_discover(); // We call this later to avoid deadlocks on creating the server components. _mavsdk_impl.call_user_callback([this]() { From eb35c806890495ea065a784478a1ac460f15972f Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Tue, 14 May 2024 14:22:49 +1200 Subject: [PATCH 2/3] core: Prevent potential deadlock This should fix the deadlock which can happen when a timeout happens at the exact time when a heartbeat arrives again. In that case one thread will call: SystemImpl::heartbeats_timed_out() -> acquire _connection_mutex SystemImpl::set_disconnected() MavsdkImpl::notify_on_timeout() -> acquire _systems_mutex The other thread will call: MavsdkImpl::receive_message() -> acquire _systems_mutex SystemImpl::set_connected() -> acquire _connection_mutex So both threads end up waiting for each other's mutex and dead lock. This commit fixes the problem by moving the on_discover and on_timeout calls outside of the scope of where the _connection_mutex is acquired. Signed-off-by: Julian Oes --- src/mavsdk/core/system_impl.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/mavsdk/core/system_impl.cpp b/src/mavsdk/core/system_impl.cpp index eedef99b7d..b0584ca96d 100644 --- a/src/mavsdk/core/system_impl.cpp +++ b/src/mavsdk/core/system_impl.cpp @@ -541,8 +541,6 @@ void SystemImpl::set_connected() _connected = true; - _mavsdk_impl.notify_on_discover(); - // We call this later to avoid deadlocks on creating the server components. _mavsdk_impl.call_user_callback([this]() { // Send a heartbeat back immediately. @@ -564,6 +562,9 @@ void SystemImpl::set_connected() } // If not yet connected there is nothing to do/ } + + _mavsdk_impl.notify_on_discover(); + if (enable_needed) { if (has_autopilot()) { send_autopilot_version_request_async(nullptr); @@ -587,10 +588,10 @@ void SystemImpl::set_disconnected() //_heartbeat_timeout_cookie = nullptr; _connected = false; - _mavsdk_impl.notify_on_timeout(); _is_connected_callbacks.queue( false, [this](const auto& func) { _mavsdk_impl.call_user_callback(func); }); } + _mavsdk_impl.notify_on_timeout(); _mavsdk_impl.stop_sending_heartbeats(); From 04dff589017deeef94adc80da43278e140505cd6 Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Tue, 14 May 2024 14:28:40 +1200 Subject: [PATCH 3/3] core/info: make version request more forgiving Instead of doing too many commands at once, we only do one at a time, and don't immediately call ourselves again. We don't want to hold up the plugins' enable() calls for too long. Also, we can make the request interval slightly longer. Signed-off-by: Julian Oes --- src/mavsdk/core/system_impl.cpp | 4 ++-- src/mavsdk/plugins/info/info_impl.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mavsdk/core/system_impl.cpp b/src/mavsdk/core/system_impl.cpp index b0584ca96d..bfa1980c8d 100644 --- a/src/mavsdk/core/system_impl.cpp +++ b/src/mavsdk/core/system_impl.cpp @@ -504,8 +504,8 @@ void SystemImpl::send_autopilot_version_request() if (fut.get() == MavlinkCommandSender::Result::Unsupported) { _old_message_520_supported = false; - LogWarn() << "Trying alternative command (512)."; - send_autopilot_version_request(); + LogWarn() + << "Trying alternative command REQUEST_MESSAGE instead of REQUEST_AUTOPILOT_CAPABILITIES next."; } } diff --git a/src/mavsdk/plugins/info/info_impl.cpp b/src/mavsdk/plugins/info/info_impl.cpp index 03aee905d6..2e99a6e338 100644 --- a/src/mavsdk/plugins/info/info_impl.cpp +++ b/src/mavsdk/plugins/info/info_impl.cpp @@ -52,7 +52,7 @@ void InfoImpl::enable() { // We're going to retry until we have the version. _system_impl->add_call_every( - [this]() { _system_impl->send_autopilot_version_request(); }, 1.0f, &_call_every_cookie); + [this]() { _system_impl->send_autopilot_version_request(); }, 2.0f, &_call_every_cookie); if (!_flight_info_subscriptions.empty()) { // We're hoping to get flight information regularly to update flight time.