Skip to content

Commit 8716d16

Browse files
serges147serges147
authored
Fixes for issues 431 & 399 (#441)
- `libcyphal::MonotonicClock` type definition is moved under `libcyphal::Config` - now it's possible to customize libcyphal `TimePoint` & `Duration` types. - Fixed several missing `std::chrono::duration_cast<Duration>` when converting from lizard µsec to libcyphal time. - Added two new unit tests to ensure that both CAN and UDP transports could be compiled with custom milliseconds-based clock. Without the above `duration_cast` fixes these unit tests are not-compilable.
1 parent e3c0fc1 commit 8716d16

File tree

9 files changed

+225
-37
lines changed

9 files changed

+225
-37
lines changed

‎include/libcyphal/config.hpp‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@
66
#ifndef LIBCYPHAL_CONFIG_HPP_INCLUDED
77
#define LIBCYPHAL_CONFIG_HPP_INCLUDED
88

9+
#include <chrono>
910
#include <cstddef>
11+
#include <cstdint>
1012

1113
namespace libcyphal
1214
{
@@ -24,6 +26,21 @@ namespace libcyphal
2426
///
2527
struct Config
2628
{
29+
/// Defines time representation as 64-bit microseconds.
30+
///
31+
/// This is in line with the lizards that use `uint64_t`-typed microsecond counters throughout.
32+
///
33+
struct MonotonicClock final
34+
{
35+
using rep = std::int64_t;
36+
using period = std::micro;
37+
using duration = std::chrono::duration<rep, period>;
38+
using time_point = std::chrono::time_point<MonotonicClock>;
39+
40+
static constexpr bool is_steady = true;
41+
42+
}; // MonotonicClock
43+
2744
/// Defines max footprint of a callback function in use by the executor.
2845
///
2946
static constexpr std::size_t IExecutor_Callback_FunctionMaxSize() // NOSONAR cpp:S799

‎include/libcyphal/transport/can/can_transport_impl.hpp‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -578,8 +578,8 @@ class TransportImpl final : private TransportDelegate, public ICanTransport
578578
}
579579
const IMedia::PopResult::Metadata& pop_meta = *pop_success;
580580

581-
const auto timestamp_us =
582-
std::chrono::duration_cast<std::chrono::microseconds>(pop_meta.timestamp.time_since_epoch());
581+
const auto timestamp_us = std::chrono::duration_cast<std::chrono::microseconds>( //
582+
pop_meta.timestamp.time_since_epoch());
583583
const CanardFrame canard_frame{pop_meta.can_id, {pop_meta.payload_size, payload.data()}};
584584

585585
CanardRxTransfer out_transfer{};
@@ -605,7 +605,8 @@ class TransportImpl final : private TransportDelegate, public ICanTransport
605605

606606
const auto transfer_id = static_cast<TransferId>(out_transfer.metadata.transfer_id);
607607
const auto priority = static_cast<Priority>(out_transfer.metadata.priority);
608-
const auto timestamp = TimePoint{std::chrono::microseconds{out_transfer.timestamp_usec}};
608+
const auto timestamp = TimePoint{std::chrono::duration_cast<Duration>( //
609+
std::chrono::microseconds{out_transfer.timestamp_usec})};
609610

610611
session_delegate->acceptRxTransfer(CanardMemory{memory(), out_transfer.payload},
611612
TransferRxMetadata{{transfer_id, priority}, timestamp},
@@ -625,9 +626,10 @@ class TransportImpl final : private TransportDelegate, public ICanTransport
625626
&media.interface().getTxMemoryResource()};
626627
frame.payload = {0, nullptr, 0};
627628

628-
auto push_result = media.interface().push(TimePoint{std::chrono::microseconds{deadline}}, //
629-
frame.extended_can_id,
630-
payload);
629+
auto push_result = media.interface().push( //
630+
TimePoint{std::chrono::duration_cast<Duration>(std::chrono::microseconds{deadline})},
631+
frame.extended_can_id,
632+
payload);
631633

632634
if (const auto* const push = cetl::get_if<IMedia::PushResult::Success>(&push_result))
633635
{
@@ -716,7 +718,8 @@ class TransportImpl final : private TransportDelegate, public ICanTransport
716718
// Otherwise, we would push it to the media interface.
717719
// We use strictly `<` (instead of `<=`) to give this frame a chance (one extra 1us) at the media.
718720
//
719-
const auto deadline = TimePoint{std::chrono::microseconds{tx_item->tx_deadline_usec}};
721+
const auto deadline = TimePoint{std::chrono::duration_cast<Duration>( //
722+
std::chrono::microseconds{tx_item->tx_deadline_usec})};
720723
if (now < deadline)
721724
{
722725
out_deadline = deadline;

‎include/libcyphal/transport/udp/msg_tx_session.hpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,8 @@ class MessageTxSession final : public IMessageTxSession
8080
CETL_NODISCARD cetl::optional<AnyFailure> send(const TransferTxMetadata& metadata,
8181
const PayloadFragments payload_fragments) override
8282
{
83-
const auto deadline_us =
84-
std::chrono::duration_cast<std::chrono::microseconds>(metadata.deadline.time_since_epoch());
83+
const auto deadline_us = std::chrono::duration_cast<std::chrono::microseconds>( //
84+
metadata.deadline.time_since_epoch());
8585

8686
const auto tx_metadata = AnyUdpardTxMetadata::Publish{static_cast<UdpardMicrosecond>(deadline_us.count()),
8787
static_cast<UdpardPriority>(metadata.base.priority),

‎include/libcyphal/transport/udp/svc_tx_sessions.hpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,8 @@ class SvcRequestTxSession final : public IRequestTxSession
9191
return ArgumentError{};
9292
}
9393

94-
const auto deadline_us =
95-
std::chrono::duration_cast<std::chrono::microseconds>(metadata.deadline.time_since_epoch());
94+
const auto deadline_us = std::chrono::duration_cast<std::chrono::microseconds>( //
95+
metadata.deadline.time_since_epoch());
9696

9797
const auto tx_metadata = AnyUdpardTxMetadata::Request{static_cast<UdpardMicrosecond>(deadline_us.count()),
9898
static_cast<UdpardPriority>(metadata.base.priority),
@@ -171,8 +171,8 @@ class SvcResponseTxSession final : public IResponseTxSession
171171
return ArgumentError{};
172172
}
173173

174-
const auto deadline_us =
175-
std::chrono::duration_cast<std::chrono::microseconds>(metadata.tx_meta.deadline.time_since_epoch());
174+
const auto deadline_us = std::chrono::duration_cast<std::chrono::microseconds>( //
175+
metadata.tx_meta.deadline.time_since_epoch());
176176

177177
const auto tx_metadata =
178178
AnyUdpardTxMetadata::Respond{static_cast<UdpardMicrosecond>(deadline_us.count()),

‎include/libcyphal/transport/udp/udp_transport_impl.hpp‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -788,7 +788,8 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport
788788
// Otherwise, we would send it to the media TX socket interface.
789789
// We use strictly `<` (instead of `<=`) to give this frame a chance (one extra 1us) at the socket.
790790
//
791-
const auto deadline = TimePoint{std::chrono::microseconds{tx_item->deadline_usec}};
791+
const auto deadline = TimePoint{std::chrono::duration_cast<Duration>( //
792+
std::chrono::microseconds{tx_item->deadline_usec})};
792793
if (now < deadline)
793794
{
794795
out_deadline = deadline;
@@ -919,8 +920,8 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport
919920

920921
// 2. We've got a new frame from the media RX socket, so let's try to pass it into libudpard RPC dispatcher.
921922

922-
const auto timestamp_us =
923-
std::chrono::duration_cast<std::chrono::microseconds>(rx_meta.timestamp.time_since_epoch());
923+
const auto timestamp_us = std::chrono::duration_cast<std::chrono::microseconds>( //
924+
rx_meta.timestamp.time_since_epoch());
924925

925926
const auto payload_deleter = rx_meta.payload_ptr.get_deleter();
926927

@@ -961,7 +962,8 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport
961962

962963
const auto transfer_id = out_transfer.base.transfer_id;
963964
const auto priority = static_cast<Priority>(out_transfer.base.priority);
964-
const auto timestamp = TimePoint{std::chrono::microseconds{out_transfer.base.timestamp_usec}};
965+
const auto timestamp = TimePoint{std::chrono::duration_cast<Duration>( //
966+
std::chrono::microseconds{out_transfer.base.timestamp_usec})};
965967

966968
session_delegate->acceptRxTransfer(UdpardMemory{memoryResources(), out_transfer.base},
967969
TransferRxMetadata{{transfer_id, priority}, timestamp},
@@ -985,8 +987,8 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport
985987

986988
// 2. We've got a new frame from the media RX socket, so let's try to pass it into libudpard subscription.
987989

988-
const auto timestamp_us =
989-
std::chrono::duration_cast<std::chrono::microseconds>(rx_meta.timestamp.time_since_epoch());
990+
const auto timestamp_us = std::chrono::duration_cast<std::chrono::microseconds>( //
991+
rx_meta.timestamp.time_since_epoch());
990992

991993
const auto payload_deleter = rx_meta.payload_ptr.get_deleter();
992994

@@ -1017,7 +1019,8 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport
10171019
{
10181020
const auto transfer_id = out_transfer.transfer_id;
10191021
const auto priority = static_cast<Priority>(out_transfer.priority);
1020-
const auto timestamp = TimePoint{std::chrono::microseconds{out_transfer.timestamp_usec}};
1022+
const auto timestamp = TimePoint{std::chrono::duration_cast<Duration>( //
1023+
std::chrono::microseconds{out_transfer.timestamp_usec})};
10211024

10221025
session_delegate.acceptRxTransfer(UdpardMemory{memoryResources(), out_transfer},
10231026
TransferRxMetadata{{transfer_id, priority}, timestamp},

‎include/libcyphal/types.hpp‎

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
#ifndef LIBCYPHAL_TYPES_HPP_INCLUDED
77
#define LIBCYPHAL_TYPES_HPP_INCLUDED
88

9+
#include "libcyphal/config.hpp"
10+
911
#include <cetl/cetl.hpp>
1012
#include <cetl/pf17/cetlpf.hpp>
1113
#include <cetl/pmr/interface_ptr.hpp>
@@ -23,23 +25,8 @@
2325
namespace libcyphal
2426
{
2527

26-
/// @brief The internal time representation is in microseconds.
27-
///
28-
/// This is in line with the lizards that use `uint64_t`-typed microsecond counters throughout.
29-
///
30-
struct MonotonicClock final
31-
{
32-
using rep = std::int64_t;
33-
using period = std::micro;
34-
using duration = std::chrono::duration<rep, period>;
35-
using time_point = std::chrono::time_point<MonotonicClock>;
36-
37-
static constexpr bool is_steady = true;
38-
39-
}; // MonotonicClock
40-
41-
using TimePoint = MonotonicClock::time_point;
42-
using Duration = MonotonicClock::duration;
28+
using TimePoint = config::MonotonicClock::time_point;
29+
using Duration = config::MonotonicClock::duration;
4330

4431
template <typename T>
4532
using UniquePtr = cetl::pmr::InterfacePtr<T>;

‎test/unittest/custom_libcyphal_config.hpp‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,23 @@ namespace custom
2626

2727
struct MyConfig : libcyphal::Config
2828
{
29+
/// Redefines time representation as 32-bit milliseconds.
30+
///
31+
/// Milliseconds are chosen b/c there is no implicit conversion from native lizard's microseconds
32+
/// to lower precision units like milliseconds, so proper explicit `std::chrono::duration_cast` is needed.
33+
/// For details also see https://github.com/OpenCyphal-Garage/libcyphal/issues/431.
34+
///
35+
struct MonotonicClock final
36+
{
37+
using rep = std::int32_t;
38+
using period = std::milli;
39+
using duration = std::chrono::duration<rep, period>;
40+
using time_point = std::chrono::time_point<MonotonicClock>;
41+
42+
static constexpr bool is_steady = true;
43+
44+
}; // MonotonicClock
45+
2946
struct Presentation : Config::Presentation
3047
{
3148
static constexpr std::size_t SmallPayloadSize()

‎test/unittest/transport/can/test_can_transport_custom_config_millisecs.cpp‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/// @copyright
2+
/// Copyright (C) OpenCyphal Development Team <opencyphal.org>
3+
/// Copyright Amazon.com Inc. or its affiliates.
4+
/// SPDX-License-Identifier: MIT
5+
6+
// The main purpose of this test file is make sure that CAN transport
7+
// could be compiled with custom time representation (32-bit milliseconds) instead of the default one.
8+
// Milliseconds are chosen b/c there is no implicit conversion from native lizard's microseconds
9+
// to lower precision units like milliseconds, so proper explicit `std::chrono::duration_cast` is needed.
10+
// For details also see https://github.com/OpenCyphal-Garage/libcyphal/issues/431.
11+
#include "custom_libcyphal_config.hpp" // NOLINT(misc-include-cleaner)
12+
13+
#include "media_mock.hpp"
14+
#include "tracking_memory_resource.hpp"
15+
#include "virtual_time_scheduler.hpp"
16+
17+
#include <canard.h>
18+
#include <cetl/pf17/cetlpf.hpp>
19+
#include <libcyphal/transport/can/can_transport.hpp>
20+
#include <libcyphal/transport/can/can_transport_impl.hpp>
21+
#include <libcyphal/transport/can/media.hpp>
22+
#include <libcyphal/types.hpp>
23+
24+
#include <gmock/gmock.h>
25+
#include <gtest/gtest.h>
26+
27+
#include <array>
28+
#include <utility>
29+
30+
namespace
31+
{
32+
33+
using libcyphal::UniquePtr;
34+
using namespace libcyphal::transport::can; // NOLINT This our main concern here in the unit tests.
35+
36+
using testing::Eq;
37+
using testing::Return;
38+
using testing::IsEmpty;
39+
using testing::NotNull;
40+
using testing::ReturnRef;
41+
using testing::StrictMock;
42+
using testing::VariantWith;
43+
44+
class TestCanTransportCustomConfigMilliseconds : public testing::Test
45+
{
46+
protected:
47+
void SetUp() override
48+
{
49+
cetl::pmr::set_default_resource(&mr_);
50+
51+
EXPECT_CALL(media_mock_, getMtu()).WillRepeatedly(Return(CANARD_MTU_CAN_CLASSIC));
52+
EXPECT_CALL(media_mock_, getTxMemoryResource()).WillRepeatedly(ReturnRef(mr_));
53+
}
54+
55+
void TearDown() override
56+
{
57+
EXPECT_THAT(mr_.allocations, IsEmpty());
58+
EXPECT_THAT(mr_.total_allocated_bytes, mr_.total_deallocated_bytes);
59+
}
60+
61+
// MARK: Data members:
62+
63+
// NOLINTBEGIN
64+
libcyphal::VirtualTimeScheduler scheduler_{};
65+
TrackingMemoryResource mr_;
66+
StrictMock<MediaMock> media_mock_{};
67+
// NOLINTEND
68+
};
69+
70+
// MARK: - Tests:
71+
72+
TEST_F(TestCanTransportCustomConfigMilliseconds, makeTransport_getLocalNodeId)
73+
{
74+
std::array<IMedia*, 1> media_array{&media_mock_};
75+
auto maybe_transport = makeTransport(mr_, scheduler_, media_array, 0);
76+
ASSERT_THAT(maybe_transport, VariantWith<UniquePtr<ICanTransport>>(NotNull()));
77+
78+
const auto transport = cetl::get<UniquePtr<ICanTransport>>(std::move(maybe_transport));
79+
EXPECT_THAT(transport->getLocalNodeId(), Eq(cetl::nullopt));
80+
}
81+
82+
} // namespace

‎test/unittest/transport/udp/test_udp_transport_custom_config_millisecs.cpp‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/// @copyright
2+
/// Copyright (C) OpenCyphal Development Team <opencyphal.org>
3+
/// Copyright Amazon.com Inc. or its affiliates.
4+
/// SPDX-License-Identifier: MIT
5+
6+
// The main purpose of this test file is make sure that UDP transport
7+
// could be compiled with custom time representation (32-bit milliseconds) instead of the default one.
8+
// Milliseconds are chosen b/c there is no implicit conversion from native lizard's microseconds
9+
// to lower precision units like milliseconds, so proper explicit `std::chrono::duration_cast` is needed.
10+
// For details also see https://github.com/OpenCyphal-Garage/libcyphal/issues/431.
11+
#include "custom_libcyphal_config.hpp" // NOLINT(misc-include-cleaner)
12+
13+
#include "media_mock.hpp"
14+
#include "tracking_memory_resource.hpp"
15+
#include "virtual_time_scheduler.hpp"
16+
17+
#include <cetl/pf17/cetlpf.hpp>
18+
#include <libcyphal/transport/udp/media.hpp>
19+
#include <libcyphal/transport/udp/udp_transport.hpp>
20+
#include <libcyphal/transport/udp/udp_transport_impl.hpp>
21+
#include <libcyphal/types.hpp>
22+
23+
#include <gmock/gmock.h>
24+
#include <gtest/gtest.h>
25+
26+
#include <array>
27+
#include <utility>
28+
29+
namespace
30+
{
31+
32+
using libcyphal::UniquePtr;
33+
using namespace libcyphal::transport::udp; // NOLINT This our main concern here in the unit tests.
34+
35+
using testing::Eq;
36+
using testing::IsEmpty;
37+
using testing::NotNull;
38+
using testing::ReturnRef;
39+
using testing::StrictMock;
40+
using testing::VariantWith;
41+
42+
class TestUpdTransportCustomConfigMilliseconds : public testing::Test
43+
{
44+
protected:
45+
void SetUp() override
46+
{
47+
cetl::pmr::set_default_resource(&mr_);
48+
49+
EXPECT_CALL(media_mock_, getTxMemoryResource()).WillRepeatedly(ReturnRef(mr_));
50+
}
51+
52+
void TearDown() override
53+
{
54+
EXPECT_THAT(mr_.allocations, IsEmpty());
55+
EXPECT_THAT(mr_.total_allocated_bytes, mr_.total_deallocated_bytes);
56+
}
57+
58+
// MARK: Data members:
59+
60+
// NOLINTBEGIN
61+
libcyphal::VirtualTimeScheduler scheduler_{};
62+
TrackingMemoryResource mr_;
63+
StrictMock<MediaMock> media_mock_{};
64+
// NOLINTEND
65+
};
66+
67+
// MARK: - Tests:
68+
69+
TEST_F(TestUpdTransportCustomConfigMilliseconds, makeTransport_getLocalNodeId)
70+
{
71+
std::array<IMedia*, 1> media_array{&media_mock_};
72+
auto maybe_transport = makeTransport({mr_}, scheduler_, media_array, 0);
73+
ASSERT_THAT(maybe_transport, VariantWith<UniquePtr<IUdpTransport>>(NotNull()));
74+
75+
const auto transport = cetl::get<UniquePtr<IUdpTransport>>(std::move(maybe_transport));
76+
EXPECT_THAT(transport->getLocalNodeId(), Eq(cetl::nullopt));
77+
}
78+
79+
} // namespace

0 commit comments

Comments
 (0)