Issue/336 transport iterfaces by serges147 · Pull Request #343 · OpenCyphal-Garage/libcyphal

serges147 · 2024-04-15T09:00:24Z

First draft of transport interfaces

Added:

transport::ISession
transport::IMessage[Rx|Ts]Session
transport::I[Request|Response][Rx|Ts]Session

Also:

add "std: [14, 17, 20]" to the build matrix
add hash tag triggering by #verification #docs tags
strip repo absolute path prefix from doxygen file paths

Added: - `transport::ISession` - `transport::IMessage[Rx|Ts]Session` - `transport::I[Request|Response][Rx|Ts]Session` Also: - add "std: [14, 17, 20]" to the build matrix - add hash tag triggering by #verification #docs tags - strip repo absolute path prefix from doxygen file paths #docs --------- Co-authored-by: Sergei Shirokov <[email protected]>

- Added `ITransport::makeMessage[Rx|Tx]Session` methods - Added `ITransport::make[Request|Response][Rx|Tx]Session` methods - Added `Priority`, `[Service]TransferMetadata`, `PayloadFragments`, `MessageRxTransfre` and `ServiceRxTransfer` types. - Added`transport::AnyError` - Added implementation of `DynamicBuffer`. Also: - fixed "Coverage" build flavor, and artifacts upload - added `libcyphal::UniquePtr` & `libcyphal::Expected` - fixed "rule of five" deficiency

- Added `ITransport`, `ICanTransport` & `IUpdTransport` - `PayloadFragment` now immutable; `FragmentBuffer` is mutable span.

- `canardInit` is in use. - CAN transport memory resource is connected to the canard allocate/free. - Implemented `getLocalNodeId` Also: - Get rid of extra `Factory` entities.

- `cetl::VariableLengthArray` is now in use to store CAN transport media interfaces. - min MTU from all media is reported by the `ICanTransport::getProtocolParams`. - added corresponding unit tests (to cover `getProtocolParams`). Also: - a bit reworked (simplified) canard memory alloc/free

thirtytwobits

There's a complete lack of interesting ASCII art in this source so far. Very boring!

thirtytwobits · 2024-04-19T20:37:23Z

+    CETL_NODISCARD virtual cetl::optional<MessageRxTransfer> receive() = 0;
+};
+
+class IMessageTxSession : public IRunnable


same comment here as the previous review and for all types in this PR: please start with interface and class documentation. This provides us with a mini-design review as we can state the contract without having the implementation written yet.

This is sensible but:

Sometimes it is necessary to provide more holistic documentation covering multiple entities at once instead of scattering it per entity. This is one such case. We could copy-paste relevant parts of the design doc to the ITransport docs in transport.hpp and put references to that in the class comments here. The relevant sections are:

https://github.com/OpenCyphal-Garage/libcyphal/blob/main/docs/design/readme.md#session-factory-methods

https://github.com/OpenCyphal-Garage/libcyphal/blob/main/docs/design/readme.md#session-data-flow

Some methods do not require documentation. If you have Thing getThing() const noexcept, a doc comment saying "Gets the thing" adds nothing but noise. I would prefer to keep obvious cases undocumented.

As, I wrote for other PR, documentation for public stuff is definitely coming, but I believe we first need some more or less stable api agreed upon. Design doc is very good and very insightful, but at the same time sometimes leaves some (devil in) details unspecified, which might affect result interfaces/implementation quite significantly (like we already discussing about error handling strategies).

It's mostly the class-level documentation that I'm struggling with. I'm left to linking your intentions in these PRs with a single word – the class name – against the design document. If you have to c&p from that document it's fine just help me out with some context when you first define a class, interface, or data structure.

1. Less `auto` according AUTOSAR A7-1-5 2. Less `struct` according AUTOSAR A11-0-2 3. C.35: A base class destructor should be either public and virtual, or protected and non-virtual. 4. `IMedia::setFilters` now fallible.

thirtytwobits · 2024-04-22T15:44:39Z

+
+protected:
+    IRunnable()          = default;
+    virtual ~IRunnable() = default;


It's sorta a nitpick but per Core Cpp Guidelines C35 the ~IRunnable() shouldn't be virtual. Can we establish this as a standard in this codebase too?

It's a bit tricky (for me at least) to decide when we want destructor accessible and when we don't... Even for interfaces we currently do move them into unique_ptr<Concrete> → unique_ptr<Interface> which will require them public.

