Iox #25 chunk receiver building block #84

budrus · 2020-04-08T17:35:50Z

No description provided.

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

mossmaurice

The target posh_moduletests fails to build:

/home/foo/repos/iceoryx/iceoryx_posh/test/moduletests/test_popo_chunk_sender.cpp:77:106: error: wrong number of template arguments (2, should be 1) using ChunkDistributor_t = iox::popo::ChunkDistributor<MAX_NUMBER_QUEUES, iox::popo::ThreadSafePolicy>;

Please fix the test

mossmaurice · 2020-04-14T14:59:58Z

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

-/// When different threads operate on it (e.g. application sends chunks and RouDi adds and removes queues), 
-/// a locking policy must be used that ensures consistent data in the ChunkDistributorData. 


Keep fighting the trailing whitespaces :-)

doesn't your IDE automatically remove trailing whitespaces ... me starting IDE war and running away ;)

mossmaurice · 2020-04-14T15:06:10Z

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

-struct ChunkQueueData;
-


Nice catch!

mossmaurice · 2020-04-14T15:14:22Z

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

+#ifndef IOX_POSH_POPO_CHUNK_RECEIVER_HPP_
+#define IOX_POSH_POPO_CHUNK_RECEIVER_HPP_


As we decided to switch to include guards (#30), we can keep them here. Isn't the #define IOX_CHUNK_RECEIVER_HPP enough to avoid ambiguity?

I thought I use them here as we decided to use them again. But we haven't defined a rule for them yet, or?

Nope there's no rule yet. Let's define it with #30

mossmaurice · 2020-04-14T15:19:29Z

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

+    /// disappears
+    /// @return optional that has a new chunk header or no value if there are no new chunks in the underlying queue,
+    /// ChunkReceiverError on error
+    cxx::expected<cxx::optional<const mepoo::ChunkHeader*>, ChunkReceiverError> get() noexcept;


Why the optional inside an expected? Isn't it enough to either provide a ChunkHeader pointer or an ChunkReceiverError?

No, if there is no new chunk, you will get an empty optional without an error. Like a pop on an empty FiFo

mossmaurice · 2020-04-14T15:24:15Z

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

+#ifndef IOX_POSH_POPO_CHUNK_RECEIVER_DATA_HPP_
+#define IOX_POSH_POPO_CHUNK_RECEIVER_DATA_HPP_


IOX_CHUNK_RECEIVER_DATA_HPP?

no rule bro, so my decision ;-)
Shall we have at least the component name in it. Maybe threre will be modules with same names in different components. So maybe IOX_POPO_CHUNK_RECEIVER_DATA_HPP_

I'll give that a try

mossmaurice · 2020-04-14T15:27:30Z

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_sender.inl

+            // if the seqence number in the chunk info is set by the user, we still increment the internal one as this
+            // might be needed by midddleware spcific evaluation (like in introspection)


Thanks for clearing this up!

mossmaurice · 2020-04-14T15:41:55Z

iceoryx_posh/source/popo/building_blocks/chunk_queue.cpp

+    else
+    {
+        // drop the chunk if one is returned by a safe overflow
+        if ((*pushRet).has_value())


I'd prefer to call pushRet.get_value() instead of operator*(). Makes it more readable for me.

hmmm, for my not. (pushRet.get_value()).has_value()? But yes optional and expected allow a lot of different flavors. I prefer has_value() and has_error() and not the bool operators, for better readability. But I like * operator instead of get_value().

I have no hard feelings about this. My eye just notices .get_value() easier than * ;-)

mossmaurice · 2020-04-14T15:52:58Z

