Mellinger: support to change gains in bindings #1180

whoenig · 2023-01-04T09:20:59Z

To use the controller in a simulator (and for testing), we need two
changes:
1.) have a data structure that holds the state, so we can instantiate
multiple controllers.
2.) the ability to change gains without recompiling.

This change introduces a state/param struct to achieve both objectives,
similar to the logic in collision_avoidance.c.

To use the controller in a simulator (and for testing), we need two changes: 1.) have a data structure that holds the state, so we can instantiate multiple controllers. 2.) the ability to change gains without recompiling. This change introduces a state/param struct to achieve both objectives, similar to the logic in collision_avoidance.c.

knmcguire

I have some larger generic comments, just to enhance my understanding of what you are trying to achieve in the PR 😄

knmcguire · 2023-01-05T09:44:23Z

src/modules/interface/controller_mellinger.h

+                                         const state_t *state,
+                                         const uint32_t tick);
+
+#ifdef CRAZYFLIE_FW


I actually never noticed the CRAZYFLIE_FW define before. Is this to separate functionality for the python bindings build and the firmware build? Will there be an build error if you don't use it?

I copied this logic from collision_avoidance.c - might have been introduced there by @jpreiss. We want it here to avoid that these functions end up in the bindings, since they are "internal". A cleaner way could be to have separate files, but that makes code navigation more annoying.

I considered both choices. IIRC, I didn't have particularly strong motivation for choosing one file with #ifdef over two files. I probably just did it to reduce the number of files. I don't see any discussion in #567 either.

There is also the option to use the UNIT_TEST_MODE define that is used in unit tests to show that some part of the code is added/removed to make the code testable. I just added it to the python tests in PR #1183 that I'm working on. Maybe it should be renamed though?

I prefer a solution where there are as few #ifs and buts as possible in the code, especially modifications related to test. I vote for separating the generic functionality and the global state into two files, even though it is a bit trickier to navigate.

As an extra input to this I also would like to add that we have (sort of) wanted to separate code that depends on FreeRTOS and the platform in src/modules, while generic code that could be used on other platforms goes into src/utils (I can see that this is not completely true at the moment...). I'm not sure it has been communicated outside the office but has been part of internal discussions. For the mellinger controller this would translate to maybe moving the generic file to utils while the file with the global state stays in modules. I think this is outside the scope of this PR but I just wanted to put it out there. To me this is related to cleaning up dependencies and make it easier to re-use/test code.

src/modules/src/controller.c

knmcguire · 2023-01-05T09:53:48Z

src/modules/src/controller_mellinger.c

 LOG_GROUP_STOP(ctrlMel)
+
+#endif // CRAZYFLIE_FW


It is not ideal that we put all this code and logging and variables in the CRAZYFLIE_FW define... Technically the struct exists but it is just not written in ? I had a question about the use of this define in one of my earlier questions.

The if/endif preprocessor doesn't need to go around logging/params. In this file, it might not technically be needed at all, since the binding compilation process only generates bindings from a given header file.

Ah yeah that is true. Would it be okay for you to remove this define in case it is not necessary? I just want to prevent more ifdefs to be added if necessary as it complicates the code eventually.

Technically the struct exists but it is just not written in ?

In collision_avoidance.c the global state struct is defined inside the #ifdef CRAZYFLIE_FW. The bindings shouldn't need it at all.

The if/endif preprocessor doesn't need to go around logging/params.

If they are moved outside the #ifdef CRAZYFLIE_FW, then the C compiler for the bindings will need to know about the logging/param system, right? When I was working on collision_avoidance.c I assumed we wouldn't want that.

We need/want it to load default params (by copying). Otherwise we would need a function that fills in the default values.

I think the bindings already have some trickery to basically ignore the LOG/PARAM defines, so technically they can be unguarded.

jpreiss · 2023-01-05T20:47:09Z

Repeating my inline comment: There was no strong motivation for choosing #ifdef CRAZYFLIE_FW over using two files. If the Crazyflie firmware team prefers the latter, now would be a good time to change the collision avoidance module accordingly and apply the multi-file design for this & other future bindings.

knmcguire · 2023-01-05T21:03:20Z

Next to the question on what to do with define, it all looks good to me!

But for obvious reasons, I do want to flight test this 😄 Tomorrow it's a holiday in Sweden so that will be next week.

