[PATCH v5 0/2] amidi delay option

[PATCH v5 0/2] amidi delay option

[Felipe F. Tonello]

Small improvement to amidi, adding useful functionality.

changes from v4:
 * simplified code

changes from v3:
 * fixed a bug if memchr() returns NULL

changes from v2:
 * only check of end of sysex (0xf7)
 * use memchr() instead of direct loop approach
 * fixed commit message

changes from v1:
 * remove delay in between any MIDI message, only SysEx
 * added patch to use getopt_long() arguments

Felipe F. Tonello (2):
  amidi: add delay option
  amidi: use GNU getopt_long() proper argument names

 amidi/amidi.1 | 14 ++++++++++++++
 amidi/amidi.c | 61 +++++++++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 59 insertions(+), 16 deletions(-)

-- 
2.9.3

[PATCH v5 1/2] amidi: add delay option

[Felipe F. Tonello]

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 | 39 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/amidi/amidi.1 b/amidi/amidi.1
index 1b4cfb1ea956..391008177610 100644
--- a/amidi/amidi.1
+++ b/amidi/amidi.1
@@ -111,6 +111,12 @@ to stop receiving data.
 Does not ignore Active Sensing bytes (FEh) 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.
+
 .SH EXAMPLES
 
 .TP
@@ -121,6 +127,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 cedf18c5fd39..bdb04c50a50b 100644
--- a/amidi/amidi.c
+++ b/amidi/amidi.c
@@ -48,6 +48,7 @@ static int receive_file;
 static int dump;
 static int timeout;
 static int stop;
+static int delay;
 static snd_rawmidi_t *input, **inputp;
 static snd_rawmidi_t *output, **outputp;
 
@@ -77,7 +78,8 @@ static void usage(void)
 		"-d, --dump             print received data as hexadecimal bytes\n"
 		"-t, --timeout=seconds  exits when no data has been received\n"
 		"                       for the specified duration\n"
-		"-a, --active-sensing   don't ignore active sensing bytes\n");
+		"-a, --active-sensing   don't ignore active sensing bytes\n"
+		"-D, --delay=mseconds   delay in between each SysEx message\n");
 }
 
 static void version(void)
@@ -406,7 +408,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:a";
+	static const char short_options[] = "hVlLp:s:r:S::dt:aD:";
 	static const struct option long_options[] = {
 		{"help", 0, NULL, 'h'},
 		{"version", 0, NULL, 'V'},
@@ -419,6 +421,7 @@ int main(int argc, char *argv[])
 		{"dump", 0, NULL, 'd'},
 		{"timeout", 1, NULL, 't'},
 		{"active-sensing", 0, NULL, 'a'},
+		{"delay", 1, NULL, 'D'},
 		{ }
 	};
 	int c, err, ok = 0;
@@ -463,6 +466,9 @@ int main(int argc, char *argv[])
 		case 'a':
 			ignore_active_sensing = 0;
 			break;
+		case 'D':
+			delay = atoi(optarg);
+			break;
 		default:
 			error("Try `amidi --help' for more information.");
 			return 1;
@@ -538,9 +544,32 @@ 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;
+
+			while (data < (send_data + send_data_length)) {
+				int len = send_data + send_data_length - data;
+				char *temp;
+
+				if (data > send_data)
+					usleep(delay * 1000);
+
+				/* find end of SysEx */
+				if ((temp = memchr(data, 0xf7, len)))
+					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;
+			}
 		}
 	}
 
-- 
2.9.3

[PATCH v5 2/2] amidi: use GNU getopt_long() proper argument names

[Felipe F. Tonello]

