Service RX/TX Request/Response sessions #356

serges147 · 2024-05-02T05:45:13Z

Added Svc RX & TX sessions (both for Request & Response).
Now that we have more sessions, ContiguousPayload building moved to transport impl:
- less code duplication
- less to verify by unit tests
- sessions now more like facade for RX/TX - they do necessary params validation/conversion, but ultimately delegate to the transport
switch to cetl::unbounded_variant (instead of former cetl::any)

Also:

minor changes for previous PR requests
more docs for stable api; excluding *::detail::* from generated docs

Codo coverage:

…ession

…_session #verification

serges147 · 2024-05-02T05:48:59Z

.clang-tidy

+WarningsAsErrors: '*'
+HeaderFilterRegex: '.*\.hpp'
+AnalyzeTemporaryDtors: false
+FormatStyle: file


Not yet integrated, but just initial version of the config according issue #226

serges147 · 2024-05-02T05:49:54Z

cmake/modules/Findcetl.cmake

@@ -6,7 +6,7 @@

 include(FetchContent)
 set(cetl_GIT_REPOSITORY "https://github.com/OpenCyphal/cetl.git")
-set(cetl_GIT_TAG "886a0d227a043511eed6b252ea0f788590c50e75")
+set(cetl_GIT_TAG "10fbb2b7b89473d68e73db7235848b0692169e5a")


CETL's main (with recently merged cetl::unbounded_variant.

serges147 · 2024-05-02T05:50:40Z

docs/doxygen.ini

@@ -1071,7 +1071,7 @@ EXCLUDE_PATTERNS       =
 # wildcard * is used, a substring. Examples: ANamespace, AClass,
 # ANamespace::AClass, ANamespace::*Test

-EXCLUDE_SYMBOLS        =
+EXCLUDE_SYMBOLS        = *::detail::*


Internal implementation detail won't go to generated docs.

serges147 · 2024-05-02T05:59:37Z

include/libcyphal/transport/can/delegate.hpp

 ///
-class SessionDelegate
+class IRxSessionDelegate


Adding I prefix for anything like an interface (regardless whether it's internal or public stuff) - as it was requested in previous PR.

serges147 · 2024-05-02T06:04:04Z

include/libcyphal/transport/can/msg_rx_session.hpp

+        if (params.subject_id > CANARD_SUBJECT_ID_MAX)
+        {
+            return ArgumentError{};
+        }


Parameters are const (see params_ below), and their subject_id won't change. So, it makes sense fail early (at make phase) rather than later during transmissions. The same story for other session factories.

serges147 · 2024-05-02T06:19:33Z

include/libcyphal/transport/can/svc_rx_sessions.hpp

+///                      Could be either `CanardTransferKindRequest` or `CanardTransferKindResponse`.
+///
+template <typename Interface_, typename Params, CanardTransferKind TransferKind>
+class SvcRxSession final : public Interface_, private IRxSessionDelegate


Both service RX sessions (rx request and rx response) are almost the same in terms of structure and functionality. The only slight difference is their interface name, params type and "kind" of canard transfer. So, IMO making template here is fine and justifiable b/c ...:

it's detail-s, not exposed to a user

no code duplication

both instantiations (see below just after the template) are 100% covered by unit tests (at line, function and branches level). Please see attached (to the PR description) screenshot of coverage stats.

BTW, MessageRxSession is also very similar, BUT still has its differences (in terms of params validation and anonymous case handling), so it does not fit into such RX template IMO (hence there is minor code duplication - all covered of course by tests).

serges147 · 2024-05-02T06:21:41Z

include/libcyphal/transport/can/svc_rx_sessions.hpp

+                                                  &subscription_);
+        (void) result;
+        CETL_DEBUG_ASSERT(result >= 0, "There is no way currently to get an error here.");
+        CETL_DEBUG_ASSERT(result > 0, "New subscription supposed to be made.");


There is no currently path to fail - pre-validation done at factory.

Sorry, why can't this fail?

@thirtytwobits the only possible error condition occurs if we feed invalid arguments, which we pre-validate and know are correct; hence, it can't fail.

serges147 · 2024-05-02T06:29:55Z

include/libcyphal/transport/can/svc_rx_sessions.hpp

+    {
+        ::canardRxUnsubscribe(&delegate_.canard_instance(),
+                              TransferKind,
+                              static_cast<CanardPortID>(params_.service_id));


No path fail as well, but if you think its worth to assert I can do it (here and for msg rx session).

I like the debug asserts as long as we are able to verify them.

serges147 · 2024-05-02T06:36:04Z

include/libcyphal/transport/scattered_buffer.hpp

+    ///
+    /// @see ScatteredBuffer::ScatteredBuffer(AnyStorage&& any_storage)
+    ///
+    class IStorage : public cetl::rtti_helper<IStorageTypeIdType>


Here and there (including in the design doc) we use word "storage" when we talk about Lizard-specific implementation of actual (potentially) scattered storage this "facade" ScatteredBuffer is using. So, I decided to rename nested Interface to IMO more meaningful IStorage.

serges147 · 2024-05-02T06:43:24Z

test/unittest/gtest_helpers.hpp

-    auto locale = os->imbue(std::locale("en_US"));
-    *os << std::chrono::duration_cast<std::chrono::microseconds>(duration).count() << "_us";
+    auto locale = os->imbue(std::locale(""));
+    *os << std::chrono::duration_cast<std::chrono::microseconds>(duration).count() << " us";


Previous version failed on my "Estonian only" Ubuntu (b/c there is no "en_US" locale there) ;)
BTW, locale stuff here is just to have more human friendly gtest failure output (like 1'000'000 us vs 1000000 us without it).

