-
Notifications
You must be signed in to change notification settings - Fork 44
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
[WIP] Controller #60
[WIP] Controller #60
Conversation
That was in angle mode. I didn't have time to really tune the rate mode, so it's still a little hairy, but it is flyable. I wouldn't recommend using it for offboard control quite yet. Angle mode seems to be pretty robust. |
What if we merged this as-is, and instead made integrator anti-windup another pull request? It works as it currently stands, and it will require more testing to ensure that the anti-windup scheme is working as desired. |
_offboard_control.y.value = mavlink_offboard_control.y/2.0f; | ||
_offboard_control.z.value = mavlink_offboard_control.z/2.0f; | ||
_offboard_control.F.value = mavlink_offboard_control.F; | ||
_offboard_control.x.value = mavlink_offboard_control.x*500.0f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think it'd be good at some point to move the mixer to floating point as well just so that everything can get passed through in normalized units to keep things consistent rather than carrying things around in PWM units (to me it seems like the PWM units probably shouldn't show up until as close to the actual hardware as possible). But let's make that a separate issue and not worry about it yet here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
It's sweet that this is working! I think that there is definitely some cleanup to do, but for the most part I'm good with merging as-is and cleaning up later. In addition to my line comments above, it also seems like we've got a fair amount of code duplication with all the PID loops. Maybe it'd be possible to have a pid_t struct or something for each loop, then a generic PID function that runs an arbitrary loop on the values in that struct? But yeah, maybe let's just take care of a couple of the minor fixes (spelling, parameter names) that could get lost easily, then worry about the more complicated changes in separate issues/pull requests? |
Sounds good to me! I would totally be in favor of a pid_t or something like that. I was trying to figure out a way to do it cleanly without a bunch of pointers, which is the only way I could think of to not have the code redundancy. I figured that it would be easier to read in it's current form as opposed to a more complicated, but less redundant form. You're better at that sort of coding than I am, so I am totally in favor of a refractor that uses less redundancy. Let's make all of our improvements into issues and deal with them severally, rather than making some massive pull request that takes weeks to merge. |
Sorry, I accidentally merged this. master has been reverted, but since this pull request is closed, I'll have to open a new one. #65 |
The controller is working on hardware. There is a little bit of cleanup left, some that I was hoping you could help me with @dpkoch.
Both the attitude and rate loops are working from RC commands, however, I can't remember how to do integrator anti-windup. I was wondering if you had ideas on that. After the anti-windup is incorporated, then this branch is ready.