Skip to content

Conversation

@fairf4x
Copy link
Contributor

@fairf4x fairf4x commented Feb 15, 2017

Hi,

I have implemented and tested some minor features I needed for my experiments:

  1. parameter: sync_time (true/false)
    It enables/disables time and date synchronization of drone during initialization.

  2. topic: set_exposure (float from range <-3.0,3.0>)
    Enables setting of camera exposure on demand.

  3. topic: set_video_stabilization (uint8 - mode)
    Enables setting video stabilization mode (also works for snapshots):
    0 = roll_pitch: Video flat on roll and pitch
    1 = pitch: Video flat on pitch only
    2 = roll: Video flat on roll only
    3 = none: Video follows drone angles

  4. Added rough visual model to bebop 1 URDF description (works in RViz)

  5. Added camera calibration file for bebop 2

@mani-monaj mani-monaj added this to the 0.7 milestone Feb 16, 2017
@mani-monaj
Copy link
Member

Thank you for this PR. The proposed changes look good. I will review these as soon as possible. In the meantime:

  1. Regarding set_video_stabilization, is there any problem with the method currently available? Isn't it dynamically reconfigurable? http://bebop-autonomy.readthedocs.io/en/latest/autogenerated/ardrone3_settings_param.html#picturesettingsvideostabilizationmodemode
  2. The STL model looks really good. There is another one available as part of another fork of this driver here. I will contact @gstavrinos to see if it's ok to use that more detailed model instead.

@fairf4x
Copy link
Contributor Author

fairf4x commented Feb 16, 2017

Thank you for feedback.

  1. To be honest I did not noticed the dynamically reconfigurable parameter PictureSettingsVideoStabilizationModeMode. I will test it and let you know if it is OK.

It also makes me think about rewriting the set_exposure topic as reconfigurable parameter.
As far as I understand the automatic code generation process this would require describing the parameter in ardrone3.xml file and changing LIBARCOMMANDS_GIT_HASH variable accordingly in generate.py. Is this correct?

  1. I found similar model like the one from @gstavrinos before, but it was not available for free download so I created mine myself. More detailed model is really cool - thanks for the link!

@mani-monaj
Copy link
Member

mani-monaj commented Feb 18, 2017

To be honest I did not noticed the dynamically reconfigurable parameter PictureSettingsVideoStabilizationModeMode. I will test it and let you know if it is OK.

That would be great.

It also makes me think about rewriting the set_exposure topic as reconfigurable parameter.
As far as I understand the automatic code generation process this would require describing the parameter in ardrone3.xml file and changing LIBARCOMMANDS_GIT_HASH variable accordingly in generate.py. Is this correct?

That is a great idea. All dynamically reconfigurable parameters are automatically extracted from the underlying's SDK's XML files (fetched directly from upstream SDK's repo from Github). I am not sure why there is no code generated for the method you used (sendPictureSettingsExpositionSelection). I should look into this more closely.

I found similar model like the one from @gstavrinos before, but it was not available for free download so I created mine myself. More detailed model is really cool - thanks for the link!

You are welcome. I will post any updates about if we can use the other model under this thread.

…namic parameter PictureSettingsVideoStabilizationModeMode
Copy link
Member

@mani-monaj mani-monaj left a comment

Choose a reason for hiding this comment

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

Thank you again for this PR and the new changes. It looks good but requires few minor modifications and documentation update. I will merge it as soon as you get a chance to apply these changes.

@@ -0,0 +1,20 @@
image_width: 640
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add an optional argument to bebop_driver launch files so someone can switch between Bebop 1 and 2. Based on that argument those launch files can load the appropriate camera calibration params.

// Params (not dynamically reconfigurable, local)
// TODO(mani-monaj): Wrap all calls to .param() in a function call to enable logging
const bool param_reset_settings = private_nh.param("reset_settings", false);
const bool param_sync_time = private_nh.param("sync_time", false);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add this new param to the documentation?

stop_autoflight_sub_ = nh.subscribe("autoflight/stop", 1, &BebopDriverNodelet::StopAutonomousFlightCallback, this);
animation_sub_ = nh.subscribe("flip", 1, &BebopDriverNodelet::FlipAnimationCallback, this);
snapshot_sub_ = nh.subscribe("snapshot", 10, &BebopDriverNodelet::TakeSnapshotCallback, this);
exposure_sub_ = nh.subscribe("set_exposure", 10, &BebopDriverNodelet::SetExposureCallback, this);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please also add this new topic and range of accepted values to the documentation?

@fairf4x
Copy link
Contributor Author

fairf4x commented Mar 14, 2017

I have added the argument to both launch files and documentation for the sync_time parameter and set_exposure topic. Let me know if there is something else.

{
try
{
ROS_INFO("Setting exposure to %f", exposure_ptr->data);
Copy link
Member

@mani-monaj mani-monaj Mar 14, 2017

Choose a reason for hiding this comment

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

It would be great to add a CLAMP here not to let any number outside the [-3.0..3.0] range to be passed.

@mani-monaj
Copy link
Member

@fairf4x Thank you for applying the changes. I will test this changeset before the end of the week and will merge it afterwards. I will also create an issue to investigate why the code for setting the exposure is not automatically generated.

@mani-monaj mani-monaj merged commit 8a9d6c7 into AutonomyLab:indigo-devel Mar 24, 2017
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.

2 participants