ros-drivers / odva_ethernetip

Library implementing ODVA EtherNet/IP (Industrial Protocol).
94 stars 48 forks source link

Too verbose logging #13

Closed reinzor closed 5 years ago

reinzor commented 5 years ago

We are using the Session::sendCommand method in a loop of 25 Hz. Now it also logs at to console on every iteration due to https://github.com/ros-drivers/odva_ethernetip/blob/indigo-devel/src/session.cpp#L173. Shouldn't we replace the std::cout and std::cerr by ROS logging and change a lot of these logging statements to debug?

gavanderhoorn commented 5 years ago

The base library in this repository does not depend on anything "ROS" right now, and it would be nice to keep it that way.

rosconsole console_bridge is no longer really "ROS" per se (with Debian releases available, etc), but it's something to consider.

reinzor commented 5 years ago

I agree, but since it is part of roscpp, the dependency has to be added. I agree that keeping this library ROS independent is desirable. Do you have other suggestions for suppressing the console output?

gavanderhoorn commented 5 years ago

rosconsole is not part of roscpp, it never was. It was part of ros_comm, but was migrated to its own repository: ros/rosconsole.

But I was really thinking of console_bridge.

Do you have other suggestions for suppressing the console output?

removing/commenting the statements? :)

But seriously: using some form of a logging framework instead of plain output streams would be a good idea.

reinzor commented 5 years ago

console_bridge does not seem to be present in kinetic, lunar and melodic as debian. IMO there are two options then:

@mikepurvis (as a maintainer) , what are your thoughts about this?

gavanderhoorn commented 5 years ago

console_bridge does not seem to be present in kinetic, lunar and melodic as debian

Check the console_bridge wiki page:

This package has been released upstream. This is just a wrapper for backwards compatibility you should depend on libconsole-bridge-dev directly.

reinzor commented 5 years ago

Thanks, didn't get that.

reinzor commented 5 years ago

Solved by #15