search

roboticslab-uc3m / questions-and-answers

A place for general debate and question&answer
https://robots.uc3m.es/developer-manual/appendix/repository-index.html
2 stars 0 forks source link

Improve device option docs, promote use of --verbose #49

Closed PeterBowman closed 4 years ago

PeterBowman commented 6 years ago

The yarpdev utility suggests using a --verbose CLI parameter in order to list all available options, their description, default value, etc. This comes in handy when working with our own devices, which undergo frequent changes in their usage (or, simply put, are barely or not documented at all). Traditionally, we introduced the --help parameter, but it's hard to maintain (needs hardcoding plus some care to keep it up to date).

YARP offers its own mechanisms for such tasks, tightly bound to the yarp::dev::PolyDriver class: the YarpDevMonitor utility (not shown in Doxygen generated docs nor even exported, check PolyDriver.cpp). Such instance is created by all PolyDriver objects and can be propagated to subdevices (either wrapped devices, or devices opened by other devices in their DeviceDriver::open method). Example:

Try this via yarpdev --device ... with and without --verbose. Example:

yarpdev --device FakeControlboard --name /fake
yarpdev --device BasicCartesianControl --local /local --remote /fake --kinematics path.ini --verbose

Some thoughts:

jgvictores commented 6 years ago

Looks good! I'm going to be touching RobotServer/RobotClient pair at https://github.com/asrob-uc3m/yarp-devices, would look like a nice opportunity to experiment!!

PeterBowman commented 6 years ago

How to make this work on RF modules (e.g. transCoords app):

int main()
{
    MyMonitor monitor; // implements yarp::os::SearchMonitor

    yarp::os::ResourceFinder rf;
    // tweak 'rf', e.g. set default context and so on
    rf.setMonitor(&monitor, "transCoords");

    roboticslab::TransCoords mod;
    mod.configure(rf); // check return value!

    // at this point, retrieve options stored in 'monitor' and
    // print them on stdout if --verbose was passed on

    return mod.runModule(); // no need to configure again
}

We'll need a class that implements yarp::os::SearchMonitor (no Doxygen, see definition) and provides a public method for obtaining option bottles. Compare YarpDevMonitor.

jgvictores commented 6 years ago

Thanks a lot for this info! Always loved the behavior, but found the intrinsics quite cryptic, especially for modules.

PeterBowman commented 6 years ago

