unit test for multi publishers with transfer id map

Author: serges147

include/libcyphal/presentation/client.hpp

@@ -237,13 +237,13 @@ class Client final : public detail::ClientBase
                 // For request (and the following response) we need to allocate a transfer ID,
                 // which will be in use to pair the request with the response.
                 //
-                auto& shared_client   = getSharedClient();
-                auto  opt_transfer_id = shared_client.nextTransferId();
+                auto&      shared_client   = getSharedClient();
+                const auto opt_transfer_id = shared_client.nextTransferId();
                 if (!opt_transfer_id)
                 {
                     return TooManyPendingRequestsError{};
                 }
-                const auto transfer_id = *opt_transfer_id;
+                const auto transfer_id = opt_transfer_id.value();
 
                 // Create and register a response promise object, which will be used to handle the response.
                 // Its done specifically before sending the request, so that we will be ready to handle a response
@@ -325,13 +325,13 @@ class RawServiceClient final : public detail::ClientBase
         // 1. For request (and following response) we need to allocate a transfer ID,
         //    which will be in use to pair the request with the response.
         //
-        auto& shared_client   = getSharedClient();
-        auto  opt_transfer_id = shared_client.nextTransferId();
+        auto&      shared_client   = getSharedClient();
+        const auto opt_transfer_id = shared_client.nextTransferId();
         if (!opt_transfer_id)
         {
             return TooManyPendingRequestsError{};
         }
-        const auto transfer_id = *opt_transfer_id;
+        const auto transfer_id = opt_transfer_id.value();
 
         // 2. Create and register a response promise object, which will be used to handle the response.
         //    Its done specifically before sending the request, so that we will be ready to handle a response

include/libcyphal/presentation/client_impl.hpp

@@ -36,7 +36,7 @@ namespace detail
 
 class SharedClient : public common::cavl::Node<SharedClient>,
                      public SharedObject,
-                     public transport::detail::ITransferIdStorage
+                     protected transport::detail::ITransferIdStorage
 {
 public:
     using Node::remove;

include/libcyphal/presentation/publisher_impl.hpp

@@ -33,7 +33,9 @@ namespace presentation
 namespace detail
 {
 
-class PublisherImpl final : public common::cavl::Node<PublisherImpl>, public SharedObject
+class PublisherImpl final : public common::cavl::Node<PublisherImpl>,
+                            public SharedObject,
+                            private transport::detail::ITransferIdStorage
 {
 public:
     using Node::remove;
@@ -44,6 +46,7 @@ class PublisherImpl final : public common::cavl::Node<PublisherImpl>, public Sha
         , msg_tx_session_{std::move(msg_tx_session)}
         , subject_id_{msg_tx_session_->getParams().subject_id}
         , next_transfer_id_{0}
+        , transfer_id_generator_{*this}
     {
         CETL_DEBUG_ASSERT(msg_tx_session_ != nullptr, "");
 
@@ -69,11 +72,11 @@ class PublisherImpl final : public common::cavl::Node<PublisherImpl>, public Sha
 
     cetl::optional<transport::AnyFailure> publishRawData(const TimePoint                   deadline,
                                                          const transport::Priority         priority,
-                                                         const transport::PayloadFragments payload_fragments)
+                                                         const transport::PayloadFragments payload_fragments) const
     {
-        next_transfer_id_ += 1;
-        const transport::TransferTxMetadata metadata{{next_transfer_id_, priority}, deadline};
+        const auto transfer_id = transfer_id_generator_.nextTransferId();
 
+        const transport::TransferTxMetadata metadata{{transfer_id, priority}, deadline};
         return msg_tx_session_->send(metadata, payload_fragments);
     }
 
@@ -114,12 +117,25 @@ class PublisherImpl final : public common::cavl::Node<PublisherImpl>, public Sha
         destroyWithPmr(this, delegate_.memory());
     }
 
+    // MARK: ITransferIdStorage
+
+    transport::TransferId load() const noexcept override
+    {
+        return next_transfer_id_;
+    }
+
+    void save(const transport::TransferId transfer_id) noexcept override
+    {
+        next_transfer_id_ = transfer_id;
+    }
+
     // MARK: Data members:
 
     IPresentationDelegate&                        delegate_;
     const UniquePtr<transport::IMessageTxSession> msg_tx_session_;
     const transport::PortId                       subject_id_;
     transport::TransferId                         next_transfer_id_;
+    transport::detail::TrivialTransferIdGenerator transfer_id_generator_;
 
 };  // PublisherImpl
 

test/unittest/application/node/test_heartbeat_producer.cpp

@@ -126,7 +126,7 @@ TEST_F(TestHeartbeatProducer, make)
     });
     scheduler_.scheduleAt(3s, [&](const auto&) {
         //
-        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{1, Priority::Nominal}, now() + 1s}), _))  //
+        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{0, Priority::Nominal}, now() + 1s}), _))  //
             .WillOnce(Return(cetl::nullopt));
     });
     scheduler_.scheduleAt(3s + 500ms, [&](const auto&) {
@@ -143,12 +143,12 @@ TEST_F(TestHeartbeatProducer, make)
     });
     scheduler_.scheduleAt(4s, [&](const auto&) {
         //
-        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{2, Priority::Nominal}, now() + 1s}), _))  //
+        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{1, Priority::Nominal}, now() + 1s}), _))  //
             .WillOnce(Return(cetl::nullopt));
     });
     scheduler_.scheduleAt(5s, [&](const auto&) {
         //
-        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{3, Priority::Nominal}, now() + 1s}), _))  //
+        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{2, Priority::Nominal}, now() + 1s}), _))  //
             .WillOnce(Return(cetl::nullopt));
     });
     scheduler_.scheduleAt(5s + 500ms, [&](const auto&) {
@@ -161,7 +161,7 @@ TEST_F(TestHeartbeatProducer, make)
     });
     scheduler_.scheduleAt(6s, [&](const auto&) {
         //
-        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{4, Priority::Nominal}, now() + 1s}), _))  //
+        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{3, Priority::Nominal}, now() + 1s}), _))  //
             .WillOnce(Return(cetl::nullopt));
     });
     scheduler_.scheduleAt(6s + 500ms, [&](const auto&) {
@@ -173,12 +173,12 @@ TEST_F(TestHeartbeatProducer, make)
     });
     scheduler_.scheduleAt(7s, [&](const auto&) {
         //
-        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{5, Priority::Nominal}, now() + 1s}), _))  //
+        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{4, Priority::Nominal}, now() + 1s}), _))  //
             .WillOnce(Return(cetl::nullopt));
     });
     scheduler_.scheduleAt(8s, [&](const auto&) {
         //
-        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{6, Priority::Nominal}, now() + 1s}), _))  //
+        EXPECT_CALL(msg_tx_session_mock, send(TransferTxMetadataEq({{5, Priority::Nominal}, now() + 1s}), _))  //
             .WillOnce(Return(cetl::nullopt));
 
         heartbeat_producer->message().health.value = uavcan::node::Health_1_0::CAUTION;

test/unittest/presentation/test_publisher.cpp

@@ -7,6 +7,7 @@
 #include "my_custom/bar_1_0.hpp"
 #include "tracking_memory_resource.hpp"
 #include "transport/msg_sessions_mock.hpp"
+#include "transport/transfer_id_map_mock.hpp"
 #include "transport/transport_gtest_helpers.hpp"
 #include "transport/transport_mock.hpp"
 #include "verification_utilities.hpp"
@@ -17,6 +18,7 @@
 #include <libcyphal/presentation/publisher.hpp>
 #include <libcyphal/transport/errors.hpp>
 #include <libcyphal/transport/msg_sessions.hpp>
+#include <libcyphal/transport/transfer_id_map.hpp>
 #include <libcyphal/transport/types.hpp>
 #include <libcyphal/types.hpp>
 
