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

Adding Flapper platform and handling servomotors #1132

Merged
merged 3 commits into from
Oct 11, 2022

Conversation

matejkarasek
Copy link
Contributor

@matejkarasek matejkarasek commented Oct 7, 2022

This PR adds a "flapper" platform, together with a "flapper deck", default settings and gain values.
To enable servomotors, which need a non-zero neutral value, additions were made also to motors.c and supervisor.c
(This is a clean version of #1123)

Finally, I include some cleanup after #1121.

@@ -28,6 +28,7 @@
"src/utils/src/num.c",
"src/modules/src/controller_mellinger.c",
"src/modules/src/power_distribution_quadrotor.c",
# "src/modules/src/power_distribution_flapper.c",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Python bindings might require some extra work such that they function correctly with the Flapper...
If the second power distribution is included, the build test fails...

@krichardsson krichardsson marked this pull request as draft October 10, 2022 11:17
@tobbeanton
Copy link
Member

Overall I think it looks good! There are two discussion points I can identify though:

  1. The servo integration in motors.c is starting to make this file quite large. Maybe it is time to split it up and have a servo.c and maybe a higher level actuator.c file. This is quite a large change though...
  2. There is a potential conflicting PR Bigquad early access fix #1131 which is changing the external battery monitoring functionality a bit. Which one is easiest to merge first? Probably PR Bigquad early access fix #1131 since it is smaller.

Copy link
Contributor

@krichardsson krichardsson left a comment

Choose a reason for hiding this comment

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

I think this generally looks good!
I mainly have some comments related to ifdefs based on platform, see comments in the code.

src/drivers/src/motors.c Outdated Show resolved Hide resolved
src/drivers/src/motors.c Outdated Show resolved Hide resolved
src/modules/interface/power_distribution.h Outdated Show resolved Hide resolved
src/modules/src/power_distribution_flapper.c Outdated Show resolved Hide resolved
src/modules/src/power_distribution_flapper.c Outdated Show resolved Hide resolved
src/modules/src/power_distribution_flapper.c Show resolved Hide resolved
src/modules/src/supervisor.c Outdated Show resolved Hide resolved
@matejkarasek
Copy link
Contributor Author

matejkarasek commented Oct 10, 2022

Overall I think it looks good! There are two discussion points I can identify though:

Thanks, @tobbeanton!

  1. The servo integration in motors.c is starting to make this file quite large. Maybe it is time to split it up and have a servo.c and maybe a higher level actuator.c file. This is quite a large change though...

I agree. In fact I have started working on another PR that should enable more than 4 motors/servos:
https://github.com/flapper-drones/crazyflie-firmware/tree/more_motors

So how about if I introduce the servo & motor separation there?

EDIT: after digging more into the code, this would require much larger change than I thought... Don't think I would find time to do that myself, but am willing to contribute.
Let me know how you feel about the current workaround.

  1. There is a potential conflicting PR Bigquad early access fix #1131 which is changing the external battery monitoring functionality a bit. Which one is easiest to merge first? Probably PR Bigquad early access fix #1131 since it is smaller.

I agree it is easier to merge #1131 first

@matejkarasek
Copy link
Contributor Author

I think this generally looks good! I mainly have some comments related to ifdefs based on platform, see comments in the code.

Thanks for the quick review @krichardsson
Will fix try to fix these soon

@matejkarasek
Copy link
Contributor Author

All the issues are fixed, pls let me know if this works.
And also whether I should do anything about #1131.
Thanks!

@matejkarasek matejkarasek marked this pull request as ready for review October 10, 2022 18:07
@knmcguire
Copy link
Member

knmcguire commented Oct 11, 2022

Hi! I just did a test flight with our flapper with a controller and the cfclient and it had a major forward pitch eventhough I didn't control it that way. I tested different versions (we have revD):

Could you try it out and see if you see the same?

@matejkarasek
Copy link
Contributor Author

@knmcguire Thanks for testing it out!

Have you trimmed the servos (especially the pitch servo) after the firmware update? It is stored as a permanent parameter, and I have changed the name of the parameter group from "_flapper" to "flapper". So you have to do it after this firmware update...

@knmcguire
Copy link
Member

Yes that's it! the flapper.servPitchNeutr had to be adjusted. It was set to 50 but I had to change it to 58. Let me discuss with @krichardsson if there was anything else but if that is it, we'll merge it soon!

@krichardsson krichardsson merged commit a90aa33 into bitcraze:master Oct 11, 2022
@krichardsson
Copy link
Contributor

Looking nice and clean!
Merged
Welcome flapper!

@matejkarasek
Copy link
Contributor Author

matejkarasek commented Oct 11, 2022

Wohoo! Awesome, thanks!

Yes that's it! the flapper.servPitchNeutr had to be adjusted. It was set to 50 but I had to change it to 58. Let me discuss with @krichardsson if there was anything else but if that is it, we'll merge it soon!

The default value is close, but not exact, this varies from Flapper to Flapper (or from servo to servo, to be precise). The same applies for the yaw servo, though there it is less critical...

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.

4 participants