Python Bindings

[whoenig]

In the past, Python bindings for parts of the firmware have been very helpful:

• Simulation of the planning portion as part of the Crazyswarm,
• Offline parameter tuning of the EKF,
• Software-in-the-loop simulations / controller testing and development.

Other potential applications include:

• Improvement of code quality by better separation of generic quadrotor logic and FreeRTOS-specific functionality,
• Easier automated (unit) testing.

This issue is opened to discuss where to include the python bindings moving forward. There are two options:

1. Move them as part of this repository and invoke the build with make python-bindings. Pros: cleaner commit-history for changes related to the bindings, easier test integration, easy integration as part of CI. Cons: "bloat" of this repo (we can put all the logic in a sub-folder, though).

2. Have a separate repo that uses this repo as submodule (similar to the current Crazyswarm solution). Pros: Cleaner code separation. Cons: harder to maintain, difficult to prevent firmware breaking changes because of missing CI integration.

I personally feel option 1 is better in the long run, but I wanted to start this discussion on how everyone else (including @jpreiss) feel about it. The goal is to replace the current solution in the Crazyswarm with these newly proposed bindings and to extend the bindings to other parts of the firmware for improved simulation and ease of tuning.

[jpreiss]

I agree with @whoenig's list of benefits. We could use the version from Crazyswarm mostly unchanged (I think) as a starting point.

The files in that link are everything you need to generate the bindings - not that much bloat. Since one of the goals is unit testing, and unit tests usually live in the same repo as the main code, I think option 1 makes sense.

Impact on code design

I have written several firmware modules with the goal of "bindability" from the beginning. I can share some thoughts on the design changes needed to make current modules more bindable.

In general, the core logic should be implemented in a "library file" that has no transitive dependencies on ARM or FreeRTOS. The code that plugs this library into FreeRTOS, the param/log system, etc., is in a different "integration file".

396 is an example of a change that could be implemented with confidence if it were easy to compile the full control pipeline into a standalone function and test it with synthetic inputs.

Global variables

The largest shift in coding style comes from dealing with persistent state. Crazyflie's code structure essentially forces you to use global variables: each module's Init() function takes no arguments, so any persistent state in the module generally lives in file-static global variables in the same file that contains Init(). The logging and parameter systems also require global variables.

For Crazyswarm's bindings, we don't want any global variables in the library file because we want to instantiate multiple Crazyflies in a single address space. Therefore, any module that needs persistent state will have something like struct modulename_state. The integration file will contain one statically allocated instance of this struct, which can be wired into the param/logging system if needed. An example of this is planner.h/pptraj.h and their integration into the firmware in crtp_commander_high_level.c - the latter only deals with radio parsing, tasks, semaphores, logging, etc.

This might not be as important outside the multi-robot context, but I think it is good practice anyway.

Passing data between modules

Another difference is if the module's main computation has several outputs/results that need to be passed to other modules. In the current code, the module will usually call some functions of the other modules directly. For example, power_distribution.c consumes a control_t but directly calls functions to set individual motor powers instead of returning motor power values. In the header file, there is no way to get the computed values. This would make it hard/impossible to test the power distribution functionality via bindings. In bindable code, these should all be returned in an output struct so the library function does not need to know the details of how its outputs will be used.

A similar issue holds for gathering inputs from other modules. For example, sitAw queries the current motor values instead of taking them as an input.

Pure functions will always be easily bindable. Functions that modify their inputs in-place are not pure, but they are still better than functions whose outputs are only visible via side effects, or functions whose output depends on anything that is not passed as an argument.

TL;DR

I think exposing more firmware components via bindings will require some nontrivial design changes, but those changes will improve the code quality.

[knmcguire]

Hi! It is a bit empty at the office now since everybody except for me and Barbara. The rest will be returning end of July.

I will start thinking about this as well, but it would be good to also include the others in this discussion. So just letting you know that it might be a little while before we can really act on this.

[jonasdn]