iceoryx_posh/source/popo/building_blocks/chunk_queue.cpp

 {
    ChunkQueueData::ChunkTuple chunkTupleIn(chunk.releaseWithRelativePtr());

-    return !getMembers()
-                ->m_queue.push(chunkTupleIn)
-                .on_success(


Why not staying in the functional world? You can concatenate the success and error case easily ;-) m_queue.push(foo).on_success( GoodCallable ).on_error( BadCallable )

We have to talk about std::function one day... Could become a heap and ASIL D compiler support topic

I had function_ref on my mind in the last few months. Created issue #86. IMHO we should be able to replace all std::function occurrences with it.

mossmaurice · 2020-04-14T16:00:02Z

iceoryx_posh/test/moduletests/test_popo_chunk_receiver.cpp

+class ChunkReceiver_test : public ChunkReceiver_testBase
+{
+  public:
+    ChunkReceiver_test()
+        : ChunkReceiver_testBase()
+    {
+    }
+};


Why is this additional derivation necessary?

Too much copy and waste from other test I guess..

budrus · 2020-04-14T17:17:59Z

The target posh_moduletests fails to build:

/home/foo/repos/iceoryx/iceoryx_posh/test/moduletests/test_popo_chunk_sender.cpp:77:106: error: wrong number of template arguments (2, should be 1) using ChunkDistributor_t = iox::popo::ChunkDistributor<MAX_NUMBER_QUEUES, iox::popo::ThreadSafePolicy>;

Please fix the test

oopsie, I thought I could swear that I had a green build. Time for the github actions like in the rmw_iceoryx

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

mossmaurice

PosixShmMemoryProvider_Test.CreateMemory test is failing on my machine. It's not related to the changes, isn't it?
I think it would be nice to change all the expected and optional variables to maybe* e.g. maybeChunkHeader

mossmaurice · 2020-04-20T17:31:26Z

iceoryx_posh/CMakeLists.txt

+    source/popo/building_blocks/chunk_queue_pusher.cpp
+    source/popo/building_blocks/chunk_queue_popper.cpp    


Nice to have two interfaces!

mossmaurice · 2020-04-20T17:40:06Z

iceoryx_utils/include/iceoryx_utils/internal/cxx/variant_queue.inl

-        auto override =
+        auto notskipped =
            m_fifo.template get_at_index<static_cast<uint64_t>(VariantQueueTypes::SoFi_SingleProducerSingleConsumer)>()
                ->push(value, overriddenValue);
-        if (override)
+        if (notskipped)
        {
-            ret = success<optional<ValueType>>(optional<ValueType>(std::move(overriddenValue)));
+            ret = success<optional<ValueType>>(nullopt_t());
        }
        else
        {
-            ret = success<optional<ValueType>>(nullopt_t());
+            ret = success<optional<ValueType>>(optional<ValueType>(std::move(overriddenValue)));


Good catch!

budrus · 2020-04-21T16:27:24Z

* `PosixShmMemoryProvider_Test.CreateMemory` test is failing on my machine. It's not related to the changes, isn't it?

Fine on my side. Also no changes there

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

elBoberido · 2020-05-12T14:32:04Z

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

-/// When different threads operate on it (e.g. application sends chunks and RouDi adds and removes queues), 
-/// a locking policy must be used that ensures consistent data in the ChunkDistributorData. 


doesn't your IDE automatically remove trailing whitespaces ... me starting IDE war and running away ;)

elBoberido · 2020-05-12T14:34:31Z

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


-    ChunkDistributor(MemberType_t* const chunkDistrubutorDataPtr) noexcept;
+    ChunkDistributor(cxx::not_null<MemberType_t* const> chunkDistrubutorDataPtr) noexcept;


@MatthiasKillat mentioned to use a reference instead of a cxx::not_null. What do you think?

Let's stick to a pointer with cxx::not_null for now.

elBoberido · 2020-05-12T15:41:17Z

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


-    ChunkDistributor(MemberType_t* const chunkDistrubutorDataPtr) noexcept;
+    ChunkDistributor(cxx::not_null<MemberType_t* const> chunkDistrubutorDataPtr) noexcept;

    ChunkDistributor(const ChunkDistributor& other) = delete;
    ChunkDistributor& operator=(const ChunkDistributor&) = delete;


aaargh, I cannot comment on the move assignment operator. With MemberType_t* const m_chunkDistrubutorDataPtr; and = default the move assignment operator is implicitly deleted, since it's not possible to assign something to a * const

elBoberido · 2020-05-12T15:41:37Z

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

@@ -110,10 +116,12 @@ class ChunkDistributor
    MemberType_t* getMembers() noexcept;

  private:
-    MemberType_t* m_chunkDistrubutorDataPtr{nullptr};
+    MemberType_t* const m_chunkDistrubutorDataPtr;


this implicitly deletes the move assignment operator

elBoberido · 2020-05-12T15:42:28Z

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

@@ -94,8 +84,10 @@ class ChunkQueue
    MemberType_t* getMembers() noexcept;

  private:
-    MemberType_t* m_chunkQueueDataPtr{nullptr};
+    MemberType_t* const m_chunkQueueDataPtr;


again, the * const implicitly deletes the defaulted move assignment operator

OK, so the best we can do to be moveable and the better API would then be a reference in the c'tor and the non const pointer as member. Most of these objects will be created with a ...Data object that was created before and can then be passed as reference

iceoryx_posh/test/moduletests/test_popo_chunk_distributor.cpp

elBoberido · 2020-05-13T10:35:52Z

iceoryx_utils/test/moduletests/test_cxx_variant_queue.cpp

@@ -108,10 +108,13 @@ TEST_F(VariantQueue_test, handlesOverflow)
 {
    PerformTestForQueueTypes([](uint64_t typeID) {
        VariantQueue<int, 2> sut(static_cast<VariantQueueTypes>(typeID));
+        // current SOFI can hold capacity +1 values, so push some more to ensure overflow


we should fix this surprising SOFI behaviour

…ut error Signed-off-by: Kraus Mathias (CC-AD/ESW1) <[email protected]>

Signed-off-by: Kraus Mathias (CC-AD/ESW1) <[email protected]>

Poehnl Michael (CC-AD/ESW1) added 6 commits April 4, 2020 23:23

iox-eclipse-iceoryx#25: first version chunk receiver data

052f86e

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

iox-eclipse-iceoryx#25: some review findings

5a9fdc7

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

iox-eclipse-iceoryx#25: first skeletons for chunk receiver and its test

33548dd

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

iox-eclipse-iceoryx#25: chunk receiver and first tests

a943543

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

iox-eclipse-iceoryx#25: more chunk receiver tests

dd4d8a6

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

iox-eclipse-iceoryx#25: some cosmetics

0dd4f60

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

mossmaurice requested changes Apr 14, 2020

View reviewed changes

mossmaurice added the enhancement New feature label Apr 14, 2020

mossmaurice assigned budrus Apr 14, 2020

Poehnl Michael (CC-AD/ESW1) added 3 commits April 14, 2020 20:37

iox-eclipse-iceoryx#25: review findings

dfbfa43

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

iox#25: more flexibility in chunk distributor

1e48520

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

iox#25: new lines end of file

db55440

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

budrus requested a review from mossmaurice April 14, 2020 20:48

Poehnl Michael (CC-AD/ESW1) added 3 commits April 15, 2020 15:30

iox#25: ChunkTuple in types

41acea4

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

iox#25: small fixes

0d51b36

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

iox#25: fixing variant queue

472aa97

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

mossmaurice reviewed Apr 20, 2020

View reviewed changes

iox#25: some more review findings

ff74def

Signed-off-by: Poehnl Michael (CC-AD/ESW1) <[email protected]>

mossmaurice previously approved these changes May 6, 2020

View reviewed changes

elBoberido reviewed May 12, 2020

View reviewed changes

elBoberido reviewed May 13, 2020

View reviewed changes

elBoberido added 2 commits May 13, 2020 16:16

iox-eclipse-iceoryx#25 check if queues can be added up to limit witho…

f8b98da

…ut error Signed-off-by: Kraus Mathias (CC-AD/ESW1) <[email protected]>

iox-eclipse-iceoryx#25 fix warnings for comparision with different sign

2b05e87

Signed-off-by: Kraus Mathias (CC-AD/ESW1) <[email protected]>

elBoberido dismissed mossmaurice’s stale review via 2b05e87 May 13, 2020 14:21

elBoberido approved these changes May 13, 2020

View reviewed changes

mossmaurice approved these changes May 13, 2020

View reviewed changes

mossmaurice merged commit 246085a into eclipse-iceoryx:master May 13, 2020

mossmaurice linked an issue May 13, 2020 that may be closed by this pull request

Support for n:m communciation (multiple senders on a topic / CaProID) #25

Closed

13 tasks

budrus deleted the iox-#25-chunk-receiver-building-block branch May 13, 2020 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #25 chunk receiver building block #84

Iox #25 chunk receiver building block #84

budrus commented Apr 8, 2020

mossmaurice left a comment •

edited

Loading

mossmaurice Apr 14, 2020

elBoberido May 12, 2020

mossmaurice Apr 14, 2020

mossmaurice Apr 14, 2020

budrus Apr 14, 2020

mossmaurice Apr 20, 2020

mossmaurice Apr 14, 2020

budrus Apr 14, 2020

mossmaurice Apr 14, 2020

budrus Apr 14, 2020

budrus Apr 14, 2020

mossmaurice Apr 14, 2020

mossmaurice Apr 14, 2020

budrus Apr 14, 2020

mossmaurice Apr 20, 2020

mossmaurice Apr 14, 2020

budrus Apr 14, 2020

mossmaurice Apr 20, 2020

mossmaurice Apr 14, 2020

budrus Apr 14, 2020

budrus commented Apr 14, 2020

mossmaurice left a comment

mossmaurice Apr 20, 2020

mossmaurice Apr 20, 2020

budrus commented Apr 21, 2020

elBoberido May 12, 2020

elBoberido May 12, 2020

mossmaurice May 13, 2020

elBoberido May 12, 2020

elBoberido May 12, 2020

elBoberido May 12, 2020

budrus May 13, 2020

elBoberido May 13, 2020

		/// When different threads operate on it (e.g. application sends chunks and RouDi adds and removes queues),
		/// a locking policy must be used that ensures consistent data in the ChunkDistributorData.

		#ifndef IOX_POSH_POPO_CHUNK_RECEIVER_HPP_
		#define IOX_POSH_POPO_CHUNK_RECEIVER_HPP_

		#ifndef IOX_POSH_POPO_CHUNK_RECEIVER_DATA_HPP_
		#define IOX_POSH_POPO_CHUNK_RECEIVER_DATA_HPP_

		// if the seqence number in the chunk info is set by the user, we still increment the internal one as this
		// might be needed by midddleware spcific evaluation (like in introspection)

		source/popo/building_blocks/chunk_queue_pusher.cpp
		source/popo/building_blocks/chunk_queue_popper.cpp


		ChunkDistributor(MemberType_t* const chunkDistrubutorDataPtr) noexcept;
		ChunkDistributor(cxx::not_null<MemberType_t* const> chunkDistrubutorDataPtr) noexcept;

		struct ChunkQueueData;

Iox #25 chunk receiver building block #84

Iox #25 chunk receiver building block #84

Conversation

budrus commented Apr 8, 2020

mossmaurice left a comment • 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

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 commented Apr 14, 2020

mossmaurice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budrus commented Apr 21, 2020

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

mossmaurice left a comment •

edited

Loading