diff --git a/include/libcyphal/presentation/client_impl.hpp b/include/libcyphal/presentation/client_impl.hpp index 87315bd89..d09e9b1fe 100644 --- a/include/libcyphal/presentation/client_impl.hpp +++ b/include/libcyphal/presentation/client_impl.hpp @@ -37,6 +37,9 @@ namespace detail class SharedClient : public cavl::Node, public SharedObject { public: + using Node::remove; + using Node::isLinked; + class TimeoutNode : public Node { public: @@ -229,13 +232,20 @@ class SharedClient : public cavl::Node, public SharedObject CETL_DEBUG_ASSERT(cb_nodes_by_transfer_id_.empty(), ""); CETL_DEBUG_ASSERT(timeout_nodes_by_deadline_.empty(), ""); - delegate_.releaseSharedClient(this); + delegate_.markSharedObjAsUnreferenced(*this); return true; } return false; } + // MARK: SharedObject + + void destroy() noexcept override + { + delegate_.forgetSharedClient(*this); + } + protected: virtual void insertNewCallbackNode(CallbackNode& callback_node) { @@ -393,7 +403,7 @@ class SharedClient : public cavl::Node, public SharedObject /// @brief Defines a shared client implementation that uses a generic transfer ID generator. /// template -class ClientImpl final : public SharedClient, public TransferIdGeneratorMixin +class ClientImpl final : public SharedClient, private TransferIdGeneratorMixin { public: ClientImpl(IPresentationDelegate& delegate, @@ -410,10 +420,13 @@ class ClientImpl final : public SharedClient, public TransferIdGeneratorMixin void destroy() noexcept override { + Base::destroy(); destroyWithPmr(this, memory()); } private: + using Base = SharedClient; + // MARK: SharedClient CETL_NODISCARD cetl::optional nextTransferId() noexcept override @@ -440,14 +453,14 @@ class ClientImpl final : public SharedClient, public TransferIdGeneratorMixin template <> class ClientImpl final : public SharedClient, - public transport::detail::TrivialTransferIdGenerator + private transport::detail::TrivialTransferIdGenerator { public: ClientImpl(IPresentationDelegate& delegate, IExecutor& executor, UniquePtr svc_request_tx_session, UniquePtr svc_response_rx_session) - : SharedClient{delegate, executor, std::move(svc_request_tx_session), std::move(svc_response_rx_session)} + : Base{delegate, executor, std::move(svc_request_tx_session), std::move(svc_response_rx_session)} { } @@ -455,10 +468,13 @@ class ClientImpl final void destroy() noexcept override { + Base::destroy(); destroyWithPmr(this, memory()); } private: + using Base = SharedClient; + // MARK: SharedClient CETL_NODISCARD cetl::optional nextTransferId() noexcept override diff --git a/include/libcyphal/presentation/presentation.hpp b/include/libcyphal/presentation/presentation.hpp index 91192629a..2bd25ad7a 100644 --- a/include/libcyphal/presentation/presentation.hpp +++ b/include/libcyphal/presentation/presentation.hpp @@ -97,7 +97,10 @@ struct IsFixedPortIdMessageTrait /// Instance of this class is supposed to be created once per transport instance (or even per application). /// Main purpose of the presentation object is to create publishers, subscribers, and RPC clients and servers. /// -class Presentation final : private detail::IPresentationDelegate +/// No Sonar cpp:S4963 'The "Rule-of-Zero" should be followed' +/// b/c we do directly handle resources here. +/// +class Presentation final : private detail::IPresentationDelegate // NOSONAR cpp:S4963 { public: /// @brief Defines failure type of various `make...` methods of the presentation layer. @@ -112,7 +115,30 @@ class Presentation final : private detail::IPresentationDelegate : memory_{memory} , executor_{executor} , transport_{transport} + , unreferenced_nodes_{&unreferenced_nodes_, &unreferenced_nodes_} + { + unref_nodes_deleter_callback_ = executor_.registerCallback([this](const auto&) { + // + destroyUnreferencedNodes(); + }); + CETL_DEBUG_ASSERT(unref_nodes_deleter_callback_, "Should not fail b/c we pass proper lambda."); + } + + Presentation(const Presentation&) = delete; + Presentation(Presentation&&) noexcept = delete; + Presentation& operator=(const Presentation&) = delete; + Presentation& operator=(Presentation&&) noexcept = delete; + + ~Presentation() { + destroyUnreferencedNodes(); + + CETL_DEBUG_ASSERT(shared_client_nodes_.empty(), // + "RPC clients must be destroyed before presentation."); + CETL_DEBUG_ASSERT(publisher_impl_nodes_.empty(), // + "Message publishers must be destroyed before presentation."); + CETL_DEBUG_ASSERT(subscriber_impl_nodes_.empty(), // + "Message subscribers must be destroyed before presentation."); } /// @brief Gets reference to the executor instance of this presentation object. @@ -161,6 +187,11 @@ class Presentation final : private detail::IPresentationDelegate auto* const publisher_impl = std::get<0>(publisher_existing); CETL_DEBUG_ASSERT(publisher_impl != nullptr, ""); + // This publisher impl node might be in the list of previously unreferenced nodes - + // the ones that are going to be deleted asynchronously (by the `destroyUnreferencedNodes`). + // If it's the case, we need to remove it from the list b/c it's going to be referenced. + publisher_impl->unlinkIfReferenced(); + return Publisher{publisher_impl}; } @@ -456,6 +487,8 @@ class Presentation final : private detail::IPresentationDelegate } private: + using Schedule = IExecutor::Callback::Schedule; + IPresentationDelegate& asDelegate() noexcept { return static_cast(*this); @@ -517,6 +550,12 @@ class Presentation final : private detail::IPresentationDelegate auto* const subscriber_impl = std::get<0>(subscriber_existing); CETL_DEBUG_ASSERT(subscriber_impl != nullptr, ""); + + // This subscriber impl node might be in the list of previously unreferenced nodes - + // the ones that are going to be deleted asynchronously (by the `destroyUnreferencedNodes`). + // If it's the case, we need to remove it from the list b/c it's going to be referenced. + subscriber_impl->unlinkIfReferenced(); + return subscriber_impl; } @@ -571,6 +610,12 @@ class Presentation final : private detail::IPresentationDelegate auto* const shared_client = std::get<0>(shared_client_existing); CETL_DEBUG_ASSERT(shared_client != nullptr, ""); + + // This client impl node might be in the list of previously unreferenced nodes - + // the ones that are going to be deleted asynchronously (by the `destroyUnreferencedNodes`). + // If it's the case, we need to remove it from the list b/c it's going to be referenced. + shared_client->unlinkIfReferenced(); + return shared_client; } @@ -628,30 +673,59 @@ class Presentation final : private detail::IPresentationDelegate } template - void releaseSharedNode(SharedNode* const shared_node, cavl::Tree& tree) const noexcept + static void forgetSharedNode(SharedNode& shared_node) noexcept { - CETL_DEBUG_ASSERT(shared_node != nullptr, ""); + CETL_DEBUG_ASSERT(shared_node.isLinked(), ""); + CETL_DEBUG_ASSERT(!shared_node.isReferenced(), ""); - // TODO: make it async (deferred to "on idle" callback). - tree.remove(shared_node); - shared_node->destroy(); + // Remove the node from its tree (if it still there), + // as well as from the list of unreferenced nodes (b/c we gonna finally destroy it). + shared_node.remove(); // from the tree + shared_node.unlinkIfReferenced(); // from the list + } + + void destroyUnreferencedNodes() const noexcept + { + // In the loop, destruction of a shared object also removes it from the list of unreferenced nodes. + // So, it implicitly updates the `unreferenced_nodes_` list. + while (unreferenced_nodes_.next_node != &unreferenced_nodes_) + { + // Downcast is safe here b/c every `UnRefNode` instance is always a `SharedObject` one. + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-static-cast-downcast) + auto* const shared_obj = static_cast(unreferenced_nodes_.next_node); + shared_obj->destroy(); + } } // MARK: IPresentationDelegate - void releaseSharedClient(detail::SharedClient* const shared_client) noexcept override + void markSharedObjAsUnreferenced(detail::SharedObject& shared_obj) noexcept override + { + // We are not going to destroy the shared object immediately, but schedule it for deletion. + // This is b/c destruction of shared objects may be time-consuming (f.e. closing under the hood sockets). + // Double-linked list is used to avoid the need to traverse the tree of shared objects. + // + CETL_DEBUG_ASSERT(!shared_obj.isReferenced(), ""); + shared_obj.linkAsUnreferenced(unreferenced_nodes_); + // + const auto result = unref_nodes_deleter_callback_.schedule(Schedule::Once{executor_.now()}); + CETL_DEBUG_ASSERT(result, "Should not fail b/c we never reset `unref_nodes_deleter_callback_`."); + (void) result; + } + + void forgetSharedClient(detail::SharedClient& shared_client) noexcept override { - releaseSharedNode(shared_client, shared_client_nodes_); + forgetSharedNode(shared_client); } - void releasePublisherImpl(detail::PublisherImpl* const publisher_impl) noexcept override + void forgetPublisherImpl(detail::PublisherImpl& publisher_impl) noexcept override { - releaseSharedNode(publisher_impl, publisher_impl_nodes_); + forgetSharedNode(publisher_impl); } - void releaseSubscriberImpl(detail::SubscriberImpl* const subscriber_impl) noexcept override + void forgetSubscriberImpl(detail::SubscriberImpl& subscriber_impl) noexcept override { - releaseSharedNode(subscriber_impl, subscriber_impl_nodes_); + forgetSharedNode(subscriber_impl); } // MARK: Data members: @@ -662,6 +736,8 @@ class Presentation final : private detail::IPresentationDelegate cavl::Tree shared_client_nodes_; cavl::Tree publisher_impl_nodes_; cavl::Tree subscriber_impl_nodes_; + detail::UnRefNode unreferenced_nodes_; + IExecutor::Callback::Any unref_nodes_deleter_callback_; }; // Presentation diff --git a/include/libcyphal/presentation/presentation_delegate.hpp b/include/libcyphal/presentation/presentation_delegate.hpp index b880e1d0c..3c49d74b6 100644 --- a/include/libcyphal/presentation/presentation_delegate.hpp +++ b/include/libcyphal/presentation/presentation_delegate.hpp @@ -6,6 +6,8 @@ #ifndef LIBCYPHAL_PRESENTATION_DELEGATE_HPP_INCLUDED #define LIBCYPHAL_PRESENTATION_DELEGATE_HPP_INCLUDED +#include "shared_object.hpp" + #include #include @@ -84,9 +86,10 @@ class IPresentationDelegate virtual cetl::pmr::memory_resource& memory() const noexcept = 0; - virtual void releaseSharedClient(SharedClient* shared_client) noexcept = 0; - virtual void releasePublisherImpl(PublisherImpl* publisher_impl) noexcept = 0; - virtual void releaseSubscriberImpl(SubscriberImpl* subscriber_impl) noexcept = 0; + virtual void markSharedObjAsUnreferenced(SharedObject& shared_obj) noexcept = 0; + virtual void forgetSharedClient(SharedClient& shared_client) noexcept = 0; + virtual void forgetPublisherImpl(PublisherImpl& publisher_impl) noexcept = 0; + virtual void forgetSubscriberImpl(SubscriberImpl& subscriber_impl) noexcept = 0; protected: IPresentationDelegate() = default; diff --git a/include/libcyphal/presentation/publisher_impl.hpp b/include/libcyphal/presentation/publisher_impl.hpp index f544e8fdd..acf5f0bac 100644 --- a/include/libcyphal/presentation/publisher_impl.hpp +++ b/include/libcyphal/presentation/publisher_impl.hpp @@ -36,6 +36,9 @@ namespace detail class PublisherImpl final : public cavl::Node, public SharedObject { public: + using Node::remove; + using Node::isLinked; + PublisherImpl(IPresentationDelegate& delegate, UniquePtr msg_tx_session) : delegate_{delegate} , msg_tx_session_{std::move(msg_tx_session)} @@ -70,19 +73,22 @@ class PublisherImpl final : public cavl::Node, public SharedObjec { if (SharedObject::release()) { - delegate_.releasePublisherImpl(this); + delegate_.markSharedObjAsUnreferenced(*this); return true; } return false; } +private: + // MARK: SharedObject + void destroy() noexcept override { + delegate_.forgetPublisherImpl(*this); destroyWithPmr(this, delegate_.memory()); } -private: // MARK: Data members: IPresentationDelegate& delegate_; diff --git a/include/libcyphal/presentation/shared_object.hpp b/include/libcyphal/presentation/shared_object.hpp index 62afbba95..73c2f7781 100644 --- a/include/libcyphal/presentation/shared_object.hpp +++ b/include/libcyphal/presentation/shared_object.hpp @@ -25,9 +25,53 @@ namespace presentation namespace detail { +/// @brief Defines double-linked list of unreferenced nodes. +/// +struct UnRefNode +{ + void linkAsUnreferenced(UnRefNode& origin) + { + CETL_DEBUG_ASSERT((prev_node != nullptr) == (next_node != nullptr), "Should be both or none."); + + // Already linked? + if ((nullptr == prev_node) && (nullptr == next_node)) + { + // Link to the end of the list, so that the object is destroyed in the order of unreferencing. + // + next_node = &origin; + prev_node = origin.prev_node; + origin.prev_node->next_node = this; + origin.prev_node = this; + } + } + + void unlinkIfReferenced() + { + CETL_DEBUG_ASSERT((prev_node != nullptr) == (next_node != nullptr), "Should be both or none."); + + // Already unlinked? + if ((nullptr != prev_node) && (nullptr != next_node)) + { + prev_node->next_node = next_node; + next_node->prev_node = prev_node; + + next_node = nullptr; + prev_node = nullptr; + } + } + + // No Lint b/c this `UnRefNode` is a simple helper struct as base of the below `SharedObject`. + // It's under `detail` namespace and not supposed to be used directly by the users of the library. + // NOLINTBEGIN(misc-non-private-member-variables-in-classes) + UnRefNode* prev_node{nullptr}; + UnRefNode* next_node{nullptr}; + // NOLINTEND(misc-non-private-member-variables-in-classes) + +}; // UnRefNode + /// @brief Defines the base class for all classes that need to be shared (using reference count). /// -class SharedObject +class SharedObject : public UnRefNode { public: SharedObject() = default; @@ -38,6 +82,13 @@ class SharedObject SharedObject& operator=(const SharedObject&) = delete; SharedObject& operator=(SharedObject&&) noexcept = delete; + /// @brief Gets boolean indicating whether the object is referenced at least once. + /// + bool isReferenced() const noexcept + { + return ref_count_ > 0; + } + /// @brief Increments the reference count. /// void retain() noexcept diff --git a/include/libcyphal/presentation/subscriber_impl.hpp b/include/libcyphal/presentation/subscriber_impl.hpp index 19434209d..a3b9eb866 100644 --- a/include/libcyphal/presentation/subscriber_impl.hpp +++ b/include/libcyphal/presentation/subscriber_impl.hpp @@ -36,6 +36,9 @@ namespace detail class SubscriberImpl final : public cavl::Node, public SharedObject { public: + using Node::remove; + using Node::isLinked; + class CallbackNode : public Node { public: @@ -257,18 +260,13 @@ class SubscriberImpl final : public cavl::Node, public SharedObj { CETL_DEBUG_ASSERT(callback_nodes_.empty(), ""); - delegate_.releaseSubscriberImpl(this); + delegate_.markSharedObjAsUnreferenced(*this); return true; } return false; } - void destroy() noexcept override - { - destroyWithPmr(this, delegate_.memory()); - } - private: void onMessageRxTransfer(const transport::IMessageRxSession::OnReceiveCallback::Arg& arg) { @@ -292,6 +290,14 @@ class SubscriberImpl final : public cavl::Node, public SharedObj } } + // MARK: SharedObject + + void destroy() noexcept override + { + delegate_.forgetSubscriberImpl(*this); + destroyWithPmr(this, delegate_.memory()); + } + // MARK: Data members: IPresentationDelegate& delegate_; diff --git a/include/libcyphal/transport/can/can_transport_impl.hpp b/include/libcyphal/transport/can/can_transport_impl.hpp index 31c36b5a3..aba1d547a 100644 --- a/include/libcyphal/transport/can/can_transport_impl.hpp +++ b/include/libcyphal/transport/can/can_transport_impl.hpp @@ -581,9 +581,9 @@ class TransportImpl final : private TransportDelegate, public ICanTransport // IMedia::PushResult::Type push_result = media.interface().push(tx_deadline, tx_item->frame.extended_can_id, payload); - // In case of media push error we are going to drop this problematic frame + // In case of media push error, we are going to drop this problematic frame // (b/c it looks like media can't handle this frame), - // but we will continue to process with other transfer frame. + // but we will continue to process with another transfer frame. // Note that media not being ready/able to push a frame just yet (aka temporary) // is not reported as an error (see `is_pushed` below). // diff --git a/test/unittest/application/node/test_heartbeat_producer.cpp b/test/unittest/application/node/test_heartbeat_producer.cpp index d7c1dee91..ead10f4ea 100644 --- a/test/unittest/application/node/test_heartbeat_producer.cpp +++ b/test/unittest/application/node/test_heartbeat_producer.cpp @@ -93,8 +93,6 @@ class TestHeartbeatProducer : public testing::Test TEST_F(TestHeartbeatProducer, make) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_tx_session_mock; constexpr MessageTxParams tx_params{uavcan::node::Heartbeat_1_0::_traits_::FixedPortId}; EXPECT_CALL(msg_tx_session_mock, getParams()).WillOnce(Return(tx_params)); @@ -107,6 +105,8 @@ TEST_F(TestHeartbeatProducer, make) EXPECT_CALL(transport_mock_, getLocalNodeId()) // .WillRepeatedly(Return(cetl::nullopt)); + Presentation presentation{mr_, scheduler_, transport_mock_}; + cetl::optional heartbeat_producer; TimePoint start_time{}; @@ -218,8 +218,6 @@ TEST_F(TestHeartbeatProducer, move) static_assert(!std::is_default_constructible::value, "Should not be default constructible."); - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_tx_session_mock; EXPECT_CALL(msg_tx_session_mock, getParams()) // .WillOnce(Return(MessageTxParams{uavcan::node::Heartbeat_1_0::_traits_::FixedPortId})); @@ -235,6 +233,8 @@ TEST_F(TestHeartbeatProducer, move) EXPECT_CALL(transport_mock_, getLocalNodeId()) // .WillRepeatedly(Return(cetl::optional{NodeId{42U}})); + Presentation presentation{mr_, scheduler_, transport_mock_}; + std::vector calls; cetl::optional heartbeat_producer1; cetl::optional heartbeat_producer2; diff --git a/test/unittest/application/test_node.cpp b/test/unittest/application/test_node.cpp index bebe6d85e..e1f4cd4c4 100644 --- a/test/unittest/application/test_node.cpp +++ b/test/unittest/application/test_node.cpp @@ -145,13 +145,13 @@ class TestNode : public testing::Test TEST_F(TestNode, make) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - setupDefaultExpectations(); EXPECT_CALL(transport_mock_, getLocalNodeId()) // .WillRepeatedly(Return(cetl::optional{NodeId{42U}})); + Presentation presentation{mr_, scheduler_, transport_mock_}; + cetl::optional node; std::vector> calls; @@ -218,8 +218,6 @@ TEST_F(TestNode, move) static_assert(!std::is_copy_constructible::value, "Should not be copy constructible."); static_assert(!std::is_default_constructible::value, "Should not be default constructible."); - Presentation presentation{mr_, scheduler_, transport_mock_}; - setupDefaultExpectations(); EXPECT_CALL(heartbeat_msg_tx_session_mock_, send(_, _)) // @@ -229,6 +227,8 @@ TEST_F(TestNode, move) EXPECT_CALL(transport_mock_, getLocalNodeId()) // .WillRepeatedly(Return(cetl::optional{NodeId{42U}})); + Presentation presentation{mr_, scheduler_, transport_mock_}; + cetl::optional node1; cetl::optional node2; std::vector calls; @@ -257,8 +257,6 @@ TEST_F(TestNode, makeRegistryProvider) using ListService = uavcan::_register::List_1_0; using AccessService = uavcan::_register::Access_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - setupDefaultExpectations(); SvcServerContext list_svc_cnxt; @@ -274,6 +272,8 @@ TEST_F(TestNode, makeRegistryProvider) registry::IntrospectableRegistryMock registry_mock; + Presentation presentation{mr_, scheduler_, transport_mock_}; + cetl::optional node; scheduler_.scheduleAt(1s, [&](const auto&) { @@ -298,8 +298,6 @@ TEST_F(TestNode, makeRegistryProvider) TEST_F(TestNode, makeRegistryProvider_failure) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - setupDefaultExpectations(); EXPECT_CALL(heartbeat_msg_tx_session_mock_, send(_, _)) // @@ -310,14 +308,28 @@ TEST_F(TestNode, makeRegistryProvider_failure) registry::IntrospectableRegistryMock registry_mock; - auto maybe_node = Node::make(presentation); - ASSERT_THAT(maybe_node, VariantWith(_)); - auto node = cetl::get(std::move(maybe_node)); + Presentation presentation{mr_, scheduler_, transport_mock_}; - EXPECT_CALL(transport_mock_, makeRequestRxSession(_)) // - .WillOnce(Return(libcyphal::ArgumentError{})); + cetl::optional node; - EXPECT_THAT(node.makeRegistryProvider(registry_mock), Optional(VariantWith(_))); + scheduler_.scheduleAt(2s, [&](const auto&) { + // + auto maybe_node = Node::make(presentation); + ASSERT_THAT(maybe_node, VariantWith(_)); + node.emplace(cetl::get(std::move(maybe_node))); + }); + scheduler_.scheduleAt(2s, [&](const auto&) { + // + EXPECT_CALL(transport_mock_, makeRequestRxSession(_)) // + .WillOnce(Return(libcyphal::ArgumentError{})); + + EXPECT_THAT(node->makeRegistryProvider(registry_mock), Optional(VariantWith(_))); + }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + node.reset(); + }); + scheduler_.spinFor(10s); } // NOLINTEND(cppcoreguidelines-avoid-magic-numbers, readability-magic-numbers) diff --git a/test/unittest/presentation/test_client.cpp b/test/unittest/presentation/test_client.cpp index c44dc861f..57fb17d37 100644 --- a/test/unittest/presentation/test_client.cpp +++ b/test/unittest/presentation/test_client.cpp @@ -160,14 +160,14 @@ TEST_F(TestClient, copy_move_getSetPriority) static_assert(std::is_move_constructible::value, "Should be move constructible."); static_assert(!std::is_default_constructible::value, "Should not be default constructible."); - Presentation presentation{mr_, scheduler_, transport_mock_}; - constexpr ResponseRxParams rx_params{Service::Response::_traits_::ExtentBytes, Service::Request::_traits_::FixedPortId, 0x31}; const State state{mr_, transport_mock_, rx_params}; + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_client1 = presentation.makeClient(rx_params.server_node_id); ASSERT_THAT(maybe_client1, VariantWith>(_)); auto client1a = cetl::get>(std::move(maybe_client1)); @@ -202,14 +202,14 @@ TEST_F(TestClient, request_response_get_fetch_result) using Service = uavcan::node::GetInfo_1_0; using SvcResPromise = ResponsePromise; - Presentation presentation{mr_, scheduler_, transport_mock_}; - constexpr ResponseRxParams rx_params{Service::Response::_traits_::ExtentBytes, Service::Request::_traits_::FixedPortId, 0x31}; State state{mr_, transport_mock_, rx_params}; + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_client = presentation.makeClient(rx_params.server_node_id); ASSERT_THAT(maybe_client, VariantWith>(_)); const auto client = cetl::get>(std::move(maybe_client)); @@ -269,17 +269,17 @@ TEST_F(TestClient, request_response_via_callabck) using Service = uavcan::node::GetInfo_1_0; using SvcResPromise = ResponsePromise; - Presentation presentation{mr_, scheduler_, transport_mock_}; - constexpr ResponseRxParams rx_params{Service::Response::_traits_::ExtentBytes, Service::Request::_traits_::FixedPortId, 0x31}; State state{mr_, transport_mock_, rx_params}; + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_client = presentation.makeClient(rx_params.server_node_id); ASSERT_THAT(maybe_client, VariantWith>(_)); - auto client = cetl::get>(std::move(maybe_client)); + cetl::optional> client = cetl::get>(std::move(maybe_client)); constexpr TransferId transfer_id = 0; ASSERT_TRUE(state.res_rx_cb_fn_); @@ -303,7 +303,7 @@ TEST_F(TestClient, request_response_via_callabck) return cetl::nullopt; })); - auto maybe_promise = client.request(now() + 100ms, Service::Request{mr_alloc_}, now() + 500ms); + auto maybe_promise = client->request(now() + 100ms, Service::Request{mr_alloc_}, now() + 500ms); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise.emplace(cetl::get(std::move(maybe_promise))); }); @@ -358,7 +358,7 @@ TEST_F(TestClient, request_response_via_callabck) // EXPECT_CALL(state.req_tx_session_mock_, send(_, _)).WillOnce(Return(cetl::nullopt)); - auto maybe_promise = client.request(now() + 100ms, Service::Request{mr_alloc_}, now() + 500ms); + auto maybe_promise = client->request(now() + 100ms, Service::Request{mr_alloc_}, now() + 500ms); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise.emplace(cetl::get(std::move(maybe_promise))); @@ -376,6 +376,11 @@ TEST_F(TestClient, request_response_via_callabck) EXPECT_THAT(response_promise->getResult(), Optional(VariantWith(_))); EXPECT_THAT(response_promise->fetchResult(), Optional(VariantWith(_))); }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + client.reset(); + response_promise.reset(); + }); scheduler_.spinFor(10s); EXPECT_THAT(responses, @@ -388,17 +393,17 @@ TEST_F(TestClient, request_response_set_callabck_after_reception) using Service = uavcan::node::GetInfo_1_0; using SvcResPromise = ResponsePromise; - Presentation presentation{mr_, scheduler_, transport_mock_}; - constexpr ResponseRxParams rx_params{Service::Response::_traits_::ExtentBytes, Service::Request::_traits_::FixedPortId, 0x31}; State state{mr_, transport_mock_, rx_params}; + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_client = presentation.makeClient(rx_params.server_node_id); ASSERT_THAT(maybe_client, VariantWith>(_)); - auto client = cetl::get>(std::move(maybe_client)); + cetl::optional> client = cetl::get>(std::move(maybe_client)); constexpr TransferId transfer_id = 0; ASSERT_TRUE(state.res_rx_cb_fn_); @@ -422,7 +427,7 @@ TEST_F(TestClient, request_response_set_callabck_after_reception) return cetl::nullopt; })); - auto maybe_promise = client.request(now() + 100ms, Service::Request{mr_alloc_}, now() + 200ms); + auto maybe_promise = client->request(now() + 100ms, Service::Request{mr_alloc_}, now() + 200ms); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise.emplace(cetl::get(std::move(maybe_promise))); }); @@ -441,6 +446,11 @@ TEST_F(TestClient, request_response_set_callabck_after_reception) responses.emplace_back(success.metadata, arg.approx_now); }); }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + client.reset(); + response_promise.reset(); + }); scheduler_.spinFor(10s); EXPECT_THAT(responses, @@ -456,8 +466,6 @@ TEST_F(TestClient, request_response_failures) StrictMock mr_mock; mr_mock.redirectExpectedCallsTo(mr_); - Presentation presentation{mr_mock, scheduler_, transport_mock_}; - constexpr ResponseRxParams rx_params{Service::Response::_traits_::ExtentBytes, 147, 0x31}; State state{mr_, transport_mock_, rx_params}; @@ -466,9 +474,11 @@ TEST_F(TestClient, request_response_failures) // This will make the client fail to make more than 2 request. EXPECT_CALL(transport_mock_, getProtocolParams()).WillRepeatedly(Return(ProtocolParams{2, 0, 0})); + Presentation presentation{mr_mock, scheduler_, transport_mock_}; + auto maybe_client = presentation.makeClient(rx_params.server_node_id, rx_params.service_id); ASSERT_THAT(maybe_client, VariantWith>(_)); - const auto client = cetl::get>(std::move(maybe_client)); + cetl::optional> client = cetl::get>(std::move(maybe_client)); constexpr TransferId transfer_id = 0; cetl::optional response_promise; @@ -481,25 +491,25 @@ TEST_F(TestClient, request_response_failures) Service::Request request{mr_alloc_}; request.some_stuff = Service::Request::_traits_::TypeOf::some_stuff{mr_alloc_}; request.some_stuff.resize(32); // this will make it fail to serialize - const auto maybe_promise = client.request(now() + 100ms, request); + const auto maybe_promise = client->request(now() + 100ms, request); EXPECT_THAT(maybe_promise, VariantWith::Failure>( VariantWith(nunavut::support::Error::SerializationBadArrayLength))); }); scheduler_.scheduleAt(2s, [&](const auto&) { // - // Emulate problem with sending the request. + // Emulate a problem with sending the request. EXPECT_CALL(state.req_tx_session_mock_, send(_, _)) // .WillOnce(Return(CapacityError{})); - const auto maybe_promise = client.request(now() + 100ms, Service::Request{mr_alloc_}); + const auto maybe_promise = client->request(now() + 100ms, Service::Request{mr_alloc_}); EXPECT_THAT(maybe_promise, VariantWith::Failure>(VariantWith(_))); }); scheduler_.scheduleAt(3s, [&](const auto&) { // EXPECT_CALL(state.req_tx_session_mock_, send(_, _)).WillOnce(Return(cetl::nullopt)); - auto maybe_promise = client.request(now() + 100ms, Service::Request{mr_alloc_}); + auto maybe_promise = client->request(now() + 100ms, Service::Request{mr_alloc_}); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise.emplace(cetl::get(std::move(maybe_promise))); @@ -530,7 +540,7 @@ TEST_F(TestClient, request_response_failures) EXPECT_CALL(state.req_tx_session_mock_, send(_, _)).WillOnce(Return(cetl::nullopt)); - auto maybe_promise = client.request(now() + 100ms, Service::Request{mr_alloc_}); + auto maybe_promise = client->request(now() + 100ms, Service::Request{mr_alloc_}); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise.emplace(cetl::get(std::move(maybe_promise))); @@ -553,17 +563,22 @@ TEST_F(TestClient, request_response_failures) // EXPECT_CALL(state.req_tx_session_mock_, send(_, _)).WillRepeatedly(Return(cetl::nullopt)); - const auto maybe_promise1 = client.request(now() + 100ms, Service::Request{mr_alloc_}); + const auto maybe_promise1 = client->request(now() + 100ms, Service::Request{mr_alloc_}); EXPECT_THAT(maybe_promise1, VariantWith(_)); - const auto maybe_promise2 = client.request(now() + 100ms, Service::Request{mr_alloc_}); + const auto maybe_promise2 = client->request(now() + 100ms, Service::Request{mr_alloc_}); EXPECT_THAT(maybe_promise2, VariantWith(_)); - const auto maybe_promise3 = client.request(now() + 100ms, Service::Request{mr_alloc_}); + const auto maybe_promise3 = client->request(now() + 100ms, Service::Request{mr_alloc_}); EXPECT_THAT(maybe_promise3, VariantWith::Failure>( VariantWith::TooManyPendingRequestsError>(_))); }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + client.reset(); + response_promise.reset(); + }); scheduler_.spinFor(10s); } @@ -572,17 +587,17 @@ TEST_F(TestClient, multiple_requests_responses_expired) using Service = uavcan::node::GetInfo_1_0; using SvcResPromise = ResponsePromise; - Presentation presentation{mr_, scheduler_, transport_mock_}; - constexpr ResponseRxParams rx_params{Service::Response::_traits_::ExtentBytes, Service::Request::_traits_::FixedPortId, 0x31}; State state{mr_, transport_mock_, rx_params}; + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_client = presentation.makeClient(rx_params.server_node_id); ASSERT_THAT(maybe_client, VariantWith>(_)); - auto client = cetl::get>(std::move(maybe_client)); + cetl::optional> client = cetl::get>(std::move(maybe_client)); constexpr TransferId transfer_id = 0; cetl::optional response_promise1; @@ -598,7 +613,7 @@ TEST_F(TestClient, multiple_requests_responses_expired) EXPECT_CALL(state.req_tx_session_mock_, send(TransferTxMetadataEq(meta), _)) // .WillOnce(Return(cetl::nullopt)); - auto maybe_promise = client.request(now() + 100ms, Service::Request{mr_alloc_}, now() + 4s); + auto maybe_promise = client->request(now() + 100ms, Service::Request{mr_alloc_}, now() + 4s); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise1.emplace(cetl::get(std::move(maybe_promise))); response_promise1->setCallback([&responses](const auto& arg) { @@ -611,12 +626,12 @@ TEST_F(TestClient, multiple_requests_responses_expired) }); scheduler_.scheduleAt(2s, [&](const auto&) { // - client.setPriority(Priority::Fast); - const TransferTxMetadata meta{{transfer_id + 1, client.getPriority()}, now() + 200ms}; + client->setPriority(Priority::Fast); + const TransferTxMetadata meta{{transfer_id + 1, client->getPriority()}, now() + 200ms}; EXPECT_CALL(state.req_tx_session_mock_, send(TransferTxMetadataEq(meta), _)) // .WillOnce(Return(cetl::nullopt)); - auto maybe_promise = client.request(now() + 200ms, Service::Request{mr_alloc_}, now() + 2s); + auto maybe_promise = client->request(now() + 200ms, Service::Request{mr_alloc_}, now() + 2s); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise2.emplace(cetl::get(std::move(maybe_promise))); response_promise2->setCallback([&responses](const auto& arg) { @@ -629,11 +644,11 @@ TEST_F(TestClient, multiple_requests_responses_expired) }); scheduler_.scheduleAt(3s, [&](const auto&) { // - const TransferTxMetadata meta{{transfer_id + 2, client.getPriority()}, now() + 300ms}; + const TransferTxMetadata meta{{transfer_id + 2, client->getPriority()}, now() + 300ms}; EXPECT_CALL(state.req_tx_session_mock_, send(TransferTxMetadataEq(meta), _)) // .WillOnce(Return(cetl::nullopt)); - auto maybe_promise = client.request(now() + 300ms, Service::Request{mr_alloc_}, now() + 1s); + auto maybe_promise = client->request(now() + 300ms, Service::Request{mr_alloc_}, now() + 1s); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise3.emplace(cetl::get(std::move(maybe_promise))); response_promise3->setCallback([&responses](const auto& arg) { @@ -646,11 +661,11 @@ TEST_F(TestClient, multiple_requests_responses_expired) }); scheduler_.scheduleAt(4s, [&](const auto&) { // - const TransferTxMetadata meta{{transfer_id + 3, client.getPriority()}, now() + 400ms}; + const TransferTxMetadata meta{{transfer_id + 3, client->getPriority()}, now() + 400ms}; EXPECT_CALL(state.req_tx_session_mock_, send(TransferTxMetadataEq(meta), _)) // .WillOnce(Return(cetl::nullopt)); - auto maybe_promise = client.request(now() + 400ms, Service::Request{mr_alloc_}, now() + 2s); + auto maybe_promise = client->request(now() + 400ms, Service::Request{mr_alloc_}, now() + 2s); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise4.emplace(cetl::get(std::move(maybe_promise))); response_promise4->setCallback([](const auto&) { @@ -674,15 +689,15 @@ TEST_F(TestClient, raw_request_response_via_callabck) { using SvcResPromise = ResponsePromise; - Presentation presentation{mr_, scheduler_, transport_mock_}; - constexpr ResponseRxParams rx_params{16, 147, 0x31}; State state{mr_, transport_mock_, rx_params}; + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_client = presentation.makeClient(rx_params.server_node_id, rx_params.service_id, rx_params.extent_bytes); ASSERT_THAT(maybe_client, VariantWith(_)); - auto client = cetl::get(std::move(maybe_client)); + cetl::optional client = cetl::get(std::move(maybe_client)); constexpr TransferId transfer_id = 0; ASSERT_TRUE(state.res_rx_cb_fn_); @@ -706,7 +721,7 @@ TEST_F(TestClient, raw_request_response_via_callabck) return cetl::nullopt; })); - auto maybe_promise = client.request(now() + 100ms, {}, now() + 2s); + auto maybe_promise = client->request(now() + 100ms, {}, now() + 2s); EXPECT_THAT(maybe_promise, VariantWith(_)); response_promise.emplace(cetl::get(std::move(maybe_promise))); }); @@ -753,6 +768,11 @@ TEST_F(TestClient, raw_request_response_via_callabck) responses.emplace_back(success.response.size(), success.metadata, arg.approx_now); }); }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + client.reset(); + response_promise.reset(); + }); scheduler_.spinFor(10s); EXPECT_THAT(responses, @@ -764,8 +784,6 @@ TEST_F(TestClient, raw_request_response_failures) { using SvcResPromise = ResponsePromise; - Presentation presentation{mr_, scheduler_, transport_mock_}; - constexpr ResponseRxParams rx_params{4, 147, 0x31}; State state{mr_, transport_mock_, rx_params}; @@ -774,34 +792,40 @@ TEST_F(TestClient, raw_request_response_failures) // This will make the client fail to make more than 2 request. EXPECT_CALL(transport_mock_, getProtocolParams()).WillRepeatedly(Return(ProtocolParams{2, 0, 0})); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_client = presentation.makeClient(rx_params.server_node_id, rx_params.service_id, rx_params.extent_bytes); ASSERT_THAT(maybe_client, VariantWith(_)); - const auto client = cetl::get(std::move(maybe_client)); + cetl::optional client = cetl::get(std::move(maybe_client)); scheduler_.scheduleAt(1s, [&](const auto&) { // - // Emulate problem with sending the request. + // Emulate a problem with sending the request. EXPECT_CALL(state.req_tx_session_mock_, send(_, _)) // .WillOnce(Return(CapacityError{})); - const auto maybe_promise = client.request(now() + 100ms, {}); + const auto maybe_promise = client->request(now() + 100ms, {}); EXPECT_THAT(maybe_promise, VariantWith(VariantWith(_))); }); scheduler_.scheduleAt(2s, [&](const auto&) { // EXPECT_CALL(state.req_tx_session_mock_, send(_, _)).WillRepeatedly(Return(cetl::nullopt)); - const auto maybe_promise1 = client.request(now() + 100ms, {}); + const auto maybe_promise1 = client->request(now() + 100ms, {}); EXPECT_THAT(maybe_promise1, VariantWith(_)); - const auto maybe_promise2 = client.request(now() + 100ms, {}); + const auto maybe_promise2 = client->request(now() + 100ms, {}); EXPECT_THAT(maybe_promise2, VariantWith(_)); - const auto maybe_promise3 = client.request(now() + 100ms, {}); + const auto maybe_promise3 = client->request(now() + 100ms, {}); EXPECT_THAT(maybe_promise3, VariantWith( VariantWith(_))); }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + client.reset(); + }); scheduler_.spinFor(10s); } diff --git a/test/unittest/presentation/test_presentation.cpp b/test/unittest/presentation/test_presentation.cpp index b82cdda74..6387f48dd 100644 --- a/test/unittest/presentation/test_presentation.cpp +++ b/test/unittest/presentation/test_presentation.cpp @@ -215,8 +215,6 @@ TEST_F(TestPresentation, makePublisher) { using Message = uavcan::node::Heartbeat_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_tx_session_mock; constexpr MessageTxParams tx_params{Message::_traits_::FixedPortId}; EXPECT_CALL(msg_tx_session_mock, getParams()).WillOnce(Return(tx_params)); @@ -226,6 +224,8 @@ TEST_F(TestPresentation, makePublisher) return libcyphal::detail::makeUniquePtr(mr_, msg_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_pub1 = presentation.makePublisher(tx_params.subject_id); EXPECT_THAT(maybe_pub1, VariantWith>(_)); @@ -239,8 +239,6 @@ TEST_F(TestPresentation, makePublisher_custom) { using Message = Custom::PubMessage; - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_tx_session_mock; constexpr MessageTxParams tx_params{Message::_traits_::FixedPortId}; EXPECT_CALL(msg_tx_session_mock, getParams()).WillOnce(Return(tx_params)); @@ -250,6 +248,8 @@ TEST_F(TestPresentation, makePublisher_custom) return libcyphal::detail::makeUniquePtr(mr_, msg_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_pub = presentation.makePublisher(); ASSERT_THAT(maybe_pub, VariantWith>(_)); auto publisher_copy = cetl::get>(maybe_pub); @@ -262,8 +262,6 @@ TEST_F(TestPresentation, makePublisher_custom) TEST_F(TestPresentation, makePublisher_raw) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_tx_session_mock; constexpr MessageTxParams tx_params{147}; EXPECT_CALL(msg_tx_session_mock, getParams()).WillOnce(Return(tx_params)); @@ -273,6 +271,8 @@ TEST_F(TestPresentation, makePublisher_raw) return libcyphal::detail::makeUniquePtr(mr_, msg_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_pub = presentation.makePublisher(tx_params.subject_id); EXPECT_THAT(maybe_pub, VariantWith>(_)); @@ -328,8 +328,6 @@ TEST_F(TestPresentation, makeSubscriber) { using Message = uavcan::node::Heartbeat_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_rx_session_mock; constexpr MessageRxParams rx_params{Message::_traits_::ExtentBytes, Message::_traits_::FixedPortId}; EXPECT_CALL(msg_rx_session_mock, getParams()).WillOnce(Return(rx_params)); @@ -340,6 +338,8 @@ TEST_F(TestPresentation, makeSubscriber) return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_sub1 = presentation.makeSubscriber(rx_params.subject_id); EXPECT_THAT(maybe_sub1, VariantWith>(_)); @@ -353,8 +353,6 @@ TEST_F(TestPresentation, makeSubscriber_custom) { using Message = Custom::SubMessage; - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_rx_session_mock; constexpr MessageRxParams rx_params{Message::_traits_::ExtentBytes, Message::_traits_::FixedPortId}; EXPECT_CALL(msg_rx_session_mock, getParams()).WillOnce(Return(rx_params)); @@ -365,6 +363,8 @@ TEST_F(TestPresentation, makeSubscriber_custom) return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_sub = presentation.makeSubscriber([](const auto&) {}); EXPECT_THAT(maybe_sub, VariantWith>(_)); @@ -373,8 +373,6 @@ TEST_F(TestPresentation, makeSubscriber_custom) TEST_F(TestPresentation, makeSubscriber_raw) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_rx_session_mock; constexpr MessageRxParams rx_params{0, 147}; EXPECT_CALL(msg_rx_session_mock, getParams()).WillOnce(Return(rx_params)); @@ -385,6 +383,8 @@ TEST_F(TestPresentation, makeSubscriber_raw) return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_sub1 = presentation.makeSubscriber(rx_params.subject_id, rx_params.extent_bytes); EXPECT_THAT(maybe_sub1, VariantWith>(_)); @@ -449,8 +449,6 @@ TEST_F(TestPresentation, makeServer) { using Service = uavcan::node::GetInfo_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock res_tx_session_mock; StrictMock req_rx_session_mock; EXPECT_CALL(req_rx_session_mock, setOnReceiveCallback(_)) // @@ -468,6 +466,8 @@ TEST_F(TestPresentation, makeServer) return libcyphal::detail::makeUniquePtr(mr_, res_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_server = presentation.makeServer(); ASSERT_THAT(maybe_server, VariantWith>(_)); @@ -479,8 +479,6 @@ TEST_F(TestPresentation, makeServer_custom) { using Service = Custom::Service; - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock res_tx_session_mock; StrictMock req_rx_session_mock; EXPECT_CALL(req_rx_session_mock, setOnReceiveCallback(_)) // @@ -498,6 +496,8 @@ TEST_F(TestPresentation, makeServer_custom) return libcyphal::detail::makeUniquePtr(mr_, res_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_server = presentation.makeServer([](const auto&, auto) {}); ASSERT_THAT(maybe_server, VariantWith>(_)); @@ -507,8 +507,6 @@ TEST_F(TestPresentation, makeServer_custom) TEST_F(TestPresentation, makeServer_raw) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock res_tx_session_mock; StrictMock req_rx_session_mock; EXPECT_CALL(req_rx_session_mock, setOnReceiveCallback(_)) // @@ -525,6 +523,8 @@ TEST_F(TestPresentation, makeServer_raw) return libcyphal::detail::makeUniquePtr(mr_, res_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_server = presentation.makeServer(rx_params.service_id, rx_params.extent_bytes, [](const auto&, auto) {}); ASSERT_THAT(maybe_server, VariantWith(_)); @@ -589,8 +589,6 @@ TEST_F(TestPresentation, makeClient) { using Service = uavcan::node::GetInfo_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock res_rx_session_mock; StrictMock req_tx_session_mock; @@ -614,6 +612,8 @@ TEST_F(TestPresentation, makeClient) return libcyphal::detail::makeUniquePtr(mr_, req_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_client = presentation.makeClient(rx_params.server_node_id); ASSERT_THAT(maybe_client, VariantWith>(_)); @@ -623,28 +623,30 @@ TEST_F(TestPresentation, makeClient) TEST_F(TestPresentation, makeClient_multiple_custom) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - - StrictMock res_rx_session_mock; - StrictMock req_tx_session_mock; + StrictMock res_rx_session_mock1; + StrictMock res_rx_session_mock2; + StrictMock req_tx_session_mock1; + StrictMock req_tx_session_mock2; constexpr ResponseRxParams rx_params{Custom::Service::Response::_traits_::ExtentBytes, Custom::Service::Request::_traits_::FixedPortId, 0x31}; - EXPECT_CALL(res_rx_session_mock, getParams()).WillOnce(Return(rx_params)); - EXPECT_CALL(res_rx_session_mock, setTransferIdTimeout(_)).WillOnce(Return()); - EXPECT_CALL(res_rx_session_mock, setOnReceiveCallback(_)).WillOnce(Return()); + EXPECT_CALL(res_rx_session_mock1, getParams()).WillOnce(Return(rx_params)); + EXPECT_CALL(res_rx_session_mock1, setTransferIdTimeout(_)).WillOnce(Return()); + EXPECT_CALL(res_rx_session_mock1, setOnReceiveCallback(_)).WillOnce(Return()); EXPECT_CALL(transport_mock_, makeResponseRxSession(ResponseRxParamsEq(rx_params))) // .WillOnce(Invoke([&](const auto&) { // - return libcyphal::detail::makeUniquePtr(mr_, res_rx_session_mock); + return libcyphal::detail::makeUniquePtr(mr_, res_rx_session_mock1); })); constexpr RequestTxParams tx_params{rx_params.service_id, rx_params.server_node_id}; EXPECT_CALL(transport_mock_, makeRequestTxSession(RequestTxParamsEq(tx_params))) // .WillOnce(Invoke([&](const auto&) { // - return libcyphal::detail::makeUniquePtr(mr_, req_tx_session_mock); + return libcyphal::detail::makeUniquePtr(mr_, req_tx_session_mock1); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_client1a = presentation.makeClient(rx_params.server_node_id); ASSERT_THAT(maybe_client1a, VariantWith>(_)); @@ -653,9 +655,6 @@ TEST_F(TestPresentation, makeClient_multiple_custom) // The same custom service but to different server { - StrictMock res_rx_session_mock2; - StrictMock req_tx_session_mock2; - constexpr ResponseRxParams rx_params2{rx_params.extent_bytes, rx_params.service_id, 0x32}; EXPECT_CALL(res_rx_session_mock2, getParams()).WillOnce(Return(rx_params2)); EXPECT_CALL(res_rx_session_mock2, setTransferIdTimeout(_)).WillOnce(Return()); @@ -678,14 +677,12 @@ TEST_F(TestPresentation, makeClient_multiple_custom) EXPECT_CALL(req_tx_session_mock2, deinit()).Times(1); } - EXPECT_CALL(res_rx_session_mock, deinit()).Times(1); - EXPECT_CALL(req_tx_session_mock, deinit()).Times(1); + EXPECT_CALL(res_rx_session_mock1, deinit()).Times(1); + EXPECT_CALL(req_tx_session_mock1, deinit()).Times(1); } TEST_F(TestPresentation, makeClient_raw) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock res_rx_session_mock; StrictMock req_tx_session_mock; @@ -704,7 +701,10 @@ TEST_F(TestPresentation, makeClient_raw) return libcyphal::detail::makeUniquePtr(mr_, req_tx_session_mock); })); - auto maybe_client = presentation.makeClient(rx_params.server_node_id, rx_params.service_id, rx_params.extent_bytes); + Presentation presentation{mr_, scheduler_, transport_mock_}; + + auto maybe_client = + presentation.makeClient(rx_params.server_node_id, rx_params.service_id, rx_params.extent_bytes); ASSERT_THAT(maybe_client, VariantWith(_)); EXPECT_CALL(req_tx_session_mock, deinit()).Times(1); diff --git a/test/unittest/presentation/test_publisher.cpp b/test/unittest/presentation/test_publisher.cpp index 1a103371d..b76e8ca04 100644 --- a/test/unittest/presentation/test_publisher.cpp +++ b/test/unittest/presentation/test_publisher.cpp @@ -108,8 +108,6 @@ TEST_F(TestPublisher, copy_move_getSetPriority) static_assert(std::is_move_constructible>::value, "Should be move constructible."); static_assert(!std::is_default_constructible>::value, "Should not be default constructible."); - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_tx_session_mock; constexpr MessageTxParams tx_params{Message::_traits_::FixedPortId}; EXPECT_CALL(msg_tx_session_mock, getParams()).WillOnce(Return(tx_params)); @@ -119,6 +117,8 @@ TEST_F(TestPublisher, copy_move_getSetPriority) return libcyphal::detail::makeUniquePtr(mr_, msg_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_pub1 = presentation.makePublisher(tx_params.subject_id); ASSERT_THAT(maybe_pub1, VariantWith>(_)); auto pub1a = cetl::get>(std::move(maybe_pub1)); @@ -207,9 +207,9 @@ TEST_F(TestPublisher, publish) }); scheduler_.scheduleAt(9s, [&](const auto&) { // - EXPECT_CALL(msg_tx_session_mock, deinit()).Times(1); publisher.reset(); testing::Mock::VerifyAndClearExpectations(&msg_tx_session_mock); + EXPECT_CALL(msg_tx_session_mock, deinit()).Times(1); }); scheduler_.spinFor(10s); } @@ -219,8 +219,6 @@ TEST_F(TestPublisher, publish_with_serialization_failure) using SerError = nunavut::support::Error; using Message = my_custom::bar_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_tx_session_mock; constexpr MessageTxParams tx_params{0x123}; EXPECT_CALL(msg_tx_session_mock, getParams()).WillOnce(Return(tx_params)); @@ -230,6 +228,8 @@ TEST_F(TestPublisher, publish_with_serialization_failure) return libcyphal::detail::makeUniquePtr(mr_, msg_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_pub = presentation.makePublisher(tx_params.subject_id); ASSERT_THAT(maybe_pub, VariantWith>(_)); cetl::optional> publisher{cetl::get>(std::move(maybe_pub))}; @@ -248,17 +248,15 @@ TEST_F(TestPublisher, publish_with_serialization_failure) }); scheduler_.scheduleAt(9s, [&](const auto&) { // - EXPECT_CALL(msg_tx_session_mock, deinit()).Times(1); publisher.reset(); testing::Mock::VerifyAndClearExpectations(&msg_tx_session_mock); + EXPECT_CALL(msg_tx_session_mock, deinit()).Times(1); }); scheduler_.spinFor(10s); } TEST_F(TestPublisher, publishRawData) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - static_assert(std::is_copy_assignable>::value, "Should be copy assignable."); static_assert(std::is_move_assignable>::value, "Should be move assignable."); static_assert(std::is_copy_constructible>::value, "Should be copy constructible."); @@ -274,6 +272,8 @@ TEST_F(TestPublisher, publishRawData) return libcyphal::detail::makeUniquePtr(mr_, msg_tx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_pub = presentation.makePublisher(tx_params.subject_id); ASSERT_THAT(maybe_pub, VariantWith>(_)); cetl::optional> publisher{cetl::get>(std::move(maybe_pub))}; @@ -296,9 +296,9 @@ TEST_F(TestPublisher, publishRawData) }); scheduler_.scheduleAt(9s, [&](const auto&) { // - EXPECT_CALL(msg_tx_session_mock, deinit()).Times(1); publisher.reset(); testing::Mock::VerifyAndClearExpectations(&msg_tx_session_mock); + EXPECT_CALL(msg_tx_session_mock, deinit()).Times(1); }); scheduler_.spinFor(10s); } diff --git a/test/unittest/presentation/test_subscriber.cpp b/test/unittest/presentation/test_subscriber.cpp index 64017d152..105c837e9 100644 --- a/test/unittest/presentation/test_subscriber.cpp +++ b/test/unittest/presentation/test_subscriber.cpp @@ -116,8 +116,6 @@ TEST_F(TestSubscriber, move) static_assert(std::is_move_constructible>::value, "Should be move constructible."); static_assert(!std::is_default_constructible>::value, "Should not be default constructible."); - Presentation presentation{mr_, scheduler_, transport_mock_}; - StrictMock msg_rx_session_mock; constexpr MessageRxParams rx_params{Message::_traits_::ExtentBytes, Message::_traits_::FixedPortId}; EXPECT_CALL(msg_rx_session_mock, getParams()).WillOnce(Return(rx_params)); @@ -128,6 +126,8 @@ TEST_F(TestSubscriber, move) return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_sub1 = presentation.makeSubscriber(rx_params.subject_id); ASSERT_THAT(maybe_sub1, VariantWith>(_)); auto sub1a = cetl::get>(std::move(maybe_sub1)); @@ -140,17 +140,14 @@ TEST_F(TestSubscriber, move) sub1b = std::move(sub2); - EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); sub1b.reset(); - testing::Mock::VerifyAndClearExpectations(&msg_rx_session_mock); + EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); } TEST_F(TestSubscriber, onReceive) { using Message = uavcan::node::Heartbeat_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - IMessageRxSession::OnReceiveCallback::Function msg_rx_cb_fn; StrictMock msg_rx_session_mock; @@ -166,9 +163,12 @@ TEST_F(TestSubscriber, onReceive) return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_sub = presentation.makeSubscriber(rx_params.subject_id); ASSERT_THAT(maybe_sub, VariantWith>(_)); - auto subscriber = cetl::get>(std::move(maybe_sub)); + cetl::optional> subscriber; + subscriber = cetl::get>(std::move(maybe_sub)); ASSERT_TRUE(msg_rx_cb_fn); @@ -192,7 +192,7 @@ TEST_F(TestSubscriber, onReceive) })); std::vector> messages; - subscriber.setOnReceiveCallback([&](const auto& arg) { + subscriber->setOnReceiveCallback([&](const auto& arg) { // messages.emplace_back(arg.approx_now, arg.metadata.rx_meta.base.transfer_id, arg.message.uptime); EXPECT_THAT(arg.metadata.rx_meta.base.priority, Priority::Fast); @@ -226,16 +226,20 @@ TEST_F(TestSubscriber, onReceive) scheduler_.scheduleAt(4s, [&](const auto&) { // // Cancel callback, so there should be no msg reception #4. - subscriber.setOnReceiveCallback({}); + subscriber->setOnReceiveCallback({}); msg_rx_cb_fn({transfer}); }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + subscriber.reset(); + EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); + }); scheduler_.spinFor(10s); EXPECT_THAT(messages, ElementsAre(std::make_tuple(TimePoint{1s}, 123, 7), std::make_tuple(TimePoint{2s}, 124, 8), std::make_tuple(TimePoint{3s}, 125, 9))); - EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); } TEST_F(TestSubscriber, onReceive_deserialize_failure) @@ -245,8 +249,6 @@ TEST_F(TestSubscriber, onReceive_deserialize_failure) StrictMock mr_mock; mr_mock.redirectExpectedCallsTo(mr_); - Presentation presentation{mr_mock, scheduler_, transport_mock_}; - IMessageRxSession::OnReceiveCallback::Function msg_rx_cb_fn; StrictMock msg_rx_session_mock; @@ -262,9 +264,11 @@ TEST_F(TestSubscriber, onReceive_deserialize_failure) return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_mock, scheduler_, transport_mock_}; + auto maybe_sub = presentation.makeSubscriber(rx_params.subject_id); ASSERT_THAT(maybe_sub, VariantWith>(_)); - auto subscriber = cetl::get>(std::move(maybe_sub)); + cetl::optional> subscriber = cetl::get>(std::move(maybe_sub)); EXPECT_TRUE(msg_rx_cb_fn); @@ -282,7 +286,7 @@ TEST_F(TestSubscriber, onReceive_deserialize_failure) })); std::vector> messages; - subscriber.setOnReceiveCallback([&](const auto& arg) { + subscriber->setOnReceiveCallback([&](const auto& arg) { // messages.emplace_back(arg.approx_now, arg.metadata.rx_meta.base.transfer_id); EXPECT_THAT(arg.metadata.rx_meta.base.priority, Priority::Fast); @@ -327,16 +331,18 @@ TEST_F(TestSubscriber, onReceive_deserialize_failure) transfer.metadata.rx_meta.timestamp = now(); msg_rx_cb_fn({transfer}); }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + subscriber.reset(); + EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); + }); scheduler_.spinFor(10s); EXPECT_THAT(messages, ElementsAre(std::make_tuple(TimePoint{3s}, 16))); - EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); } TEST_F(TestSubscriber, onReceive_raw_message) { - Presentation presentation{mr_, scheduler_, transport_mock_}; - IMessageRxSession::OnReceiveCallback::Function msg_rx_cb_fn; StrictMock msg_rx_session_mock; @@ -352,9 +358,11 @@ TEST_F(TestSubscriber, onReceive_raw_message) return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_raw_sub = presentation.makeSubscriber(rx_params.subject_id, rx_params.extent_bytes); ASSERT_THAT(maybe_raw_sub, VariantWith>(_)); - auto raw_subscriber = cetl::get>(std::move(maybe_raw_sub)); + cetl::optional> raw_subscriber = cetl::get>(std::move(maybe_raw_sub)); EXPECT_TRUE(msg_rx_cb_fn); @@ -364,7 +372,7 @@ TEST_F(TestSubscriber, onReceive_raw_message) EXPECT_CALL(storage_mock, copy(_, _, _)).WillRepeatedly(Return(0)); std::vector> messages; - raw_subscriber.setOnReceiveCallback([&](const auto& arg) { + raw_subscriber->setOnReceiveCallback([&](const auto& arg) { // messages.emplace_back(arg.approx_now, arg.metadata.rx_meta.base.transfer_id); EXPECT_THAT(arg.metadata.rx_meta.base.priority, Priority::Fast); @@ -394,24 +402,26 @@ TEST_F(TestSubscriber, onReceive_raw_message) scheduler_.scheduleAt(4s, [&](const auto&) { // // Cancel callback, so there should be no msg reception #4. - raw_subscriber.setOnReceiveCallback({}); + raw_subscriber->setOnReceiveCallback({}); msg_rx_cb_fn({transfer}); }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + raw_subscriber.reset(); + EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); + }); scheduler_.spinFor(10s); EXPECT_THAT(messages, ElementsAre(std::make_tuple(TimePoint{1s}, 123), std::make_tuple(TimePoint{2s}, 124), std::make_tuple(TimePoint{3s}, 125))); - EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); } TEST_F(TestSubscriber, onReceive_release_same_subject_subscriber_during_callback) { using Message = my_custom::bar_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - IMessageRxSession::OnReceiveCallback::Function msg_rx_cb_fn; StrictMock msg_rx_session_mock; @@ -427,6 +437,8 @@ TEST_F(TestSubscriber, onReceive_release_same_subject_subscriber_during_callback return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_sub_a = presentation.makeSubscriber(rx_params.subject_id); ASSERT_THAT(maybe_sub_a, VariantWith>(_)); cetl::optional> sub_a = cetl::get>(std::move(maybe_sub_a)); @@ -503,8 +515,6 @@ TEST_F(TestSubscriber, onReceive_move_same_subject_subscriber_during_callback) { using Message = my_custom::bar_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - IMessageRxSession::OnReceiveCallback::Function msg_rx_cb_fn; StrictMock msg_rx_session_mock; @@ -520,6 +530,8 @@ TEST_F(TestSubscriber, onReceive_move_same_subject_subscriber_during_callback) return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_sub1 = presentation.makeSubscriber(rx_params.subject_id); ASSERT_THAT(maybe_sub1, VariantWith>(_)); cetl::optional> sub1 = cetl::get>(std::move(maybe_sub1)); @@ -558,6 +570,12 @@ TEST_F(TestSubscriber, onReceive_move_same_subject_subscriber_during_callback) transfer.metadata.rx_meta.timestamp = now(); msg_rx_cb_fn({transfer}); }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + sub1.reset(); + sub2a.reset(); + EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); + }); scheduler_.spinFor(10s); EXPECT_THAT(messages, @@ -565,15 +583,12 @@ TEST_F(TestSubscriber, onReceive_move_same_subject_subscriber_during_callback) std::make_tuple("#2", TimePoint{1s}, 43), std::make_tuple("#1", TimePoint{2s}, 44), std::make_tuple("#2", TimePoint{2s}, 44))); - EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); } TEST_F(TestSubscriber, onReceive_append_same_subject_subscriber_during_callback) { using Message = my_custom::bar_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - IMessageRxSession::OnReceiveCallback::Function msg_rx_cb_fn; StrictMock msg_rx_session_mock; @@ -589,6 +604,8 @@ TEST_F(TestSubscriber, onReceive_append_same_subject_subscriber_during_callback) return libcyphal::detail::makeUniquePtr(mr_, msg_rx_session_mock); })); + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_msg_sub1 = presentation.makeSubscriber(rx_params.subject_id); ASSERT_THAT(maybe_msg_sub1, VariantWith>(_)); cetl::optional> msg_sub1 = cetl::get>(std::move(maybe_msg_sub1)); @@ -703,8 +720,6 @@ TEST_F(TestSubscriber, onReceive_different_type_deserializers_on_same_subject) using BarMsg = my_custom::bar_1_0; using HeartbeatMsg = uavcan::node::Heartbeat_1_0; - Presentation presentation{mr_, scheduler_, transport_mock_}; - IMessageRxSession::OnReceiveCallback::Function msg_rx_cb_fn; StrictMock msg_rx_session_mock; @@ -722,31 +737,33 @@ TEST_F(TestSubscriber, onReceive_different_type_deserializers_on_same_subject) std::vector> messages; + Presentation presentation{mr_, scheduler_, transport_mock_}; + auto maybe_bar_sub1 = presentation.makeSubscriber(rx_params.subject_id); ASSERT_THAT(maybe_bar_sub1, VariantWith>(_)); - auto bar_sub1 = cetl::get>(std::move(maybe_bar_sub1)); - bar_sub1.setOnReceiveCallback([&](const auto& arg) { + cetl::optional> bar_sub1 = cetl::get>(std::move(maybe_bar_sub1)); + bar_sub1->setOnReceiveCallback([&](const auto& arg) { // messages.emplace_back("Bar_#1", arg.approx_now, arg.metadata.rx_meta.base.transfer_id); }); auto maybe_bar_sub2 = presentation.makeSubscriber(rx_params.subject_id); ASSERT_THAT(maybe_bar_sub2, VariantWith>(_)); - auto bar_sub2 = cetl::get>(std::move(maybe_bar_sub2)); - bar_sub2.setOnReceiveCallback([&](const auto& arg) { + cetl::optional> bar_sub2 = cetl::get>(std::move(maybe_bar_sub2)); + bar_sub2->setOnReceiveCallback([&](const auto& arg) { // messages.emplace_back("Bar_#2", arg.approx_now, arg.metadata.rx_meta.base.transfer_id); }); auto maybe_hb_sub = presentation.makeSubscriber(rx_params.subject_id); ASSERT_THAT(maybe_hb_sub, VariantWith>(_)); - auto hb_sub = cetl::get>(std::move(maybe_hb_sub)); - hb_sub.setOnReceiveCallback([&](const auto& arg) { + cetl::optional> hb_sub = cetl::get>(std::move(maybe_hb_sub)); + hb_sub->setOnReceiveCallback([&](const auto& arg) { // messages.emplace_back("Heartbeat", arg.approx_now, arg.metadata.rx_meta.base.transfer_id); }); auto maybe_raw_sub = presentation.makeSubscriber(rx_params.subject_id, 0x200); ASSERT_THAT(maybe_raw_sub, VariantWith>(_)); - auto raw_sub = cetl::get>(std::move(maybe_raw_sub)); - raw_sub.setOnReceiveCallback([&](const auto& arg) { + cetl::optional> raw_sub = cetl::get>(std::move(maybe_raw_sub)); + raw_sub->setOnReceiveCallback([&](const auto& arg) { // messages.emplace_back("Raw Msg", arg.approx_now, arg.metadata.rx_meta.base.transfer_id); });