[alsa-devel] [PATCH V2 1/7] BAT: Add initial functions

[Lu, Han]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Friday, October 2, 2015 6:56 PM
> To: Lu, Han
> Cc: Girdwood, Liam R; Gautier, Bernard; caleb at crome.org; alsa-devel at alsa-
> project.org
> Subject: Re: [PATCH V2 1/7] BAT: Add initial functions
> 
> On Wed, 23 Sep 2015 09:48:49 +0200,
> han.lu at intel.com wrote:
> >
> > From: "Lu, Han" <han.lu at intel.com>
> >
> > Add main entrance, command line parsing, parameter initiating and
> > thread initiating functions for BAT.
> >
> > Signed-off-by: Lu, Han <han.lu at intel.com>
> > Signed-off-by: Liam Girdwood <liam.r.girdwood at intel.com>
> > Signed-off-by: Bernard Gautier <bernard.gautier at intel.com>
> >
> > diff --git a/bat/bat.c b/bat/bat.c
> > new file mode 100644
> > index 0000000..54fe1ec
> > --- /dev/null
> > +++ b/bat/bat.c
> > @@ -0,0 +1,603 @@
> > +/*
> > + * Copyright (C) 2013-2015 Intel Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include <unistd.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <stdbool.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <pthread.h>
> > +#include <getopt.h>
> > +#include <math.h>
> > +#include <limits.h>
> > +
> > +#include "aconfig.h"
> > +#include "gettext.h"
> > +#include "version.h"
> > +
> > +#include "common.h"
> > +
> > +#include "alsa.h"
> > +#include "convert.h"
> > +#include "analyze.h"
> > +
> > +static int get_duration(struct bat *bat) {
> > +	float duration_f;
> > +	long duration_i;
> > +	char *ptrf, *ptri;
> > +
> > +	duration_f = strtof(bat->narg, &ptrf);
> > +	if (duration_f == HUGE_VALF || duration_f == -HUGE_VALF) {
> > +		fprintf(bat->err, _("duration float overflow: %f %d\n"),
> > +				duration_f, -errno);
> > +		return -errno;
> > +	} else if (duration_f == 0.0 && errno != 0) {
> > +		fprintf(bat->err, _("duration float underflow: %f %d\n"),
> > +				duration_f, -errno);
> > +		return -errno;
> > +	}
> > +
> > +	duration_i = strtol(bat->narg, &ptri, 10);
> > +	if (duration_i == LONG_MAX) {
> > +		fprintf(bat->err, _("duration long overflow: %ld %d\n"),
> > +				duration_i, -errno);
> > +		return -errno;
> > +	} else if (duration_i == LONG_MIN) {
> > +		fprintf(bat->err, _("duration long underflow: %ld %d\n"),
> > +				duration_i, -errno);
> > +		return -errno;
> > +	}
> > +
> > +	if (*ptrf == 's') {
> > +		bat->frames = duration_f * bat->rate;
> > +	} else if (*ptri == 0) {
> > +		bat->frames = duration_i;
> > +	} else {
> > +		fprintf(bat->err, _("invalid duration: %s\n"), bat->narg);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (bat->frames <= 0 || bat->frames > MAX_FRAMES) {
> > +		fprintf(bat->err, _("duration out of range: (0, %d(%ds))\n"),
> > +				MAX_FRAMES, (bat->frames / bat->rate));
> 
> The messages strings could be unified to a single error message.
> Otherwise you'll annoy translators, as they will need to work on each
> different variant.

[Han] may I just change the strings to
	" duration overflow ..."
	" duration underflow ..."
	" duration overflow ..."
	" duration underflow ..."
	" invalid duration ..."
	" duration out of range ..."
Or change the sequence and keep 2 or 3 strings like:
	"duration overflow/underflow ..."
	"invalid duration ..."
	"duration out of range ..."
Since they would be clearer to developer/user?

> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void get_sine_frequencies(struct bat *bat, char *freq) {
> > +	char *tmp1;
> > +
> > +	tmp1 = strchr(freq, ',');
> > +	if (tmp1 == NULL) {
> > +		bat->target_freq[1] = bat->target_freq[0] = atof(optarg);
> > +	} else {
> > +		*tmp1 = '\0';
> > +		bat->target_freq[0] = atof(optarg);
> > +		bat->target_freq[1] = atof(tmp1 + 1);
> > +	}
> 
> Note that parsing a floting point string is tricky.  For example, if you run it in a
> locale that handles comma as the decimal point, this gets messy.  Did you
> check it, e.g. with LANG=en_GB or such?

[Han] OK, I'll check the implementation.

> 
> 
> > +static void usage(struct bat *bat, char *argv[]) {
> > +	fprintf(bat->log,
> > +_("Usage:%s [Option]...\n"
> > +"\n"
> > +"-h, --help             help\n"
> > +"-D                     sound card\n"
> > +"-P                     playback pcm\n"
> > +"-C                     capture pcm\n"
> > +"-f                     sample size\n"
> > +"-c                     number of channels\n"
> > +"-r                     sampling rate\n"
> > +"-n                     frames to capture\n"
> > +"-k                     sigma k\n"
> > +"-F                     target frequency\n"
> > +"-p                     total number of periods to play/capture\n"
> > +"    --log=#            path of log file. if not set, logs be put to stdout,\n"
> > +"                       and errors be put to stderr.\n"
> > +"    --file=#           input file\n"
> > +"    --saveplay=#       save playback content to target file, for debug\n"
> > +"    --local            internal loop, bypass hardware\n"
> 
> Why there are options with and without indentation?
> 
> > +), argv[0]);
> 
> The program name should be a const string.

[Han] OK. I followed the indentation style of aplay, to align with multiple-character options such as "--log=#". Shall I remove indentation for these options?

> 
> 
> Takashi

BR,
Han Lu