[alsa-devel] [PATCH 0/3] fix sending the same MIDI bytes in two transactions
Hi,
In former commits, MIDI functionality is added to TASCAM driver, while it includes a bug to send the same MIDI bytes in two continuous transactions.
This patchset is to fix this bug, including some code improvements.
Takashi Sakamoto (3): ALSA: tascam: remove buffer initialization in driver side ALSA: tascam: improve loop condition to filling MIDI bytes in transaction ALSA: tascam: clear extra MIDI byte of asynchronous transaction
sound/firewire/tascam/tascam-transaction.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
The given buffer to callback function is cleared in caller side.
This commit removes buffer initialization in callee side.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/tascam/tascam-transaction.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 1c9a88b..7fcee81 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -67,8 +67,6 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) u8 status; int consume;
- buf[0] = buf[1] = buf[2] = buf[3] = 0x00; - len = snd_rawmidi_transmit_peek(substream, buf + 1, 3); if (len == 0) return 0;
It includes meaningless condition. This commit improve it.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/tascam/tascam-transaction.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 7fcee81..4ffaa8a 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) /* On exclusive message. */ if (tscm->on_sysex[port]) { /* Seek the end of exclusives. */ - for (i = 1; i < 4 || i < len; ++i) { + for (i = 1; i < len + 1; ++i) { if (buf[i] == 0xf7) { tscm->on_sysex[port] = false; break;
On Sun, 18 Oct 2015 12:02:17 +0200, Takashi Sakamoto wrote:
It includes meaningless condition. This commit improve it.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/firewire/tascam/tascam-transaction.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 7fcee81..4ffaa8a 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) /* On exclusive message. */ if (tscm->on_sysex[port]) { /* Seek the end of exclusives. */
for (i = 1; i < 4 || i < len; ++i) {
for (i = 1; i < len + 1; ++i) {
This doesn't look like an improvement... The fix should be rather like: for (i = 1; i < 4 && i < len; ++i) {
Takashi
On Oct 18 2015 19:10, Takashi Iwai wrote:
On Sun, 18 Oct 2015 12:02:17 +0200, Takashi Sakamoto wrote:
It includes meaningless condition. This commit improve it.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/firewire/tascam/tascam-transaction.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 7fcee81..4ffaa8a 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) /* On exclusive message. */ if (tscm->on_sysex[port]) { /* Seek the end of exclusives. */
for (i = 1; i < 4 || i < len; ++i) {
for (i = 1; i < len + 1; ++i) {This doesn't look like an improvement... The fix should be rather like: for (i = 1; i < 4 && i < len; ++i) {
The 'len' variable is assigned to the return value of snd_rawmidi_transmit_peek(). The third argument to call this function is 3, thus the value of this variable equals to or is less than 3.
The aim of this loop is to find EOX (end of exclusive) byte in retrieved bytes, then break the loop.The index goes from 1 to the end of retrieved bytes. Thus, 'i < 4 || i < len' is not so proper, here.
Thanks
Takashi Sakamoto
On Sun, 18 Oct 2015 12:51:33 +0200, Takashi Sakamoto wrote:
On Oct 18 2015 19:10, Takashi Iwai wrote:
On Sun, 18 Oct 2015 12:02:17 +0200, Takashi Sakamoto wrote:
It includes meaningless condition. This commit improve it.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/firewire/tascam/tascam-transaction.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 7fcee81..4ffaa8a 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) /* On exclusive message. */ if (tscm->on_sysex[port]) { /* Seek the end of exclusives. */
for (i = 1; i < 4 || i < len; ++i) {
for (i = 1; i < len + 1; ++i) {This doesn't look like an improvement... The fix should be rather like: for (i = 1; i < 4 && i < len; ++i) {
The 'len' variable is assigned to the return value of snd_rawmidi_transmit_peek(). The third argument to call this function is 3, thus the value of this variable equals to or is less than 3.
The aim of this loop is to find EOX (end of exclusive) byte in retrieved bytes, then break the loop.The index goes from 1 to the end of retrieved bytes. Thus, 'i < 4 || i < len' is not so proper, here.
Then the patch description is incorrect. You actually fix something that was a buggy behavior.
Overall, the code is confusing because the real data buffer begins at offset 1 and len doesn't mean the size of the values written to buf[]. Maybe it's better to be like: char *msg = buf + 1;
and use msg[x] instead of buf[1+x]. Then len really corresponds to the size of msg[].
Takashi
On Oct 18 2015 21:00, Takashi Iwai wrote:
On Sun, 18 Oct 2015 12:51:33 +0200, Takashi Sakamoto wrote:
On Oct 18 2015 19:10, Takashi Iwai wrote:
On Sun, 18 Oct 2015 12:02:17 +0200, Takashi Sakamoto wrote:
It includes meaningless condition. This commit improve it.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/firewire/tascam/tascam-transaction.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 7fcee81..4ffaa8a 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) /* On exclusive message. */ if (tscm->on_sysex[port]) { /* Seek the end of exclusives. */
for (i = 1; i < 4 || i < len; ++i) {
for (i = 1; i < len + 1; ++i) {This doesn't look like an improvement... The fix should be rather like: for (i = 1; i < 4 && i < len; ++i) {
The 'len' variable is assigned to the return value of snd_rawmidi_transmit_peek(). The third argument to call this function is 3, thus the value of this variable equals to or is less than 3.
The aim of this loop is to find EOX (end of exclusive) byte in retrieved bytes, then break the loop.The index goes from 1 to the end of retrieved bytes. Thus, 'i < 4 || i < len' is not so proper, here.
Then the patch description is incorrect. You actually fix something that was a buggy behavior.
It's not so buggy behaviour because the loop continue to seek the buffer till its end (4th byte), always. But this is against my intention to stop the seek in the end of retrieved byte.
Anyway, the code is not good.
Overall, the code is confusing because the real data buffer begins at offset 1 and len doesn't mean the size of the values written to buf[]. Maybe it's better to be like: char *msg = buf + 1;
and use msg[x] instead of buf[1+x]. Then len really corresponds to the size of msg[].
That's a good idea. OK, I'll post the revised version, later.
Thanks
Takashi Sakamoto
When MIDI buffer stores two or more MIDI messages, TASCAM driver transfers asynchronous transactions including one MIDI message and extra bytes in second MIDI message.
This commit fixes this bug by clear needless bytes in the buffer.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/tascam/tascam-transaction.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 4ffaa8a..4bedfe4 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -130,6 +130,9 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) buf[0] = (port << 4) | (buf[1] >> 4); }
+ if (len < 3) + memset(buf + 1, 0, 3 - len); + return len; }
participants (2)
-
Takashi Iwai -
Takashi Sakamoto