Iox #25 Introduce condition variable handling in RouDi and do a bit of housekeeping #258

mossmaurice · 2020-08-25T06:15:53Z

Introduce condition variable handling in RouDi
Rename RouDiMultiProcess to RouDi
Add noexcept to many methods
Minor cleanups of Minor RouDi cleanups #91

Things that will be done in the next PR of #25

Return expected in all creation methods of port_manager.hpp
Rename CString100 to something saner

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…ndition variable on RouDi side Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…yx#25-roudi-housekeeping-with-new-building-blocks

…riable Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…condition variables Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…ger and PortPool Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…xisting PoshRuntime errors Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…RouDiConfig Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iceoryx_posh/source/roudi/roudi.cpp

iceoryx_posh/source/roudi/roudi_process.cpp

…ocess Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…yx#25-roudi-housekeeping-with-new-building-blocks

… from Martin Hintz Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iceoryx_posh/include/iceoryx_posh/internal/roudi/port_manager.hpp

iceoryx_posh/include/iceoryx_posh/internal/roudi/roudi_process.hpp

iceoryx_posh/include/iceoryx_posh/internal/runtime/message_queue_interface.hpp

iceoryx_posh/include/iceoryx_posh/roudi/iceoryx_roudi_components.hpp

iceoryx_posh/source/roudi/port_manager.cpp

budrus · 2020-08-25T20:03:36Z

iceoryx_posh/source/roudi/roudi.cpp

