From b81a91c2f7908d9d75f0bfafcda2feabcc8b4114 Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Tue, 9 May 2023 16:21:05 -0700 Subject: [PATCH] [systems/lcm] Use shared_ptr for LCM serializers For a given message type, it's OK to use one const serializer across all systems that publish or subscribe for efficiency. This also allows us to greatly simplify the Python bindings. Deprecate the now-useless SerializerInterface::Clone. --- bindings/pydrake/systems/_lcm_extra.py | 11 +++- bindings/pydrake/systems/lcm_py.cc | 59 ++++++++++++------- bindings/pydrake/systems/lcm_pybind.h | 7 ++- bindings/pydrake/systems/test/lcm_test.py | 6 +- systems/lcm/BUILD.bazel | 9 +++ systems/lcm/lcm_publisher_system.cc | 4 +- systems/lcm/lcm_publisher_system.h | 6 +- systems/lcm/lcm_subscriber_system.cc | 2 +- systems/lcm/lcm_subscriber_system.h | 4 +- systems/lcm/serializer.h | 9 ++- .../lcm/test/serializer_deprecated_test.cc | 30 ++++++++++ systems/lcm/test/serializer_test.cc | 8 --- 12 files changed, 106 insertions(+), 49 deletions(-) create mode 100644 systems/lcm/test/serializer_deprecated_test.cc diff --git a/bindings/pydrake/systems/_lcm_extra.py b/bindings/pydrake/systems/_lcm_extra.py index 986a653ae5c7..7e26a94d2746 100644 --- a/bindings/pydrake/systems/_lcm_extra.py +++ b/bindings/pydrake/systems/_lcm_extra.py @@ -1,7 +1,8 @@ # See `ExecuteExtraPythonCode` in `pydrake_pybind.h` for usage details and # rationale. -from pydrake.common.value import AbstractValue +from pydrake.common.value import AbstractValue as _AbstractValue +from pydrake.common.deprecation import deprecated as _deprecated class PySerializer(SerializerInterface): @@ -16,18 +17,22 @@ def __init__(self, lcm_type): def __repr__(self): return f"PySerializer({self._lcm_type.__name__})" + @_deprecated( + "PySerializer objects are immutable, there is no need to copy nor " + "clone them.", date="2023-09-01") def Clone(self): + """(Deprecated.)""" return PySerializer(self._lcm_type) def CreateDefaultValue(self): - return AbstractValue.Make(self._lcm_type()) + return _AbstractValue.Make(self._lcm_type()) def Deserialize(self, buffer, abstract_value): message = self._lcm_type.decode(buffer) abstract_value.set_value(message) def Serialize(self, abstract_value): - assert isinstance(abstract_value, AbstractValue) + assert isinstance(abstract_value, _AbstractValue) message = abstract_value.get_value() assert isinstance(message, self._lcm_type) return message.encode() diff --git a/bindings/pydrake/systems/lcm_py.cc b/bindings/pydrake/systems/lcm_py.cc index 875b7d97a30f..b64359a1306c 100644 --- a/bindings/pydrake/systems/lcm_py.cc +++ b/bindings/pydrake/systems/lcm_py.cc @@ -41,10 +41,13 @@ class PySerializerInterface : public py::wrapper { // `PySerializerInterface`). C++ implementations will use the bindings on the // interface below. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" std::unique_ptr Clone() const override { PYBIND11_OVERLOAD_PURE( std::unique_ptr, SerializerInterface, Clone); } +#pragma GCC diagnostic pop std::unique_ptr CreateDefaultValue() const override { PYBIND11_OVERLOAD_PURE(std::unique_ptr, SerializerInterface, @@ -113,7 +116,8 @@ PYBIND11_MODULE(lcm, m) { { using Class = SerializerInterface; constexpr auto& cls_doc = doc.SerializerInterface; - py::class_ cls(m, "SerializerInterface"); + py::class_> cls( + m, "SerializerInterface"); cls // BR // Adding a constructor permits implementing this interface in Python. .def(py::init( @@ -144,7 +148,24 @@ PYBIND11_MODULE(lcm, m) { message_bytes.size()); }, py::arg("abstract_value"), cls_doc.Serialize.doc); - DefClone(&cls); +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" + constexpr char kCloneDeprecation[] = + "PySerializer objects are immutable, there is no need to copy nor " + "clone them. The deprecated code will be removed from Drake on or " + "after 2023-09-01."; + cls // BR + .def("Clone", WrapDeprecated(kCloneDeprecation, &Class::Clone), + kCloneDeprecation) + .def("__copy__", WrapDeprecated(kCloneDeprecation, &Class::Clone), + kCloneDeprecation) + .def("__deepcopy__", + WrapDeprecated(kCloneDeprecation, + [](const Class* self, py::dict /* memo */) { + return self->Clone(); + }), + kCloneDeprecation); +#pragma GCC diagnostic pop } { @@ -171,36 +192,32 @@ PYBIND11_MODULE(lcm, m) { constexpr auto& cls_doc = doc.LcmPublisherSystem; py::class_> cls(m, "LcmPublisherSystem"); cls // BR - .def(py::init, + .def(py::init, LcmInterfaceSystem*, double>(), py::arg("channel"), py::arg("serializer"), py::arg("lcm"), py::arg("publish_period") = 0.0, - // Keep alive, ownership: `serializer` keeps `self` alive. - py::keep_alive<3, 1>(), // Keep alive, reference: `self` keeps `lcm` alive. py::keep_alive<1, 4>(), cls_doc.ctor.doc_4args) - .def(py::init, - DrakeLcmInterface*, double>(), + .def(py::init, DrakeLcmInterface*, + double>(), py::arg("channel"), py::arg("serializer"), py::arg("lcm"), py::arg("publish_period") = 0.0, - // Keep alive, ownership: `serializer` keeps `self` alive. - py::keep_alive<3, 1>(), // Keep alive, reference: `self` keeps `lcm` alive. py::keep_alive<1, 4>(), cls_doc.ctor.doc_4args) - .def(py::init, + .def(py::init, LcmInterfaceSystem*, const systems::TriggerTypeSet&, double>(), py::arg("channel"), py::arg("serializer"), py::arg("lcm"), py::arg("publish_triggers"), py::arg("publish_period") = 0.0, - // Keep alive, ownership: `serializer` keeps `self` alive. - py::keep_alive<3, 1>(), // Keep alive, reference: `self` keeps `lcm` alive. py::keep_alive<1, 4>(), cls_doc.ctor.doc_4args) - .def(py::init, - DrakeLcmInterface*, const systems::TriggerTypeSet&, double>(), + .def(py::init, DrakeLcmInterface*, + const systems::TriggerTypeSet&, double>(), py::arg("channel"), py::arg("serializer"), py::arg("lcm"), py::arg("publish_triggers"), py::arg("publish_period") = 0.0, - // Keep alive, ownership: `serializer` keeps `self` alive. - py::keep_alive<3, 1>(), // Keep alive, reference: `self` keeps `lcm` alive. py::keep_alive<1, 4>(), cls_doc.ctor.doc_4args); } @@ -209,18 +226,16 @@ PYBIND11_MODULE(lcm, m) { using Class = LcmSubscriberSystem; constexpr auto& cls_doc = doc.LcmSubscriberSystem; py::class_>(m, "LcmSubscriberSystem") - .def(py::init, + .def(py::init, LcmInterfaceSystem*>(), py::arg("channel"), py::arg("serializer"), py::arg("lcm"), - // Keep alive, ownership: `serializer` keeps `self` alive. - py::keep_alive<3, 1>(), // Keep alive, reference: `self` keeps `lcm` alive. py::keep_alive<1, 4>(), doc.LcmSubscriberSystem.ctor.doc) - .def(py::init, + .def(py::init, DrakeLcmInterface*>(), py::arg("channel"), py::arg("serializer"), py::arg("lcm"), - // Keep alive, ownership: `serializer` keeps `self` alive. - py::keep_alive<3, 1>(), // Keep alive, reference: `self` keeps `lcm` alive. py::keep_alive<1, 4>(), doc.LcmSubscriberSystem.ctor.doc) .def("WaitForMessage", &Class::WaitForMessage, diff --git a/bindings/pydrake/systems/lcm_pybind.h b/bindings/pydrake/systems/lcm_pybind.h index 6a332a7d3a2d..a00020c20848 100644 --- a/bindings/pydrake/systems/lcm_pybind.h +++ b/bindings/pydrake/systems/lcm_pybind.h @@ -3,6 +3,7 @@ /// @file /// Helpers for defining C++ LCM type serializers. +#include #include #include @@ -32,9 +33,9 @@ py::object BindCppSerializer(const std::string& lcm_package) { py::object py_type = py::module::import(lcm_package.c_str()).attr(CppType::getTypeName()); py::module lcm_py = py::module::import("pydrake.systems.lcm"); - auto py_cls = - DefineTemplateClassWithDefault, SerializerInterface>( - lcm_py, "_Serializer", py::make_tuple(py_type)); + auto py_cls = DefineTemplateClassWithDefault, + SerializerInterface, std::shared_ptr>>( + lcm_py, "_Serializer", py::make_tuple(py_type)); py_cls.def(py::init()); // We use move here because the type of py_class differs from our declared // return type. diff --git a/bindings/pydrake/systems/test/lcm_test.py b/bindings/pydrake/systems/test/lcm_test.py index f28c9ea62bc4..e485622e0cde 100644 --- a/bindings/pydrake/systems/test/lcm_test.py +++ b/bindings/pydrake/systems/test/lcm_test.py @@ -79,7 +79,8 @@ def test_serializer(self): reconstruct = lcmt_quaternion.decode(raw) self.assert_lcm_equal(reconstruct, model_message) # Check cloning. - cloned_dut = dut.Clone() + with catch_drake_warnings(expected_count=1): + cloned_dut = dut.Clone() fresh_value = dut.CreateDefaultValue().get_value() self.assertIsInstance(fresh_value, lcmt_quaternion) @@ -92,7 +93,8 @@ def test_serializer_cpp(self): def test_serializer_cpp_clone(self): serializer = mut._Serializer_[lcmt_quaternion]() - serializer.Clone().CreateDefaultValue() + with catch_drake_warnings(expected_count=1): + serializer.Clone().CreateDefaultValue() def test_all_serializers_exist(self): """Checks that all of Drake's Python LCM messages have a matching C++ diff --git a/systems/lcm/BUILD.bazel b/systems/lcm/BUILD.bazel index 95b2cc715992..2884cba93fba 100644 --- a/systems/lcm/BUILD.bazel +++ b/systems/lcm/BUILD.bazel @@ -216,4 +216,13 @@ drake_cc_googletest( ], ) +drake_cc_googletest( + name = "serializer_deprecated_test", + copts = ["-Wno-deprecated-declarations"], + deps = [ + ":serializer", + "//lcmtypes:drake_signal", + ], +) + add_lint_tests() diff --git a/systems/lcm/lcm_publisher_system.cc b/systems/lcm/lcm_publisher_system.cc index d79118382859..ed62fd48c608 100644 --- a/systems/lcm/lcm_publisher_system.cc +++ b/systems/lcm/lcm_publisher_system.cc @@ -18,7 +18,7 @@ using systems::TriggerTypeSet; LcmPublisherSystem::LcmPublisherSystem( const std::string& channel, - std::unique_ptr serializer, + std::shared_ptr serializer, DrakeLcmInterface* lcm, const TriggerTypeSet& publish_triggers, double publish_period) @@ -73,7 +73,7 @@ LcmPublisherSystem::LcmPublisherSystem( LcmPublisherSystem::LcmPublisherSystem( const std::string& channel, - std::unique_ptr serializer, + std::shared_ptr serializer, DrakeLcmInterface* lcm, double publish_period) : LcmPublisherSystem(channel, std::move(serializer), lcm, (publish_period > 0.0) ? diff --git a/systems/lcm/lcm_publisher_system.h b/systems/lcm/lcm_publisher_system.h index 9deea7804c44..f5021b9ea1e8 100644 --- a/systems/lcm/lcm_publisher_system.h +++ b/systems/lcm/lcm_publisher_system.h @@ -141,7 +141,7 @@ class LcmPublisherSystem : public LeafSystem { * @pre publish_period is non-negative. */ LcmPublisherSystem(const std::string& channel, - std::unique_ptr serializer, + std::shared_ptr serializer, drake::lcm::DrakeLcmInterface* lcm, double publish_period = 0.0); @@ -173,7 +173,7 @@ class LcmPublisherSystem : public LeafSystem { * @pre publish_triggers contains a subset of {kForced, kPeriodic, kPerStep}. */ LcmPublisherSystem(const std::string& channel, - std::unique_ptr serializer, + std::shared_ptr serializer, drake::lcm::DrakeLcmInterface* lcm, const systems::TriggerTypeSet& publish_triggers, double publish_period = 0.0); @@ -235,7 +235,7 @@ class LcmPublisherSystem : public LeafSystem { InitializationPublisher initialization_publisher_; // Converts Value objects into LCM message bytes. - const std::unique_ptr serializer_; + const std::shared_ptr serializer_; // If we're not given a DrakeLcm object, we allocate one and keep it here. // The unique_ptr is const, not the held object. diff --git a/systems/lcm/lcm_subscriber_system.cc b/systems/lcm/lcm_subscriber_system.cc index a54c344f1569..8fff2b68452f 100644 --- a/systems/lcm/lcm_subscriber_system.cc +++ b/systems/lcm/lcm_subscriber_system.cc @@ -22,7 +22,7 @@ constexpr int kMagic = 6832; // An arbitrary value. LcmSubscriberSystem::LcmSubscriberSystem( const std::string& channel, - std::unique_ptr serializer, + std::shared_ptr serializer, drake::lcm::DrakeLcmInterface* lcm) : channel_(channel), serializer_(std::move(serializer)), diff --git a/systems/lcm/lcm_subscriber_system.h b/systems/lcm/lcm_subscriber_system.h index a817eb02d280..e0e66d25137a 100644 --- a/systems/lcm/lcm_subscriber_system.h +++ b/systems/lcm/lcm_subscriber_system.h @@ -79,7 +79,7 @@ class LcmSubscriberSystem : public LeafSystem { * @param lcm A non-null pointer to the LCM subsystem to subscribe on. */ LcmSubscriberSystem(const std::string& channel, - std::unique_ptr serializer, + std::shared_ptr serializer, drake::lcm::DrakeLcmInterface* lcm); ~LcmSubscriberSystem() override; @@ -134,7 +134,7 @@ class LcmSubscriberSystem : public LeafSystem { // Converts LCM message bytes to Value objects. // Will be non-null iff our output port is abstract-valued. - const std::unique_ptr serializer_; + const std::shared_ptr serializer_; // The mutex that guards received_message_ and received_message_count_. mutable std::mutex received_message_mutex_; diff --git a/systems/lcm/serializer.h b/systems/lcm/serializer.h index ca5f4853069c..928bdd4919c7 100644 --- a/systems/lcm/serializer.h +++ b/systems/lcm/serializer.h @@ -6,6 +6,7 @@ #include "drake/common/drake_assert.h" #include "drake/common/drake_copyable.h" +#include "drake/common/drake_deprecated.h" #include "drake/common/drake_throw.h" #include "drake/common/value.h" @@ -16,8 +17,8 @@ namespace lcm { /** * %SerializerInterface translates between LCM message bytes and * drake::AbstractValue objects that contain LCM messages, e.g., a - * Value. See Serializer for a message-specific concrete - * subclass. + * Value. All `const` methods should be threadsafe. + * See Serializer for a message-specific concrete subclass. */ class SerializerInterface { public: @@ -25,7 +26,9 @@ class SerializerInterface { virtual ~SerializerInterface(); - /** Creates a deep copy of this. */ + DRAKE_DEPRECATED( + "2023-09-01", + "Use a shared_ptr instead of cloning.") virtual std::unique_ptr Clone() const = 0; /** diff --git a/systems/lcm/test/serializer_deprecated_test.cc b/systems/lcm/test/serializer_deprecated_test.cc new file mode 100644 index 000000000000..d238e7b5c30b --- /dev/null +++ b/systems/lcm/test/serializer_deprecated_test.cc @@ -0,0 +1,30 @@ +#include + +#include "drake/common/is_cloneable.h" +#include "drake/lcmt_drake_signal.hpp" +#include "drake/systems/lcm/serializer.h" + +namespace drake { +namespace systems { +namespace lcm { +namespace { + +GTEST_TEST(SerializerDeprecatedTest, BasicTest) { + // The device under test. + auto dut = std::make_unique>(); + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" + // Cloning works. + EXPECT_TRUE(is_cloneable::value); + auto fresh = dut->Clone(); + ASSERT_NE(fresh, nullptr); + auto fresh_value = fresh->CreateDefaultValue(); + EXPECT_EQ(fresh_value->get_value().dim, 0); +#pragma GCC diagnostic pop +} + +} // namespace +} // namespace lcm +} // namespace systems +} // namespace drake diff --git a/systems/lcm/test/serializer_test.cc b/systems/lcm/test/serializer_test.cc index c7617591ae2a..eafe52ace862 100644 --- a/systems/lcm/test/serializer_test.cc +++ b/systems/lcm/test/serializer_test.cc @@ -5,7 +5,6 @@ #include -#include "drake/common/is_cloneable.h" #include "drake/lcm/lcmt_drake_signal_utils.h" #include "drake/lcmt_drake_signal.hpp" @@ -43,13 +42,6 @@ GTEST_TEST(SerializerTest, BasicTest) { abstract_value.get()); EXPECT_TRUE(CompareLcmtDrakeSignalMessages( abstract_value->get_value(), sample_data)); - - // Cloning works. - EXPECT_TRUE(is_cloneable::value); - auto fresh = dut->Clone(); - ASSERT_NE(fresh, nullptr); - auto fresh_value = fresh->CreateDefaultValue(); - EXPECT_EQ(fresh_value->get_value().dim, 0); } } // namespace