From 731d804f7df298bd1a66064e433664dec7db6f18 Mon Sep 17 00:00:00 2001
From: Fabian Peller-Konrad <fabian.peller-konrad@kit.edu>
Date: Tue, 22 Mar 2022 14:47:53 +0100
Subject: [PATCH] Fixed bug in skills.Manager gui plugin

---
 .../SkillManagerMonitorWidgetController.cpp   | 32 +++++++++++++------
 .../libraries/armem_skills/aron/Skill.xml     | 19 ++++++++---
 .../segment/ExecutableSkillLibrarySegment.cpp |  8 +++++
 .../server/segment/SkillEventSegment.cpp      | 12 ++-----
 .../segment/SkillExecutionRequestSegment.cpp  |  9 ++----
 .../manager/SkillManagerComponentPlugin.cpp   |  9 ++++++
 6 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/source/RobotAPI/gui-plugins/SkillManagerPlugin/SkillManagerMonitorWidgetController.cpp b/source/RobotAPI/gui-plugins/SkillManagerPlugin/SkillManagerMonitorWidgetController.cpp
index 3b57b3012..431ce92ca 100644
--- a/source/RobotAPI/gui-plugins/SkillManagerPlugin/SkillManagerMonitorWidgetController.cpp
+++ b/source/RobotAPI/gui-plugins/SkillManagerPlugin/SkillManagerMonitorWidgetController.cpp
@@ -195,13 +195,18 @@ namespace armarx
         }
 
         // remove providers from tree
-        for (int i = 0; i < widget.treeWidgetSkills->topLevelItemCount(); ++i)
+        int i = 0;
+        while (i < widget.treeWidgetSkills->topLevelItemCount())
         {
             QTreeWidgetItem* item = widget.treeWidgetSkills->topLevelItem(i);
             if (std::find(removedProviders.begin(), removedProviders.end(), item->text(0).toStdString()) != removedProviders.end())
             {
                 delete widget.treeWidgetSkills->takeTopLevelItem(i);
             }
+            else
+            {
+                ++i;
+            }
         }
 
         // add new providers
@@ -223,17 +228,26 @@ namespace armarx
         // update status
         for (int i = 0;  i < widget.treeWidgetSkills->topLevelItemCount(); ++i)
         {
-            QTreeWidgetItem* item = widget.treeWidgetSkills->topLevelItem(i);
-            auto providerName = item->text(0).toStdString();
-            for (int j = 0; j < item->childCount(); ++j)
+            try
             {
-                QTreeWidgetItem* skillItem = item->child(j);
-                auto skillName = skillItem->text(0).toStdString();
+                QTreeWidgetItem* item = widget.treeWidgetSkills->topLevelItem(i);
+                auto providerName = item->text(0).toStdString();
+                for (int j = 0; j < item->childCount(); ++j)
+                {
+                    QTreeWidgetItem* skillItem = item->child(j);
+                    auto skillName = skillItem->text(0).toStdString();
 
-                auto& providerPrx = skills.at(providerName).skillProviderPrx;
-                auto statusUpdate = providerPrx->getSkillExecutionStatus(skillName);
+                    auto& providerPrx = skills.at(providerName).skillProviderPrx;
+                    auto statusUpdate = providerPrx->getSkillExecutionStatus(skillName);
 
-                skillItem->setText(2, QString::fromStdString(ExecutionStatus2String.at(statusUpdate.status)));
+                    skillItem->setText(2, QString::fromStdString(ExecutionStatus2String.at(statusUpdate.status)));
+                }
+            }
+            catch (const std::exception& e)
+            {
+                // Perhaps the skill provider died after the check at the beginning of this method
+                // Continue
+                continue;
             }
         }
     }
diff --git a/source/RobotAPI/libraries/armem_skills/aron/Skill.xml b/source/RobotAPI/libraries/armem_skills/aron/Skill.xml
index 0e539ff08..b546afb89 100644
--- a/source/RobotAPI/libraries/armem_skills/aron/Skill.xml
+++ b/source/RobotAPI/libraries/armem_skills/aron/Skill.xml
@@ -34,7 +34,10 @@ The memory should look like the following:
                 <long />
             </ObjectChild>
 
-            <!-- accepted type as any type -->
+            <ObjectChild key='acceptedType'>
+                <string />
+            </ObjectChild>
+
         </Object>
 
         <Object name='armarx::skills::arondto::SkillExecutionRequest'>
@@ -50,7 +53,10 @@ The memory should look like the following:
                 <String />
             </ObjectChild>
 
-            <!-- ToDo: add params wih any type -->
+            <ObjectChild key='params'>
+                <AnyObject shared_ptr="1" />
+            </ObjectChild>
+
         </Object>
 
         <Object name='armarx::skills::arondto::SkillExecutionEvent'>
@@ -66,8 +72,13 @@ The memory should look like the following:
                 <String />
             </ObjectChild>
 
-            <!-- ToDo: add params with any type -->
-            <!-- ToDo: add result with any type -->
+            <ObjectChild key='params'>
+                <AnyObject shared_ptr="1" />
+            </ObjectChild>
+
+            <ObjectChild key='data'>
+                <AnyObject shared_ptr="1" />
+            </ObjectChild>
         </Object>
 
     </GenerateTypes>
diff --git a/source/RobotAPI/libraries/armem_skills/server/segment/ExecutableSkillLibrarySegment.cpp b/source/RobotAPI/libraries/armem_skills/server/segment/ExecutableSkillLibrarySegment.cpp
index 1419ddf44..7a4b75514 100644
--- a/source/RobotAPI/libraries/armem_skills/server/segment/ExecutableSkillLibrarySegment.cpp
+++ b/source/RobotAPI/libraries/armem_skills/server/segment/ExecutableSkillLibrarySegment.cpp
@@ -40,6 +40,12 @@ namespace armarx::skills::segment
             skillDescription.targets = desc.targets;
             skillDescription.timeoutMs = desc.timeoutMs;
 
+            if (desc.acceptedType)
+            {
+                auto t = aron::type::Object::FromAronObjectDTO(desc.acceptedType);
+                skillDescription.acceptedType = aron::converter::AronNlohmannJSONConverter::ConvertToNlohmannJSON(t).dump(2);
+            }
+
             armem::Commit commit;
             auto& entityUpdate = commit.add();
             entityUpdate.confidence = 1.0;
@@ -55,5 +61,7 @@ namespace armarx::skills::segment
     void ExecutableSkillLibraryCoreSegment::removeSkillProvider(const std::string& providerName)
     {
         skills.erase(providerName);
+
+        // TODO also add info about removed provider to memory?
     }
 }