Is this something that we are interested in doing? If so can we come up with a step-by-step plan? Or should this be closed?

[whoenig]

I hope you are very excited about this:-) We already have this as part of the Crazyswarm, but would like to "hand it over" and extend it. I think the first step is to agree where this should functionality should be (see option 1 and 2 in the initial post). Once we agreed on that, the next step is to port what we have to the agreed on model from the Crazyswarm project and enable CI and tests for it. The third step is to extend it to the new, but very important use-cases of EKF and controller bindings, that will allow offline experiments and tuning much easier.

[whoenig]

Based on our recent in-person discussion, I just wanted to emphasize the advantages of this for the user and Bitcraze:

User:

• It simplifies the transition from idea -> realtime implementation (a transition that is very difficult for non-embedded engineers/scientists).
• It enables more realistic simulations (see https://github.com/USC-ACTLab/crazyswarm/tree/master/ros_ws/src/crazyswarm/scripts/pycrazyswarm/cfsim for an example where we use it in the Crazyswarm)

Bitcraze:

• Allows unit testing of code that is otherwise only executed during flight (EKF, controllers)
• It will guide a better, more modular coding style (see @jpreiss comment earlier for details)

I'd also like to note that this issue is mostly for discussion a) if it should be done; and b) where the code should be. As noted before, I have prototypes for it already, which I can clean-up for review once those decisions are made.

[jpreiss]

Reiterating my support :) I see this as an essential feature. If I were developing a new firmware module that had nothing to do with multi-robot systems or motion capture, I would still use the Crazyswarm binding system to test it.

I also think Python bindings are a better option than "refactor for testability but write unit tests in C". Generating inputs and processing outputs is easier with NumPy/SciPy. It's also easy to plot data, which can be extremely useful for debugging.

[jonasdn]

Thank you @jpreiss and @whoenig!

We are going to discuss this within a couple of weeks. Before that, could you help us assess the work needed and the impact on current firmware code. You touch a bit on it in your earlier post @jpreiss but it would be nice to hear more, what effect will this have on current and future firmware code?

• What are we giving up in order to have this?
• What is the maintenance cost of this?

Thanks!

[whoenig]

• I think there is a one-time effort for refactoring for modules (c-file) that should get a Python interface.
• The on-going maintenance is that now each commit can potentially break the python bindings. We will catch this in CI of course, but it is still something that could be potentially annoying for developers (assuming that make will only make the firmware image and make bindings-python is a new target).
• I don't think we give up a lot with this, because the bindings are per-module and a optional build target. For modules that are chosen to be bindings, there will be less programming flexibility, because a (IMO) better code design will be enforced (more modular design, as outlined by James earlier.)
• In my opinion, the largest downside of this proposal is that it's yet another feature, so, like all features, it can potentially break and create maintenance work once it does. The best antidote is to have automated tests in place (which shouldn't be hard here, since those bindings are partially made to be used for tests).

[jpreiss]

Here is my forecast of tasks/cost. I assume that we will continue using SWIG as the binding-generator tool. Discussion at the end.

Immediate tasks

• Copy the current Crazyswarm implementation and unit tests.
• Take the opportunity to do minor cleanup, renaming, etc.
• Add support in the toolbelt? The only dependencies should be crazyflie-firmware, libpython3-dev, SWIG, and the platform C compiler.
• Set up CI to build bindings and run tests
• (Me and @whoenig) Change Crazyswarm to use the Bitcraze bindings. This does not technically need to be done at the same time, but I think it would be a good idea, to ensure that the new bindings are easy to use from an outside project.

Short-term tasks

• Refactoring the EKF and controllers to be bindable.
• (Possibly) writing SWIG glue code to make the EKF and controller bindings more "Pythonic".
• Writing unit tests for the EKF and controllers.

Ongoing tasks

Maintaining bindings during other changes

Like @whoenig mentioned, commits should not break the bindings. So far, this has happened when changes introduced an ARM-only (transitive) header dependency to code that was previously platform-independent. I believe this will always be solvable by one of the options:

• Splitting a header file.
• Replacing an #include with a forward declaration in the firmware code, e.g. https://github.com/bitcraze/crazyflie-firmware/pull/581.
• typedefing an ARM type that was not needed in the binding to a dummy type, e.g. in https://github.com/bitcraze/crazyflie-firmware/pull/676 (closed without merge because the change is only in bindings code).

Adding new bindings

Contributors who want to add bindings for existing or new code will need to add the file(s) to the SWIG interface file and setup.py. This is often very easy. But occasionally, the auto-generated bindings will be clumsy to use from Python. Then the author may want to write some glue code to make the bindings more "Pythonic" and easier to use. For example:

• C glue helper functions for struct poly4d: link.
• C glue wrapper function for a function that requires a "workspace" for math intermediate results, so the Python code does not need to worry about constructing the workspace (this should be rare): link.
• SWIG instruction to generate NumPy array wrappers (this should also be rare because most of the firmware does not need to handle variable-length arrays, but perhaps it will be common in multi-robot modules): link.
• Python glue methods providing operator overloading and other OOP convenience for struct vec: link. Probably a unique case to have so much glue, but other structs might benefit from one or two glue methods.

Discussion

I agree with Wolfgang that the main cost is yet another feature. However, I also think that introducing SWIG is a cost. It is something else to learn.

I think the situation with SWIG would be similar to the crazyflie-firmware makefile. Most contributors would need to learn a lot about Make before they could recreate that makefile from scratch. But when adding one new feature, they can usually figure out what to do by copying existing patterns.

There are other tools for generating Python bindings from C code. In 2016, Wolfgang and I evaluated them and decided that SWIG was still a good choice despite its age. Each system has different tradeoffs. Ultimately C and Python are such different languages that you cannot avoid writing per-function or per-struct glue code occasionally.

Another option would be refactoring the firmware for testability on x86 but writing the unit tests in C. As in my previous comment, I think this would make it harder to write sophisticated tests. For Crazyswarm we would still need to maintain Python bindings externally. But I can understand why it might be desirable from a simplicity perspective.

[jonasdn]

Thank you @jpreiss and @whoenig !

We have discussed this at Bitcraze and we are positive to the bindings! Our (mine only?) fear has been that by having these bindings we reduce agility and limit ways to change the firmware architecture going forward. But you and my team mates have managed to convince me that is not necessarily the case.

And we do, absolutely, see the benefit in having these bindings (not least me having sent droves of Crazyflies into walls this week) and we want to move forward! Would it be possible to see a PR from one of you in the near future? Moving the bindings from Crazyswarm to this repo?

Thanks again!

[jpreiss]

I have time to work on it this week or next week.

I guess the easiest thing to do at first is copy the Crazyswarm version with as few as possible changes. Once we get CI up and running and have the PR, we can discuss details like renaming things.

Until now it didn't occur to me: the Crazyswarm unit tests don't call firmware functions directly because they go through our simulation/ROS abstraction layer. Maybe it would be better for Bitcraze to have tests written directly against the firmware bindings?

We could copy the Crazyswarm tests with the abstraction layer, but plan to rewrite them in the future.

[whoenig]

I think ultimately, the tests should directly use the firmware bindings. It would also be nice to have examples (e.g., visualizing the EKF from a logfile). But the first step is to integrate into the build system (this is different for us, because we build externally) and CI.

[knmcguire]

Just a question for this issue, what is the current state of getting the EKF and controller python bindings out there? There has been some prep made I saw but I don't know what the timeline is and if some more hands are required?

Also another question, perhaps an obvious one, as we are making bindings for python, I assume that it is very well possible to make these bindings for c/c++ code too as well right?

[whoenig]

The next step is to support collision avoidance, because than we can finally switch over to the official bindings in the Crazyswarm. For EKF and controllers exist prototypes for older firmware versions. I don't have a concrete timeline on any of these, but it would be nice to get it done rather sooner than later (especially before any other upcoming release!).

