API review

Proposer: Josh Faust

Present at review:

  • List reviewers
    • Eric
    • Brian
    • Rob
    • Tully
    • Conor
    • Jeremy

Question / concerns / comments

For reviewers: Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting

Brian:

  • How are output handlers added? In particular, how does output get routed to rosout?
  • How is verbosity level determined externally? Environment variables, cmdline args?
  • Do we know which compilers don't support variadic macros?

Conor:

  • Could we provide an enablement scheme that is symbolic based on expression matching in addition or instead. Provides higher fidelity in targetting debug output, and avoid arbitrary thresholding decisions
  • Usage: ROS_DEBUG(MyPackage.Scoped.Name, foo.output().str() << ":" << this doubleValue << " Is not what is epxected");

  • Enabled by a configuration input such as MyPackage*

  • An implementation of this kind of scheme could be pulled from EUROPA library code.
  • Also supports conditional debug output, when enabled: ROS_COND_DEBUG(!expectedSuccessValue, MyPackage.Scoped.Name, foo.output().str() << ":" << this doubleValue << " Is not what is epxected");

  • Should make sure the cost when not enabling a debug message is neglible.

Rob:

  • I don't like the idea of exposing log4cxx:Level in the API for ROS_LOG
  • Why is g_logger public if getLogger() is supported?
  • Can ROS_INFO, ROS_DEBUG, etc... be implemented as calls to ROS_LOG?
  • Does ROSCONSOLE_AUTOINIT work as implemented (naked call to initialize())?

Meeting agenda

  • Brief overview of log4cxx (http://logging.apache.org/log4cxx/index.html). Should answer:

    • How are output handlers added? In particular, how does output get routed to rosout?
    • Could we provide an enablement scheme that is symbolic based on expression matching in addition or instead. Provides higher fidelity in targetting debug output, and avoid arbitrary thresholding decisions
    • Usage: ROS_DEBUG(MyPackage.Scoped.Name, foo.output().str() << ":" << this doubleValue << " Is not what is epxected");

    • Enabled by a configuration input such as MyPackage*

  • Should make sure the cost when not enabling a debug message is neglible.
  • Rob: I don't like the idea of exposing log4cxx:Level in the API for ROS_LOG
  • Why is g_logger public if getLogger() is supported?
    • To avoid the function call (worth it?)
  • Can ROS_INFO, ROS_DEBUG, etc... be implemented as calls to ROS_LOG?
    • Yes
  • Conditional output
    • Yes, we can support this
  • Does ROSCONSOLE_AUTOINIT work as implemented (naked call to initialize())?
    • Yes
  • Do we know which compilers don't support variadic macros?
    • Do support: The GNU Compiler Collection, since 3.0, C++Builder 2006 and Visual Studio 2005 [1] support variable-argument macros, both when compiling C and C++ code (wikipedia)
    • If we want to support gcc 2, it supports an alternate syntax
  • Possibly pull assertions into this library?

Minutes

  • Use log4cxx calls directly to add appenders
  • Don't wrap log4cxx calls; let the user (in many cases, ros::node) call them directly. Allows for easier interaction with other applications /
    • libraries that use log4cxx.
    • Design goal is to hide log4cxx from most users
  • log4cxx allows hierarchical loggers, each with its own verbosity level
  • Conor wants symbolic debug "levels," with efficient symbol matching to
    • determine which ones are actually output.
  • Support for "foo*" is needed, but "*foo" is probably not.

TODO

  • Add ROS_WARN_NAMED() that takes a symbol name for the message.
    • ROS_WARN() uses a default name of the package name itself.
  • Add COND variations.
  • Implement STREAM variations.
  • Implement static enable-checking.
  • Within ros::console, define enum of levels, which essentially typedef to
    • log4cxx levels.
  • To work well with 3rd-party packages that also use log4cxx, add our
    • appenders to our own logger, rather than the root logger.
      • Potentially provide a mechanism within ros::node to adding our
        • appenders to somebody else's logger.
  • Pull assertions ROS_ASSERT and ROS_BREAK into rosconsole
    • Eventually add new stuff like breaking within debugger.

Conclusion

Package status change (also mark change on PackageStatusDict page)

  • /!\ Action items that need to be taken.

  • {X} Major issues that need to be resolved

Conditional pass.


Wiki: rosconsole/Reviews/2008-09-19_API_Review (last edited 2009-08-14 20:54:50 by localhost)