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

using LOGNAME variable rather than strings for logging #1079

Merged
merged 1 commit into from
Oct 16, 2018
Merged

using LOGNAME variable rather than strings for logging #1079

merged 1 commit into from
Oct 16, 2018

Conversation

mlautman
Copy link
Contributor

@mlautman mlautman commented Oct 8, 2018

Description

Using name_ variable for logging rather than "robot_state"

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Created tests, which fail without this PR reference
  • Decide if this should be cherry-picked to other current ROS branches
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

In other files, we have used a static variable LOGNAME, which I would prefer here as well.
I think, we shouldn't clutter the class's API with another member variable.
If you insist in using a member variable, I would prefer to name it LOGNAME as well. What do you think?

@mlautman
Copy link
Contributor Author

@rhaschke I'm not sure that declaring the variable outside the class would work. In this case name_ is used in both robot_state.h and .cpp so we would have to declare the string in the .h. This results in redefinition errors if we have multiple classes that share namespace as is the case with moveit_core. Philosophically, I don't see the rational for keeping this data outside of the class in a shared namespace as this variable is being used by the class to relay to the logger it's identity. While planning_scene_monitor uses LOGNAME, it is the only one that does so. Since trajectory_execution_manager uses name_, I see these as equally valid. name_ seems better to me because it implies the name of the class whereas logname_ implies the name of the logger. (Also, since it is a member variable, it would need to be lower case)

@rhaschke
Copy link
Contributor

The single use case in .h I would move into .cpp indeed. There is no reason for this function to be inline.
But my key point is that the name_ member suggests that a RobotState instance can be named. The LOGNAME variable will much better emphasize the usage of this variable.

@davetcoleman
Copy link
Member

I agree with @rhaschke's point that the name_ of a RobotState would lead me to expect "panda" or "pr2" for example.

I also agree that there shouldn't be function code in the .h file and that should be moved. inline is not necessary with modern compilers.

Perhaps CLASSNAME would be best, but I don't like the two extra characters so agree with LOGNAME

Please document this new "standard" on the Contributing page:
http://moveit.ros.org/documentation/contributing/code/

@mlautman
Copy link
Contributor Author

@rhaschke Requested changes made. I also made the same formatting change to robot_model.cpp and robot_state/conversions.cpp.

@mlautman mlautman changed the title using name_ variable rather than robot_state string using LOGNAME variable rather than strings for logging Oct 16, 2018
@rhaschke rhaschke merged commit 2c2b3f6 into moveit:kinetic-devel Oct 16, 2018
rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request Oct 16, 2018
@davetcoleman davetcoleman deleted the logging-cleanup branch October 17, 2018 00:50
pull bot pushed a commit to shadow-robot/moveit that referenced this pull request Sep 3, 2020
JafarAbdi pushed a commit to JafarAbdi/moveit that referenced this pull request Mar 24, 2022
* Off by one in getAverageSegmentDuration

* Case for one waypoint

Co-authored-by: AndyZe <andyz@utexas.edu>

* Warn if too few waypoints to get duration

Co-authored-by: AndyZe <andyz@utexas.edu>

* Discount first duration_from_previous from average duration if it is 0

* Restore empty duration from previous check as per Andy's suggestion

* Changed warning message for case with 1 segment with 0 duration to be distinct from empty durations

Co-authored-by: AndyZe <andyz@utexas.edu>
Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
Co-authored-by: AndyZe <zelenak@picknik.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants