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