We shouldn't manage the lifecycle of an object through it's interface. This can lead to dangerous assumptions and amputations (https://godbolt.org/z/1Tj4nzehd). This is what the ~~voldemort_ptr~~, ~~dark-ptr~~, interface_ptr type is meant to solve.

Thank You Scott for very representable example (with legless dog). Please see latest commit where I moved destructors for "external" entities (IMedia-s and IMultiplexer) to protected area. BUT, I can't currently do the same for IRunnable up to various IXxxSession-s we return to user as unique ptrs. This is b/c of our current pmr implementation of UniquePtr - it REQUIRES currently that ~Interface will be available for deleter. The only way I see to remove such requirement is to have type-erased deleter which will capture in a closure original concrete type allocator. It will require cetl::function (not yet available). IMHO it has to be type-erased - otherwise we can't have proper UniquePtr<IX> → UniquePtr<IY> conversions (assuming of course that X and Y convertible). I discussed this with Pavel - it seems that he agrees with me.

BTW, such type-erased deleter should also solve current standing issue OpenCyphal/CETL#118 (see 2nd bullet)

Okay. Let's track this with TODOs in the code. Perhaps I'll take on this work in CETL. It'll give me a reason to move my godbolt example into github.

thirtytwobits · 2024-04-22T15:45:53Z

+    IMedia(const IMedia&)            = delete;
+    IMedia& operator=(IMedia&&)      = delete;
+    IMedia& operator=(const IMedia&) = delete;
+    virtual ~IMedia()                = default;


protected/non-virtual

thirtytwobits

Generally all good here. Please tighten up the interface destructors as this can be hard to fix as the design evolved.

The biggest issue is the open issue on media-layer memory ownership. If we need a interim design call I can do that.

thirtytwobits · 2024-04-22T15:47:01Z

+/// Copyright Amazon.com Inc. or its affiliates.
+/// SPDX-License-Identifier: MIT
+
+#ifndef LIBCYPHAL_TRANSPORT_DEFINES_HPP_INCLUDED


can we call this types.hpp instead? We specifically do not want #defines

I will rename it in the next (347) pr.

thirtytwobits · 2024-04-22T15:49:50Z

+    IMultiplexer(const IMultiplexer&)            = delete;
+    IMultiplexer& operator=(IMultiplexer&&)      = delete;
+    IMultiplexer& operator=(const IMultiplexer&) = delete;
+    virtual ~IMultiplexer()                      = default;


Same here. Interface destructors should always be protected/non-virtual

I get it for external entity interfaces (like media and multiplexer), but for our own entities we implement (and return them via makeXxx as unique ptrs) it still blurry which pattern to use.

Internal implementations would not be destructed via the interface though? They would use their complete internal types.

thirtytwobits · 2024-04-22T15:52:17Z

+
+    // MARK: IRunnable
+
+    void run(const TimePoint) override {}


prefer final to override. If actually using override document how subclasses must handle the base-class implementation (e.g. they shall-or-shall-not invoke it. They shall-invoke it first/last. etc).

At class level we have final - afaik it means that all the virtual methods are final as well. do you want make it even more explicit?

About "shall-or-shall-not invoke" I can't comment yet (due to lack of fine understanding of requirements), but I agree we should have (eventually) such documentation.

Eh. I guess if the class is final the sure. It's fine.

thirtytwobits

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

pavel-kirienko · 2024-04-24T17:49:44Z

+        Interface()                                = default;
+        Interface(Interface&&) noexcept            = default;
+        Interface& operator=(Interface&&) noexcept = default;
+


the rule of 5 requires the dtor to be explicitly defaulted

will do it at after-next pr

serges147 and others added 5 commits April 5, 2024 11:04

Sshirokov/336 transport (#340)

8e16a88

- Added `ITransport`, `ICanTransport` & `IUpdTransport` - `PayloadFragment` now immutable; `FragmentBuffer` is mutable span.

First integration with canard. (#341)

49b1579

- `canardInit` is in use. - CAN transport memory resource is connected to the canard allocate/free. - Implemented `getLocalNodeId` Also: - Get rid of extra `Factory` entities.

serges147 self-assigned this Apr 15, 2024

serges147 requested review from pavel-kirienko and thirtytwobits April 15, 2024 09:04

pavel-kirienko previously approved these changes Apr 15, 2024

View reviewed changes

Comment thread .github/workflows/tests.yml Outdated

Comment thread include/libcyphal/transport/defines.hpp

Comment thread test/unittest/CMakeLists.txt Outdated

pr fixes

3e0b4bb

serges147 dismissed pavel-kirienko’s stale review via 3e0b4bb April 16, 2024 07:08

serges147 requested a review from pavel-kirienko April 16, 2024 07:08

pavel-kirienko previously approved these changes Apr 16, 2024

View reviewed changes

pavel-kirienko linked an issue Apr 16, 2024 that may be closed by this pull request

Declare libcyphal transport interfaces #336

Closed

thirtytwobits reviewed Apr 19, 2024

View reviewed changes

pavel-kirienko mentioned this pull request Apr 20, 2024

Implementation of CAN Msg Rx Session #347

Merged

serges147 mentioned this pull request Apr 22, 2024

Enhance canardTxPush to support scattered transfer payload buffers. OpenCyphal/libcanard#223

Closed

PR fixes #verification

80d0ea7

1. Less `auto` according AUTOSAR A7-1-5 2. Less `struct` according AUTOSAR A11-0-2 3. C.35: A base class destructor should be either public and virtual, or protected and non-virtual. 4. `IMedia::setFilters` now fallible.

serges147 dismissed pavel-kirienko’s stale review via 80d0ea7 April 22, 2024 10:11

serges147 requested review from pavel-kirienko and thirtytwobits April 22, 2024 10:12

pavel-kirienko previously approved these changes Apr 22, 2024

View reviewed changes

thirtytwobits reviewed Apr 22, 2024

View reviewed changes

Rename DynamicBuffer → ScatteredBuffer.

e2cca06

serges147 dismissed pavel-kirienko’s stale review via e2cca06 April 23, 2024 04:37

Move ~destructor to protected for IMedia-s and IMultiplexer.

50c021c

serges147 requested review from pavel-kirienko and thirtytwobits April 23, 2024 09:21

pavel-kirienko mentioned this pull request Apr 24, 2024

Reconsider the IMedia design to support low-copy/zero-copy rx/tx operation #352

Open

thirtytwobits approved these changes Apr 24, 2024

View reviewed changes

pavel-kirienko approved these changes Apr 24, 2024

View reviewed changes

serges147 added this pull request to the merge queue Apr 25, 2024

Merged via the queue into main with commit ee5f1e2 Apr 25, 2024

serges147 deleted the issue/336_transport_iterfaces branch April 25, 2024 13:38

Conversation

serges147 commented Apr 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thirtytwobits left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serges147 Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thirtytwobits Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thirtytwobits left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thirtytwobits left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

serges147 Apr 22, 2024 •

edited

Loading

thirtytwobits Apr 22, 2024 •

edited

Loading