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

Pass config options to CanBusControlboard's subdevices #207

Closed
PeterBowman opened this issue Apr 5, 2019 · 4 comments · Fixed by #229
Closed

Pass config options to CanBusControlboard's subdevices #207

PeterBowman opened this issue Apr 5, 2019 · 4 comments · Fixed by #229

Comments

@PeterBowman
Copy link
Member

Originally a subtask of roboticslab-uc3m/questions-and-answers#49, I'm opening this to deal with the issues that led to 6467003 (rollback commit of 3fcf1b5).

#175 (comment)

if((nodes[i]->getValue("device")).asString() == "TechnosoftIpos"){

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

#175 (comment)

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.

roboticslab-uc3m/questions-and-answers#49 (comment)

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 #175 (comment) for full explanation. Might need to circumvent the way we use getValue in that repo (example) or enhance the value lookup mechanism upstream.

roboticslab-uc3m/questions-and-answers#49 (comment)

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
Copy link
Member

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):

if( types.get(i).asString() == "CuiAbsolute" )
{
int driverCanId = ids.get(i).asInt() - 100; //-- \todo{Document the dangers: ID must be > 100, driver must be instanced.}

@PeterBowman
Copy link
Member Author

Work on #211 might shed some light regarding how class inheritance is going to be shaped.

@PeterBowman
Copy link
Member Author

Probably blocked by #74.

@PeterBowman
Copy link
Member Author

Circumvented at ab72254 and cbb840c while working on #74.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants