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

Added CMSIS DSP_Lib #129

Closed
wants to merge 3 commits into from
Closed

Added CMSIS DSP_Lib #129

wants to merge 3 commits into from

Conversation

mikehamer
Copy link
Contributor

Note that arm_math.h and the DSP library comes from CMSIS v4.10, whereas the CF2 Firmware uses CMSIS 3.00. This doesn't seem to be causing problems, since the DSP library is fairly self contained.

The CMSIS version used for the CF1 is, however, too old to be compatible with the newer DSP_Lib, and would need to be updated if DSP_Lib were required for the CF1.

Signed-off-by: Mike Hamer [email protected]

Previously, this line reset LDFLAGS, whereas it should just add flags.

Signed-off-by: Mike Hamer <[email protected]>
Note that arm_math.h and the DSP library comes from CMSIS v4.10, whereas the CF2 Firmware uses CMSIS 3.00. This doesn't seem to be causing problems, since the DSP library is fairly self contained.

The CMSIS version used for the CF1 is, however, too old to be compatible with the newer DSP_Lib, and would need to be updated if DSP_Lib were required for the CF1.

Signed-off-by: Mike Hamer <[email protected]>
@ataffanel
Copy link
Member

Do we have a link to the source of the library? I think we need to link to it somehow if we use the binary version. Otherwise can we add the source code?

@mikehamer
Copy link
Contributor Author

We could add the source code (I had done this initially), however there are two big reasons to use the library (tldr: simplicity and simplicity):

  1. When adding the source code, we need to modify a few targets in targets.mk since the DSP_Lib source contains .S files (a mix of C preprocessors and ASM) which are currently not handled by the makefile targets. And these need to be handled differently to .s (pure assembly) files, so adding them to the AS target won't work.
  2. Optimization of the source makes a HUGE impact on the speed of the DSP_Lib. Which is fine if the firmware is compiled with -O2, but if you run a debug build (and compile with -O0), the DSP_Lib is an order of magnitude slower (roughly the speed of nested for loops for matrix multiplication). Meaning if the DSP_Lib source were to be included and compiled from scratch with the firmware, we would need additional makefile targets to always compile the DSP_Lib source with optimizations. But the DSP_Lib would be about 5% faster, than using the library version (on the limited set of functions I tested).

The DSP Lib was taken from the (now open-sourced) CMSIS development: https://github.com/ARM-software/CMSIS

At some point in time, now that the above repository is available online, we could consider moving the CMSIS implementation into the vendor folder as a git submodule.

@whoenig
Copy link
Contributor

whoenig commented Jun 28, 2016

I can see Mikes points, but there are also huge disadvantages with submitting binaries:
1.) Compatibility issues. If we upgrade the compiler there might be issues when linking against versions generated with an older compiler. This is less an issue with C, but with C++, but still good practice to avoid whenever possible.
2.) Losing Optimizations. The compiler can optimize code as a whole, decide to inline functions etc., only if it has the source available. Furthermore, if switching to a newer compiler (with better optimizations) this will only improve performance if the source version is checked in. Mike mentioned he measured 5% overhead, but it is unclear how that was measured and if it will change in the future and become more significant.
3.) Better version control. If the code is submitted it is easier to see what changed if we upgrade to a new DSP_Lib rather than dropping in a new magic binary blob.

I like the idea of having a git submodule most, even if it increases compile time and Makefile efforts.

@ataffanel
Copy link
Member

@whoenig Agreed here, I will add it to the build system as a git submodule. Having it to compile with different compile flag than the rest of the system is a small complication but I think I will just make another Makefile and compile the .a lib with it. This .a will be a dependency of the Crazyflie 2 firmware.

@ataffanel ataffanel closed this in 2f55d07 Jun 28, 2016
@ataffanel
Copy link
Member

I pushed a commit that adds the CMSIS lib from source. It is compiled with a separate Makefile into a lib file so we can chose the compile flags for the lib independently from the rest of the Firmware. This is enabled for CF2 only as there is no need of it for CF1 for now.

@mikehamer mikehamer deleted the dsp_lib branch June 28, 2016 19:09
ataffanel added a commit that referenced this pull request Jun 29, 2016
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.

3 participants