camera1394 and "absolute" parameters

[jbohren]

I think there's a pretty serious issue with how camera1394 handles parameters. Currently it uses the dynamic-reconfigure-generated Config class for storing all of the parameters, each of which is roughly truncated between 0 and 4095 (except for gamma which is limited between 0.0 and 10.0). Then when it goes to apply these parameters, it attempts to apply the "absolute" versions which are specified as float values between some a priori unknown ranges. At this point the given parameters might have been truncated by the Config class and it's not even clear if a given parameter is supposed to be in absolute or integer form.

@jack-oquin Have you noticed this issue on any cameras? For me it meant that I had no way of accurately controlling the gamma on a couple of Flea2 sensors.

[jack-oquin]

I don't have any cameras that provide absolute parameters. I tried to write the code to support both, but have no doubt there could be problems. I suspect absolute parameters were a later addition to the standard. I have not read that section in several years. The whole DC1394 parameter design is rife with device dependencies.

How do you suggest we should better support the full range of possible inputs?

It might be possible to open the range to (-inf .. +inf). That would make the GUI sliders useless, but they are not much use, anyway. Would that solve your problem with the Flea2?

[jbohren]

I don't have any cameras that provide absolute parameters. I tried to write the code to support both, but have no doubt there could be problems. I suspect absolute parameters were a later addition to the standard. I have not read that section in several years. The whole DC1394 parameter design is rife with device dependencies.

Yeah, as far as I can tell support for absolute params is all over the map, but it seems like all features support the integer parameters.

How do you suggest we should better support the full range of possible inputs?

I think either (a) only support integer parameters or (b) have people explicitly declare whether they want to use absolute or integer parameters. Due to the way dynamic-reconfigure works, I'm pretty sure (b) would require having an additional parameter for each feature, and then you could add additional options to the mode enum for using the manual vs integer values.

It might be possible to open the range to (-inf .. +inf). That would make the GUI sliders useless, but they are not much use, anyway. Would that solve your problem with the Flea2?

I temporarily patched it to do (a) above, and make all of the sliders 0-4095, treat all parameters as integer ranges, and as integer (non-absolute) params. This works very cleanly in practice.

See the fork here: https://github.com/jhu-lcsr-forks/camera1394/tree/integer-features-only

[jack-oquin]

My initial DC1394 Features design coerced everything to floating point numbers in an effort to represent every possible value consistently. I had problems with implementing the absolute part of the specification, but no longer remember quite what they were, other than the lack of test coverage.

You suggestion to keep it all integer with range limits of 0 to 4095 will probably work fine for most devices.

What is the advantage of that approach?

[jbohren]

What is the advantage of that approach?

It means people don't get surprised when they open up coriander, copy down some numbers from sliders, and then put them into a roslaunch file, and they don't find that it's impossible to set the gamma or shutter values because.

[jbohren]

... Because they get truncated by unknown absolute limits.

[jack-oquin]

What happens if you put integer values into the roslaunch file? How do dynamic reconfigure types interact with those in the ROS parameter server? I did not test that case.

I am not enthusiastic about converting everything to int and removing support for devices declaring abs_control. A double can represent integers from 0 to 4095 exactly. Defining a parameter without a decimal point is contrary to the package documentation, although people might do it anyway. What effect would that change have on existing launch files that already do it correctly?

I see no objection to enlarging the range of gamma to whatever is necessary for all known devices.

[jbohren]

What happens if you put integer values into the roslaunch file? How do dynamic reconfigure types interact with those in the ROS parameter server? I did not test that case. I am not enthusiastic about converting everything to int and removing support for devices declaring abs_control.

Yeah, this is why I didn't necessarily make a PR. I just don't have the cycles to submit a patch that supports all the things.

A double can represent integers from 0 to 4095 exactly. Defining a parameter without a decimal point is contrary to the package documentation, although people might do it anyway. What effect would that change have on existing launch files that already do it correctly?

This would only basically break launchfiles which rely on absolute settings.

I see no objection to enlarging the range of gamma to whatever is necessary for all known devices.

Yeah, but I found that the absolute limits as reported by the device were incorrect, and were preventing me from setting even the absolute value. Furthermore, some of the absolute parameters are in the negative range, and I don't know (and we might not be able to know) the limits for all the absolute settings a prior. Without knowing those limits, you can't use Dynamic reconfigure in the way that it's currently being used.

For the limits it probably just makes sense to set them all to +/-inf and forgo having sliders. Then to use a single global flag for setting whether absolute values should be used if available (current behavior) or to just use integer values (my fork's behavior).

[chadrockey]

There were also some pretty untested but merged dynamic reconfigure options to dynamically change limits and I think hide options from the ui.

Example here: https://github.com/ros-drivers/urg_node/blob/indigo-devel/src/urg_node.cpp#L110

[jbohren]

There were also some pretty untested but merged dynamic reconfigure options to dynamically change limits and I think hide options from the ui.

That's cool, but the proposal with +/- inf limits and an optional param to force 12-bit features instead of absolute features will definitely work and can be done in a straightforward amount of time.

[jack-oquin]

That's cool, but the proposal with +/- inf limits and an optional param to force 12-bit features instead of absolute features will definitely work and can be done in a straightforward amount of time.

I agree. Plus depending on new dynamic configure features is risky, as it appears not to be very actively maintained these days.

I would still like to understand the root cause for needing to force 12-bit features. Is your device claiming to support absolute features, when it does not? Or, is there some bug in that part of the camera1394 implementation (which is not well-tested)?

[jack-oquin]

I am back from vacation now, if anyone wants to pursue this issue further.