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

Hi Takashi,

On 30/08/16 00:19, Takashi Sakamoto wrote:

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@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'?

Sure.

...

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

Right. It will make the code cleaner.