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

Implement Precision Time Protocol and Synchronous Free Run options #1

Open
wants to merge 1 commit into
base: iron-master
Choose a base branch
from

Conversation

petercheng00
Copy link

  • PTP and sync free run can be enabled at startup via ros
    parameters. They are set up within the gige camera's
    applyCamSpecificStartupSettings. Controls are not exposed for
    for further usage like many of the other camera controls though.

  • PTP startup will block until sufficient clock synchronization is
    reached, though the detection is not perfect.

  • Sync free run requires continuous acquisition mode, which does not
    play nicely with the rest of the code that uses software triggering.
    To accommodate the 2 acquisition modes, we also switch between 2
    grab strategies.

  • Also some general bugfixes, e.g. always setting bandwidth parameters
    and device_user_id_

- PTP and sync free run can be enabled at startup via ros
  parameters. They are set up within the gige camera's
  applyCamSpecificStartupSettings. Controls are not exposed for
  for further usage like many of the other camera controls though.

- PTP startup will block until sufficient clock synchronization is
  reached, though the detection is not perfect.

- Sync free run requires continuous acquisition mode, which does not
  play nicely with the rest of the code that uses software triggering.
  To accommodate the 2 acquisition modes, we also switch between 2
  grab strategies.

- Also some general bugfixes, e.g. always setting bandwidth parameters
  and device_user_id_
@petercheng00
Copy link
Author

@jonbinney @bhomberg Here's changes to the official basler library to implement PTP and synchronous free run. Their camera interaction model is very different from what we're using (they do software-triggered capture and an emphasis on camera control via services). I first added PTP/syncing via those same lines, but it was incredibly messy. So this code just sets up what we need on startup and has minimal effect on the rest of the code.

@jonbinney
Copy link

@petercheng00 could you create an iron-master branch in this repo, and retarget this PR to it? It can get confusing if "master" in the fork is different than "master" in the parent repo.

@petercheng00 petercheng00 changed the base branch from master to iron-master May 1, 2020 17:22
Copy link

@bhomberg bhomberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick pass and gave a few comments -- I'll do a more thorough pass on Monday; I want to read through more of the surrounding code.

@@ -201,7 +201,7 @@ bool PylonGigECamera::applyCamSpecificStartupSettings(const PylonCameraParameter
// Check if gamma is available, print range
if ( !GenApi::IsAvailable(cam_->Gamma) )
{
ROS_WARN("Cam gamma not available, will keep the default (auto).");
ROS_INFO("Cam gamma not available, will keep the default (auto).");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this switched from WARN to INFO?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of warns that happen on startup which seem left over from debugging and in my opinion are not warn-worthy. We'll see what upstream thinks if/when I do a PR there.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Since we're upstreaming this, I'd say it's better to stay consistent with the existing codebase (if this were our code, I'd want all of the similar messages to be either all WARN's or all INFO's or DEBUG's and we could decide which to choose based on the context). And honestly if you think these aren't errors maybe they should be DEBUG statements anyway? If you feel strongly about this, I think the right place for it is probably a separate PR changing all of them (I think this was the only one I noticed changed).

// it as a slave, letting camera B pass this code. Then, camera A syncs to the
// desired master clock, leaving the 2 cameras out of sync. This and other edge
// cases should resolve within a few seconds, but it's not ideal. One option could
// be to have camera nodes communicate between themselves, and all block until all
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking until all conditions are satisfied simultaneously sounds ideal -- how hard would that be to implement?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure - this is definitely where I have a gap in ROS architecture experience. I'm not sure how each node would know which other nodes or messages to monitor for, and I suspect even then there could still be race conditions.

It's definitely simpler architecture-wise to just pass in the required master clock id, but that's then an extra per-machine config thing to set.

Although, it just occurred to me, if we do a setup where cameras are on separate network interfaces, then it's sufficient to just ensure they are slave clocks and we won't have any issues.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have plenty of other configs -- if you think the best way to do this is to pass in a master clock ID, that's fine on our end (not sure how the library feels about it).

Is there any way you can have one node monitor both cameras, or would that be a total re-write of this software?

@@ -328,7 +328,7 @@ bool PylonCameraNode::initAndRegister()
return false;
}

if ( !pylon_camera_->registerCameraConfiguration() )
if ( !pylon_camera_->registerSoftwareTriggerConfiguration() )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be an option where the registerContinuousConfiguration function is run (probably by default)? Ideally we would keep the default settings the same so that users of the library don't have unexpected changes in what happens.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it used to be registerCameraConfiguration, used for setting software trigger config. Because I added ability to use continuous configuration, I renamed registerCameraConfiguration to registerSoftwareTriggerConfiguration. So nothing has changed here, and default behavior is still software trigger.

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

Successfully merging this pull request may close these issues.

3 participants