camera1394: invalid trigger settings are not set back in dynamic reconfigure

[bgromov]

Usually when certain parameter's value is not correct it is reset back to previous or to one that set on device. Current version is doing that, but only with internal variables. So it doesn't write it back to newconfig variable and thus dynamic_reconfigure is not notified.

This has to be fixed in Trigger::reconfigure() for all trigger parameters except for external_trigger and software_trigger.

[jack-oquin]

Updates to newconfig are returned in the response to the set_parameters service, it looks like the Groovy and Hydro rqt_reconfigure command does not display those updates correctly. The reconfigure_gui of Fuerte and before did it right.

The camera1394 driver heavily depends on that being handled correctly. It looks like the parameter server does get updated, but the Query option no longer works interactively, it seems.

I'll open an issue with rqt_common_plugins for that. Unfortunately, it appears that rqt_reconfigure is badly broken with many open issues. It was supposed to be a replacement for reconfigure_gui, but the implementation was apparently botched. I'll try to figure out if it is actually being maintained.

[chadrockey]

I've ticketed almost this exact issue here:

ros-visualization/rqt_common_plugins#107

It has no official active maintainer, but I think Dirk, Aaron Blasdel, and Issac Soto are working out the more serious bugs this week.

[jack-oquin]

Thanks, Chad. I had just found another of your issues: ros-visualization/rqt_common_plugins#108.

The good news is: it looks like Issac is actively maintaining that package.

[130s]

Yes I am going to look at ros-visualization/rqt_common_plugins#107 and ros-visualization/rqt_common_plugins#108 this week.

[jack-oquin]

Thanks, Isaac.

I will be happy to test any fixes you come up with.

[bgromov]

Just to clarify. The bug I'm talking about is solely belong to class Trigger, the latter do not modify variable newconfig as it should, that is why dynamic_reconfigure server is not notified about the change and thus rqt_reconfigure as well. Sorry for possible confusion.

There is another problem associated with rqt_reconfigure though, which I guess you are talking about. When a parameter is reset by the driver both dynamic_reconfigure and rqt_reconfigure are notified. However, some of internal rqt_reconfigure variables are not reset. So, if user tick the same box again no event generated and sent to dynamic_reconfigure server. Then, when the box unticked by user again everything appears in sync. That is due to ros-visualization/rqt_common_plugins#107, right?

[jack-oquin]

Boris: yes, I do understand the basic issue.

I was concerned that the rqt problem might confuse us while testing the fixes. It confused me when I was trying to reproduce the error.

[jack-oquin]

For consistency with the way this driver handles other features, invalid parameter values should be reset to whatever value the camera firmware returns. That way the user can see what's actually there.

[bgromov]

There will be a problem to return the values returned by firmware since libdc1394 may produce errors even for get calls (similar to those mentioned in #12 ).

The best option would be to disable part of controls based on hasTrigger() that you have introduced, but I don't know if such functionality exists in dynamic_reconfigure and rqt_reconfigure.

[jack-oquin]

I agree that we should not query those values unless hasTrigger() is true. I am not yet convinced that hasTrigger() is completely reliable for all 1394 cameras, but the goal is to make it so.

We could just ignore those values in that case. But, reverting to previous settings might make the situation clearer to users. The Features class has a copy of the previous config in oldconfig_, so it would be simple for it to call some Trigger method to reset its parameters to their previous values, logging a warning for each that changed. (Feature itself would not know which parameters to check.)

There has long been an intention to add parameter groups to dynamic_reconfigure. I've seen some hints lately that some work may have been done in Groovy. This driver would greatly benefit from that, if it works correctly. We can certainly look into it, but I'd rather handle it as a separate enhancement.

[jack-oquin]

@bgromov: I would like to release these new features in the next week or two. I think we need to fix this issue before doing that.

Would you prefer to fix it yourself? Or, should I make the changes? It does not look particularly difficult.

We also need to update the documentation and decide what to do about the driver hang problem you mentioned.

Would you please open a new issue for hang problem, with detailed directions for how to reproduce the bug? I want to test it on my cameras, and that information would save me time.

Would you like to have push access to this repository?

[jack-oquin]

The driver currently allows setting of Format 7 parameters even when configured in a different video mode, but those settings are ignored until a Format 7 mode is selected. That actually seems to be useful.

Perhaps we should do something similar here, accepting but ignoring parameter changes when triggering is not enabled, which should always be true when hasTrigger() is false.

[bgromov]

Jack, I will do it in the next couple of days. Hopefully by tomorrow evening (KST).

[jack-oquin]

This fix is not working with my Sony XCD-XS90CR test camera. It can't set trigger parameters via dynparam until external_trigger is set, at which point it hangs, probably due to #22.

My Unibrain Fire-i test camera, which has no trigger support allows setting any of the trigger parameters, except auto_trigger which remains set to "None" (appropriately).

I'm re-opening this issue to experiment with other solutions, such as removing the effect of 1f69dfd. I believe that will work better.

[jack-oquin]

The commit above removes the 1f69dfd restriction on setting other trigger parameters before setting external_trigger.

I find that much more convenient, being able to set everything else up before enabling external triggering. Please try it and see if you agree.

[bgromov]

I am confused. My intention with 1f69dfd was to achieve exactly what you have suggested (at least how I understood it), i.e. setting all parameters first and then 'commit' them at once when external_trigger has been enabled. So 1f69dfd simply allows to set all the fields without bothering libdc1394. Thus, all parameters are stored in features_->newconfig, but are not fed further to libdc1394 if external_trigger is False.

[jack-oquin]

Maybe I made some mistake. It's really hard to test this stuff with rqt_reconfigure broken.

The dynparam command is confusing and difficult to use, but it really did look like the parameters were not getting set.

What is the reason for not passing those parameters to libdc1394?

[bgromov]

Just spotted one more thing in IIDC v1.31 specs (p.31):

• TRIGGER_MODE ON_OFF [6]:

• Write: ON or OFF this feature,

• Read: read a status

  0: OFF, 1: ON

  If this bit =0, other fields will be read only

Does it basically mean that it is not possible to set other settings unless external_trigger is True?

[bgromov]

What is the reason for not passing those parameters to libdc1394?

As you mentioned software_trigger do not affect anything if external_trigger is off, so I felt it is natural to do not propagate subordinate parameters further in such case. Nevertheless now I think there is a clear reason to ignore such parameters since they are read-only anyway.

What we really need in rqt_reconfigure is ability to disable certain parameters. With such a feature user experience would improve drastically.

[jack-oquin]

@bgromov: I am finally working on this driver, again. We need to release it to Indigo.

I am not sure if this issue got resolved or not. Did 3ba625e leave things in a satisfactory state?

I want to include trigger support in Indigo. I am reluctant to release it for Hydro because major documentation updates will be needed. It is simpler to document those changes as part of a new distribution. Hydro has been stable for a long time now.

Is that a problem for you? We can release it in Hydro if needed.

[bgromov]

@jack-oquin Sorry for late reply. Unfortunately I do not have access to hardware anymore and also a bit away from ROS world. If I remember correctly, 3ba625e did not introduce any issues at that time.

[jack-oquin]

@bgromov: OK, thanks for all your contributions.

I'll close this issue now.