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,125In 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