Opening a device with OpenNI2Grabber

[Morwenn]

Is there a way to initialize an OpenNI2Grabber once the OpenNI2Device I want to work with is known (which is to say I've got an instance of the device class)?

Your Environment

• Operating System and version: Ubuntu 18
• Compiler: g++ (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0
• PCL Version: 1.9.1

Context

I am trying to use the OpenNI2Grabber class and have noticed an issue while trying to use both the StructureSensor and the Orbbec Astra at the same time: I thought that the expected device_id passed to its constructor corresponded to a device's getUri, but it took me quite some time to realize that the grabber didn't really work with that parameter, and from reading the code it seems that it's using OpenNI2DeviceManager::getAnyDevice() under the hood and thus both my devices are initialized with the default device (in my case the StructureSensor).

To give a bit more context: I inherited a code base where we use OpenNI2DeviceManager::getNumOfConnectedDevices and getDeviceByIndex to discover the existing devices compatible with OpenNI2. We save a value somewhere (currently the result of getUri) and later reuse that value to instantiate an OpenNI2Grabber and work with the device we're interested in. However it doesn't seem that OpenNI2Grabber is expecting the result of getUri, but I failed to find which kind of value I could retrieve from the OpenNI2Device to make sure that it will grab the appropriate device. How can I retrieve the relevant information to construct a grabber matching the device I want?

The getUri function returns values such as 1d27/0600@1/11 or 2bc5/0402@1/12 on my computer.

Expected Behavior

I expect the following code to work (or at least that there is a function in OpenNI2Device that will return a value sufficient to construct a corresponding OpenNI2Grabber):

auto deviceManager = pcl::io::openni2::OpenNI2DeviceManager::getInstance();
auto device = deviceManager->getDeviceByIndex(idx);
auto uri = device->getUri();
auto grabber = std::make_unique<pcl::io::OpenNI2Grabber>(uri);
assert(uri == grabber->getDevice()->getUri());

Current Behavior

The code above works as expected when a single device is found by the OpenNI2DeviceManager but fails when two devices are connected. Both return the same URI as if the grabber was constructed with OpenNI2DeviceManager::getAnyDevice().

[taketwo]

Can you try getStringID() instead URI?

[Morwenn]

I get the following error:

terminate called after throwing an instance of 'pcl::io::IOException'
  what():  pcl::io::openni2::OpenNI2Device::OpenNI2Device(const string&) @ [...]/io/src/openni2/openni2_device.cpp @ 75 : Initialize failed
	DeviceOpen: Couldn't open device 'Astra_Orbbec'


Abandon (core dumped)

[taketwo]

AFAIK Orbec requires custom OpenNI2 libraries and does not work with the libs you get with agt-get. (My knowledge may be a bit dated though.)

[Morwenn]

Yeah, I installed the OpenNI2 library from here: https://orbbec3d.com/develop/

I indeed had to work around a few issues (like the Structure Sensor being recognized as an Astra), and eventually the only thing that I haven't fixed yet is getting the correct OpenNI2Grabber when I already have an OpenNI2Device. Would it make sense to have a grabber constructor taking a pointer to a device? I'm not sure what issues it would raise.

[taketwo]

Would it make sense to have a grabber constructor taking a pointer to a device? I'm not sure what issues it would raise.

I'm not familiar enough with the code base to predict.

One other thing, did you try to pass a string like "#1" or "#2" as grabber constructor argument?

[Morwenn]

No, I didn't try that. I will try a few things and report back when I have something that looks like a solution :)

[Morwenn]

Update on the state of things: the exception I got was unrelated (my bad), but feeding getStringID to PCL instead of getUri didn't work. As a matter of fact getStringID returns the value "Astra_Orbbec" for both the P1080 StructureSensor and the Orbbec Astra with the only OpenNI2 version I found that somewhat worked with both devices (the one linked above).

Failing to find another option, I resumed sending getUri to the grabber's constructor, but I also patched PCL in a rather ugly way, which is to say that I changed the following line in openni2_grabber.cpp:

device_ = deviceManager->getAnyDevice ();

I replaced it with a loop that checks whether the device_id corresponds to the getUri() of any device handled by the device manager, and failing that it still resorts to getAnyDevice:

bool found = false;
for (int idx = 0; idx < deviceManager->getNumOfConnectedDevices (); ++idx) {
    auto device = deviceManager->getDeviceByIndex (idx);
    if (device->getUri () == device_id) {
        device_ = device;
        found = true;
        break;
    }
}
if (not found) {
    device_ = deviceManager->getAnyDevice ();
}

It's rather inelegant, but it finally works for our use case, and even though the string comparison against getUri instead of getStringID isn't great, the former at least produces a unique value (unlike the latter) and the end result is no worse than selecting any device anyway.

I entertained the idea of passing device indices as you proposed, but our devices are sometimes discovered separately in different places, so it's difficult to maintain a proper device count. If we had to restart the project from the ground up, we would probably do it differently.

I don't know whether there could be an elegant solution to this issue in PCL, so if you don't wish to discuss the problem further, I will close this issue.

[taketwo]

Thanks for reporting back.

if you don't wish to discuss the problem further, I will close this issue.

I'd rather prefer to find a solution and merge a patch.

The device manager seems to have a method called getDevice(device_URI). Can we use it instead of the loop you posted?

[Morwenn]

Hum, I will try this, just the time to recompile everything again. Won't it throw an exception if the URI doesn't match anything though? We will most likely want to catch it and fall back to getAnyDevice if it is the case to avoid breaking code that relies on the getAnyDevice behaviour.

[taketwo]

Totally agree.

[Morwenn]

I replaced the previous loop by the following code and everything seems to work fine when device_id corresponds to the URI:

try {
    device_ = deviceManager->getDevice (device_id);
} catch (const pcl::io::IOException&) {
    device_ = deviceManager->getAnyDevice ();
}

[taketwo]

Great -- please send a patch.

If it wasn't for backwards compatibility, I would not catch the exception; it's up to the user to decide what to do if the device with given URI is not available. Now that we do have a try/catch, I think we should at least notify the user with a PCL_WARN warning.

[Morwenn]

I think that the only case when getAnyDevice is expected is when an empty string is passed to the grabber's constructor, which would still supported with the proposed change since it's exactly what getDevice() does when passed an empty string. Warning on any other string that fails to identify a device seems reasonable, I'll send a patch later today.

How should we document that: in the bragger constructor documentation or in the setupDevice one? I know that the question is a bit theoretical since OpenNI2Grabber's documentation is not online (as mentioned in #1864), but the valid options for device_id are currently quite difficult to grasp without reading the source code.

[taketwo]

It'd be super-helpful if you add the explanation of valid options for device_id (and their formats) to the constructor docstring. Even though the documentation is not online (you have a better grasp on our issues, I've long forgotten about #1864 😆), I would naturally expect users to check the header -> constructor for information.

[SergioRAgostinho]

If it wasn't for backwards compatibility, I would not catch the exception; it's up to the user to decide what to do if the device with given URI is not available. Now that we do have a try/catch, I think we should at least notify the user with a PCL_WARN warning.

I skimmed through openni2 setupDevice code and a number of exceptions are "leaking out" for the user to handle. So there would be margin to add the new logic proposed and still let the user handle things if problems arise. What backwards behavior are you referring to?

[taketwo]

What backwards behavior are you referring to?

Undocumented behavior that supplying an invalid device URI results in opening a device (albeit random) without exceptions.

[SergioRAgostinho]

I suspect that this is a behavior worth "breaking" :)

[taketwo]

OK, well, I wouldn't be against actually. @Morwenn what's your opinion? In case you agree, let's not catch/warn and let the user deal with the exception.

[Morwenn]

I'm all for loud errors rather than silent ones; actually it would have saved me a few hours this week if my code had been rejected instead of picking the first available device. If it's ok with you I'll just replace the getAnyDevice with the following one:

device_ = deviceManager->getDevice (device_id);

The only case where where picking the first available device makes sense is when providing an empty string, which is already handled by the constructor of OpenNI2Device, so the line above should be sufficient.