Fix small details (#14)

* Add last implementation details

Signed-off-by: jparisu <[email protected]>

* Add details

Signed-off-by: jparisu <[email protected]>

* uncrustify

Signed-off-by: jparisu <[email protected]>

---------

Signed-off-by: jparisu <[email protected]>

Author: jparisu

.github/workflows/test.yml

@@ -67,7 +67,6 @@ on:
     - cron: '0 5 * * *'
 
 env:
-  # TODO: add more packages
   code_packages_names: 'fastddsspy_participants fastddsspy_yaml fastddsspy_tool'
   default_dependencies_artifact_postfix: '_nightly'
 

docs/rst/developer_manual/installation/sources/linux.rst

@@ -193,7 +193,6 @@ Colcon installation
     Being based on CMake_, it is possible to pass the CMake configuration options to the :code:`colcon build` command.
     For more information on the specific syntax, please refer to the `CMake specific arguments <https://colcon.readthedocs.io/en/released/reference/verb/build.html#cmake-specific-arguments>`_ page of the colcon_ manual.
 
-.. TODO check if this is correctly parsed, because in local it does not render correctly
 
 CMake installation
 ==================

docs/rst/developer_manual/installation/sources/windows.rst

@@ -229,8 +229,6 @@ Colcon installation (recommended)
     For more information on the specific syntax, please refer to the `CMake specific arguments <https://colcon.readthedocs.io/en/released/reference/verb/build.html#cmake-specific-arguments>`_ page of the colcon_ manual.
 
 
-.. TODO check if this is correctly parsed, because in local it does not render correctly
-
 CMake installation
 ==================
 

fastddsspy_participants/include/fastddsspy_participants/configuration/SpyParticipantConfiguration.hpp

@@ -22,7 +22,7 @@ namespace spy {
 namespace participants {
 
 /**
- * TODO
+ * TODO comment
  */
 struct SpyParticipantConfiguration : ddspipe::participants::ParticipantConfiguration
 {

fastddsspy_participants/include/fastddsspy_participants/model/DataStreamer.hpp

@@ -30,7 +30,7 @@ namespace spy {
 namespace participants {
 
 /**
- * TODO
+ * TODO comment
  */
 class DataStreamer : public TopicRateCalculator
 {

fastddsspy_participants/include/fastddsspy_participants/model/NetworkDatabase.hpp

@@ -27,7 +27,7 @@ namespace spy {
 namespace participants {
 
 /**
- * TODO
+ * TODO comment
  */
 struct NetworkDatabase
 {

fastddsspy_participants/include/fastddsspy_participants/model/SpyModel.hpp

@@ -22,7 +22,7 @@ namespace spy {
 namespace participants {
 
 /**
- * TODO
+ * TODO comment
  */
 class SpyModel : public NetworkDatabase, public DataStreamer
 {

fastddsspy_participants/include/fastddsspy_participants/model/TopicRateCalculator.hpp

@@ -28,7 +28,7 @@ namespace spy {
 namespace participants {
 
 /**
- * TODO
+ * TODO comment
  */
 class TopicRateCalculator : public ddspipe::participants::ISchemaHandler
 {

fastddsspy_participants/include/fastddsspy_participants/participant/SpyDdsParticipant.hpp

@@ -25,7 +25,7 @@ namespace spy {
 namespace participants {
 
 /**
- * TODO
+ * TODO comment
  */
 class SpyDdsParticipant : public ddspipe::participants::DynTypesParticipant
 {

fastddsspy_participants/include/fastddsspy_participants/participant/SpyParticipant.hpp

@@ -27,13 +27,13 @@ namespace spy {
 namespace participants {
 
 /**
- * TODO
+ * TODO comment
  */
 class SpyParticipant : public ddspipe::participants::SchemaParticipant
 {
 public:
 
-    // TODO
+    // TODO comment
     SpyParticipant(
             const std::shared_ptr<ddspipe::participants::ParticipantConfiguration>& participant_configuration,
             const std::shared_ptr<ddspipe::core::PayloadPool>& payload_pool,

fastddsspy_participants/include/fastddsspy_participants/types/EndpointInfo.hpp

@@ -28,7 +28,7 @@ namespace participants {
 using EndpointInfo = ddspipe::core::types::Endpoint;
 
 /**
- * TODO
+ * TODO comment
  */
 struct EndpointInfoData : public ddspipe::core::IRoutingData
 {

fastddsspy_participants/include/fastddsspy_participants/types/ParticipantInfo.hpp

@@ -25,7 +25,7 @@ namespace spy {
 namespace participants {
 
 /**
- * TODO
+ * TODO comment
  */
 struct ParticipantInfo
 {
@@ -40,7 +40,7 @@ struct ParticipantInfo
 };
 
 /**
- * TODO
+ * TODO comment
  */
 struct ParticipantInfoData : public ddspipe::core::IRoutingData
 {

fastddsspy_participants/src/cpp/model/DataStreamer.cpp

@@ -87,30 +87,50 @@ void DataStreamer::add_data(
 {
     TopicRateCalculator::add_data(topic, data);
 
-    std::shared_lock<std::shared_timed_mutex> _(mutex_);
+    fastrtps::types::DynamicType_ptr dyn_type;
 
-    if (activated_)
     {
-        // If all activated, add it only if schema is available, otherwise skip
-        if (activated_all_ && !is_topic_type_discovered_nts_(topic))
+        std::shared_lock<std::shared_timed_mutex> _(mutex_);
+
+        if (!activated_)
         {
+            // If not activated, do nothing.
             return;
         }
 
-        if (activated_all_ || activated_topic_ == topic)
+        if (activated_all_)
+        {
+            if (!is_topic_type_discovered_nts_(topic))
+            {
+                // If all activated, add it only if schema is available, otherwise skip
+                return;
+            }
+        }
+        else
         {
-            auto it = types_discovered_.find(topic.type_name);
-            if (it == types_discovered_.end())
+            if (!(activated_topic_ == topic))
             {
-                // The topic that is supposed to be activated has no associated type. This should not happen
-                utils::tsnh(STR_ENTRY
-                        << "Topic <" << topic << "> must not be activated if its type is not registered.");
+                // If not all activated, and this is not the activated topic skip
+                return;
             }
+        }
 
-            // TODO: make map search more safe
-            (*callback_)(topic, it->second, data);
+        auto it = types_discovered_.find(topic.type_name);
+        if (it == types_discovered_.end())
+        {
+            // The topic that is supposed to be activated has no associated type. This should not happen
+            utils::tsnh(STR_ENTRY
+                    << "Topic <" << topic << "> must not be activated if its type is not registered.");
+        }
+        else
+        {
+            dyn_type = it->second;
         }
     }
+
+    // This should be called without mutex taken
+    // Here should only arrive if it must call callback. Otherwise must return somewhere before
+    (*callback_)(topic, dyn_type, data);
 }
 
 bool DataStreamer::is_topic_type_discovered(

fastddsspy_participants/src/cpp/model/TopicRateCalculator.cpp

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License\.
 
+#include <limits>
+
 #include <fastddsspy_participants/model/TopicRateCalculator.hpp>
 
 namespace eprosima {
@@ -54,8 +56,8 @@ TopicRateCalculator::RateType TopicRateCalculator::get_topic_rate(
     float seconds_elapsed = data.last_data_time.seconds() - data.first_data_time.seconds();
     if (seconds_elapsed == 0)
     {
-        // TODO decide what to do in this case
-        return data.data_received;
+        // If not enough data to set the rate, return invalid value
+        return std::numeric_limits<float>::infinity();
     }
 
     return static_cast<float>(data.data_received) / seconds_elapsed;

fastddsspy_participants/src/cpp/participant/SpyDdsParticipant.cpp

@@ -84,7 +84,8 @@ void SpyDdsParticipant::on_subscriber_discovery(
     }
 
     EndpointInfo endpoint_info = create_endpoint_from_info_(info);
-    // TODO: check if this notification arrives when a participants leaves
+
+    // If participant left or dropped, this notification arrives as well
     endpoint_info.active = !(info.status == fastrtps::rtps::ReaderDiscoveryInfo::DISCOVERY_STATUS::REMOVED_READER);
 
     internal_notify_endpoint_discovered_(endpoint_info);
@@ -101,7 +102,8 @@ void SpyDdsParticipant::on_publisher_discovery(
     }
 
     EndpointInfo endpoint_info = create_endpoint_from_info_(info);
-    // TODO: check if this notification arrives when a participants leaves
+
+    // If participant left or dropped, this notification arrives as well
     endpoint_info.active = !(info.status == fastrtps::rtps::WriterDiscoveryInfo::DISCOVERY_STATUS::REMOVED_WRITER);
 
     internal_notify_endpoint_discovered_(endpoint_info);

fastddsspy_participants/src/cpp/visualization/ModelParser.cpp

@@ -218,7 +218,7 @@ std::vector<ComplexEndpointData> ModelParser::writers_verbose(
 
     for (const auto& endpoint : model.endpoint_database_)
     {
-        if (endpoint.second.is_writer())
+        if (endpoint.second.active && endpoint.second.is_writer())
         {
             result.push_back(writers(model, endpoint.second.guid));
         }
@@ -255,7 +255,7 @@ std::vector<ComplexEndpointData> ModelParser::readers_verbose(
 
     for (const auto& endpoint : model.endpoint_database_)
     {
-        if (endpoint.second.is_reader())
+        if (endpoint.second.active && endpoint.second.is_reader())
         {
             result.push_back(readers(model, endpoint.second.guid));
         }
@@ -284,7 +284,10 @@ std::set<eprosima::ddspipe::core::types::DdsTopic> get_topics(
     std::set<eprosima::ddspipe::core::types::DdsTopic> result;
     for (const auto& endpoint : model.endpoint_database_)
     {
-        result.insert(endpoint.second.topic);
+        if (endpoint.second.active)
+        {
+            result.insert(endpoint.second.topic);
+        }
     }
     return result;
 }
@@ -296,7 +299,7 @@ ddspipe::core::types::DdsTopic ModelParser::get_topic(
 
     for (const auto& endpoint : model.endpoint_database_)
     {
-        if (endpoint.second.topic.m_topic_name == topic_name)
+        if (endpoint.second.active && endpoint.second.topic.m_topic_name == topic_name)
         {
             return endpoint.second.topic;
         }
@@ -317,7 +320,7 @@ std::vector<SimpleTopicData> ModelParser::topics(
         int datareaders = 0;
         for (const auto& endpoint : model.endpoint_database_)
         {
-            if (endpoint.second.topic.m_topic_name == topic.m_topic_name)
+            if (endpoint.second.active && endpoint.second.topic.m_topic_name == topic.m_topic_name)
             {
                 if (endpoint.second.is_reader())
                 {
@@ -383,7 +386,7 @@ ComplexTopicData ModelParser::topics(
 
     for (const auto& it : model.endpoint_database_)
     {
-        if (topic.m_topic_name == it.second.topic.m_topic_name)
+        if (it.second.active && topic.m_topic_name == it.second.topic.m_topic_name)
         {
             if (it.second.is_reader())
             {

fastddsspy_tool/src/cpp/tool/Backend.hpp

@@ -53,34 +53,34 @@ class Backend
 
 protected:
 
-    // TODO
+    // TODO comment
     yaml::Configuration configuration_;
 
-    //! TODO
+    //! TODO comment
     std::shared_ptr<ddspipe::core::PayloadPool> payload_pool_;
 
-    //! TODO
+    //! TODO comment
     std::shared_ptr<ddspipe::core::DiscoveryDatabase> discovery_database_;
 
-    //! TODO
+    //! TODO comment
     std::shared_ptr<ddspipe::core::AllowedTopicList> allowed_topics_;
 
-    //! TODO
+    //! TODO comment
     std::shared_ptr<eprosima::utils::SlotThreadPool> thread_pool_;
 
-    //! TODO
+    //! TODO comment
     std::shared_ptr<ddspipe::core::ParticipantsDatabase> participant_database_;
 
-    //! TODO
+    //! TODO comment
     std::shared_ptr<eprosima::spy::participants::SpyModel> model_;
 
-    //! TODO
+    //! TODO comment
     std::shared_ptr<eprosima::spy::participants::SpyDdsParticipant> dds_participant_;
 
-    //! TODO
+    //! TODO comment
     std::shared_ptr<eprosima::spy::participants::SpyParticipant> spy_participant_;
 
-    //! TODO
+    //! TODO comment
     std::unique_ptr<ddspipe::core::DdsPipe> pipe_;
 
 };

fastddsspy_tool/src/cpp/tool/Command.hpp

@@ -39,14 +39,14 @@ eProsima_ENUMERATION_BUILDER(
     CommandValue,
         {
             { CommandValue::empty_command COMMA {""}} COMMA
-            { CommandValue::help COMMA {"h" COMMA "help" COMMA "H" COMMA "man"}} COMMA
-            { CommandValue::exit COMMA {"q" COMMA "x" COMMA "quit" COMMA "quit()" COMMA "exit" COMMA "exit()"}} COMMA
+            { CommandValue::help COMMA {"help" COMMA "h" COMMA "H" COMMA "man"}} COMMA
+            { CommandValue::exit COMMA {"quit" COMMA "q" COMMA "Q" COMMA "quit()" COMMA "exit" COMMA "x" COMMA "exit()"}} COMMA
             { CommandValue::version COMMA {"version" COMMA "v" COMMA "V"}} COMMA
             { CommandValue::participant COMMA {"participant" COMMA "participants" COMMA "p" COMMA "P"}} COMMA
-            { CommandValue::datawriter COMMA {"datawriter" COMMA "datawriters" COMMA "writer" COMMA "writers" COMMA "publication" COMMA "publications" COMMA "w" COMMA "W"}} COMMA
-            { CommandValue::datareader COMMA {"datareader" COMMA "datareaders" COMMA "reader" COMMA "readers" COMMA "subscription" COMMA "subscriptions" COMMA "r" COMMA "R"}} COMMA
+            { CommandValue::datawriter COMMA {"datawriter" COMMA "datawriters" COMMA "w" COMMA "W" COMMA "writer" COMMA "writers" COMMA "publication" COMMA "publications"}} COMMA
+            { CommandValue::datareader COMMA {"datareader" COMMA "datareaders" COMMA "r" COMMA "R" COMMA "reader" COMMA "readers" COMMA "subscription" COMMA "subscriptions"}} COMMA
             { CommandValue::topic COMMA {"topic"  COMMA "topics" COMMA "t" COMMA "T"}} COMMA
-            { CommandValue::print COMMA {"print" COMMA "show"}} COMMA
+            { CommandValue::print COMMA {"print" COMMA "show" COMMA "s" COMMA "S"}} COMMA
         }
     );