@@ -176,7 +178,7 @@ TEST_F(TestPublisher, publish)
         EXPECT_CALL(msg_tx_session_mock, send(_, _))  //
             .WillOnce(Invoke([now = now()](const auto& metadata, const auto) {
                 //
-                EXPECT_THAT(metadata.base.transfer_id, 1);
+                EXPECT_THAT(metadata.base.transfer_id, 0);
                 EXPECT_THAT(metadata.base.priority, Priority::Exceptional);
                 EXPECT_THAT(metadata.deadline, now + 200ms);
                 return cetl::nullopt;
@@ -189,7 +191,7 @@ TEST_F(TestPublisher, publish)
         EXPECT_CALL(msg_tx_session_mock, send(_, _))  //
             .WillOnce(Invoke([now = now()](const auto& metadata, const auto) {
                 //
-                EXPECT_THAT(metadata.base.transfer_id, 2);
+                EXPECT_THAT(metadata.base.transfer_id, 1);
                 EXPECT_THAT(metadata.base.priority, Priority::Fast);
                 EXPECT_THAT(metadata.deadline, now + 100ms);
                 return cetl::nullopt;
@@ -283,7 +285,7 @@ TEST_F(TestPublisher, publishRawData)
         EXPECT_CALL(msg_tx_session_mock, send(_, _))  //
             .WillOnce(Invoke([now = now()](const auto& metadata, const auto frags) {
                 //
-                EXPECT_THAT(metadata.base.transfer_id, 1);
+                EXPECT_THAT(metadata.base.transfer_id, 0);
                 EXPECT_THAT(metadata.base.priority, Priority::Nominal);
                 EXPECT_THAT(metadata.deadline, now + 200ms);
                 EXPECT_THAT(frags.size(), 1);
@@ -303,6 +305,103 @@ TEST_F(TestPublisher, publishRawData)
     scheduler_.spinFor(10s);
 }
 
+TEST_F(TestPublisher, multiple_publishers_with_transfer_id_map)
+{
+    using Message     = uavcan::node::Heartbeat_1_0;
+    using SessionSpec = ITransferIdMap::SessionSpec;
+
+    StrictMock<TransferIdMapMock> transfer_id_map_mock;
+
+    Presentation presentation{mr_, scheduler_, transport_mock_};
+    presentation.setTransferIdMap(&transfer_id_map_mock);
+
+    EXPECT_CALL(transport_mock_, getLocalNodeId())  //
+        .WillOnce(Return(cetl::nullopt));
+
+    StrictMock<MessageTxSessionMock>   msg7_tx_session_mock;
+    constexpr MessageTxParams          tx7_params{7};
+    cetl::optional<Publisher<Message>> publisher7;
+    {
+        EXPECT_CALL(msg7_tx_session_mock, getParams()).WillOnce(Return(tx7_params));
+
+        EXPECT_CALL(transport_mock_, makeMessageTxSession(MessageTxParamsEq(tx7_params)))  //
+            .WillOnce(Invoke([&](const auto&) {                                            //
+                return libcyphal::detail::makeUniquePtr<UniquePtrMsgTxSpec>(mr_, msg7_tx_session_mock);
+            }));
+
+        auto maybe_pub7 = presentation.makePublisher<Message>(tx7_params.subject_id);
+        ASSERT_THAT(maybe_pub7, VariantWith<Publisher<Message>>(_));
+
+        publisher7.emplace(cetl::get<Publisher<Message>>(std::move(maybe_pub7)));
+    }
+
+    constexpr NodeId local_node_id = 0x13;
+    EXPECT_CALL(transport_mock_, getLocalNodeId())  //
+        .WillRepeatedly(Return(cetl::optional<NodeId>{NodeId{local_node_id}}));
+
+    StrictMock<MessageTxSessionMock>   msg9_tx_session_mock;
+    constexpr MessageTxParams          tx9_params{9};
+    cetl::optional<Publisher<Message>> publisher9;
+    {
+        EXPECT_CALL(msg9_tx_session_mock, getParams()).WillOnce(Return(tx9_params));
+
+        EXPECT_CALL(transport_mock_, makeMessageTxSession(MessageTxParamsEq(tx9_params)))  //
+            .WillOnce(Invoke([&](const auto&) {                                            //
+                return libcyphal::detail::makeUniquePtr<UniquePtrMsgTxSpec>(mr_, msg9_tx_session_mock);
+            }));
+
+        EXPECT_CALL(transfer_id_map_mock, getIdFor(SessionSpec{tx9_params.subject_id, local_node_id}))
+            .WillOnce(Return(90));
+
+        auto maybe_pub9 = presentation.makePublisher<Message>(tx9_params.subject_id);
+        ASSERT_THAT(maybe_pub9, VariantWith<Publisher<Message>>(_));
+
+        publisher9.emplace(cetl::get<Publisher<Message>>(std::move(maybe_pub9)));
+    }
+
+    scheduler_.scheduleAt(1s, [&](const auto&) {
+        //
+        EXPECT_CALL(msg7_tx_session_mock, send(_, _))  //
+            .WillOnce(Invoke([now = now()](const auto& metadata, const auto) {
+                //
+                EXPECT_THAT(metadata.base.transfer_id, 0);
+                return cetl::nullopt;
+            }));
+
+        EXPECT_THAT(publisher7->publish(now() + 200ms, Message{&mr_}), Eq(cetl::nullopt));
+    });
+    scheduler_.scheduleAt(2s, [&](const auto&) {
+        //
+        EXPECT_CALL(msg9_tx_session_mock, send(_, _))  //
+            .WillOnce(Invoke([now = now()](const auto& metadata, const auto) {
+                //
+                EXPECT_THAT(metadata.base.transfer_id, 90);
+                return cetl::nullopt;
+            }));
+
+        EXPECT_THAT(publisher9->publish(now() + 200ms, Message{&mr_}), Eq(cetl::nullopt));
+    });
+    scheduler_.scheduleAt(8s, [&](const auto&) {
+        //
+        EXPECT_CALL(transfer_id_map_mock, setIdFor(SessionSpec{tx9_params.subject_id, local_node_id}, 90 + 1))  //
+            .WillOnce(Return());
+
+        publisher9.reset();
+        testing::Mock::VerifyAndClearExpectations(&msg9_tx_session_mock);
+        EXPECT_CALL(msg9_tx_session_mock, deinit()).Times(1);
+    });
+    scheduler_.scheduleAt(9s, [&](const auto&) {
+        //
+        EXPECT_CALL(transfer_id_map_mock, setIdFor(SessionSpec{tx7_params.subject_id, local_node_id}, 1))  //
+            .WillOnce(Return());
+
+        publisher7.reset();
+        testing::Mock::VerifyAndClearExpectations(&msg7_tx_session_mock);
+        EXPECT_CALL(msg7_tx_session_mock, deinit()).Times(1);
+    });
+    scheduler_.spinFor(10s);
+}
+
 // NOLINTEND(cppcoreguidelines-avoid-magic-numbers, readability-magic-numbers)
 
 }  // namespace

test/unittest/presentation/test_publisher_custom_config.cpp

@@ -110,7 +110,7 @@ TEST_F(TestPublisherCustomConfig, publish_with_custom_buffer_size)
         EXPECT_CALL(msg_tx_session_mock, send(_, _))  //
             .WillOnce(Invoke([now = now()](const auto& metadata, const auto) {
                 //
-                EXPECT_THAT(metadata.base.transfer_id, 1);
+                EXPECT_THAT(metadata.base.transfer_id, 0);
                 EXPECT_THAT(metadata.base.priority, Priority::Exceptional);
                 EXPECT_THAT(metadata.deadline, now + 200ms);
                 return cetl::nullopt;
@@ -123,7 +123,7 @@ TEST_F(TestPublisherCustomConfig, publish_with_custom_buffer_size)
         EXPECT_CALL(msg_tx_session_mock, send(_, _))  //
             .WillOnce(Invoke([now = now()](const auto& metadata, const auto) {
                 //
-                EXPECT_THAT(metadata.base.transfer_id, 2);
+                EXPECT_THAT(metadata.base.transfer_id, 1);
                 EXPECT_THAT(metadata.base.priority, Priority::Fast);
                 EXPECT_THAT(metadata.deadline, now + 100ms);
                 return cetl::nullopt;