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

Add dynamic reconfigure to so3_control #92

Merged
merged 31 commits into from
Apr 28, 2020

Conversation

kartikmohta
Copy link
Collaborator

No description provided.

justinthomas and others added 21 commits October 27, 2016 14:41
* master:
  Install quadrotor_simulator config folder
  mavros_interface: Add mavros_msgs_INCLUDE_DIRS to CMakeLists.txt
  mavros_interface: Only warn when psi check fails, don't set throttle = 0
  mavros_interface: Add a num_props param, change kf to per prop value
  Update asio_serial_device submodule
  quadrotor_msgs: "DEPENDS eigen" -> "DEPENDS Eigen" in CMakeLists.txt
  Some big changes to so3_control (#86)
  Add safety checks in mav manager (#82)
  Initialize publishers before subscribers

# Conflicts:
#	so3_control/config/gains.yaml
#	so3_control/include/so3_control/SO3Control.h
#	so3_control/src/SO3Control.cpp
#	so3_control/src/so3_control_nodelet.cpp
@tdinesh tdinesh mentioned this pull request Aug 10, 2019
8 tasks
@tdinesh
Copy link
Collaborator

tdinesh commented Apr 18, 2020

@kartikmohta @versatran01 lets get this merged next?

@versatran01
Copy link
Collaborator

versatran01 commented Apr 18, 2020

Is there a review interface on github?

  1. I didn't find where the gains are limited in cfg callback.
  2. I don't think this mutex is needed in line 62 of SO3Control.cpp

@tdinesh
Copy link
Collaborator

tdinesh commented Apr 18, 2020

Is there a review interface on github?

The "files changed" tab on the PR allows for review

@kartikmohta
Copy link
Collaborator Author

Give me a couple of days to get to this

@tdinesh tdinesh marked this pull request as draft April 21, 2020 23:36
@kartikmohta kartikmohta marked this pull request as ready for review April 26, 2020 23:45
@kartikmohta
Copy link
Collaborator Author

Replying to comments from @versatran01

1. I didn't find where the gains are limited in cfg callback.

Yes, they are not. Do they need to be? If the user wants to set custom gains from the command line, they need not be restricted.

2. I don't think this mutex is needed in line 62 of SO3Control.cpp

That mutex is used by the dynamic_reconfigure::Server. Providing a mutex to the constructor is highly recommended instead of relying on the internal mutex.

@kartikmohta
Copy link
Collaborator Author

Some important warnings about the changes in this branch:

  • I did not update the pid_control package in the repo to handle the use_msg_gains_flags in the updated PositionCommand message, so it does not work now. I don't think there is any need for the pid_control package since we have the TRPY controller in so3_control, so I'm strongly in favor of just removing it.
  • Similarly, any controller or custom tracker (which uses non-standard gains) outside this repo will need to be updated to properly handle the use_msg_gains_flags in the updated PositionCommand message.

@versatran01
Copy link
Collaborator

@kartikmohta
What I meant is to relax the max and min range of the gains, but while tuning online, the user can only change by no more than say 10% of the current gain. This is to prevent a huge change in gains. Does that make sense?

@jpaulos
Copy link
Collaborator

jpaulos commented Apr 27, 2020

@versatran01 I would find a flat 10% change restriction extremely frustrating. I'm sure you could pick a better magic number, but my point is how will you convey these restrictions to the user in a transparent manner? If one was to introduce feature creep in the gain tuning experience, I'd press in the direction of getting better feedback to the user and providing convenience functions for excitation.

@versatran01
Copy link
Collaborator

There needs to be some mechanism to prevent large gain changes, otherwise there will be a day when somebody accidentally types an extra 0.

@kartikmohta
Copy link
Collaborator Author

@versatran01 I think the current limits on the gains are reasonable for most of the robots we use and also prevent very big changes in the gains.

There needs to be some mechanism to prevent large gain changes, otherwise there will be a day when somebody accidentally types an extra 0.

True, can happen but I think rare enough to not be worth adding complexity to the code. Also I assume most people would just use the sliders in the rqt_reconfigure interface for basic gain tuning where making such mistakes is harder.

@tdinesh
Copy link
Collaborator

tdinesh commented Apr 27, 2020

Some important warnings about the changes in this branch:

  • I did not update the pid_control package in the repo to handle the use_msg_gains_flags in the updated PositionCommand message, so it does not work now. I don't think there is any need for the pid_control package since we have the TRPY controller in so3_control, so I'm strongly in favor of just removing it.

I rather keep it as good easy to read through example. We could just add CATKIN_IGNORE

  • Similarly, any controller or custom tracker (which uses non-standard gains) outside this repo will need to be updated to properly handle the use_msg_gains_flags in the updated PositionCommand message.

Good point!

@tdinesh
Copy link
Collaborator

tdinesh commented Apr 28, 2020

Everything looks good to me, tested in sim. Squash and merge?

@tdinesh tdinesh merged commit c69fa25 into master Apr 28, 2020
@kartikmohta
Copy link
Collaborator Author

Wow, took three years to get this merged! 😄

@tdinesh
Copy link
Collaborator

tdinesh commented Apr 28, 2020

Well one final PR remaining #88. Dreading the initial merge, going to be lot of conflicts 👎

@kartikmohta
Copy link
Collaborator Author

I'm not really in favor of merging #88, it has too many changes in one PR.

@kartikmohta kartikmohta deleted the feature/so3_control_dynamic_reconfigure branch April 28, 2020 01:31
This pull request was closed.
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.

5 participants