diff --git a/source/RobotAPI/libraries/armem_skills/server/segment/SkillEventSegment.cpp b/source/RobotAPI/libraries/armem_skills/server/segment/SkillEventSegment.cpp
index 43e314fc8..47270f33b 100644
--- a/source/RobotAPI/libraries/armem_skills/server/segment/SkillEventSegment.cpp
+++ b/source/RobotAPI/libraries/armem_skills/server/segment/SkillEventSegment.cpp
@@ -11,7 +11,7 @@ namespace armarx::skills::segment
 {
 
     SkillEventCoreSegment::SkillEventCoreSegment(armem::server::MemoryToIceAdapter& iceMemory):
-        Base(iceMemory, CoreSegmentName)
+        Base(iceMemory, CoreSegmentName, armarx::skills::arondto::SkillExecutionRequest::ToAronType())
     {
     }
 
@@ -42,20 +42,14 @@ namespace armarx::skills::segment
         event.providerName = update.providerName;
         event.skillName = update.skillName;
         event.status = ExecutionStatus2String.at(update.status);
-
-        aron::data::DictPtr aron_params = nullptr;
-        if (update.usedParams) aron_params = std::make_shared<aron::data::Dict>(update.usedParams);
-
-        aron::data::DictPtr aron_data = nullptr;
-        if (update.data) aron_data = std::make_shared<aron::data::Dict>(update.data);
+        event.params = aron::data::Dict::FromAronDictDTO(update.usedParams);
+        event.data = aron::data::Dict::FromAronDictDTO(update.data);
 
         armem::MemoryID commitId = id();
         commitId.providerSegmentName = event.providerName;
         commitId.entityName = event.skillName;
 
         auto aron = event.toAron();
-        aron->addElement("return", aron_data); // how to name?!?
-        aron->addElement("params", aron_params);
 
         armem::Commit comm;
         auto& entityUpdate = comm.add();
diff --git a/source/RobotAPI/libraries/armem_skills/server/segment/SkillExecutionRequestSegment.cpp b/source/RobotAPI/libraries/armem_skills/server/segment/SkillExecutionRequestSegment.cpp
index 5068cb04d..39b2ace4b 100644
--- a/source/RobotAPI/libraries/armem_skills/server/segment/SkillExecutionRequestSegment.cpp
+++ b/source/RobotAPI/libraries/armem_skills/server/segment/SkillExecutionRequestSegment.cpp
@@ -12,7 +12,7 @@ namespace armarx::skills::segment
 {
 
     SkillExecutionRequestCoreSegment::SkillExecutionRequestCoreSegment(armem::server::MemoryToIceAdapter& iceMemory):
-        Base(iceMemory, CoreSegmentName/*, skills::arondto::SkillExecutionRequest::ToAronType()*/)
+        Base(iceMemory, CoreSegmentName, skills::arondto::SkillExecutionRequest::ToAronType())
     {
     }
 
@@ -35,12 +35,11 @@ namespace armarx::skills::segment
         // we got a skill execution request
         skills::arondto::SkillExecutionRequest request;
         request.fromAron(commitDataAron);
-        auto params = aron::data::Dict::DynamicCastAndCheck(commitDataAron->at("params")); // ToDo remov and add to request
 
         skills::manager::dto::SkillExecutionInfo info;
         info.providerName = request.providerName;
         info.skillName = request.skillName;
-        info.params = aron::data::Dict::ToAronDictDTO(params);
+        info.params = request.params->toAronDictDTO();
         return info;
     }
 
@@ -55,12 +54,10 @@ namespace armarx::skills::segment
         request.clientId = "";
         request.providerName = info.providerName;
         request.skillName = info.skillName;
+        request.params = aron::data::Dict::FromAronDictDTO(info.params);
 
         auto aron = request.toAron();
 
-        aron::data::DictPtr aron_params = aron::data::Dict::FromAronDictDTO(info.params);
-        aron->addElement("params", aron_params); // todo add as any type
-
         armem::MemoryID skillExecutionMemID = id();
         skillExecutionMemID.providerSegmentName = request.providerName;
         skillExecutionMemID.entityName = request.skillName;
diff --git a/source/RobotAPI/libraries/skills/manager/SkillManagerComponentPlugin.cpp b/source/RobotAPI/libraries/skills/manager/SkillManagerComponentPlugin.cpp
index 268c2cf4d..1b97951eb 100644
--- a/source/RobotAPI/libraries/skills/manager/SkillManagerComponentPlugin.cpp
+++ b/source/RobotAPI/libraries/skills/manager/SkillManagerComponentPlugin.cpp
@@ -30,6 +30,10 @@ namespace armarx
             ARMARX_INFO << "Adding a provider with name '" << info.providerName << "'.";
             skillProviderMap.insert({info.providerName, info.provider});
         }
+        else
+        {
+            ARMARX_INFO << "Trying to add a provider with name '" << info.providerName << "' but the provider already exists.";
+        }
     }
 
     void SkillManagerComponentPluginUser::removeProvider(const std::string& providerName, const Ice::Current&)
@@ -37,8 +41,13 @@ namespace armarx
         std::lock_guard l(skillProviderMapMutex);
         if (auto it = skillProviderMap.find(providerName); it != skillProviderMap.end())
         {
+            ARMARX_INFO << "Removing a provider with name '" << providerName << "'.";
             skillProviderMap.erase(it);
         }
+        else
+        {
+            ARMARX_INFO << "Trying to remove a provider with name '" << providerName << "' but it couldn't found.";
+        }
     }
 
     skills::provider::dti::SkillProviderMap SkillManagerComponentPluginUser::getSkillProviders(const Ice::Current&)
-- 
GitLab