+    {
+        if (message.getNumberOfElements() != 2)
+        {
+            LogError() << "Wrong number of parameter for \"MqMessageType::IMPL_CONDITION_VARIABLE\" from \""


Log or MODERATE error?

This is part of the internal message exchange protocol between Roudi and Runtime. Since we only send the messages ourselves (i.e. no external user does) it should be an error. I do not know what the consequences are, so at least MODERATE I think.

I'd propose to first define a concept with #130 and then adapt the code base accordingly.

Yes, I agree.

iceoryx_posh/source/runtime/posh_runtime.cpp

…yx#25-roudi-housekeeping-with-new-building-blocks

MatthiasKillat

I think we need a universal string interface on top of string to make using it less cumbersome while it still allows us to avoid dynamic memory. This is not scope of this PR however. Cstring100 needs to be removed, it was just an intermediate solution to make the transition to string easier.

Error levels need to be revisited and potentially corrected were marked. Otherwise the PR is fine from my point of view.

iceoryx_posh/include/iceoryx_posh/internal/roudi/port_manager.hpp

iceoryx_posh/source/roudi/port_manager.cpp

MatthiasKillat · 2020-08-26T09:42:22Z

iceoryx_posh/source/roudi/port_pool.cpp

+    }
+    else
+    {
+        errorHandler(Error::kPORT_POOL__CONDITION_VARIABLE_LIST_OVERFLOW, nullptr, ErrorLevel::MODERATE);


More like FULL, we have this logic precisely to avoid the overflow. But I am fine with all those names.

MatthiasKillat · 2020-08-26T09:47:11Z

iceoryx_posh/source/roudi/roudi.cpp

-                                       const std::string& processName)
+void RouDi::processMessage(const runtime::MqMessage& message,
+                           const iox::runtime::MqMessageType& cmd,
+                           const std::string& processName)


We cannot get rid of this std::string right now apparently. Could be done when we (maybe) add the universal string interface type I mentioned earlier. Up for discussion.

@marthtz will tackle this with #260

MatthiasKillat · 2020-08-26T09:49:59Z

iceoryx_posh/source/roudi/roudi.cpp

+    {
+        if (message.getNumberOfElements() != 2)
+        {
+            LogError() << "Wrong number of parameter for \"MqMessageType::IMPL_CONDITION_VARIABLE\" from \""


This is part of the internal message exchange protocol between Roudi and Runtime. Since we only send the messages ourselves (i.e. no external user does) it should be an error. I do not know what the consequences are, so at least MODERATE I think.

MatthiasKillat · 2020-08-26T09:52:33Z

iceoryx_posh/source/roudi/roudi.cpp

        }
        break;
    }
    case runtime::MqMessageType::KEEPALIVE:
    {
-        m_prcMgr.updateLivlinessOfProcess(processName);
+        m_prcMgr.updateLivelinessOfProcess(cxx::CString100(cxx::TruncateToCapacity, processName));


Having to introduce this explicit construction of the CString100 everytime is ugly and probably also has bad performance. Would be better if processName already was of the correct type. Can be done in a string improvement story.

Yep, as already mentioned, will be done in #260

iceoryx_posh/source/runtime/posh_runtime.cpp

iceoryx_posh/test/moduletests/test_roudi_shm.cpp

… from Michael Poehnl and Matthias Killat Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

dkroenke · 2020-08-26T13:26:55Z

iceoryx_posh/include/iceoryx_posh/roudi/roudi_app.hpp

-    /// @deprecated Will be deprecated soon, please port to RouDiApp(const CmdLineParser&, const RouDiConfig_T&)
-    RouDiApp(int argc, char* argv[], const mepoo::MePooConfig* mePooConfig = nullptr) noexcept;
+    /// @deprecated Please port to RouDiApp(const CmdLineParser&, const RouDiConfig_T&)
+    [[gnu::deprecated]]  RouDiApp(int argc, char* argv[], const mepoo::MePooConfig* mePooConfig = nullptr) noexcept;


please mark functions not with gnu::deprecated, because we do not want to introduce warnings. The use the doxygen tag /// @deprecated is already enough

As discussed previously with @elBoberido we follow a warning-free four step approach:

Mark the method /// @deprecated, internal usage is still allowed no warning printed

Refactor our own code to use new method

Mark the old method with [[gnu::deprecated]]

Remove the method with the next release

All review findings addressed

mossmaurice added 16 commits August 13, 2020 11:40

iox-eclipse-iceoryx#25 Add condition variable handling to PoshRuntime

ea2be30

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#25 Noexcept all the things and add handling of co…

855995f

…ndition variable on RouDi side Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#25 Saner variable names aka #RefactorLikeABob

679a211

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#25 Revert some name changes

4c29469

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Merge remote-tracking branch 'upstream/master' into iox-eclipse-iceor…

5fdcf44

…yx#25-roudi-housekeeping-with-new-building-blocks

iox-eclipse-iceoryx#25 Add test cases to PoshRuntime for condition va…

683a9b3

…riable Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#25 Remove 'signaler'

05dc18c

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#25 Adapt PortManager and RoudiProcess to support …

30f62bd

…condition variables Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#25 Add creation of condition variable in PortMana…

195283e

…ger and PortPool Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#25 Add error handling in PoshRuntime and rename e…

4005403

…xisting PoshRuntime errors Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#25 Remove unused variable in PoshRuntime

91c0472

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#91 Rename RouDiMultiProcess

53aacd3

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#91 Add error handling in PortManager

95a6c05

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#91 First step of deprecating the old RouDiApp c'tors

bf1c9c6

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#91 Add missing error handler call in RouDiComponents

5a3049c

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#91 Remove obsolete shared memory base address in …

5ddecf3

…RouDiConfig Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

mossmaurice added enhancement New feature refactoring Refactor code without adding features labels Aug 25, 2020

mossmaurice requested review from dkroenke, budrus and MatthiasKillat August 25, 2020 06:15

marthtz reviewed Aug 25, 2020

View reviewed changes

iceoryx_posh/source/roudi/roudi.cpp Outdated Show resolved Hide resolved

iceoryx_posh/source/roudi/roudi_process.cpp Outdated Show resolved Hide resolved

iceoryx_posh/source/roudi/roudi_process.cpp Outdated Show resolved Hide resolved

mossmaurice added 4 commits August 25, 2020 10:48

iox-eclipse-iceoryx#91 Replace std::string in PortManager and RouDiPr…

4f6251a

…ocess Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Merge remote-tracking branch 'upstream/master' into iox-eclipse-iceor…

a01d471

…yx#25-roudi-housekeeping-with-new-building-blocks

iox-eclipse-iceoryx#25 iox-eclipse-iceoryx#91 Address review comments…

e8571c4

… from Martin Hintz Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#25 Fix broken build

8af6d62

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

budrus previously requested changes Aug 25, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into iox-eclipse-iceor…

18df59c

…yx#25-roudi-housekeeping-with-new-building-blocks

MatthiasKillat requested changes Aug 26, 2020

View reviewed changes

iox-eclipse-iceoryx#25 iox-eclipse-iceoryx#91 Address review findings…

f3372f7

… from Michael Poehnl and Matthias Killat Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

mossmaurice requested review from MatthiasKillat and budrus August 26, 2020 12:18

MatthiasKillat previously approved these changes Aug 26, 2020

View reviewed changes

iox-eclipse-iceoryx#91 Keep fighting CString100 in RouDi context

7a7818f

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

mossmaurice dismissed MatthiasKillat’s stale review via 7a7818f August 26, 2020 13:54

dkroenke approved these changes Aug 26, 2020

View reviewed changes

MatthiasKillat approved these changes Aug 26, 2020

View reviewed changes

mossmaurice merged commit 9b20d9d into eclipse-iceoryx:master Aug 26, 2020

marthtz mentioned this pull request Sep 7, 2020

Name clash in RouDi process removeProcess() #271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #25 Introduce condition variable handling in RouDi and do a bit of housekeeping #258

Iox #25 Introduce condition variable handling in RouDi and do a bit of housekeeping #258

mossmaurice commented Aug 25, 2020

budrus Aug 25, 2020

MatthiasKillat Aug 26, 2020

mossmaurice Aug 26, 2020

MatthiasKillat Aug 26, 2020

MatthiasKillat left a comment •

edited

Loading

MatthiasKillat Aug 26, 2020

MatthiasKillat Aug 26, 2020

mossmaurice Aug 26, 2020

MatthiasKillat Aug 26, 2020

MatthiasKillat Aug 26, 2020

mossmaurice Aug 26, 2020

dkroenke Aug 26, 2020

mossmaurice Aug 26, 2020

Iox #25 Introduce condition variable handling in RouDi and do a bit of housekeeping #258

Iox #25 Introduce condition variable handling in RouDi and do a bit of housekeeping #258

Conversation

mossmaurice commented Aug 25, 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

MatthiasKillat 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

MatthiasKillat left a comment •

edited

Loading