LinuxLists.cc - [PATCH 1/2] midi: Fix SysEx parser in MIDI plugin

2020-06-19 06:03:29

Subject: [PATCH 1/2] midi: Fix SysEx parser in MIDI plugin

From: Tedd Ho-Jeong An <[email protected]>

The SysEx end of message parser didn't consider the fact that
timestampsLow are in the stream and it might have the EOX (F7)
byte as well.

Fix that by always assuming the first system message byte is
the timestampLow.

Future work would involve support other type of system message
bytes, such as real-time.
---
profiles/midi/libmidi.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/profiles/midi/libmidi.c b/profiles/midi/libmidi.c
index 4b4df799f..cc5f079e7 100644
--- a/profiles/midi/libmidi.c
+++ b/profiles/midi/libmidi.c
@@ -331,6 +331,24 @@ static size_t handle_end_of_sysex(struct midi_read_parser *parser,
return sysex_length + 1; /* +1 because of timestampLow */
}

+/* Searches the end of a SysEx message that contains a timestampLow
+ * before the SysEx end byte. Returns the number of bytes of valid
+ * SysEx payload in the buffer.
+ */
+static size_t sysex_eox_len(const uint8_t *data, size_t size)
+{
+ size_t i = 0;
+
+ MIDI_ASSERT(size > 0);
+
+ if (data[i] == 0xF0)
+ i++;
+
+ /* search for timestamp low */
+ while (i < size && !(data[i++] & 0x80));
+
+ return (i < size && data[i] == 0xF7) ? i : 0;
+}

size_t midi_read_raw(struct midi_read_parser *parser, const uint8_t *data,
@@ -368,14 +386,13 @@ size_t midi_read_raw(struct midi_read_parser *parser, const uint8_t *data,

/* System Common Messages */
case 0xF0: /* SysEx Start */ {
- uint8_t *pos;
+ size_t sysex_length;

/* cleanup Running Status Message */
parser->rstatus = 0;

- /* Avoid parsing if SysEx is contained in one BLE packet */
- if ((pos = memchr(data + i, 0xF7, size - i))) {
- const size_t sysex_length = pos - (data + i);
+ /* Search for End of SysEx message in one BLE message */
+ if ((sysex_length = sysex_eox_len(data + i, size - i)) > 0) {
midi_size = handle_end_of_sysex(parser, ev, data + i,
sysex_length);
} else {
@@ -424,10 +441,9 @@ size_t midi_read_raw(struct midi_read_parser *parser, const uint8_t *data,

/* Check for SysEx messages */
if (parser->sysex_stream.len > 0) {
- uint8_t *pos;
+ size_t sysex_length;

- if ((pos = memchr(data + i, 0xF7, size - i))) {
- const size_t sysex_length = pos - (data + i);
+ if ((sysex_length = sysex_eox_len(data + i, size - i)) > 0) {
midi_size = handle_end_of_sysex(parser, ev, data + i,
sysex_length);
} else {
--
2.25.4

2020-06-19 06:06:26

by Tedd Ho-Jeong An

[permalink] [raw]

Subject: [PATCH 2/2] midi: Fix random empty timestamp_high value

From: Tedd Ho-Jeong An <[email protected]>

The timestamp_high value is assigned from the monotonic time but there
is a chance that the value becomes zero becuase it reads the value
between bit8 and bit13.

This patch makes sure the timestamp_high value get a non-zero value.
---
profiles/midi/libmidi.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/profiles/midi/libmidi.c b/profiles/midi/libmidi.c
index cc5f079e7..fd40b038b 100644
--- a/profiles/midi/libmidi.c
+++ b/profiles/midi/libmidi.c
@@ -77,8 +77,13 @@ inline static void append_timestamp_high_maybe(struct midi_write_parser *parser)
if (midi_write_has_data(parser))
return;

- parser->rtime = g_get_monotonic_time() / 1000; /* convert µs to ms */
- timestamp_high |= (parser->rtime & 0x1F80) >> 7;
+ /* Make sure timesampt_high is assigned a non-zero value */
+ do {
+ /* convert µs to ms */
+ parser->rtime = g_get_monotonic_time() / 1000;
+ timestamp_high |= (parser->rtime & 0x1F80) >> 7;
+ } while (timestamp_high == 0x80);
+
/* set timestampHigh */
buffer_append_byte(&parser->midi_stream, timestamp_high);
}
--
2.25.4

2020-06-19 06:23:30

by bluez.test.bot

[permalink] [raw]

Subject: RE: [2/2] midi: Fix random empty timestamp_high value

2020-06-19 06:23:30

by bluez.test.bot

[permalink] [raw]

Subject: RE: [1/2] midi: Fix SysEx parser in MIDI plugin

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
checkpatch Failed

Outputs:
ERROR:TRAILING_STATEMENTS: trailing statements should be on next line
#38: FILE: profiles/midi/libmidi.c:348:
+ while (i < size && !(data[i++] & 0x80));

ERROR:ASSIGN_IN_IF: do not use assignment in if condition
#59: FILE: profiles/midi/libmidi.c:395:
+ if ((sysex_length = sysex_eox_len(data + i, size - i)) > 0) {

WARNING:LONG_LINE: line over 80 characters
#72: FILE: profiles/midi/libmidi.c:446:
+ if ((sysex_length = sysex_eox_len(data + i, size - i)) > 0) {

ERROR:ASSIGN_IN_IF: do not use assignment in if condition
#72: FILE: profiles/midi/libmidi.c:446:
+ if ((sysex_length = sysex_eox_len(data + i, size - i)) > 0) {

- total: 3 errors, 1 warnings, 53 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

---
Regards,
Linux Bluetooth