This has no affect besides more clarity to the code.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 amidi/amidi.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/amidi/amidi.c b/amidi/amidi.c
index bdb04c50a50b..0be5e63119c0 100644
--- a/amidi/amidi.c
+++ b/amidi/amidi.c
@@ -410,18 +410,18 @@ int main(int argc, char *argv[])
 {
 	static const char short_options[] = "hVlLp:s:r:S::dt:aD:";
 	static const struct option long_options[] = {
-		{"help", 0, NULL, 'h'},
-		{"version", 0, NULL, 'V'},
-		{"list-devices", 0, NULL, 'l'},
-		{"list-rawmidis", 0, NULL, 'L'},
-		{"port", 1, NULL, 'p'},
-		{"send", 1, NULL, 's'},
-		{"receive", 1, NULL, 'r'},
-		{"send-hex", 2, NULL, 'S'},
-		{"dump", 0, NULL, 'd'},
-		{"timeout", 1, NULL, 't'},
-		{"active-sensing", 0, NULL, 'a'},
-		{"delay", 1, NULL, 'D'},
+		{"help", no_argument, NULL, 'h'},
+		{"version", no_argument, NULL, 'V'},
+		{"list-devices", no_argument, NULL, 'l'},
+		{"list-rawmidis", no_argument, NULL, 'L'},
+		{"port", required_argument, NULL, 'p'},
+		{"send", required_argument, NULL, 's'},
+		{"receive", required_argument, NULL, 'r'},
+		{"send-hex", optional_argument, NULL, 'S'},
+		{"dump", no_argument, NULL, 'd'},
+		{"timeout", required_argument, NULL, 't'},
+		{"active-sensing", no_argument, NULL, 'a'},
+		{"delay", required_argument, NULL, 'D'},
 		{ }
 	};
 	int c, err, ok = 0;
-- 
2.9.3

Re: [PATCH v5 0/2] amidi delay option

[Takashi Sakamoto]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

Hi,

On Aug 16 2016 01:37, Felipe F. Tonello wrote:
> Small improvement to amidi, adding useful functionality.
> 
> changes from v4:
>  * simplified code
> 
> changes from v3:
>  * fixed a bug if memchr() returns NULL
> 
> changes from v2:
>  * only check of end of sysex (0xf7)
>  * use memchr() instead of direct loop approach
>  * fixed commit message
> 
> changes from v1:
>  * remove delay in between any MIDI message, only SysEx
>  * added patch to use getopt_long() arguments
> 
> Felipe F. Tonello (2):
>   amidi: add delay option
>   amidi: use GNU getopt_long() proper argument names
> 
>  amidi/amidi.1 | 14 ++++++++++++++
>  amidi/amidi.c | 61 +++++++++++++++++++++++++++++++++++++++++++----------------
>  2 files changed, 59 insertions(+), 16 deletions(-)

Please notice the other developers' work. Clemens already posted his
patchset with similar improvements.

http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111737.html

And I reviewed and tested it.
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111745.html

It's not merged yet, because subsystem maintainer has a summer vacation.
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111741.html

It's better for you to wait for merging or work based on Clemens'
patchset, I think.


Regards

Takashi Sakamoto

[-- Attachment #2: 0xD66FD341.asc --]
[-- Type: application/pgp-keys, Size: 10205 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 0/2] amidi delay option

[Takashi Iwai]

On Mon, 15 Aug 2016 21:54:49 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Aug 16 2016 01:37, Felipe F. Tonello wrote:
> > Small improvement to amidi, adding useful functionality.
> > 
> > changes from v4:
> >  * simplified code
> > 
> > changes from v3:
> >  * fixed a bug if memchr() returns NULL
> > 
> > changes from v2:
> >  * only check of end of sysex (0xf7)
> >  * use memchr() instead of direct loop approach
> >  * fixed commit message
> > 
> > changes from v1:
> >  * remove delay in between any MIDI message, only SysEx
> >  * added patch to use getopt_long() arguments
> > 
> > Felipe F. Tonello (2):
> >   amidi: add delay option
> >   amidi: use GNU getopt_long() proper argument names
> > 
> >  amidi/amidi.1 | 14 ++++++++++++++
> >  amidi/amidi.c | 61 +++++++++++++++++++++++++++++++++++++++++++----------------
> >  2 files changed, 59 insertions(+), 16 deletions(-)
> 
> Please notice the other developers' work. Clemens already posted his
> patchset with similar improvements.
> 
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111737.html
> 
> And I reviewed and tested it.
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111745.html
> 
> It's not merged yet, because subsystem maintainer has a summer vacation.
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111741.html
> 
> It's better for you to wait for merging or work based on Clemens'
> patchset, I think.

Yep, now Clemens' patches were merged to git tree.  Please rebase and
resubmit if anything is missing.


thanks,

Takashi

Re: [PATCH v5 1/2] amidi: add delay option

[Clemens Ladisch]

Felipe F. Tonello wrote:
> +.I \-D, \-\-delay=mseconds
> +Adds a delay in between each SysEx message sent to a device.

> +			while (data < (send_data + send_data_length)) {
> +				int len = send_data + send_data_length - data;
> +				char *temp;
> +
> +				if (data > send_data)
> +					usleep(delay * 1000);

The MIDI data is sent asynchonously, so this delay is not _between_ messages.
You'd have to add 320 µs for any byte not yet sent (I guess you have to use
snd_rawmidi_status_get_avail() to find out how many have been sent).


> +				/* find end of SysEx */
> +				if ((temp = memchr(data, 0xf7, len)))

This would be clearer with "!= 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;
> +			}
>  		}
>  	}


