[alsa-devel] [PATCH v6 1/2] amidi: add delay option

[Takashi Sakamoto]

Hi,

On Aug 23 2016 23:09, Felipe F. Tonello wrote:
> This patch adds a new options to amidi tool to add a delay (in milliseconds)
> to each SysEx message.
> 
> This is very useful when sending firmware updates to a remote device via SysEx
> or any other use that requires this delay in between messages.
> 
> Signed-off-by: Felipe F. Tonello <eu at felipetonello.com>
> ---
>  amidi/amidi.1 | 14 ++++++++++++++
>  amidi/amidi.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 67 insertions(+), 5 deletions(-)
> 
> diff --git a/amidi/amidi.1 b/amidi/amidi.1
> index 86beb27a1e3b..4ca2164788f1 100644
> --- a/amidi/amidi.1
> +++ b/amidi/amidi.1
> @@ -120,6 +120,12 @@ received MIDI commands.
>  Does not ignore Clock bytes (F8h) when saving or printing received
>  MIDI commands.
>  
> +.TP
> +.I \-D, \-\-delay=mseconds
> +Adds a delay in between each SysEx message sent to a device. It is
> +useful when sending firmware updates via SysEx messages to a remote
> +device.
> +

I think it's helpful to name this option so that it keeps intervals
somewhat between each system exclusives. For example, how do you think
about using 'sysex-interval'?

>  .SH EXAMPLES
>  
>  .TP
> @@ -130,6 +136,14 @@ to port
>  .I hw:0.
>  
>  .TP
> +.B amidi \-p hw:1,0,0 -s firmware.syx \-D 100
> +will send the MIDI commands in
> +.I firmware.syx
> +to port
> +.I hw:1,0,0
> +with 100 milliseconds delay in between each SysEx message.
> +
> +.TP
>  .B amidi \-S 'F0 43 10 4C 00 00 7E 00 F7'
>  sends an XG Reset to the default port.
>  
> diff --git a/amidi/amidi.c b/amidi/amidi.c
> index c20512cc96a7..d0dbb322f80a 100644
> --- a/amidi/amidi.c
> +++ b/amidi/amidi.c
> @@ -52,6 +52,7 @@ static int receive_file;
>  static int dump;
>  static float timeout;
>  static int stop;
> +static int delay;
>  static snd_rawmidi_t *input, **inputp;
>  static snd_rawmidi_t *output, **outputp;
>  
> @@ -82,7 +83,8 @@ static void usage(void)
>  		"-t, --timeout=seconds  exits when no data has been received\n"
>  		"                       for the specified duration\n"
>  		"-a, --active-sensing   include active sensing bytes\n"
> -		"-c, --clock            include clock bytes\n");
> +		"-c, --clock            include clock bytes\n"
> +		"-D, --delay=mseconds   delay in between each SysEx message\n");
>  }
>  
>  static void version(void)
> @@ -411,7 +413,7 @@ static void add_send_hex_data(const char *str)
>  
>  int main(int argc, char *argv[])
>  {
> -	static const char short_options[] = "hVlLp:s:r:S::dt:ac";
> +	static const char short_options[] = "hVlLp:s:r:S::dt:acD:";
>  	static const struct option long_options[] = {
>  		{"help", 0, NULL, 'h'},
>  		{"version", 0, NULL, 'V'},
> @@ -425,6 +427,7 @@ int main(int argc, char *argv[])
>  		{"timeout", 1, NULL, 't'},
>  		{"active-sensing", 0, NULL, 'a'},
>  		{"clock", 0, NULL, 'c'},
> +		{"delay", 1, NULL, 'D'},
>  		{ }
>  	};
>  	int c, err, ok = 0;
> @@ -474,6 +477,9 @@ int main(int argc, char *argv[])
>  		case 'c':
>  			ignore_clock = 0;
>  			break;
> +		case 'D':
> +			delay = atoi(optarg);
> +			break;
>  		default:
>  			error("Try `amidi --help' for more information.");
>  			return 1;
> @@ -549,9 +555,51 @@ int main(int argc, char *argv[])
>  			error("cannot set blocking mode: %s", snd_strerror(err));
>  			goto _exit;
>  		}
> -		if ((err = snd_rawmidi_write(output, send_data, send_data_length)) < 0) {
> -			error("cannot send data: %s", snd_strerror(err));
> -			goto _exit;
> +		if (!delay) {
> +			if ((err = snd_rawmidi_write(output, send_data, send_data_length)) < 0) {
> +				error("cannot send data: %s", snd_strerror(err));
> +				return err;
> +			}
> +		} else {
> +			char *data = send_data;
> +			size_t buffer_size;
> +			snd_rawmidi_params_t *param;
> +			snd_rawmidi_status_t *st;
> +
> +			snd_rawmidi_status_malloc(&st);
> +
> +			snd_rawmidi_params_malloc(&param);
> +			snd_rawmidi_params_current(output, param);
> +			buffer_size = snd_rawmidi_params_get_buffer_size(param);
> +			snd_rawmidi_params_free(param);
> +
> +			while (data < (send_data + send_data_length)) {
> +				int len = send_data + send_data_length - data;
> +				char *temp;
> +
> +				if (data > send_data) {
> +					snd_rawmidi_status(output, st);
> +					do {
> +						/* 320 µs per byte as noted in Page 1 of MIDI spec */
> +						usleep((buffer_size - snd_rawmidi_status_get_avail(st)) * 320);
> +						snd_rawmidi_status(output, st);
> +					} while(snd_rawmidi_status_get_avail(st) < buffer_size);
> +					usleep(delay * 1000);
> +				}
> +
> +				/* find end of SysEx */
> +				if ((temp = memchr(data, 0xf7, len)) != NULL)
> +					len = temp - data + 1;
> +
> +				if ((err = snd_rawmidi_write(output, data, len)) < 0) {
> +					error("cannot send data: %s", snd_strerror(err));
> +					return err;
> +				}
> +
> +				data += len;
> +			}
> +
> +			snd_rawmidi_status_free(st);
>  		}
>  	}

In my opinion, it's better to split function specific to this purpose
and call it here. An appropriate name of the function might help readers.


Regards

Takashi Sakamoto