serges147 · 2024-05-02T06:48:06Z

test/unittest/gtest_helpers.hpp

+{
+    return DestinationNodeCanIdMatcher(node_id);
+}
+


All these gmock matchers (and PrintTo helpers) have no use when all the tests are green (b/c there is no output), but it makes big difference when something goes wrong. Using them (in conjunction with EXPECT_THAT macro) gives you nice print of what was expected and what actually is. Stay tuned, even more will be added...

thirtytwobits · 2024-05-02T14:46:09Z

docs/doxygen.ini

@@ -1071,7 +1071,7 @@ EXCLUDE_PATTERNS       =
 # wildcard * is used, a substring. Examples: ANamespace, AClass,
 # ANamespace::AClass, ANamespace::*Test

-EXCLUDE_SYMBOLS        =
+EXCLUDE_SYMBOLS        = *::detail::*


thirtytwobits · 2024-05-02T15:04:58Z

include/libcyphal/transport/can/svc_rx_sessions.hpp

+                                                  &subscription_);
+        (void) result;
+        CETL_DEBUG_ASSERT(result >= 0, "There is no way currently to get an error here.");
+        CETL_DEBUG_ASSERT(result > 0, "New subscription supposed to be made.");


Sorry, why can't this fail?

thirtytwobits · 2024-05-02T15:05:35Z

include/libcyphal/transport/can/svc_rx_sessions.hpp

+    {
+        ::canardRxUnsubscribe(&delegate_.canard_instance(),
+                              TransferKind,
+                              static_cast<CanardPortID>(params_.service_id));


I like the debug asserts as long as we are able to verify them.

thirtytwobits · 2024-05-02T15:08:16Z

include/libcyphal/transport/can/svc_rx_sessions.hpp

+
+    void run(const TimePoint) final
+    {
+        // Nothing to do here currently.


thirtytwobits · 2024-05-02T15:23:16Z

include/libcyphal/transport/udp/media.hpp

+    /// This value may change arbitrarily at runtime. The transport implementation will query it before every
+    /// transmission on the port. This value has no effect on the reception pipeline as it can accept arbitrary MTU.
+    ///
+    CETL_NODISCARD virtual std::size_t getMtu() const noexcept = 0;


This is a public API and nothing catastrophic happens if the value isn't used. Please omit CETL_NODISCARD value

ok, I will do it. But there is separate issue about revising everything in terms of "to discard or not to discard" question.

serges147 and others added 30 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

first draft of can msg tx session

6e1c79b

unit test for no memory for tx session #verification

a68fadb

buffer

a705deb

Added ContiguousPayload helper. #verification

96b65ae

try fix build #verification

c4e478b

disable/ignore exceptions during coverage #verification

27a04f5

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

b0c2876

…ession

PR fixes

11bdbea

Merge branch 'sshirokov/346_msg_rx_session' into sshirokov/346_msg_tx…

9e2a394

…_session #verification

minor fixes #verification

784665a

serges147 and others added 7 commits April 30, 2024 08:16

docs improvements

e366c98

added .clang-tidy file #verification

ffa98c5

Merge branch 'sshirokov/xxx_svc_tx' into sshirokov/xxx1_svc_tx_rx

814de68

Merge branch 'sshirokov/xxx1_svc_tx_rx' into sshirokov/xxx2_docs

03a1e60

RxSessionDelegate → IRxSessionDelegate; Storage → IStorage

377f0e0

switch to CETL's main branch commit #verification

a4c8ff1

Merge branch 'issue/346_can_msg_sessions' into sshirokov/xxx1_svc_tx_rx

03e1da1

serges147 commented May 2, 2024

View reviewed changes

serges147 changed the title ~~Sshirokov/346 svc tx rx~~ Service RX/TX Request/Response sessions May 2, 2024

serges147 requested review from pavel-kirienko, lydia-at-amazon and thirtytwobits May 2, 2024 06:57

serges147 self-assigned this May 2, 2024

minor change

975e6a7

thirtytwobits requested changes May 2, 2024

View reviewed changes

PR fixes

2b06c32

serges147 requested a review from thirtytwobits May 3, 2024 16:58

thirtytwobits approved these changes May 3, 2024

View reviewed changes

thirtytwobits merged commit e271e12 into issue/346_can_sessions May 3, 2024
18 checks passed

thirtytwobits deleted the sshirokov/346_svc_tx_rx branch May 3, 2024 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service RX/TX Request/Response sessions #356

Service RX/TX Request/Response sessions #356

serges147 commented May 2, 2024 •

edited

Loading

serges147 May 2, 2024

serges147 May 2, 2024

serges147 May 2, 2024

thirtytwobits May 2, 2024

serges147 May 2, 2024

serges147 May 2, 2024

serges147 May 2, 2024 •

edited

Loading

serges147 May 2, 2024

serges147 May 2, 2024

thirtytwobits May 2, 2024

pavel-kirienko May 2, 2024

serges147 May 2, 2024

thirtytwobits May 2, 2024

serges147 May 2, 2024

serges147 May 2, 2024

serges147 May 2, 2024

thirtytwobits May 2, 2024

thirtytwobits May 2, 2024

thirtytwobits May 2, 2024

thirtytwobits May 2, 2024

thirtytwobits May 2, 2024

serges147 May 3, 2024

Service RX/TX Request/Response sessions #356

Service RX/TX Request/Response sessions #356

Conversation

serges147 commented May 2, 2024 • edited Loading

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 May 2, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serges147 commented May 2, 2024 •

edited

Loading

serges147 May 2, 2024 •

edited

Loading