The code cleanup we made to allow Python bindings means that you should already be able to directly add the corresponding c files to any C(++) project. For the bindings we rely on SWIG, which has other backends than Python, including Lua, R, Go, C#.

[jpreiss]

Don't we still need to port the bindings for math3d.h, pptraj.h and planner.h as well as collision avoidance?

[whoenig]

No, I did those other ones a while ago:-) See https://github.com/bitcraze/crazyflie-firmware/blob/c7a178ba6783deff69ff78819aa9e75a93375540/bindings/cffirmware.i#L8-L11.

[whoenig]

@jpreiss: I added a draft PR at https://github.com/bitcraze/crazyflie-firmware/pull/914. Adding collision avoidance is easy, but I am not sure about testing. It looks like the existing tests in the Crazyswarm are more like system-level tests, not unittests.

I have not tried adding this to Crazyswarm, but technically after this change we should have all the features we need to make it happen.

[knmcguire]

Ah good to know, I'm not familiar with SWIG myself so I'll look into that, but good to know it is possible.

Just on the comment of the next release, we are planning to release quite quickly after our Q1 meeting, so that's end next week (20th) or the week after. Probably it will be a bit too soon to get all the python bindings in that is in the pipeline?

[whoenig]

Not all, but it would be nice to get the collision-avoidance one in, so that we can switch the Crazyswarm over to the official bindings.

[knmcguire]

I believe that @jonasdn have already merged your PR #914 for the collission avoidance after I've notified him, so it should be okay:)

[jpreiss]

@whoenig some of the Crazyswarm tests depend on CrazyflieSim.integrate(). In particular, anything that uses velocity commands. I am not sure if it makes sense to port those to firmware tests.

test_highLevel.py should be easy to port.

For now, maybe our first goal should be to make Crazyswarm use the official firmware bindings. Then we can at least use the Crazyswarm test suite as-is to verify that the bindings are generated correctly.

[knmcguire]

We are quite close to finishing testing on the release so if there is any more binding that you want in the release, it's better to push a PR either today or tomorrow 😁

[krichardsson]

@whoenig I would like to get the EKF into the python bindings and I had a quick look the other day. You mentioned earlier that there are some old prototype code, is this available? Is is still useful?

To me it looks like the main hurdle when refactoring this code (mainly kalman_core.c) is the arm matrix types and functions. I guess there are two main tracks:

1. Keep the arm types and functions and write new implementations that are used when running in the python bindings (and possibly unit tests).
2. Write/find new matrix functions that can be compiled and used from the python bindings (similar to using the math3d lib)

I think the arm functions are hardware optimized in some cases and are possibly more efficient than generic implementations. Not sure in which cases though.

[whoenig]

ekf_dbg.zip

I am attaching here what I had. I basically ended up doing the easy route, trying to change as little as possible in the EKF itself. So for the arm-types, I just re-implemented (or I guess, rather copied) the original C-code from ARM for the functions that were called.

I think this is still useful, although based on an older firmware version (2019ish), so perhaps other changes are required on top of it.

One note about optimization: I think the ARM function is well optimized for generic sized matrices. The idea behind math3d is to use matrices/vectors that are known at compile time, which, in theory, should allow the compiler to do many more optimizations. Since we know the matrix size for the EKF, this could result in some perf improvements when moving away from the arm lib.

[krichardsson]

Thanks I'll check it out!

[knmcguire]

There is a python test now for kalman core with this PR https://github.com/bitcraze/crazyflie-firmware/pull/1322.

Also we have several issues open about different parts of the pythonbindings namely:

• https://github.com/bitcraze/crazyflie-firmware/issues/1320
• https://github.com/bitcraze/crazyflie-firmware/issues/1067

Is it maybe a good idea to close this issue and make smaller ones for each inclusion of new features?

[knmcguire]

This is considered closed now as we have started smaller issues already to improve the python bindings further.