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
58 changes: 58 additions & 0 deletions rmw_fastrtps_cpp/src/rmw_logging.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// 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 "rmw/error_handling.h"

#include "fastrtps/log/Log.h"

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

rmw_ret_t
rmw_set_log_severity(rmw_log_severity_t severity)
{
Log::Kind _severity;

switch (severity) {
case RMW_LOG_SEVERITY_WARN:
_severity = Log::Kind::Warning;
break;
case RMW_LOG_SEVERITY_INFO:
_severity = Log::Kind::Info;
break;
// Fast-RTPS supports the following logging 'Kind's.
// Error : Max priority
// Warning : Medium priority
// Info : Low priority
// From rmw logging severity there is FATAL severity type we map it
// to ERROR type of Fast-RTPS which has maximum priority
case RMW_LOG_SEVERITY_DEBUG:
_severity = Log::Kind::Warning;
Copy link
Member

Choose a reason for hiding this comment

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

This mapping still doesn't make any sense. The expectation is that DEBUG is more verbose than INFO. Therefore mapping it to Warning were INFO is mapped to Info seems wrong to me. I think this needs to be Info instead.

break;
case RMW_LOG_SEVERITY_ERROR:
case RMW_LOG_SEVERITY_FATAL:
_severity = Log::Kind::Error;
break;
default:
RMW_SET_ERROR_MSG("Unknown logging severity type, please check");
return RMW_RET_ERROR;
}

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