Bad thing is, YARP should have already provided an implementation, otherwise we are forced to write a tiny library only for this and replicate the same code in several places... This is what yarpdev runs behind the scene: ref. The sample main function from my previous comment would check verbose and reuse that code to print the report (replace PolyDriver with MyMonitor in the function's signature).

jgvictores commented 6 years ago

Maybe we could develop some kind of helper class that could be PR'ed directly into YARP?

PeterBowman commented 6 years ago

Sure, the sooner the better. I'm afraid of those find_package(YARP 3.0.0 REQUIRED) in each single app (unless we stick to preprocessor conditionals).

PeterBowman commented 6 years ago

need to review all check calls and add missing comments/defaults

Speaking of the yarp::os::Searchable class (see inheritance diagram, it has several implementations), prefer

bool check(const ConstString &key, const ConstString &comment) const;
Bottle findGroup(const ConstString &key, const ConstString &comment) const;

instead of

check(const ConstString &key) const;
Bottle findGroup(const ConstString &key) const;

The former generate a report that may be used by the monitor in verbose mode. Also, the following are available:

bool check(const ConstString &key, Value *&result, const ConstString &comment="") const;
Value check(const ConstString &key, const Value &fallback, const ConstString &comment="") const;
PeterBowman commented 6 years ago

I'm going to list here all repos that have been improved by providing or expanding on option documentation (now passing appropriate defaults and description in config.check() and the like) and propagating it through device wrappers:

jgvictores commented 6 years ago

Thanks a lot for this!

PeterBowman commented 6 years ago

Sometimes, device options hide each other, like this:

if (config.check("optionA"))
{
    yarp::os::Value v = config.check("optionB", yarp::os::Value(MY_DEFAULT));
}

The check for optionB could be pulled out from its enclosing if clause so that it's parsed by --verbose regardless of the presence of optionA. Then, additional logic would take care of the implicit relationship between both options. Is this the way to go?

PeterBowman commented 6 years ago

Or, since I like simplicity:

if (config.check("optionA", "enables A (and additional options)"))
{
    // ...
}

Thus, users would be aware of the existence of hidden options (and how to enable them).

PeterBowman commented 6 years ago

Custom devices wrapped by controlboardwrapper2 cannot display options via --verbose due to an upstream bug (tracked at https://github.com/robotology/yarp/pull/1608).

PeterBowman commented 6 years ago

Related upstream PR regarding --help: https://github.com/robotology/yarp/pull/1473.

PeterBowman commented 6 years ago

I'm going to list here all repos that have been improved...

All set but openrave-yarp-plugins. @jgvictores I'd really appreciate if you could take a look at existing device options and review/fill their description fields :) (low priority).

jgvictores commented 6 years ago

Okay, it'll take a while, but I will find a moment to do it!

PeterBowman commented 6 years ago

yarp-devices still WIP due to https://github.com/roboticslab-uc3m/yarp-devices/issues/175, marking as blocked.

PeterBowman commented 6 years ago

Note to self: use simpler unit notation and replace parens with square brackets. Example: [m/s] or [deg/s] instead of (meters/second or degrees/second).

jgvictores commented 6 years ago

Added some documentation on ORYP at https://github.com/roboticslab-uc3m/openrave-yarp-plugins/commit/e0adfbc6953659a5d20568eee27c1c6691e279e2

jgvictores commented 6 years ago

Applied to asrob-uc3m/yarp-devices here; works, but ran into https://github.com/asrob-uc3m/yarp-devices/issues/12

PeterBowman commented 6 years ago

I'd rather close https://github.com/roboticslab-uc3m/yarp-devices/issues/175 and track that bug here. To sum up, PolyDriver::getValue does not work as expected when the instance has been configured and opened with a Searchable object to which a monitor has been attached to (usually via options.setMonitor(config.getMonitor())). See https://github.com/roboticslab-uc3m/yarp-devices/issues/175#issuecomment-375406583 for full explanation. Might need to circumvent the way we use getValue in that repo (example) or enhance the value lookup mechanism upstream. For that reason, I'd keep the blocked label for now.

PeterBowman commented 6 years ago

There is no simple way to sort this out on the upstream side without proposing a breaking change of the (undocumented) SearchMonitor interface, which lacks option getters (check sources).

On our end, the CanBusControlboard device might hold a vector of Property instances besides the existing one for PolyDriver objects (ref) in order to store and access device configurations. This is a hack.

jgvictores commented 6 years ago

IMHO, a big issue is too much being done on the CanbusControlboard. Things such as home should be done within Technosoft open or new method which is involved for all devices and each one implements their behavior via polymorphism.

The only special case where I introduced the original ugly hackish code was connecting the CuiAbsolute to the TechnosoftIpos, but that (apart from different) could be done better.

PeterBowman commented 6 years ago

Custom devices wrapped by controlboardwrapper2 cannot display options via --verbose due to an upstream bug (tracked at robotology/yarp#1608).

Merged into YARP's master branch, scheduled for the 2.3.72.1 release.

PeterBowman commented 5 years ago

This issue has been stalled for too long, therefore...

I'd rather close roboticslab-uc3m/yarp-devices#175 and track that bug here.

I changed my mind (again) and opened https://github.com/roboticslab-uc3m/yarp-devices/issues/207, in the only repo affected by said bug.

The list https://github.com/roboticslab-uc3m/questions-and-answers/issues/49#issuecomment-372620773 is done except for a few undocumented config options in openrave-yarp-plugins. @jgvictores could you please take a look? Apart from that, I need to check whether any new devices that have been added in the past year conform to these new standards.

One last question: is the behavior of --verbose actually better than the old --help? Also, note to self: look for remaining occurrences of the latter.

Regarding RFModule: either open an issue upstream considering https://github.com/roboticslab-uc3m/questions-and-answers/issues/49#issuecomment-368339553, or forget.

jgvictores commented 5 years ago

The list #49 (comment) is done except for a few undocumented config options in openrave-yarp-plugins. @jgvictores could you please take a look?

I have that repo pretty abandoned, but I'll take a look. Specifically, I had done advances on https://github.com/roboticslab-uc3m/openrave-yarp-plugins/issues/82, raised its priority to get done with it.

One last question: is the behavior of --verbose actually better than the old --help? Also, note to self: look for remaining occurrences of the latter.

My intuition behind --help was to be implement a --verbose that additionally quits before running anything, mostly to be able to read the output before all the verbosity of actually running. Would implementing this explicitly make any sense, or do you think we can just force users to stop a running program and scroll up?

PeterBowman commented 5 years ago

There is not much to do at or-yp, anyway, just a few undocumented occurrences of .check( at OpenraveYarpPluginLoader.

Would implementing this explicitly make any sense, or do you think we can just force users to stop a running program and scroll up?

It's a pity we don't know the outcome of https://github.com/robotology/yarp/pull/1473. Some thoughts:

PeterBowman commented 4 years ago

To sum up:

Closing due to inactivity/too broad a scope/lack of interest on tedious subtasks.