[WIP] Ros1 conversion #1

cevans87 · 2019-02-07T20:07:32Z

DO NOT MERGE.
This pull request is currently incomplete. It is missing

Code coverage from 0% -> 80%
Documentation updates

Description of changes:

Pulled in master branch of github.com/aws-robotics/cloudwatchlogs-ros1
Made minimal changes to all files related to building and running cloudwatch_logger to support ROS2.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

juanrh

The general approach looks good, but I think some refactoring would be beneficial to improve the testeability of the code, and to avoid to repeat ourselves by moving code to the common package

cloudwatch_logger/include/cloudwatch_logger/log_node.h

cloudwatch_logger/src/log_node.cpp

juanrh · 2019-02-14T23:45:50Z

cloudwatch_logger/src/log_node.cpp

+  this->min_log_severity_ = min_log_severity;
+}
+
+void LogNode::RecordLogs(const rcl_interfaces::msg::Log::SharedPtr log_msg)


This method looks exactly the same as LogNode::RecordLogs in the ROS 1 version, but taking const rcl_interfaces::msg::Log::SharedPt instead of const rosgraph_msgs::Log::ConstPtr &. This code duplication is not very nice, instead we could work on moving this to cloudwatch_logs_common, for example which a simple type to represent logs, and a method RecordLogs that uses a conversion method of function from a ROS message into that log type. We could for example use the template method or the strategy pattern for the conversion function

juanrh · 2019-02-14T23:46:35Z

cloudwatch_logger/src/log_node.cpp

+  }
+}
+
+void LogNode::TriggerLogPublisher() { this->log_manager_->Service(); }


This is also a duplicate of the ROS 1 node https://github.com/aws-robotics/cloudwatchlogs-ros1/blob/master/cloudwatch_logger/src/log_node.cpp#L61

juanrh · 2019-02-14T23:49:12Z

cloudwatch_logger/src/log_node.cpp

+  return log_severity_level >= this->min_log_severity_;
+}
+
+const std::string LogNode::FormatLogs(const rcl_interfaces::msg::Log::SharedPtr & log_msg)


This is a slight variant of LogNode::FormatLogs of ROS 1 but we could also arrange the code to avoid repetition in several ways, for example converting to a ROS inpendent ROS type so we can have a single method FormatLogs that works on the conversion of a ROS 1 or ROS 2 message

wiki/images/cpu.svg

wiki/images/memory.svg

juanrh · 2019-02-14T23:54:03Z

cloudwatch_logger/src/log_client.cpp

