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

moving some physical constants so they can be config dependants #1338

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

atoums
Copy link
Contributor

@atoums atoums commented Jan 4, 2024

Hello,
The mass of the crazyflie and the arm length has been moved so they can easily be integrated to a configuration.
Have a nice day !

Copy link
Member

@knmcguire knmcguire left a comment

Choose a reason for hiding this comment

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

Oh yes nice catch! That would be a better place for it.

To make it even better I think that these values should actually also be placed in platform_defaults_cf2.h, since these are platform-specific.

@knmcguire
Copy link
Member

Also, an added thought, after we merge this an issue should be made that we also add these values for the flapper/bolt, although the latter might be difficult as anyone can make 'their own' platform.

Also the brushless version of the crazyflie that is coming out soon, is still part of the cf2 platform with some adjustments, but that will be a bit tricky too.

All of what I mention here in this post doesn't need to be changed in this PR, but just me thinking out loud :). Just the change indicated in the previous comment should be added.

@atoums
Copy link
Contributor Author

atoums commented Jan 5, 2024

Those changes are idndefs to setup a default value that is needed for a platform to compile; as the mass is required for all platforms in the brescianini and mellinger controllers and the length is for the tag, cf2 and bolt configs. they are overridden by those #defined in a platform config.
Another security of provided it in the def config with ifndefs is not to break the homemade configs of those running the code that haven't redefined the mass in their config.
So provinding a default value in the def config seems important.

I can redefine those a second time with the same values in the cf2 platform config file to make it explicit for this platform, but i do not see how i can be mush more specific.
Sorry if i missed the target of your comments.

@knmcguire
Copy link
Member

Yes that is true, but I think it is still good to indicate where these default values come from. The mass and arms are significant to the CF2.1 specifically, so I do think it should go in the platform file. The ifndefs are just to make sure that it still compiles for the other platforms, which are the bolt and flappers, so I don't see it as a duplication since the intention is different. Technically these ifdefs should not be necessary and just as a failsafe and in the future, if a platform file does not have these values defined, that should generate a build error (but not for now)

After this PR, I will make a ticket that these values need to be specifically defined in the platform's file for the Bolt or flapper.

Hope that makes sense! It will make the code more understandable.

@atoums
Copy link
Contributor Author

atoums commented Jan 8, 2024

Thank you for explaining again. I hope this will do.

@atoums atoums requested a review from knmcguire January 8, 2024 10:59
@knmcguire
Copy link
Member

Thanks! I'll probably do a quick build and flight test soon before merging but it looks good so far to me :)

@knmcguire
Copy link
Member

flight tested and it looks all good. I'll merge it. Thanks!

@knmcguire knmcguire merged commit 09bcfb0 into bitcraze:master Jan 30, 2024
24 checks passed
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