From d1c19790cd44cda16528b00d80a9477e090f4b34 Mon Sep 17 00:00:00 2001 From: Kevin Dewald Date: Sun, 3 Aug 2025 00:24:02 -0700 Subject: [PATCH] Migrated to new message forwarding logic on SimpleDBus. Fixed race condition. --- docs/changelog.rst | 6 +- examples/simpleble/src/scan.cpp | 2 +- simpleble/src/Config.cpp | 2 +- simplebluez/src/Bluez.cpp | 11 +- .../include/simpledbus/advanced/Proxy.h | 1 - .../include/simpledbus/base/Connection.h | 3 - simpledbus/src/advanced/Interface.cpp | 7 +- simpledbus/src/advanced/Proxy.cpp | 127 +++++++++++------- simpledbus/src/base/Connection.cpp | 27 +--- 9 files changed, 89 insertions(+), 97 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 8c7b01d..84f2b56 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -16,21 +16,23 @@ The format is based on `Keep a Changelog`_, and this project adheres to `Semanti **Added** - Introduced scaffolding for advanced low-level features. +- Configuration option to select which BlueZ backend to use. - (Android) Added support to set and retrieve the JavaVM pointer. - (Linux) Added frozen BlueZ backend in preparation for upcoming changes. -- Configuration option to select which BlueZ backend to use. **Changed** - (MacOS) Use a single Adapter object across all users of the CoreBluetooth backend. - (Android) Use a single Adapter object across all users of the Android backend. +- (SimpleDBus) Messages are now directly forwarded to the appropriate proxy object, no more chaining required. **Fixed** +- Improper handling of configuration settings when consuming SimpleBLE as a shared library. - (MacOS) Freeze when attempting a double disconnection. - (Android) Solved "local reference table overflow" error. *(Thanks Nicole S.!)* - (Android) Fixed unexpected initialization of SimpleJNI. -- Improper handling of configuration settings when consuming SimpleBLE as a shared library. +- (SimpleDBus) Fixed race condition when handling property updates of DBus objects. **Removed** diff --git a/examples/simpleble/src/scan.cpp b/examples/simpleble/src/scan.cpp index 0f8129b..e4cf646 100644 --- a/examples/simpleble/src/scan.cpp +++ b/examples/simpleble/src/scan.cpp @@ -30,7 +30,7 @@ int main() { adapter.set_callback_on_scan_stop([]() { std::cout << "Scan stopped." << std::endl; }); // Scan for 5 seconds. - adapter.scan_for(5000); + adapter.scan_for(15000); std::cout << "Scan complete." << std::endl; diff --git a/simpleble/src/Config.cpp b/simpleble/src/Config.cpp index 001991a..ef5da8b 100644 --- a/simpleble/src/Config.cpp +++ b/simpleble/src/Config.cpp @@ -3,7 +3,7 @@ namespace SimpleBLE { namespace Config { namespace SimpleBluez { - bool use_legacy_bluez_backend = true; + bool use_legacy_bluez_backend = false; std::chrono::steady_clock::duration connection_timeout = std::chrono::seconds(2); std::chrono::steady_clock::duration disconnection_timeout = std::chrono::seconds(1); } // namespace SimpleBluez diff --git a/simplebluez/src/Bluez.cpp b/simplebluez/src/Bluez.cpp index f80a69c..033b464 100644 --- a/simplebluez/src/Bluez.cpp +++ b/simplebluez/src/Bluez.cpp @@ -26,16 +26,7 @@ void Bluez::init() { } void Bluez::run_async() { - // TODO: UNCOMMENT THIS WHEN MIGRATING TO NEW PROXY FORWARDING LOGIC - //_conn->read_write_dispatch(); - - // BELOW IS THE LEGACY LOGIC - _conn->read_write(); - SimpleDBus::Message message = _conn->pop_message(); - while (message.is_valid()) { - _bluez_root->message_forward(message); - message = _conn->pop_message(); - } + _conn->read_write_dispatch(); } std::vector> Bluez::get_adapters() { return _bluez_root->get_adapters(); } diff --git a/simpledbus/include/simpledbus/advanced/Proxy.h b/simpledbus/include/simpledbus/advanced/Proxy.h index 8edf7dc..082c245 100644 --- a/simpledbus/include/simpledbus/advanced/Proxy.h +++ b/simpledbus/include/simpledbus/advanced/Proxy.h @@ -66,7 +66,6 @@ class Proxy { void path_append_child(const std::string& path, std::shared_ptr child); void path_remove_child(const std::string& path); // ----- MESSAGE HANDLING ----- - void message_forward(Message& msg); void message_handle(Message& msg); // ----- CALLBACKS ----- diff --git a/simpledbus/include/simpledbus/base/Connection.h b/simpledbus/include/simpledbus/base/Connection.h index 81c0e56..a753040 100644 --- a/simpledbus/include/simpledbus/base/Connection.h +++ b/simpledbus/include/simpledbus/base/Connection.h @@ -22,10 +22,7 @@ class Connection { void add_match(std::string rule); void remove_match(std::string rule); - void read_write(); void read_write_dispatch(); - Message pop_message(); - void send(Message& msg); Message send_with_reply_and_block(Message& msg); diff --git a/simpledbus/src/advanced/Interface.cpp b/simpledbus/src/advanced/Interface.cpp index 3b84ec9..23f40f6 100644 --- a/simpledbus/src/advanced/Interface.cpp +++ b/simpledbus/src/advanced/Interface.cpp @@ -104,22 +104,25 @@ void Interface::property_refresh(const std::string& property_name) { } bool cb_property_changed_required = false; - _property_update_mutex.lock(); + try { // NOTE: Due to the way Bluez handles underlying devices and the fact that // they can be removed before the callback reaches back (race condition), // `property_get` can sometimes fail. Because of this, the update // statement is surrounded by a try-catch statement. Holder property_latest = property_get(property_name); + _property_update_mutex.lock(); _property_valid_map[property_name] = true; if (_properties[property_name] != property_latest) { _properties[property_name] = property_latest; cb_property_changed_required = true; } + _property_update_mutex.unlock(); } catch (const Exception::SendFailed& e) { + _property_update_mutex.lock(); _property_valid_map[property_name] = true; + _property_update_mutex.unlock(); } - _property_update_mutex.unlock(); if (cb_property_changed_required) { property_changed(property_name); diff --git a/simpledbus/src/advanced/Proxy.cpp b/simpledbus/src/advanced/Proxy.cpp index d44a4cb..a9cc90b 100644 --- a/simpledbus/src/advanced/Proxy.cpp +++ b/simpledbus/src/advanced/Proxy.cpp @@ -5,6 +5,7 @@ #include #include #include +#include // #include @@ -12,16 +13,14 @@ using namespace SimpleDBus; Proxy::Proxy(std::shared_ptr conn, const std::string& bus_name, const std::string& path) : _conn(conn), _bus_name(bus_name), _path(path), _valid(true), _registered(false) { - // TODO: UNCOMMENT THIS WHEN MIGRATING TO NEW PROXY FORWARDING LOGIC - // register_object_path(); + register_object_path(); - //_interfaces.emplace(std::make_pair("org.freedesktop.DBus.Properties", std::make_shared(conn, bus_name, - //path))); + // TODO: At some point in the future, proxy objects will own their own Properties interface. + //_interfaces.emplace(std::make_pair("org.freedesktop.DBus.Properties", std::make_shared(conn, bus_name, path))); } Proxy::~Proxy() { - // TODO: UNCOMMENT THIS WHEN MIGRATING TO NEW PROXY FORWARDING LOGIC - // unregister_object_path(); + unregister_object_path(); on_child_created.unload(); on_signal_received.unload(); } @@ -34,8 +33,7 @@ bool Proxy::valid() const { return _valid; } void Proxy::invalidate() { _valid = false; - // TODO: UNCOMMENT THIS WHEN MIGRATING TO NEW PROXY FORWARDING LOGIC - // unregister_object_path(); + unregister_object_path(); } std::string Proxy::path() const { return _path; } @@ -314,52 +312,79 @@ void Proxy::path_remove_child(const std::string& path) { // ----- MESSAGE HANDLING ----- -void Proxy::message_forward(Message& msg) { - // If the message is for the current proxy, then forward it to the message handler. - if (msg.get_path() == _path) { - // If the message is involves a property change, forward it to the correct interface. - if (msg.is_signal("org.freedesktop.DBus.Properties", "PropertiesChanged")) { - Holder interface_h = msg.extract(); - std::string iface_name = interface_h.get_string(); - msg.extract_next(); - Holder changed_properties = msg.extract(); - msg.extract_next(); - Holder invalidated_properties = msg.extract(); - - // If the interface is not loaded, then ignore the message. - if (!interface_exists(iface_name)) { - return; - } - - interface_get(iface_name)->signal_property_changed(changed_properties, invalidated_properties); - - } else if (interface_exists(msg.get_interface())) { - interface_get(msg.get_interface())->message_handle(msg); - } - - return; - } - - // If the message is for a child proxy or a descendant, forward it to that child proxy. - for (auto& [child_path, child] : _children) { - if (child_path == msg.get_path()) { - child->message_forward(msg); - - if (msg.get_type() == Message::Type::SIGNAL) { - child->on_signal_received(); - } - - return; - } else if (PathUtils::is_descendant(child_path, msg.get_path())) { - child->message_forward(msg); - return; - } - } -} - void Proxy::message_handle(Message& msg) { bool handled = false; + // ! The following block emulates the presence of a Properties interface. + // TODO: Remove this block when the Properties interface is implemented. + + const std::string properties_interface_name = "org.freedesktop.DBus.Properties"; + + if (msg.is_method_call(properties_interface_name, "GetAll")) { + Holder interface_h = msg.extract(); + std::string iface_name = interface_h.get_string(); + + // TODO: Handle the case where the interface does not exist, returning an error. + std::shared_ptr interface = interface_get(iface_name); + Holder result = interface->property_collect(); + + SimpleDBus::Message reply = SimpleDBus::Message::create_method_return(msg); + reply.append_argument(result, "a{sv}"); + _conn->send(reply); + handled = true; + + } else if (msg.is_method_call(properties_interface_name, "Get")) { + Holder interface_h = msg.extract(); + std::string iface_name = interface_h.get_string(); + msg.extract_next(); + + Holder property_h = msg.extract(); + std::string property_name = property_h.get_string(); + + // TODO: Handle the case where the interface does not exist, returning an error. + std::shared_ptr interface = interface_get(iface_name); + + Holder result = interface->property_collect_single(property_name); + + SimpleDBus::Message reply = SimpleDBus::Message::create_method_return(msg); + reply.append_argument(result, "v"); + _conn->send(reply); + handled = true; + + } else if (msg.is_method_call(properties_interface_name, "Set")) { + Holder interface_h = msg.extract(); + std::string iface_name = interface_h.get_string(); + msg.extract_next(); + + Holder property_h = msg.extract(); + std::string property_name = property_h.get_string(); + msg.extract_next(); + + Holder value_h = msg.extract(); + + // TODO: Handle the case where the interface does not exist, returning an error. + std::shared_ptr interface = interface_get(iface_name); + interface->property_modify(property_name, value_h); + + SimpleDBus::Message reply = SimpleDBus::Message::create_method_return(msg); + _conn->send(reply); + handled = true; + + } else if (msg.is_signal(properties_interface_name, "PropertiesChanged")) { + Holder interface_h = msg.extract(); + std::string iface_name = interface_h.get_string(); + msg.extract_next(); + Holder changed_properties = msg.extract(); + msg.extract_next(); + Holder invalidated_properties = msg.extract(); + + if (interface_exists(iface_name)) { + interface_get(iface_name)->signal_property_changed(changed_properties, invalidated_properties); + handled = true; + } + } else + + // ! This is the only block that should be used to forward messages to interfaces. if (interface_exists(msg.get_interface())) { interface_get(msg.get_interface())->message_handle(msg); handled = true; diff --git a/simpledbus/src/base/Connection.cpp b/simpledbus/src/base/Connection.cpp index acbcfe0..fb77303 100644 --- a/simpledbus/src/base/Connection.cpp +++ b/simpledbus/src/base/Connection.cpp @@ -49,8 +49,7 @@ void Connection::uninit() { SimpleDBus::Message message; do { std::this_thread::sleep_for(std::chrono::milliseconds(10)); - read_write(); - message = pop_message(); + read_write_dispatch(); } while (message.is_valid()); dbus_connection_unref(_conn); @@ -99,18 +98,6 @@ void Connection::remove_match(std::string rule) { } } -void Connection::read_write() { - // TODO: DEPRECATE - if (!_initialized) { - throw Exception::NotInitialized(); - } - - std::lock_guard lock(_mutex); - - // Non blocking read of the next available message - dbus_connection_read_write(_conn, 0); -} - void Connection::read_write_dispatch() { if (!_initialized) { throw Exception::NotInitialized(); @@ -125,18 +112,6 @@ void Connection::read_write_dispatch() { while (dbus_connection_dispatch(_conn) == DBUS_DISPATCH_DATA_REMAINS) {} } -Message Connection::pop_message() { - // TODO: DEPRECATE - if (!_initialized) { - throw Exception::NotInitialized(); - } - - std::lock_guard lock(_mutex); - - DBusMessage* msg = dbus_connection_pop_message(_conn); - return Message::from_acquired(msg); -} - void Connection::send(Message& msg) { if (!_initialized) { throw Exception::NotInitialized();