Regards,
Clemens

Re: [PATCH v5 0/2] amidi delay option

[Felipe Ferreri Tonello]

[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]

Hi Takashi,

On 22/08/16 12:47, Takashi Iwai wrote:
> On Mon, 15 Aug 2016 21:54:49 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On Aug 16 2016 01:37, Felipe F. Tonello wrote:
>>> Small improvement to amidi, adding useful functionality.
>>>
>>> changes from v4:
>>>  * simplified code
>>>
>>> changes from v3:
>>>  * fixed a bug if memchr() returns NULL
>>>
>>> changes from v2:
>>>  * only check of end of sysex (0xf7)
>>>  * use memchr() instead of direct loop approach
>>>  * fixed commit message
>>>
>>> changes from v1:
>>>  * remove delay in between any MIDI message, only SysEx
>>>  * added patch to use getopt_long() arguments
>>>
>>> Felipe F. Tonello (2):
>>>   amidi: add delay option
>>>   amidi: use GNU getopt_long() proper argument names
>>>
>>>  amidi/amidi.1 | 14 ++++++++++++++
>>>  amidi/amidi.c | 61 +++++++++++++++++++++++++++++++++++++++++++----------------
>>>  2 files changed, 59 insertions(+), 16 deletions(-)
>>
>> Please notice the other developers' work. Clemens already posted his
>> patchset with similar improvements.
>>
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111737.html
>>
>> And I reviewed and tested it.
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111745.html
>>
>> It's not merged yet, because subsystem maintainer has a summer vacation.
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111741.html
>>
>> It's better for you to wait for merging or work based on Clemens'
>> patchset, I think.
> 
> Yep, now Clemens' patches were merged to git tree.  Please rebase and
> resubmit if anything is missing.

Ok.


-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 1/2] amidi: add delay option

[Felipe Ferreri Tonello]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

Hi Clemens,

On 22/08/16 13:43, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> +.I \-D, \-\-delay=mseconds
>> +Adds a delay in between each SysEx message sent to a device.
> 
>> +			while (data < (send_data + send_data_length)) {
>> +				int len = send_data + send_data_length - data;
>> +				char *temp;
>> +
>> +				if (data > send_data)
>> +					usleep(delay * 1000);
> 
> The MIDI data is sent asynchonously, so this delay is not _between_ messages.
> You'd have to add 320 µs for any byte not yet sent (I guess you have to use
> snd_rawmidi_status_get_avail() to find out how many have been sent).

Right. Or I can just use snd_rawmidi_drain()? Otherwise it will add too
much complexity.
> 
> 
>> +				/* find end of SysEx */
>> +				if ((temp = memchr(data, 0xf7, len)))
> 
> This would be clearer with "!= NULL".

Ok.

> 
>> +					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;
>> +			}
>>  		}
>>  	}
> 

Thanks

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 1/2] amidi: add delay option

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On 22/08/16 13:43, Clemens Ladisch wrote:
>> Felipe F. Tonello wrote:
>>> +				if (data > send_data)
>>> +					usleep(delay * 1000);
>>
>> The MIDI data is sent asynchonously, so this delay is not _between_ messages.
>> You'd have to add 320 µs for any byte not yet sent (I guess you have to use
>> snd_rawmidi_status_get_avail() to find out how many have been sent).
>
> Right. Or I can just use snd_rawmidi_drain()?

