roboticslab-uc3m / yarp-devices

A place for YARP devices
https://robots.uc3m.es/yarp-devices/
9 stars 7 forks source link

Pass config options to CanBusControlboard's subdevices #207

Closed PeterBowman closed 4 years ago

PeterBowman commented 5 years ago

Originally a subtask of https://github.com/roboticslab-uc3m/questions-and-answers/issues/49, I'm opening this to deal with the issues that led to https://github.com/roboticslab-uc3m/yarp-devices/commit/6467003079ffe5c468b5b082dd3ce76df3c7b4d5 (rollback commit of https://github.com/roboticslab-uc3m/yarp-devices/commit/3fcf1b5aeda71a234c2a61b2b7905213ca8d5f85).

https://github.com/roboticslab-uc3m/yarp-devices/issues/175#issuecomment-375351186

https://github.com/roboticslab-uc3m/yarp-devices/blob/08f6a4162ef0af342ff9489c21901d062fc88e8f/libraries/YarpPlugins/CanBusControlboard/DeviceDriverImpl.cpp#L301

Looks like getValue performs a lookup on the monitor instance (not on options passed through open), ref.

https://github.com/roboticslab-uc3m/yarp-devices/issues/175#issuecomment-375406583

From PolyDriver.cpp:

Value PolyDriver::getValue(const char *option) {
    if (system_resource==nullptr) {
        return Value::getNullValue();
    }
    return HELPER(system_resource).getValue(option);
}

The system_resource member is a local instance of YarpDevMonitor (which extends SearchMonitor). However, we don't use it at all if the config parameter to PolyDriver::open already has a monitor object attached to itself:

bool PolyDriver::open(yarp::os::Searchable& config) {
    if (isValid()) {
        // already open - should close first
        return false;
    }
    if (system_resource==nullptr) {
        system_resource = new YarpDevMonitor;
    }
    yAssert(system_resource!=nullptr);
    bool removeMonitorAfterwards = false;
    if (config.getMonitor()==nullptr) {
        config.setMonitor(&HELPER(system_resource));
        removeMonitorAfterwards = true;
    }

    //dd = Drivers::factory().open(config);
    coreOpen(config);
    HELPER(system_resource).info.fromString(config.toString());
    if (removeMonitorAfterwards) {
        config.setMonitor(nullptr);
    }
    return isValid();
}

The monitor linked to config registers a device key in coreOpen. If this is not the local monitor instantiated in open, getValue won't return what we actually expect.

https://github.com/roboticslab-uc3m/questions-and-answers/issues/49#issuecomment-375959740

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.

https://github.com/roboticslab-uc3m/questions-and-answers/issues/49#issuecomment-375966543

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 5 years ago

Please note that while DeviceDriverImpl.cpp has been populated with this kind of string comparisons, I'd much rather all to be moved to their corresponding classes and reached via polymorphism. The only reason there are string comparisons in the first place is actually my fault, a hack I think we could do better now (there was some issue related to passing a pointer in a Property, but since a while ago we know this can be passed as a blob): https://github.com/roboticslab-uc3m/yarp-devices/blob/08f6a4162ef0af342ff9489c21901d062fc88e8f/libraries/YarpPlugins/CanBusControlboard/DeviceDriverImpl.cpp#L170-L172

PeterBowman commented 5 years ago

Work on https://github.com/roboticslab-uc3m/yarp-devices/issues/211 might shed some light regarding how class inheritance is going to be shaped.

PeterBowman commented 5 years ago

Probably blocked by https://github.com/roboticslab-uc3m/yarp-devices/issues/74.

PeterBowman commented 5 years ago

Circumvented at https://github.com/roboticslab-uc3m/yarp-devices/commit/ab72254c0eb033efaf424c5d5380f4580c0cc075 and https://github.com/roboticslab-uc3m/yarp-devices/commit/cbb840c01163b1afef6b388aa136afce3a818e78 while working on https://github.com/roboticslab-uc3m/yarp-devices/issues/74.