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 SI-units to control structure #1154

Merged
merged 3 commits into from
Nov 18, 2022
Merged

Conversation

krichardsson
Copy link
Contributor

A first small step to implement SI-units, see #537.

Basic implementation of force/thrust using code from Mike Hamer. Still old battery compensation.

Copy link
Member

@ataffanel ataffanel left a comment

Choose a reason for hiding this comment

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

Looks good to me, it is about time we add some SI units in the chain :-).

Copy link
Contributor

@whoenig whoenig left a comment

Choose a reason for hiding this comment

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

Your change is of course nice and clean, is usual!

I am mostly concerned about the design decision of exposing SI units in the first place. Given that the tendency of the platform is now to support many different frames/motors, having SI units will be pretty painful to maintain (one would essentially need to rescale the gains for each possible platform). That's why I'd suggest to only expose normalized thrust/torque and force new controllers to use that mode. Then, as long as the thrust-to-weight ratio is similar, very little tuning needs to be done. Of course, one could add SI units and later the normalized ones, but there is a risk that controllers simply will stick around with the SI units and that those will be almost impossible to use on custom frames (since domain knowledge is needed for tuning).

See also #537 (comment) (and discussion below).

@krichardsson
Copy link
Contributor Author

krichardsson commented Nov 18, 2022

I see what you mean @whoenig but I'm not sure it is a problem to support SI units in the API as well. To me the SI units API is something that can be used by anyone that finds it useful, and if you chose to use it, the controller will not be easy to use on a different platform. We are still in control of what we merge and can chose if we want to merge a contribution using the SI unit API, or not.

As far as I can see in your implementation in the power distribution of the SI units/normalized forces, there is not a lot of extra code to support both APIs. When my fast copy/paste implementation is upgraded to something better (probably inspired by your code), I assume it will not be too much extra work to maintain both.

What about adding a comment to the SI unit API describing the drawbacks of using it?

I'm of course arguing like this, partly because of self interest as I'm planning to add Mike's controller that uses SI-units. As I'm short on time, I want to minimize my work for now but I do hope to convert it to normalized forces later on :-)

@whoenig
Copy link
Contributor

whoenig commented Nov 18, 2022

I think that makes sense - just be careful when you merge more controllers - at some point it will be difficult to get rid of them:-)

@krichardsson krichardsson merged commit 8cbdcee into master Nov 18, 2022
@krichardsson krichardsson deleted the krichardsson/control-si-units branch November 18, 2022 09:24
@krichardsson krichardsson added this to the 2022.12 milestone Dec 13, 2022
@jpreiss
Copy link
Contributor

jpreiss commented Jul 11, 2023

From browsing the current cflib commander.py and the CRTP commander docs it's not clear to me if there's any way to send a setpoint with physically meaningful thrust unit or not. Is that right?

If such a feature does not exist yet, would it be straightforward to add now that we have SI-unit power distribution?

@ataffanel
Copy link
Member

It seems that the setpoint structure in the Crazyflie is not updated for SI units yet, the thrust in there is still unit-less.

The way to implement a setpoint packet with SI unit would be to add new packets to the generic commander. This should already be possible but the generic commander packet handler will currently have to do the conversion from SI to the set-point structure in the Crazyflie. Updating the setpoint structure first would be the cleanest but is not absolutely required to expose a setpoint packet with SI thrust.

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