Implementation of CAN Msg Rx Session #347

serges147 · 2024-04-19T11:41:56Z

Implemented MessageRxSession
Added intermediate can::TransportDelegate entity:
- needed for all sessions to be able reference their parent transport
- contains canard related (kinda) mixin logic (like "canard" memory management or errors translation)

Also:

switch to toolshed 22.4.7
disable asserts from "Coverage" build flavor - to not pollute branch coverage

…ession

cmake/compiler_flag_sets/default.cmake

cmake/modules/Findlibcanard.cmake

include/libcyphal/transport/can/delegate.hpp

include/libcyphal/transport/can/msg_rx_session.hpp

include/libcyphal/transport/can/transport.hpp

test/unittest/transport/can/test_can_msg_rx_session.cpp

thirtytwobits

I really need some high-level comments on the classes to comprehend your intentions.

cmake/modules/Findlibcanard.cmake

thirtytwobits · 2024-04-19T18:43:58Z

include/libcyphal/transport/can/delegate.hpp

+namespace detail
+{
+
+struct TransportDelegate


let's not fall behind on the comments in this code base. Please ensure all classes, interfaces, and public methods have some level of documentation. Especially as things first go in it helps to know what you are thinking

all classes, interfaces, and public methods

See my comment here about omitting documentation in obvious cases (I am not saying that this case in particular is obvious): #343 (comment)

Yes, documentation will come, just a bit later - when api will be more stable. For now I'm not documenting yet to faster get something to play with.

As in the other review, my main concern is interface/class/struct documentation. It doesn't have to be complete at the start but I need some help understanding your design intention in these early reviews.

include/libcyphal/transport/can/delegate.hpp

thirtytwobits · 2024-04-19T19:12:59Z

include/libcyphal/transport/can/delegate.hpp

+    {
+        auto& self = getSelfFrom(ins);
+
+        const auto memory_size   = sizeof(CanardMemoryHeader) + amount;


Let's follow AUTOSAR A7-1-5 and only use auto to:

declare that a variable has the same type as return type of a function call

declare that a variable has the same type as initializer of non-fundamental type

declare parameters of a generic lambda expression

declare a function template using trailing return type syntax.

I will read this A7-1-5 carefully I try fix obvious cases.

I believe/hope it's fine to use more auto-s in unit tests.

Sure. Just remember humans need to read code too so sometimes auto helps by simplifying a statement and sometimes it obfuscates by making the type inscrutable. You can always use github co-pilot to reduce the amount of typing ;-)

include/libcyphal/transport/can/delegate.hpp

include/libcyphal/transport/can/msg_rx_session.hpp

thirtytwobits · 2024-04-19T19:58:39Z

include/libcyphal/types.hpp

@@ -65,6 +65,18 @@ using PmrAllocator = cetl::pmr::polymorphic_allocator<T>;
 template <typename T>
 using VarArray = cetl::VariableLengthArray<T, PmrAllocator<T>>;

+template <typename Tag, typename... Args>
+CETL_NODISCARD UniquePtr<typename Tag::Interface> makeUniquePtr(cetl::pmr::memory_resource& memory, Args&&... args)


I'd like to promote this pattern up into cetl::pmr::Factory at some point. As such, let's adopt a more appropriate name that distinguishes this from unique_ptr. Pavel and I had discussed calling this technique: dark_ptr since it hides the concrete type from the user of the smart-pointer-to-interface type. So dark_ptr and make_dark

I thought it's voldemort_ptr and make_voldemort

What about interface_ptr and makeInterfacePtr ? Both "dark" and "voldemort" are funny, but IMHO doesn't look serious...

interface_ptr seems reasonable. I'm fairly certain voldemort is a trademark anyway.

…ession

thirtytwobits · 2024-04-22T16:48:49Z

I think my biggest sticking point here is CETL_NODISCARD. I was really hoping to avoid a macro on every single method in this code base.

…ession

thirtytwobits

As discussed on call: we have the right issues captured now to ensure my overarching concerns with this PR will be addressed moving forward

pavel-kirienko · 2024-04-24T17:53:40Z

include/libcyphal/transport/can/delegate.hpp

+        canard_instance().user_reference = this;
+    }
+
+    CETL_NODISCARD inline CanardInstance& canard_instance() noexcept


Suggested change

CETL_NODISCARD inline CanardInstance& canard_instance() noexcept

CETL_NODISCARD CanardInstance& canard_instance() noexcept

