Iox #27 1 client server port interfaces #275

budrus · 2020-09-07T16:26:28Z

initial PR for #27, step 1 in the list

API proposal for review
no implementation yet
also some cleanup in naming of our config parameters
when renaming, I realized that ChunSender template parameter has to be ChunkSenderData and not ChunkDistributor and same issue for ChunkReceiver.

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iceoryx_dds/include/iceoryx_dds/dds/dds_config.hpp

doc/installation-guide.md

iceoryx_dds/include/iceoryx_dds/gateway/gateway_config.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_roudi.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_user.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/receiver_port.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_user.hpp

elfenpiff · 2020-09-08T11:39:41Z

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_user.hpp

+
+    /// @brief check if there was a queue overflow since the last call of hasLostResponseChunks
+    /// @return true if the underlying queue overflowed since last call of this method, otherwise false
+    bool hasLostResponses() noexcept;


This method should be const.

this will reset the flag in ChunkQueueData, and I would not make it mutable because this is a state change for the user

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_user.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_server_port_types.hpp

elfenpiff · 2020-09-08T11:50:13Z

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_server_port_types.hpp

+    }
+
+  private:
+    int64_t m_sequenceNumber{0};


Why is the sequence number not an uint64_t. Do we have negative sequence numbers?

because I saw that ROS is using an int64....

Maybe it's worth raising a PR in rcl? O:-) Or where did you find it?

http://docs.ros2.org/crystal/api/rmw/structrmw__request__id__t.html

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_server_port_types.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/server_port_user.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/receiver_port_data.hpp

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

…clipse-iceoryx#27-1-client_server_port_interfaces

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iceoryx_posh/include/iceoryx_posh/internal/popo/receiver_port_data.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_receiver_data.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_sender_data.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/server_port_user.hpp

mossmaurice · 2020-09-10T14:26:10Z

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_roudi.hpp

+    ClientPortRouDi(ClientPortRouDi&& rhs) = default;
+    ClientPortRouDi& operator=(ClientPortRouDi&& rhs) = default;


noexcept missing?

Good question. I saw that we normally don't have the no except keyword for the default c'tors. We didn't do that when we had the big noexcept refactoring last year. So if it shall be no except we have to change > 100 files I guess. I would do it in a separate PR if we decide to go this way
@elfenpiff @elBoberido your opinion?

IMHO we should always try to be explicit as possible. Then we are aligned with MISRA and the AUTOSAR rules most of the time.
Standard says default c'tors are mostly not throwing, however, there are some exemptions http://eel.is/c++draft/except.spec#7 I'll vote for the explicit way. I'll check ISO/IEC14882:2014 (C++14)

Cppref also provides a good read on this topic: https://en.cppreference.com/w/cpp/language/noexcept_spec

As @mossmaurice said "c'tors are mostly not throwing", which means some still can. So it's probably good to be explicit.

Will not be handled here, I added it to #130

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_roudi.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_user.hpp

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_server_port_types.hpp

mossmaurice · 2020-09-10T14:33:01Z

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_server_port_types.hpp

+    }
+
+  private:
+    int64_t m_sequenceNumber{0};


Maybe it's worth raising a PR in rcl? O:-) Or where did you find it?

mossmaurice · 2020-09-10T14:33:21Z

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_server_port_types.hpp

+    relative_ptr<ClientChunkQueueData_t> m_responseQueue;
+};
+
+class ResponseHeader


Move class to its own file

will do when I implement things. This was just the API so far

Well, it's not a big deal to create a new file, isn't it? ;-) But no hard feelings on this, but IMHO it would be cleaner start.

Sure. My intention was also to get feedback on the general interface. Maybe it would have turned out that the request and response headers are no good idea. So I wanted to save effort until it is consolidated

mossmaurice · 2020-09-10T14:36:29Z

