Enable setting log levels in DDS implementation #124

sriramster · 2017-10-13T15:27:23Z

…l DDS api's

This patch enables support for three basic log levels
There needs an API inside the DDS specific rmw implementation i.e
rmw_fastrtps/rmw_opensplice etc to convert the enum to DDS specific variable

Signed-off-by: Sriram Raghunathan sriram.max@gmail.com

…l DDS api's * This patch enables support for three basic log levels * There needs an API inside the DDS specific rmw implementation i.e rmw_fastrtps/rmw_opensplice etc to convert the enum to DDS specific variable Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

dirk-thomas · 2017-10-13T16:20:35Z

The package rcutils defines logging levels and a logging API:https://github.com/ros2/rcutils/blob/6d6d5507362415fd377098c0b5973b91072dfafa/include/rcutils/logging.h#L60-L67 These should be used in rmw (or at least be referenced by rmw specific symbols).

sriramster · 2017-10-13T16:28:44Z

I'll change the patch accordingly, I did check rcutils for the same. But missed checking for the actual enum.

sriramster · 2017-10-13T17:51:52Z

Hi @dirk-thomas

I read the code from rcutils and rmw, find two ways of doing it.

To define rmw specific log_level enums and convert rcutil specific enum types to rmw specific ones.
Or to use rcutil types inside API's which might not need any changes inside rmw.h by doing that we'll have to include rcutils/logging.h inside rmw_implementation (I don't see this as the right approach) we are going against the design of rmw and rcutils already followed.

Let me know your thoughts on this.

dirk-thomas · 2017-10-13T18:03:55Z

I would suggest to define an rmw specific enum but use the enum values from rcutils to define their values. That way the rmw API uses rmw types but is still compatible with rcutils.

ros2#124 (comment) Using rcutils based log level, mapping them onto rmw specific ones. Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

sriramster · 2017-10-15T17:39:35Z

Hi @dirk-thomas

Have update the pull request based on the discussions above. Could you review the changes?

dirk-thomas

Beside the minor comments the added enum looks fine.

Please also add the new function declaration to actually pass the log severity.

dirk-thomas · 2017-10-16T17:33:49Z

rmw/include/rmw/types.h

@@ -229,6 +231,18 @@ typedef struct RMW_PUBLIC_TYPE rmw_message_info_t

 enum {RMW_QOS_POLICY_DEPTH_SYSTEM_DEFAULT = 0};

+// Creating a map of rcutil log level types to
+// rmw specific types. Check with dds available types
+// before using these.


Please remove the DDS reference from the comment since the rmw interface doesn't require it to be implemented by DDS.

dirk-thomas · 2017-10-16T17:34:08Z

rmw/include/rmw/types.h

+// Creating a map of rcutil log level types to
+// rmw specific types. Check with dds available types
+// before using these.
+enum RMW_PUBLIC_TYPE rmw_log_level_t


Please use consistent terms: severity instead of level.

dirk-thomas · 2017-10-16T17:34:36Z

rmw/include/rmw/types.h

+enum RMW_PUBLIC_TYPE rmw_log_level_t
+{
+  RMW_LOG_SEVERITY_DEBUG = RCUTILS_LOG_SEVERITY_DEBUG,
+  RMW_LOG_SEVERITY_INFO  = RCUTILS_LOG_SEVERITY_INFO,


Please remove the double space to align the equal sign.

Same next line.

…onsistency. Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

sriramster · 2017-10-20T07:08:59Z

@dirk-thomas fixed the comments. Have a look and let me know.

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

ros2/rmw#124 url on rmw. The patch enables setting log severity from userland applications. The parameter passed is defined inside rmw/include/types.h Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

dirk-thomas · 2017-10-20T16:05:00Z

Please also add the new function declaration to actually pass the log severity.

This comment is still pending.

sriramster · 2017-10-24T03:28:15Z

@dirk-thomas ros2/rmw_implementation#30 (comment) have implemented the same here.

dirk-thomas · 2017-10-24T04:04:14Z

The new function also needs to be declared in this file: https://github.com/ros2/rmw/blob/master/rmw/include/rmw/rmw.h

And then it needs to be implemented for all currently supported rmw implementations: rmw_fastrtps_cpp, rmw_connext_cpp, rmw_opensplice_cpp.

patches across ros2 sources for setting log levels inside DDS implementation Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

sriramster · 2017-10-25T07:31:36Z

@dirk-thomas the API is included in rmw/rmw.h. I'll update the rmw_dds specific ones in other commits.

…as'nt needed Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

ros2/rmw#124 * The API defined is used to set the logging level particular to the DDS implementation. * This change is specific to DDS type Fast-RTPS. * The file is named as rmw_configure since we could write API's of type 'system level config setting' types here. Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

dirk-thomas · 2017-10-30T23:16:42Z

I have added the phrase "Connect to " to the initial description of the related PRs so that they are being grouped on the Waffle.io board.

sriramster · 2017-11-06T08:32:45Z

@dirk-thomas I've updated the patch sets on all the linked commits. Could you review?

Except for one discussion which is still active.

sriramster · 2017-12-26T16:11:28Z

@dirk-thomas @dhood Could you please review this patch? This is connected to another approved patch by Dirk

dirk-thomas · 2018-01-03T00:14:21Z

I triggered a set of builds with these changes:

Linux
Linux-aarch64
macOS
Windows

I think the failures are because your branches are forked from a much older version. Can you please rebase all PRs to the latest state of the default branch. Then I can rerun the CI builds.

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

sriramster · 2018-01-04T06:59:43Z

@dirk-thomas Have update all the connecting PR's to master. Could you please trigger the CI build? Just asking, may I trigger the build in future? (If I'm allowed to do so, could you share an link or document for the same?)

dirk-thomas · 2018-01-04T18:57:23Z

Could you please trigger the CI build?

Linux skipped
Linux-aarch64
macOS
Windows aborted

The code doesn't compile. Please make sure to compile your changes locally, run the tests and once that passed successfully request a new round of CI build.

Just asking, may I trigger the build in future?

Currently we can't provide access to ci.ros2.org. In the future build.ros2.org should provide that service to the community.

sriramster · 2018-01-05T15:14:32Z

@dirk-thomas fixed the error, creeped in due to the change from error msg setting to parameterized logging made. Could you check now? I've compiled, tested. And, it is passing locally.

dirk-thomas · 2018-01-05T23:39:19Z

Some linters still fail (they should have also failed for you locally?):

Linux
Linux-aarch64
macOS
Windows

sriramster · 2018-01-07T05:16:18Z

@dirk-thomas Done. Missed them again! I need to practice "ros" style more. Sorry again.

dirk-thomas · 2018-01-08T19:50:47Z

Linux
Linux-aarch64
macOS
Windows

Please also address the compiler warnings reported: http://ci.ros2.org/job/ci_linux/3848/warnings22Result/new/

sriramster · 2018-01-09T16:34:03Z

@dirk-thomas checked, it is due to unused parameter "severity" with our new API's, would you like a (void) severity; declaration just to nullify the warning. The better solution would be to implement the logging usage with connext and opensplice. Let me know your thoughts on this.

dirk-thomas · 2018-01-09T16:55:07Z

As mentioned before implementing the currently empty functions is the goal. If you would like to merge the current set of PR before that happens please use (void)severity to avoid the warning.

sriramster · 2018-01-10T10:39:53Z

@dirk-thomas Thanks. I've made the necessary changes to prevent the warning on the connected patches.

dirk-thomas · 2018-01-11T04:33:31Z

Linux
Linux-aarch64
macOS
Windows

I can't say if the remaining test failure are related to the patch or have a different cause. Will need to be checked before merging...

sriramster · 2018-01-18T07:28:50Z

@dirk-thomas I checked those from the logs. They don't seem like regressions because of our changes. Could you let me know if you've found the cause for the same?

* The patch follows the discussion at this ros2/rmw#124 url on rmw. The patch enables setting log severity from userland applications. The parameter passed is defined inside rmw/include/types.h Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com> * Changing rmw_log_severity_t from const * to rmw_log_severity_t addressing comments Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

* Fastrtps supports rich logging facilities, which could be used on any ros2 development. * To enable the same, enable setting log levels from rmw_fastrtps layers, * Add generic logging level parameters to 'rmw.h' which would be used by other layers above like rcl/rclcpp. * Hide DDS specific implementation inside rmw_fastrtps. Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com> * The changes follow the discussions ros2/rmw#124 * The API defined is used to set the logging level particular to the DDS implementation. * This change is specific to DDS type Fast-RTPS. * The file is named as rmw_configure since we could write API's of type 'system level config setting' types here. Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com> * Rename file from rmw_fastrtps_configure to rmw_logging, which follows the patterns being used already. like rmw_node, rmw_publisher.. Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com> * - Fix, coding errors. - Renmae API to suite ros styles Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com> * Fix ament_cpplint warnings Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com> * Remove utility function to create a 'error' returnable API for setting logging Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com> * Fix, logging msg bug. * Modify logging mapping for type 'debug' from warning to info. Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com> * order log levels, use different local variable name, include unknown severity in error message * Fix error due to parameterized logging * Fix code style issues from uncrustify

dirk-thomas · 2018-01-18T18:10:24Z

Thank you for the patches.

Please try to follow up with pull requests to add the missing implementation for Connext and OpenSplice when you have a chance.

sriramster · 2018-01-19T05:43:44Z

Thank you. Sure, I'll look into it.

dirk-thomas mentioned this pull request Oct 13, 2017

Enable logging level manipulation from rmw_fastrtps ros2/rmw_fastrtps#156

Merged

dirk-thomas added the requires-changes label Oct 13, 2017

Update changes based on github discussion based on

2e2725c

ros2#124 (comment) Using rcutils based log level, mapping them onto rmw specific ones. Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

dirk-thomas reviewed Oct 16, 2017

View reviewed changes

mikaelarguedas added the in progress Actively being worked on (Kanban column) label Oct 18, 2017

Minor changes to existing code. Fix comment consistency and spacing c…

4d9a652

…onsistency. Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

Change variable name for consistency

c92c0f0

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

sriramster mentioned this pull request Oct 20, 2017

API to enable log severity setting. ros2/rmw_implementation#30

Merged

Add, rmw_log_set_severity API to rmw.h. This follows the set of

3eeeaba

patches across ros2 sources for setting log levels inside DDS implementation Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

Remove pointer to variable severity, from the github discussions it w…

b8cdfec

…as'nt needed Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

dirk-thomas removed the requires-changes label Oct 25, 2017

Fix enum declaration errors

546619a

Signed-off-by: Sriram Raghunathan <sriram.max@gmail.com>

This was referenced Oct 30, 2017

Logging manipulation API from rmw_connext ros2/rmw_connext#266

Merged

API to manipulate log setting macros ros2/rmw_opensplice#209

Merged

dirk-thomas added the requires-changes label Jan 3, 2018

Merge into master

a4b075c

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

Fix code styling issues

fe8daed

dirk-thomas merged commit d75889e into ros2:master Jan 18, 2018

dirk-thomas removed in progress Actively being worked on (Kanban column) requires-changes labels Jan 18, 2018

mikaelarguedas mentioned this pull request Mar 22, 2018

/home/pedestrian/ros2_java_ws/src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp ros2/rmw_implementation#38

Closed

christophebedard mentioned this pull request Apr 28, 2021

Make rcl call rmw_set_log_severity ros2/rcl#915

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable setting log levels in DDS implementation #124

Enable setting log levels in DDS implementation #124

sriramster commented Oct 13, 2017

dirk-thomas commented Oct 13, 2017

sriramster commented Oct 13, 2017

sriramster commented Oct 13, 2017

dirk-thomas commented Oct 13, 2017

sriramster commented Oct 15, 2017

dirk-thomas left a comment

dirk-thomas Oct 16, 2017

dirk-thomas Oct 16, 2017

dirk-thomas Oct 16, 2017

sriramster commented Oct 20, 2017

dirk-thomas commented Oct 20, 2017

sriramster commented Oct 24, 2017

dirk-thomas commented Oct 24, 2017 •

edited

Loading

sriramster commented Oct 25, 2017

dirk-thomas commented Oct 30, 2017

sriramster commented Nov 6, 2017

sriramster commented Dec 26, 2017

dirk-thomas commented Jan 3, 2018

sriramster commented Jan 4, 2018

dirk-thomas commented Jan 4, 2018

sriramster commented Jan 5, 2018

dirk-thomas commented Jan 5, 2018 •

edited

Loading

sriramster commented Jan 7, 2018

dirk-thomas commented Jan 8, 2018

sriramster commented Jan 9, 2018

dirk-thomas commented Jan 9, 2018

sriramster commented Jan 10, 2018

dirk-thomas commented Jan 11, 2018

sriramster commented Jan 18, 2018

dirk-thomas commented Jan 18, 2018

sriramster commented Jan 19, 2018

Enable setting log levels in DDS implementation #124

Enable setting log levels in DDS implementation #124

Conversation

sriramster commented Oct 13, 2017

dirk-thomas commented Oct 13, 2017

sriramster commented Oct 13, 2017

sriramster commented Oct 13, 2017

dirk-thomas commented Oct 13, 2017

sriramster commented Oct 15, 2017

dirk-thomas left a comment

Choose a reason for hiding this comment

dirk-thomas Oct 16, 2017

Choose a reason for hiding this comment

dirk-thomas Oct 16, 2017

Choose a reason for hiding this comment

dirk-thomas Oct 16, 2017

Choose a reason for hiding this comment

sriramster commented Oct 20, 2017

dirk-thomas commented Oct 20, 2017

sriramster commented Oct 24, 2017

dirk-thomas commented Oct 24, 2017 • edited Loading

sriramster commented Oct 25, 2017

dirk-thomas commented Oct 30, 2017

sriramster commented Nov 6, 2017

sriramster commented Dec 26, 2017

dirk-thomas commented Jan 3, 2018

sriramster commented Jan 4, 2018

dirk-thomas commented Jan 4, 2018

sriramster commented Jan 5, 2018

dirk-thomas commented Jan 5, 2018 • edited Loading

sriramster commented Jan 7, 2018

dirk-thomas commented Jan 8, 2018

sriramster commented Jan 9, 2018

dirk-thomas commented Jan 9, 2018

sriramster commented Jan 10, 2018

dirk-thomas commented Jan 11, 2018

sriramster commented Jan 18, 2018

dirk-thomas commented Jan 18, 2018

sriramster commented Jan 19, 2018

dirk-thomas commented Oct 24, 2017 •

edited

Loading

dirk-thomas commented Jan 5, 2018 •

edited

Loading