whoenig · 2023-01-06T09:45:24Z

I got rid of the define in the *.c file (and made sure it still works fine). For the one in the header I see four options:

Keep as is, i.e., use CRAZYFLIE_FW to not include the *Firmware functions in the bindings
Remove the guard. Then, the *Firmware functions will also be in the bindings (but are not useful IMO)
Have a separate header file for the *Firmware functions, e.g., controller_mellinger_firmware.h
Remove the *Firmware functions from the header. Use "extern" declarations in controller.c, rather than a include.

I think options 1 or 3 would be best, but perhaps best if the firmware maintainers raise their preferences.

Flight test: I did one before submitting the PR, but a second one is certainly even better!

krichardsson

Looking good!

krichardsson · 2023-01-06T10:49:34Z

src/modules/interface/controller_mellinger.h

+                                         const state_t *state,
+                                         const uint32_t tick);
+
+#ifdef CRAZYFLIE_FW


There is also the option to use the UNIT_TEST_MODE define that is used in unit tests to show that some part of the code is added/removed to make the code testable. I just added it to the python tests in PR #1183 that I'm working on. Maybe it should be renamed though?

I prefer a solution where there are as few #ifs and buts as possible in the code, especially modifications related to test. I vote for separating the generic functionality and the global state into two files, even though it is a bit trickier to navigate.

As an extra input to this I also would like to add that we have (sort of) wanted to separate code that depends on FreeRTOS and the platform in src/modules, while generic code that could be used on other platforms goes into src/utils (I can see that this is not completely true at the moment...). I'm not sure it has been communicated outside the office but has been part of internal discussions. For the mellinger controller this would translate to maybe moving the generic file to utils while the file with the global state stays in modules. I think this is outside the scope of this PR but I just wanted to put it out there. To me this is related to cleaning up dependencies and make it easier to re-use/test code.

knmcguire · 2023-01-09T15:12:34Z

I flight tested btw, so I'll give the approve! @whoenig do you want to do something still with @krichardsson comments or do you like us to merge it?

whoenig · 2023-01-09T15:34:15Z

I like @krichardsson ideas, just not sure about the naming utils as this usually implies little helper functions. Since this version is flight tested, perhaps we should merge it and then have a separate change for moving files around, without touching any computation.

krichardsson · 2023-01-09T15:54:41Z

@whoenig I agree, let's keep this PR as it is and refactor later.

whoenig requested review from knmcguire and krichardsson January 4, 2023 09:21

knmcguire reviewed Jan 5, 2023

View reviewed changes

remove unnecessary ifdef

5b0dd9a

whoenig mentioned this pull request Jan 9, 2023

sim - np backend - multi robot support IMRCLab/crazyswarm2#167

Closed

krichardsson approved these changes Jan 9, 2023

View reviewed changes

knmcguire approved these changes Jan 9, 2023

View reviewed changes

knmcguire merged commit 7a1bf59 into bitcraze:master Jan 9, 2023

knmcguire mentioned this pull request Jan 24, 2023

controllers: Unify data structures for gains and parameters #1198

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mellinger: support to change gains in bindings #1180

Mellinger: support to change gains in bindings #1180

whoenig commented Jan 4, 2023

knmcguire left a comment

knmcguire Jan 5, 2023

whoenig Jan 5, 2023

jpreiss Jan 5, 2023

krichardsson Jan 6, 2023

knmcguire Jan 5, 2023

whoenig Jan 5, 2023

knmcguire Jan 5, 2023

jpreiss Jan 5, 2023

whoenig Jan 5, 2023

jpreiss commented Jan 5, 2023

knmcguire commented Jan 5, 2023

whoenig commented Jan 6, 2023

krichardsson left a comment

krichardsson Jan 6, 2023

knmcguire commented Jan 9, 2023

whoenig commented Jan 9, 2023

krichardsson commented Jan 9, 2023

Mellinger: support to change gains in bindings #1180

Mellinger: support to change gains in bindings #1180

Conversation

whoenig commented Jan 4, 2023

knmcguire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpreiss commented Jan 5, 2023

knmcguire commented Jan 5, 2023

whoenig commented Jan 6, 2023

krichardsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knmcguire commented Jan 9, 2023

whoenig commented Jan 9, 2023

krichardsson commented Jan 9, 2023