inline is redundant and should be removed; see the other methods as well

include/libcyphal/transport/can/delegate.hpp

include/libcyphal/transport/can/msg_rx_session.hpp

pavel-kirienko · 2024-04-25T11:27:32Z

include/libcyphal/transport/can/delegate.hpp

+        type_id_type<0x11, 0x41, 0xF5, 0xC0, 0x2E, 0x61, 0x44, 0xBF, 0x9F, 0x0E, 0xFA, 0x1C, 0x51, 0x8C, 0xD5, 0x17>;
+
+public:
+    class CanardMemory final : public cetl::rtti_helper<CanardMemoryTypeIdType, ScatteredBuffer::Interface>


Maybe CanardScatteredBuffer makes a better name? The current name is a bit too generic.

maybe... in the next pr I will already have ScatteredBuffer (renamed DynamicBuffer)

pavel-kirienko · 2024-04-25T11:28:18Z

include/libcyphal/transport/can/delegate.hpp

+            }
+
+            const auto bytes_to_copy = std::min(length_bytes, payload_size_ - offset_bytes);
+            memcpy(destination, static_cast<cetl::byte*>(buffer_) + offset_bytes, bytes_to_copy);


Suggested change

memcpy(destination, static_cast<cetl::byte*>(buffer_) + offset_bytes, bytes_to_copy);

std::memmove(destination, static_cast<cetl::byte*>(buffer_) + offset_bytes, bytes_to_copy);

pavel-kirienko · 2024-04-25T11:43:18Z

include/libcyphal/transport/errors.hpp

 {
    std::uint32_t code;
 };

+struct SessionAlreadyExistsError final


Suggested change

struct SessionAlreadyExistsError final

struct AlreadyExistsError final

This way, the error type will be reusable and whether it's related to a session or anything else will be evident from context.

pavel-kirienko · 2024-04-25T11:45:25Z

test/unittest/gtest_helpers.hpp

@@ -0,0 +1,53 @@
+/// @file
+/// libcyphal common header.


copy/paste error

pavel-kirienko · 2024-04-25T11:54:50Z

include/libcyphal/transport/can/transport.hpp