This function not only drains the ALSA buffer, but also waits for the
hardware FIFO to empty.  And most drivers do not implement this, in
which case the framework just does a fixed wait of 50 ms, which would
correspond to a FIFO size of 156 bytes.


Regards,
Clemens

Re: [PATCH v5 1/2] amidi: add delay option

[Felipe Ferreri Tonello]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

Hi Clemens,

On 23/08/16 11:37, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 22/08/16 13:43, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
>>>> +				if (data > send_data)
>>>> +					usleep(delay * 1000);
>>>
>>> The MIDI data is sent asynchonously, so this delay is not _between_ messages.
>>> You'd have to add 320 µs for any byte not yet sent (I guess you have to use
>>> snd_rawmidi_status_get_avail() to find out how many have been sent).
>>
>> Right. Or I can just use snd_rawmidi_drain()?
> 
> This function not only drains the ALSA buffer, but also waits for the
> hardware FIFO to empty.  And most drivers do not implement this, in
> which case the framework just does a fixed wait of 50 ms, which would
> correspond to a FIFO size of 156 bytes.
> 

Ok, so does that mean that it is not safe to use drain()? If that is the
case, than I will use snd_rawmidi_status_get_avail().

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7310 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 1/2] amidi: add delay option

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On 23/08/16 11:37, Clemens Ladisch wrote:
>> Felipe Ferreri Tonello wrote:
>>> On 22/08/16 13:43, Clemens Ladisch wrote:
>>>> Felipe F. Tonello wrote:
>>>>> +				if (data > send_data)
>>>>> +					usleep(delay * 1000);
>>>>
>>>> The MIDI data is sent asynchonously, so this delay is not _between_ messages.
>>>> You'd have to add 320 µs for any byte not yet sent (I guess you have to use
>>>> snd_rawmidi_status_get_avail() to find out how many have been sent).
>>>
>>> Right. Or I can just use snd_rawmidi_drain()?
>>
>> This function not only drains the ALSA buffer, but also waits for the
>> hardware FIFO to empty.  And most drivers do not implement this, in
>> which case the framework just does a fixed wait of 50 ms, which would
>> correspond to a FIFO size of 156 bytes.
>
> Ok, so does that mean that it is not safe to use drain()?

That additional 50 ms delay might be safe, but not what you want.


Regards,
Clemens

Re: [PATCH v5 1/2] amidi: add delay option

[Felipe Ferreri Tonello]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

Hi Clemens,

On 23/08/16 11:51, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 23/08/16 11:37, Clemens Ladisch wrote:
>>> Felipe Ferreri Tonello wrote:
>>>> On 22/08/16 13:43, Clemens Ladisch wrote:
>>>>> Felipe F. Tonello wrote:
>>>>>> +				if (data > send_data)
>>>>>> +					usleep(delay * 1000);
>>>>>
>>>>> The MIDI data is sent asynchonously, so this delay is not _between_ messages.
>>>>> You'd have to add 320 µs for any byte not yet sent (I guess you have to use
>>>>> snd_rawmidi_status_get_avail() to find out how many have been sent).
>>>>
>>>> Right. Or I can just use snd_rawmidi_drain()?
>>>
>>> This function not only drains the ALSA buffer, but also waits for the
>>> hardware FIFO to empty.  And most drivers do not implement this, in
>>> which case the framework just does a fixed wait of 50 ms, which would
>>> correspond to a FIFO size of 156 bytes.
>>
>> Ok, so does that mean that it is not safe to use drain()?
> 
> That additional 50 ms delay might be safe, but not what you want.

Ok.

BTW: Why 320 µs for any byte not yet sent?

Thanks,

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 1/2] amidi: add delay option

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> Why 320 µs for any byte not yet sent?

31250 baud; 10 bits per byte.


Regards,
Clemens

Re: [PATCH v5 1/2] amidi: add delay option

[Felipe Ferreri Tonello]

[-- Attachment #1: Type: text/plain, Size: 191 bytes --]

Hi Clemens,

On 23/08/16 12:46, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> Why 320 µs for any byte not yet sent?
> 
> 31250 baud; 10 bits per byte.

Right. Thanks!

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]