iceoryx_posh/source/popo/ports/subscriber_port_user.cpp

 {
-    return m_chunkReceiver.get();
+    return m_chunkReceiver.tryGet();


Try to get what? Bananas or apples?

A chunk receiver gets chunks. somehow obvious. On the next API layer we are more precise and have allocateChunk. I was thinking about that when I implemented the ChunkReceiver. But then decided against it as I felt it is somehow redundant. The ChunkReceiver has only one job and this is receiving chunks. So I thought get() gets a chunk. Like open() for a Door class and not openDoor()

marthtz · 2020-09-10T14:41:11Z

iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp

-constexpr uint64_t SHARED_MEMORY_ALIGNMENT = 32u;
-constexpr uint32_t MAX_NUMBER_OF_MEMPOOLS = 32u;
-constexpr uint32_t MAX_SHM_SEGMENTS = 100u;
+constexpr uint64_t SHARED_MEMORY_ALIGNMENT = 32U;


I like capital literals. Makes it harder to confuse a "one" and a lower case L. So, that's the new style for iceoryx! 👍

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

mossmaurice

LGTM, having RPCBaseHeader, RequestHeader and ResponseHeader in their own hpp and cpp would be nice. You might want to do it now or in the follow-up pull request.

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

dkroenke · 2020-09-14T11:47:47Z

iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp

+constexpr uint32_t MAX_CLIENTS_PER_SERVER = 256U;
+constexpr uint32_t MAX_REQUESTS_PROCESSED_SIMULTANEOUSLY = 4U;
+constexpr uint32_t MAX_RESPONSES_ALLOCATED_SIMULTANEOUSLY = MAX_REQUESTS_PROCESSED_SIMULTANEOUSLY;
+constexpr uint32_t MAX_REQUEST_QUEUE_CAPACITY = 1024;


hemalbavishi · 2020-11-06T12:17:20Z

Hello,

We've requirement related to request/response communication and would like to know when this solution would be available?

budrus · 2020-11-06T12:28:55Z

Hello,

We've requirement related to request/response communication and would like to know when this solution would be available?

Thanks for asking @hemalbavishi. Top prio has the new C and C++ API for pub/sub communication. This we want to finalize in the next weeks and this is prio 1. But request/response is the next on the list. I would assume that we have a first version end of this or latest beginning of next year

Poehnl Michael (CC-AD/ESW1) added 9 commits September 4, 2020 17:40

iox-eclipse-iceoryx#27-1: client port data

eb0a33a

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: server port data

56b532e

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: rename MAX_PORT_NUMBER

8194f99

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: building block structure fix

677eca6

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: renaming of config consts

9f78db5

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: client and server ports RouDi side skeletons

4416528

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: client port user skeleton

6784d01

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: server port user skeleton

381281d

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: request and response headers on API

8391731

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

budrus commented Sep 7, 2020

View reviewed changes

iceoryx_dds/include/iceoryx_dds/dds/dds_config.hpp Outdated Show resolved Hide resolved

budrus requested review from mossmaurice, elBoberido and elfenpiff September 7, 2020 16:32

elBoberido reviewed Sep 8, 2020

View reviewed changes

elfenpiff reviewed Sep 8, 2020

View reviewed changes

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_user.hpp Outdated Show resolved Hide resolved

elfenpiff reviewed Sep 8, 2020

View reviewed changes

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_user.hpp Outdated Show resolved Hide resolved

elfenpiff requested changes Sep 8, 2020

View reviewed changes

Poehnl Michael (CC-AD/ESW1) added 3 commits September 10, 2020 15:21

iox-eclipse-iceoryx#27-1: try prefix

6d5171f

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

Merge branch 'master' of https://github.com/eclipse/iceoryx into iox-e…

5f8fc84

…clipse-iceoryx#27-1-client_server_port_interfaces

iox-eclipse-iceoryx#27-1: try prefix for C API

7a3ef6b

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

dkroenke reviewed Sep 10, 2020

View reviewed changes

mossmaurice reviewed Sep 10, 2020

View reviewed changes

marthtz reviewed Sep 10, 2020

View reviewed changes

Poehnl Michael (CC-AD/ESW1) added 6 commits September 10, 2020 16:53

iox-eclipse-iceoryx#27-1: review findings

59ec268

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: fixing C API tests

214968e

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: review findings

7ab1a7b

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: new parameter names in c binding tests

a11e143

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: removed obsolete member in publisher port

21de183

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: request and response header should be moveable

e7a7782

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

budrus requested a review from mossmaurice September 11, 2020 07:48

budrus requested review from dkroenke, elfenpiff and elBoberido September 11, 2020 07:48

mossmaurice added the enhancement New feature label Sep 11, 2020

Poehnl Michael (CC-AD/ESW1) added 4 commits September 11, 2020 15:43

iox-eclipse-iceoryx#27-1: renaming getCaProMessage

17c9ee1

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: more review findigs

5abb907

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: missing adapatation of new C API

c66159b

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

iox-eclipse-iceoryx#27-1: clang format

b9f3c3a

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

mossmaurice previously approved these changes Sep 14, 2020

View reviewed changes

elfenpiff previously approved these changes Sep 14, 2020

View reviewed changes

iox-eclipse-iceoryx#27-1: merging changes from master

e3d6212

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <michael.poehnl@de.bosch.com>

budrus dismissed stale reviews from elfenpiff and mossmaurice via e3d6212 September 14, 2020 11:08

mossmaurice approved these changes Sep 14, 2020

View reviewed changes

dkroenke approved these changes Sep 14, 2020

View reviewed changes

budrus merged commit 8bd8b70 into eclipse-iceoryx:master Sep 14, 2020

budrus linked an issue Sep 14, 2020 that may be closed by this pull request

Request/Response communication with iceoryx #27

Closed

22 tasks

marthtz mentioned this pull request Sep 16, 2020

Iox #218 improve pub sub api #291

Merged

budrus deleted the iox-#27-1-client_server_port_interfaces branch November 6, 2020 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #27 1 client server port interfaces #275

Iox #27 1 client server port interfaces #275

budrus commented Sep 7, 2020 •

edited

Loading

elfenpiff Sep 8, 2020

budrus Sep 10, 2020

elfenpiff Sep 8, 2020

budrus Sep 10, 2020

mossmaurice Sep 10, 2020

budrus Sep 10, 2020

mossmaurice Sep 10, 2020

budrus Sep 10, 2020 •

edited

Loading

mossmaurice Sep 11, 2020 •

edited

Loading

mossmaurice Sep 11, 2020

marthtz Sep 11, 2020

budrus Sep 11, 2020

mossmaurice Sep 10, 2020

mossmaurice Sep 10, 2020

budrus Sep 10, 2020

mossmaurice Sep 11, 2020

budrus Sep 11, 2020

mossmaurice Sep 10, 2020

budrus Sep 10, 2020 •

edited

Loading

marthtz Sep 10, 2020

mossmaurice left a comment

dkroenke Sep 14, 2020

hemalbavishi commented Nov 6, 2020

budrus commented Nov 6, 2020

		ClientPortRouDi(ClientPortRouDi&& rhs) = default;
		ClientPortRouDi& operator=(ClientPortRouDi&& rhs) = default;

Iox #27 1 client server port interfaces #275

Iox #27 1 client server port interfaces #275

Conversation

budrus commented Sep 7, 2020 • 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

budrus Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

mossmaurice Sep 11, 2020 • 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

budrus Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmaurice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemalbavishi commented Nov 6, 2020

budrus commented Nov 6, 2020

budrus commented Sep 7, 2020 •

edited

Loading

budrus Sep 10, 2020 •

edited

Loading

mossmaurice Sep 11, 2020 •

edited

Loading

budrus Sep 10, 2020 •

edited

Loading