Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable logging level manipulation from rmw_fastrtps #156

Merged
merged 14 commits into from
Jan 18, 2018
Merged
1 change: 1 addition & 0 deletions rmw_fastrtps_cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ add_library(rmw_fastrtps_cpp
src/identifier.cpp
src/namespace_prefix.cpp
src/qos.cpp
src/rmw_logging.cpp
src/rmw_client.cpp
src/rmw_compare_gids_equal.cpp
src/rmw_count.cpp
Expand Down
56 changes: 56 additions & 0 deletions rmw_fastrtps_cpp/src/rmw_logging.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright 2016 Proyectos y Sistemas de Mantenimiento SL (eProsima).
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "rmw/rmw.h"

#include "fastrtps/log/Log.h"

extern "C"
{
using eprosima::fastrtps::Log;

eprosima::fastrtps::Log::Kind convert_rmw_severity_type(rmw_log_severity_t severity)
{
switch(severity)
{
case RMW_LOG_SEVERITY_WARN:
return Log::Kind::Warning;
case RMW_LOG_SEVERITY_INFO:
return Log::Kind::Info;
/* Fast-RTPS supports the following logging 'Kind's.
* Error : Max priority
* Warning : Medium priority
* Info : Low priority
* From rmw logging severity there are FATAL & DEBUG types as well
* We map them to ERROR type of Fast-RTPS which has maximum priority
*/
case RMW_LOG_SEVERITY_ERROR:
case RMW_LOG_SEVERITY_FATAL:
case RMW_LOG_SEVERITY_DEBUG:
return Log::Kind::Error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that Log::Kind::Error is a reasonable mapping for RMW_LOG_SEVERITY_DEBUG.

default:
// Fallback to Info if undefined types
return Log::Kind::Info;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code block currently doesn't pass the linter tests. Please run the linters (either with ament test or directly ament_uncrustify / ament_cpplint) and address the style warnings.


rmw_ret_t
rmw_set_log_severity(rmw_log_severity_t severity)
{
Log::Kind _severity = convert_rmw_severity_type(severity);
eprosima::fastrtps::Log::SetVerbosity(_severity);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the severity can't be mapped (the default case in the other function) it might be better to print an error message rather than setting a fallback level without letting the user know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dirk-thomas ok. In that case I'll have to change the API to return a type rmw_ret_t. And, may I use RMW_SET_ERROR_MSG for error setting on unknown types? There has been instances where inside rmw RCUTILS macro for error logging has been used. Let me know on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dirk-thomas Could you let me know your thoughts on the previous comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this function is private, only being used once in the same .cpp file, and the calling function is only a couple of lines ong I would suggest to just remove the separate function and move the implementation into rmw_set_log_severity. That would simplify the code. rmw_set_log_severity can then indeed return an error code and set the error message accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Thanks. I'll make the necessary changes and update the commit.


return RMW_RET_OK;
}
} // extern "C"
2 changes: 0 additions & 2 deletions rmw_fastrtps_cpp/src/rmw_node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ create_node(
return nullptr;
}

eprosima::fastrtps::Log::SetVerbosity(eprosima::fastrtps::Log::Error);

// Declare everything before beginning to create things.
rmw_guard_condition_t * graph_guard_condition = nullptr;
CustomParticipantInfo * node_impl = nullptr;
Expand Down