+                    CanardRxSubscription* out_subscription{};
+
+                    // TODO: Handle errors.
+                    const auto result = canardRxAccept(&canard_instance(),


I think we need an assertion check here that we never get an argument error back from libcanard

pavel-kirienko · 2024-04-25T11:55:30Z

include/libcyphal/transport/can/transport.hpp

+                    const auto result = canardRxAccept(&canard_instance(),
+                                                       static_cast<CanardMicrosecond>(timestamp_us.count()),
+                                                       &canard_frame,
+                                                       static_cast<uint8_t>(media_index),


Suggested change

static_cast<uint8_t>(media_index),

static_cast<std::uint8_t>(media_index),

AFAIK C++ does not define symbols in the global namespace, this thing comes from C.

My thinking was that in such conversions I covert to terms/types Canard is using... in this particular case the 4th parameter of ::canardRxAccept.

pavel-kirienko · 2024-04-25T11:57:37Z

include/libcyphal/transport/can/transport.hpp


-    CETL_NODISCARD static inline TransportImpl& getSelfFrom(const CanardInstance* const ins)
+    CETL_NODISCARD inline TransportDelegate& asDelegate()


Another redundant inline here

…ession

serges147 and others added 20 commits April 11, 2024 08:30

#verification #docs

08a337d

Added MessageRxSession implementation.

88c4478

Implemented CanardMemory for DynamicBuffer.

74d030c

Merge branch 'issue/336_transport_interfaces' into sshirokov/msg_rx_s…

bff389e

…ession

set proper type id

a90c2fe

minor fixes

47c4218

introduce TestCanTransport test fixture

e9c5133

Added TrackingMemoryResource test helper

ec91d2a

temporary disable memory tracking #verification

dd2e74c

implemented memory copying #verification

ec53ae0

more unit tests #verification

a34dd90

try fix gcc build #verification

fdb7480

Merge branch 'sshirokov/msg_rx_session' into sshirokov/try_ts22.4.7

f56d006

#verification

478e36d

#verification

34b6422

Merge branch 'sshirokov/try_ts22.4.7' into sshirokov/msg_rx_session

9a4703b

cover delegate with unit tests #verification

d318b1e

more unit tests to cover memory errors #verification

7b729e1

fix unit tests on cpp17+ #verification

b92db32

try to fix seg fault on release #verification

4952f81

serges147 commented Apr 19, 2024

View reviewed changes

cmake/compiler_flag_sets/default.cmake Show resolved Hide resolved

serges147 commented Apr 19, 2024

View reviewed changes

cmake/modules/Findlibcanard.cmake Show resolved Hide resolved

serges147 commented Apr 19, 2024

View reviewed changes

include/libcyphal/transport/can/delegate.hpp Show resolved Hide resolved

serges147 commented Apr 19, 2024

View reviewed changes

include/libcyphal/transport/can/msg_rx_session.hpp Show resolved Hide resolved

serges147 commented Apr 19, 2024

View reviewed changes

include/libcyphal/transport/can/transport.hpp Show resolved Hide resolved

serges147 commented Apr 19, 2024

View reviewed changes

test/unittest/transport/can/test_can_msg_rx_session.cpp Show resolved Hide resolved

serges147 self-assigned this Apr 19, 2024

serges147 requested a review from pavel-kirienko April 19, 2024 12:14

thirtytwobits reviewed Apr 19, 2024

View reviewed changes

Merge branch 'issue/346_can_msg_sessions' into sshirokov/346_msg_rx_s…

b0c2876

…ession

PR fixes

11bdbea

serges147 added 3 commits April 23, 2024 07:43

Merge branch 'issue/346_can_msg_sessions' into sshirokov/346_msg_rx_s…

7847d92

…ession

rename defines.hpp → types.hpp

8d9ff46

Merge branch 'issue/346_can_msg_sessions' into sshirokov/346_msg_rx_s…

445c3d0

…ession

thirtytwobits approved these changes Apr 24, 2024

View reviewed changes

pavel-kirienko mentioned this pull request Apr 24, 2024

Only one concrete base type per class #353

Closed

pavel-kirienko approved these changes Apr 25, 2024

View reviewed changes

Merge branch 'issue/346_can_msg_sessions' into sshirokov/346_msg_rx_s…

05388d9

…ession

serges147 merged commit dadf7d7 into issue/346_can_msg_sessions Apr 26, 2024
18 checks passed

serges147 deleted the sshirokov/346_msg_rx_session branch April 26, 2024 05:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of CAN Msg Rx Session #347

Implementation of CAN Msg Rx Session #347

serges147 commented Apr 19, 2024 •

edited

Loading

thirtytwobits left a comment

thirtytwobits Apr 19, 2024

pavel-kirienko Apr 20, 2024

serges147 Apr 20, 2024

thirtytwobits Apr 22, 2024

thirtytwobits Apr 19, 2024

serges147 Apr 20, 2024

serges147 Apr 22, 2024

thirtytwobits Apr 22, 2024

thirtytwobits Apr 19, 2024

pavel-kirienko Apr 20, 2024

serges147 Apr 20, 2024

thirtytwobits Apr 22, 2024

thirtytwobits commented Apr 22, 2024

thirtytwobits left a comment

pavel-kirienko Apr 24, 2024

pavel-kirienko Apr 25, 2024

serges147 Apr 26, 2024

pavel-kirienko Apr 25, 2024

pavel-kirienko Apr 25, 2024

pavel-kirienko Apr 25, 2024

serges147 Apr 26, 2024

pavel-kirienko Apr 25, 2024

pavel-kirienko Apr 25, 2024

serges147 Apr 26, 2024

pavel-kirienko Apr 25, 2024

	CETL_NODISCARD inline CanardInstance& canard_instance() noexcept
	CETL_NODISCARD CanardInstance& canard_instance() noexcept

	memcpy(destination, static_cast<cetl::byte*>(buffer_) + offset_bytes, bytes_to_copy);
	std::memmove(destination, static_cast<cetl::byte*>(buffer_) + offset_bytes, bytes_to_copy);

	struct SessionAlreadyExistsError final
	struct AlreadyExistsError final

	static_cast<uint8_t>(media_index),
	static_cast<std::uint8_t>(media_index),


		CETL_NODISCARD static inline TransportImpl& getSelfFrom(const CanardInstance* const ins)
		CETL_NODISCARD inline TransportDelegate& asDelegate()

Implementation of CAN Msg Rx Session #347

Implementation of CAN Msg Rx Session #347

Conversation

serges147 commented Apr 19, 2024 • edited Loading

thirtytwobits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thirtytwobits commented Apr 22, 2024

thirtytwobits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serges147 commented Apr 19, 2024 •

edited

Loading