Re: [PATCH 2/2] Add MIDI stream support with data rate restriction

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Clemens,

Thank you to review my patches and I'm really sorry to include much 
mistakes, misleads and so on in my patches...

 >> It's maximum rate is 3,150 bytes per seconds
 >                       3,125

Yes. This is my mistake.

 > In practice, many MPU-401 compatible devices transmit with two stop
 > bits and thus limit the actual rate to about 2841 bps.
 > That the hardware MIDI ports on Fireworks devices can transmit at
 > the full 3125 bps allowed by the spec is an exception.

I have a little knowledgement about MIDI specification and 
implementation. Here I should refer just to MMA/AMEI RP-027 and 
shouldn't have menthioned MPU-401..

 > It should be possible to adjust the interval between consecutive MIDI
 > bytes so that it is 320 µs (= 3125 bps) on average; this would be
 > similar to how packets are allocated to frames in blocking mode.  This
 > would require no more than one byte of buffering in the device.
 >
 > However, this can be added later.

OK. From the first I planed to improve these codes after snd-fireworks 
discussion.

 > This comment might be misleading.  There is no such "negotiation
 > procedure"; RP27 mentions this only to allow for future extension.

Yes, it's misleading. It should be rewritten.

Here I can't find such a specification that define this "negotiation 
procedure". Do you know about it?

 > snd_rawmidi_receive() already reports errors; you can just ignore its
 > return value.

I understand you mean "ALSA core prints debug message if 
snd_rawmidi_receive() failed so you don't need to check return value."

 > The bytes in the b[] array are arranged in big-endian order.
 > Converting them to CPU order and then back feels unnecessary.

This is my bad mistake. I was preoccupied with the restriction and 
snd-fireworks...

 > "Extract" would imply that the removed value is then used
 > for something else.  Just name the functions "add"/"remove".

OK. I name them "amdtp_stream_midi_add()" and
"amdtp_stream_midi_remove()". And I'm sorry that I added wrong prototype 
declarations in a patch for amdtp.h.

 > Please note that this driver always uses "unsigned int" unless
 > negative values are actually needed.

 > i++ would be more idiomatic.

 > len can never be negative.

 > unsigned

 > i++

 > This comment's accuracy is capable of improvement.

All OK.

Regards


Takashi Sakamoto
o-takashi@sakamocchi.jp

(Jun 3 2013 17:20), Clemens Ladisch wrote:
> o-takashi@sakamocchi.jp wrote:
>> IEC 61883-6 defines MIDI comformant deta and MMA/AMEI RP-027 defines its
>> implementation. This patch add handling MIDI stream according to them.
>>
>> According to MMA/AMEI RP-027, one MIDI channel in AMDTP stream can handle
>> 8 MIDI streams. The index of MIDI stream in one MIDI comformant
>> data channel is defined as "mod(data_block_count, 8)".
>>
>> And one MIDI comformant data can send MIDI message up to 3 bytes. Every MIDI
>> comformant data includes "label" to indicate the number of bytes in its most
>> significant byte.
>>
>> Then theoretically one MIDI stream can transmit MIDI messages up to 72,000
>> bytes per second at 192.0kHz (= 192,000 / 8 * 3).
>>
>> But MIDI interface is based on MPU401-UART.
>
> Roland's MPU-401 was simply based on the MIDI specification.
>
>> It's maximum rate is 3,150 bytes per seconds
>
>                         3,125
>
> In practice, many MPU-401 compatible devices transmit with two stop
> bits and thus limit the actual rate to about 2841 bps.  That the
> hardware MIDI ports on Fireworks devices can transmit at the full
> 3125 bps allowed by the spec is an exception.
>
> I vaguely remember some MIDI spec saying that the maximum _sustained_
> data rate must be about 2500 bps.
>
>> Actually Echo's Fireworks cannot handle higer data rate. The devices can
>> overflow MIDI messages when receiving at higher data rate.
>
> Actually, higher data rates can be handled if the driver takes care to
> throttle when the device's internal FIFO is about to overflow.  My
> AudioFire2 has a 4 KB buffer, so that is what my old snd-fireworks
> driver assumes; I don't know how your AF Pre8 handles this.
>
>> This patch add an restriction of data rate up to 2,756 or 3,000 bytes per second
>> with simple logic. This restriction causes quite a small delay comparing to the
>> maximum data rate. But I hope to keep codes to be as simple as possible.
>
> It should be possible to adjust the interval between consecutive MIDI
> bytes so that it is 320 µs (= 3125 bps) on average; this would be
> similar to how packets are allocated to frames in blocking mode.  This
> would require no more than one byte of buffering in the device.
>
> However, this can be added later.
>
>>   int amdtp_stream_init(struct amdtp_stream *s, struct fw_unit *unit,
>>   		enum amdtp_stream_direction direction, enum cip_flags flags)
>>   {
>> +	int i;
>
> Please note that this driver always uses "unsigned int" unless negative
> values are actually needed.
>
>> +	for (i = 0; i < AMDTP_MAX_MIDI_STREAMS; i += 1)
>
> i++ would be more idiomatic.
>
>>   static void amdtp_fill_midi(struct amdtp_stream *s,
>>   			    __be32 *buffer, unsigned int frames)
>
>> +	 * This module can't support "negotiation procedure" in
>> +	 * MMA/AMEI RP-027.
>
> This comment might be misleading.  There is no such "negotiation
> procedure"; RP27 mentions this only to allow for future extension.
>
>> +			buffer[c] = (b[0] << 24) | (b[1] << 16) |
>> +							(b[2] << 8) | b[3];
>> +			buffer[c] = be32_to_cpu(buffer[c]);
>
> The bytes in the b[] array are arranged in big-endian order.  Converting
> them to CPU order and then back feels unnecessary.
>
> You could just let b point to the actual buffer:  u8 *p = &buffer[c];
>
>>   static void amdtp_pull_midi(struct amdtp_stream *s,
>>   			    __be32 *buffer, unsigned int frames)
>>   {
>> +	int len, ret;
>
> len can never be negative.
>
>> +			ret = snd_rawmidi_receive(s->midi[port], b + 1, len);
>> +			if (ret != len) {
>> +				dev_err(&s->unit->device,
>
> snd_rawmidi_receive() already reports errors; you can just ignore its
> return value.
>
>> + * amdtp_stream_midi_insert - add MIDI stream
>> + * amdtp_stream_midi_extract - remove MIDI stream
>
> "Extract" would imply that the removed value is then used for something
> else.  Just name the functions "add"/"remove".
>
>> +bool amdtp_stream_midi_running(struct amdtp_stream *s)
>> +{
>> +	int i;
>
> unsigned
>
>> +	for (i = 0; i < AMDTP_MAX_MIDI_STREAMS; i += 1) {
>
> i++
>
>> + * This module support maximum 8 MIDI streams
>> +#define AMDTP_MAX_MIDI_STREAMS 16
>
> This comment's accuracy is capable of improvement.
>
>
> Regards,
> Clemens


------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. A cloud service to automate IT design, transition and operations
2. Dashboards that offer high-level views of enterprise services
3. A single system of record for all IT processes
http://p.sf.net/sfu/servicenow-d2d-j