Re: [PATCH V2 1/7] BAT: Add initial functions

[Liam Girdwood <liam.r.girdwood@intel.com>]

On Mon, 2015-10-12 at 08:56 +0000, Lu, Han wrote:
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Friday, October 2, 2015 6:56 PM
> > To: Lu, Han
> > Cc: Girdwood, Liam R; Gautier, Bernard; caleb@crome.org; alsa-devel@alsa-
> > project.org
> > Subject: Re: [PATCH V2 1/7] BAT: Add initial functions
> > 
> > On Wed, 23 Sep 2015 09:48:49 +0200,
> > han.lu@intel.com wrote:
> > >
> > > From: "Lu, Han" <han.lu@intel.com>
> > >
> > > Add main entrance, command line parsing, parameter initiating and
> > > thread initiating functions for BAT.
> > >
> > > Signed-off-by: Lu, Han <han.lu@intel.com>
> > > Signed-off-by: Liam Girdwood <liam.r.girdwood@intel.com>
> > > Signed-off-by: Bernard Gautier <bernard.gautier@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?
> 

I would probably just have "duration overflow/underflow" and "invalid
duration". That cuts down 6 strings to only 2. Likewise for other
strings.

Remember to also include variables in the error output that will help
identify the error cause (since we wont be able to determine the exact
error line by string text).

> > 
> > > +		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.

You could even try using a : or similar (instead of commas) to
deliminate the input string. e.g. -values 1.2, 4.5 would become -values
1.2 : 4.5

Liam

> 
> > 
> > 
> > > +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
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel


---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.