[Buildroot] [PATCH 1/1] liquid-dsp: new package
Thomas Petazzoni
thomas.petazzoni at free-electrons.com
Fri Oct 9 09:08:13 UTC 2015
Dear Guillaume William Bres,
Thanks for your patch! I have a few comments below.
On Thu, 8 Oct 2015 14:37:26 -0600, Guillaume William Bres wrote:
> diff --git a/package/liquid-dsp/Config.in b/package/liquid-dsp/Config.in
> new file mode 100644
> index 0000000..6ab7011
> --- /dev/null
> +++ b/package/liquid-dsp/Config.in
> @@ -0,0 +1,21 @@
> +comment "liquid-dsp requires a (e)glibc toolchain"
> + depends on BR2_TOOLCHAIN_USES_GLIBC
Can you comment on what makes liquid-dsp work with glibc only, and not
with uClibc or musl?
> +config BR2_PACKAGE_LIQUID_DSP
> + bool "liquid-dsp"
> + help
> + Liquid-DSP is a free and open-source signal processing library
> + for software-defined radios written in C.
> + Its purpose is to provide a set of extensible DSP modules
> + that do no rely on external dependencies or cumbersome frameworks.
> +
> + http://liquidsdr.org/
> +
> +if BR2_PACKAGE_LIQUID_DSP
> +
> +config BR2_PACKAGE_LIQUID_DSP_FAST
> + bool "optimize for speed over accuracy"
> + help
> + Optimize for speed over accuracy.
Indentation should be just one tab + two spaces for the help text. You
got it correct above, but not here.
> diff --git a/package/liquid-dsp/liquid-dsp.mk b/package/liquid-dsp/liquid-dsp.mk
> new file mode 100644
> index 0000000..6a3e8dc
> --- /dev/null
> +++ b/package/liquid-dsp/liquid-dsp.mk
> @@ -0,0 +1,50 @@
> +################################################################################
> +#
> +# liquid-dsp
> +#
> +################################################################################
> +
> +LIQUID_DSP_VERSION = master
Incorrect. This is a branch name, so it is constantly changing, which
means that depending on when you build the package, you will not get
the same code since more commits might have been made to the master
branch. Please use either a tag, or a full commit ID.
> +LIQUID_DSP_SITE = https://github.com/jgaeddert/liquid-dsp.git
Use the github helper function (see the manual for details).
> +LIQUID_DSP_SITE_METHOD = git
Not needed if you use the Github helper.
> +LIQUID_DSP_LICENSE = GPL
Please specify the version (i.e GPLv2, GPLv2+, GPLv3, GPLv3+).
> +LIQUID_DSP_LICENSE_FILES = COPYING
> +LIQUID_DSP_INSTALL_STAGING = yes
This should be "YES" instead of "yes".
> +
> +LIQUID_DSP_DEPENDENCIES = host-autoconf host-automake
> +
> +define LIQUID_DSP_PRE_CONFIGURE_BOOTSTRAP
> + rm -f $(LIQUID_DSP_DIR)/config.cache
> + rm -f $(LIQUID_DSP_DIR)/aclocal.m4
> + cd $(LIQUID_DSP_DIR) && $(ACLOCAL) -I./scripts && $(AUTOCONF) && $(AUTOHEADER)
> +endef
Why don't you use:
LIBQUID_DSP_AUTORECONF = YES
?
> +LIQUID_DSP_PRE_CONFIGURE_HOOKS += LIQUID_DSP_PRE_CONFIGURE_BOOTSTRAP
> +
> +# Speed over accuracy trade off
> +ifeq ($(BR2_PACKAGE_LIQUID_DSP_FAST),y)
> +LIQUID_DSP_CFLAGS += -O3 -ffast-math
> +endif
I think we should include -O3 here. The optimization level is decided
globally by the BR2_OPTIMIZE options.
> +# ARM Optimizations
> +ifeq ($(BR2_ARM_ENABLE_NEON),y)
> +LIQUID_DSP_CFLAGS += -mfpu=neon
> +LIQUID_DSP_CFLAGS += -mfloat-abi=hard
> +endif
Using BR2_ARM_ENABLE_NEON is wrong here. You should use
BR2_ARM_CPU_HAS_NEON instead. However:
1/ Using NEON as the FPU is not recommended as the computations are
not IEEE 754 compliant.
2/ Forcing -mfloat-abi=hard is clearly wrong, since it would only work
when the selected ABI is EABIhf.
> +# use FFTW instead of built-in FFT
> +ifeq ($(BR2_PACKAGE_FFTW_PRECISION_SINGLE),y)
> +LIQUID_DSP_CFLAGS += -lfftw3f
> +endif
> +
> +ifeq ($(BR2_PACKAGE_FFTW_PRECISION_DOUBLE),y)
> +LIQUID_DSP_CFLAGS += -lfftw3
> +endif
> +
> +ifeq ($(BR2_PACKAGE_FFTW_PRECISION_LONG_DOUBLE),y)
> +LIQUID_DSP_CFLAGS += -lfftw3l
> +endif
These are not CFLAGS, but LDFLAGS.
Thanks!
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
More information about the buildroot
mailing list