+constexpr double kNodePublishFrequencyDefaultValue = 5.0;
+constexpr bool kNodeSubscribeToRosoutDefaultValue = true;
+
+Aws::AwsError ReadPublishFrequency(


The ROS 1 version of this node also has something similar to this, I personally prefer a param helper like in the Lex node for parameter handling. However Avishay commented once that he thinks that these methods are somewhat redundant, and that are duplicating what the parameter reader does. So I'd check with him to try to avoid this, ideally replacing all the ReadX methods for straightforward usages of ParameterReaderInterface

It would be nice to move some of this to the common libs as the params are specific to the client config. It would be nice to break out non-AWS config components and maybe we can avoid code duplication between each implementation.

cloudwatch_logger/src/log_client.cpp

ross-desmond · 2019-02-22T18:37:20Z

@juanrh has done a fantastic job summarizing changes that should be done before this goes into master. Since this PR is in an org public branch, let's keep this PR open and revisit it when re prioritizing.

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

dabonnie

I don't see any obvious issues, especially if it's working well.

mm318 · 2019-08-08T18:23:03Z

Unfortunately, this is not working at all right now because of ros2/rmw_fastrtps#265. I have not yet succeeded in getting the workaround (__log_disable_rosout:=true) to work either.

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

nburek

Haven't made it through everything yet, but here are some initial comments.

README.md

nburek · 2019-08-09T00:10:11Z

README.md

+Send logs in a ROS system to AWS CloudWatch Logs service.
+
+#### Subscribed Topics
+- **`/rosout_agg`**


Is the /other_topics part actually presenting correct information?

The way it's presented seems a bit odd, but it does have the ability to subscribe to whatever topics you define in the list in your config file.

I think it's saying all topics named /cloudwatch_logger/* are implicitly subscribed to. Is this information correct, @dabonnie?

I think it's saying all topics named /cloudwatch_logger/* are implicitly subscribed to. Is this information correct, @dabonnie?

If I read this (https://github.com/aws-robotics/cloudwatchlogs-ros1), it doesn't appear to match what you're saying.

Then explain to me what the sentence "The cloudwatch_logger subscribes to other topics if the parameter server has the topic names in cloudwatch_logger namespace" means.

Actually, don't bother explaining, just say if that sentence presents correct information or not.

I don't know the answer and would have to dig into ROS2 logging (https://index.ros.org/doc/ros2/Concepts/Logging/). Not sure if this is what @nburek was referencing when mentioning outdated.

As part of porting the application you might want to verify that the behavior detailed in the ROS1 README matches this.

Do you know if it applies to ROS1 logging? Because that sentence is from the ROS1 CloudWatch Logs README.

Yeah, if you look at the "topics" config param and the ReadSubscriberList method, the doc appears to be accurate.

cloudwatch_logger/CMakeLists.txt

nburek · 2019-08-09T00:50:16Z

cloudwatch_logger/launch/cloudwatch_logger.launch.py

+from launch_ros.actions import Node
+
+
+def generate_launch_description():


The ros1 CE had a lot more in its launch file that was meant as a way to provide automated integration with RoboMaker (such as detecting environment variables and such). Is this not possible in ROS2 launch files or did we decide not to support that anymore?

@AAlon, can you comment on this?

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

cloudwatch_logger/CMakeLists.txt

dabonnie · 2019-08-09T22:08:28Z

cloudwatch_logger/src/log_node.cpp

+
+bool LogNode::checkIfOnline(std_srvs::srv::Trigger::Request & request, std_srvs::srv::Trigger::Response & response)
+{
+  // AWS_LOGSTREAM_DEBUG(__func__, "received request " << request);


why commented out?

There's no operator<<() for std_srvs::srv::Trigger::Request for some reason.

cloudwatch_logger/src/log_node.cpp

dabonnie · 2019-08-09T22:11:19Z

cloudwatch_logger/src/log_node.cpp

+      ss << log_msg->level << " ";
+  }
+  ss << "[node name: " << log_msg->name << "] ";
+


Why is this missing the topics section from the ROS1 code?

It's not available in rcl_interfaces::msg::Log.

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

Cameron Evans added 2 commits February 7, 2019 10:59

Source from aws-robotics/cloudwatchlogs-ros1 master branch

13e35ab

Convert all code, build files, launch files to ROS2.

c0de912

cevans87 requested review from nburek, juanrh and ross-desmond February 7, 2019 20:07

cevans87 changed the title ~~Ros1 conversion~~ [WIP] Ros1 conversion Feb 7, 2019

juanrh suggested changes Feb 15, 2019

View reviewed changes

mm318 added 2 commits August 7, 2019 12:25

port cloudwatch_logger offline support to ros2

8277bd7

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

fix "ros2 launch" behavior

8480f81

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

dabonnie approved these changes Aug 8, 2019

View reviewed changes

workaround fastrtps typesupport crash

eed3800

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

nburek reviewed Aug 9, 2019

View reviewed changes

mm318 added 4 commits August 8, 2019 22:56

correct the information in the README and cleanup the CMakeLists.txt

9a7896c

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

add log_node_param_helper unit test

0f6d6ee

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

add disclaimer to README

d01cc69

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

remove unnecessary files

a1f6fe5

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

dabonnie suggested changes Aug 9, 2019

View reviewed changes

address PR comments

995e19f

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

mm318 merged commit 8323a52 into master Aug 9, 2019

timrobotson deleted the ros1-conversion branch August 12, 2019 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Ros1 conversion #1

[WIP] Ros1 conversion #1

cevans87 commented Feb 7, 2019

juanrh left a comment

juanrh Feb 14, 2019

juanrh Feb 14, 2019

juanrh Feb 14, 2019

juanrh Feb 14, 2019

dabonnie May 15, 2019

ross-desmond commented Feb 22, 2019

dabonnie left a comment

mm318 commented Aug 8, 2019

nburek left a comment

nburek Aug 9, 2019

mm318 Aug 9, 2019

nburek Aug 9, 2019

mm318 Aug 9, 2019

dabonnie Aug 9, 2019

mm318 Aug 9, 2019

mm318 Aug 9, 2019

dabonnie Aug 9, 2019

mm318 Aug 9, 2019

dabonnie Aug 9, 2019

nburek Aug 9, 2019

mm318 Aug 9, 2019

dabonnie Aug 9, 2019

mm318 Aug 9, 2019

dabonnie Aug 9, 2019

mm318 Aug 9, 2019

		from launch_ros.actions import Node


		def generate_launch_description():

[WIP] Ros1 conversion #1

[WIP] Ros1 conversion #1

Conversation

cevans87 commented Feb 7, 2019

juanrh left a 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

ross-desmond commented Feb 22, 2019

dabonnie left a comment

Choose a reason for hiding this comment

mm318 commented Aug 8, 2019

nburek left a 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