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

Info -> Debug #692

Merged
merged 1 commit into from
Sep 22, 2021
Merged

Info -> Debug #692

merged 1 commit into from
Sep 22, 2021

Conversation

vatanaksoytezer
Copy link
Contributor

@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #692 (ff87793) into main (fc3909e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #692   +/-   ##
=======================================
  Coverage   54.16%   54.16%           
=======================================
  Files         192      192           
  Lines       20180    20180           
=======================================
  Hits        10929    10929           
  Misses       9251     9251           
Impacted Files Coverage Δ
...ics_plugin_loader/src/kinematics_plugin_loader.cpp 68.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc3909e...ff87793. Read the comment docs.

Copy link
Contributor

@nbbrooks nbbrooks left a comment

Choose a reason for hiding this comment

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

Should we hold off merging this in until we have fix for the different expected kinematics namespace this revealed?

@vatanaksoytezer
Copy link
Contributor Author

Should we hold off merging this in until we have fix for the different expected kinematics namespace this revealed?

That's a good question: Maybe @JafarAbdi or @henningkayser can chime in?

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Should we hold off merging this in until we have fix for the different expected kinematics namespace this revealed?

The logs are not gone in this patch, so I don't see how the changed log-level does not reveal something anymore?
I expect you can configure the log-levels in ROS2 similarly to ROS's ROSCONSOLE_CONFIG_FILE? If involved developers do not use this approach I would say they should set it up in their workflows to avoid spamming logs for everyone.

@tylerjw
Copy link
Member

tylerjw commented Sep 21, 2021

I expect you can configure the log-levels in ROS2 similarly to ROS's ROSCONSOLE_CONFIG_FILE?

@v4hn last time I tried this was not yet a working feature in ROS 2.

@nbbrooks
Copy link
Contributor

I expect you can configure the log-levels in ROS2 similarly to ROS's ROSCONSOLE_CONFIG_FILE?

@v4hn last time I tried this was not yet a working feature in ROS 2.

Yeah I also don't think this is implemented yet. We can always change it back for debugging, I just think if we push this in it will be easy to ignore the loading issue (I'm unclear on how much of an issue the loading failure is though)

@v4hn
Copy link
Contributor

v4hn commented Sep 22, 2021

Excuse me? 😖

It seems it's still only supported to set the global log level on startup, but not the level of individual namespaces.
I really shouldn't be surprised about this, but I still am. MoveIt uses this feature extensively, so we (should) have a strong interest in implementing support for it upstream!

@JafarAbdi JafarAbdi merged commit 584618d into moveit:main Sep 22, 2021
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

5 participants