ros-drivers / flir_camera_driver

153 stars 145 forks source link

Add support for command nodes + Oryx parameter file #163

Closed Sir-Photch closed 3 months ago

Sir-Photch commented 3 months ago

Currently, parameter definition files do not support declaring command nodes. I suggest adding this to the camera driver, as some commands are useful to be executed during camera operation.

This would enable, for example, online sequencer configuration or single-frame acquisition via enumeration node AcquisitionMode: SingleFrame and command node AcqusitionStart that is then executed.

As I see it now, an issue is that executing these nodes would require passing a value, even though there should be none required. (cp. ros2 param set camera_driver acquisition_start foo, foo being unnecessary.)

berndpfrommer commented 3 months ago

So the idea is that you trigger executions by setting a parameter, rather than using the standard ROS service mechanism? A bit unconventional but it allows for a uniform implementation. And one can always implement a service call mechanism alongside later, if desired. Question: have you tested that you can repeatedly trigger an execution by setting the parameter repeatedly (to the same value)? If you had to change the (fake) value every time to effect a callback, that would be kinda tedious on the caller's side. Request: add an example execution node to one of the camera yaml config files. Needs some fixes to the formatting. Use "colcon test --packages-select spinnaker_camera_driver" to verify before committing. Other than that LGTM.

Sir-Photch commented 3 months ago

Question: have you tested that you can repeatedly trigger an execution by setting the parameter repeatedly (to the same value)? If you had to change the (fake) value every time to effect a callback, that would be kinda tedious on the caller's side.

Here a small demonstration via software-triggering:

cast.webm

Request: add an example execution node to one of the camera yaml config files.

I only have Oryx cameras on site where I can verify correct node names. (AcquisitionControl/TriggerSoftware should also exist on Blackfly cameras but I cannot verify that.)

Other than that, something like this is enough:

- name: trigger_software
  type: command
  node: AcquisitionControl/TriggerSoftware

Unrelated: how complete do you expect a camera parameter file to be? I am asking since as mentioned I am working with some Oryx cameras where I needed parameters that were not available in the provided .yaml files.

berndpfrommer commented 3 months ago

Any camera config file is better than none! People are much more willing to experiment if they have a file to start with. Please by all means commit your oryx.yaml file with this PR. If you don't get error messages on startup, that's good enough. Remove parameters that you copy-and-pasted from e.g. blackfly and that you are not setting, i.e. that are untested and may throw errors if used.

If people need more parameters than you, they should figure out how to define them. Not your job.

Sir-Photch commented 3 months ago

Sure, I added it to the PR. I can't say it is production ready though. Especially for my use case which is sequencer configuration, I right now rather run a script after launching the node for setting the parameters, instead of passing the parameters during launch.

Example for two sequences with different exposure times ```bash #!/bin/bash node=$1 function setp { ( set -x; ros2 param set $node $1 $2 ) } set -e setp sequencer_mode "'Off'" setp sequencer_feature_selector ExposureTime setp sequencer_feature_enable True setp sequencer_feature_selector Gain setp sequencer_feature_enable False setp sequencer_configuration_mode "'On'" setp sequencer_set_selector 0 setp sequencer_feature_selector ExposureTime setp exposure_auto "'Off'" setp exposure_mode Timed setp exposure_time 5000.0 setp sequencer_trigger_source FrameStart setp sequencer_trigger_activation RisingEdge setp sequencer_set_next 1 setp sequencer_set_save "_" setp sequencer_set_selector 1 setp exposure_time 30000.0 setp sequencer_trigger_source FrameStart setp sequencer_trigger_activation RisingEdge setp sequencer_set_next 0 setp sequencer_set_save "_" setp sequencer_set_start 0 setp sequencer_set_selector 0 setp sequencer_set_load "_" setp sequencer_configuration_mode "'Off'" setp sequencer_mode "'On'" ```
berndpfrommer commented 3 months ago

I am planning on rewriting the documentation for the driver. Will add this new feature to the docs then. @Sir-Photch thanks again for your contribution!

Sir-Photch commented 3 months ago

You are welcome. Will this change also land in iron? I just based this on humble-devel since it was indicated as the default branch

berndpfrommer commented 3 months ago

I will merge this one into iron as well. It may take several months though before iron gets synced again. I'd like to also improve the documentation before the next release.

On Mon, Mar 25, 2024, 8:58 PM Christoph @.***> wrote:

You are welcome. Will this change also land in iron? I just based this on humble-devel since it was indicated as the default branch

— Reply to this email directly, view it on GitHub https://github.com/ros-drivers/flir_camera_driver/pull/163#issuecomment-2018802589, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPLK2VDFYTORXS76XNGQPDY2B6VHAVCNFSM6AAAAABFDHZCNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJYHAYDENJYHE . You are receiving this because you modified the open/close state.Message ID: @.***>

berndpfrommer commented 3 months ago

Has been released into iron, now just waiting for the sync.

On Mon, Mar 25, 2024 at 5:28 PM Bernd Pfrommer @.***> wrote:

I will merge this one into iron as well. It may take several months though before iron gets synced again. I'd like to also improve the documentation before the next release.

On Mon, Mar 25, 2024, 8:58 PM Christoph @.***> wrote:

You are welcome. Will this change also land in iron? I just based this on humble-devel since it was indicated as the default branch

— Reply to this email directly, view it on GitHub https://github.com/ros-drivers/flir_camera_driver/pull/163#issuecomment-2018802589, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPLK2VDFYTORXS76XNGQPDY2B6VHAVCNFSM6AAAAABFDHZCNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJYHAYDENJYHE . You are receiving this because you modified the open/close state.Message ID: @.***>