Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch  
> wrote:
>> Felipe Ferreri Tonello wrote:
>>> On 03/03/16 11:38, Clemens Ladisch wrote:
 But in what way was the old state machine not "proper"?
>>>
>>> Because it didn't reflect all the correct and possible MIDI states
>>
>> The whole point of the one-byte real-time messages is that they do not
>> affect the parsing of the surrounding MIDI stream.  So not making them
>> part of the state machine is the proper way of handling them.  (Also
>> see the flowchart in appendix A of the spec.)
>
> I really don't get your point. So why do we have a state machine at all?

To parse all the other messages.


Regards,
Clemens

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch  
> wrote:
>> Felipe Ferreri Tonello wrote:
>>> On 03/03/16 11:38, Clemens Ladisch wrote:
 But in what way was the old state machine not "proper"?
>>>
>>> Because it didn't reflect all the correct and possible MIDI states
>>
>> The whole point of the one-byte real-time messages is that they do not
>> affect the parsing of the surrounding MIDI stream.  So not making them
>> part of the state machine is the proper way of handling them.  (Also
>> see the flowchart in appendix A of the spec.)
>
> I really don't get your point. So why do we have a state machine at all?

To parse all the other messages.


Regards,
Clemens

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens, 

On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch  
wrote:
>Felipe Ferreri Tonello wrote:
>> On 03/03/16 11:38, Clemens Ladisch wrote:
>>> But in what way was the old state machine not "proper"?
>>
>> Because it didn't reflect all the correct and possible MIDI states
>
>The whole point of the one-byte real-time messages is that they do not
>affect the parsing of the surrounding MIDI stream.  So not making them
>part of the state machine is the proper way of handling them.  (Also
>see the flowchart in appendix A of the spec.)

I really don't get your point. So why do we have a state machine at all? 

>
>> This patch doesn't change any functionality. But the important thing
>> here is that it improves the driver maintainability [...]
>
>Then I won't get in the way of this driver's maintainer.


Clemens, I really value your feedback. I just want to understand what's the 
problem of this patch. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens, 

On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch  
wrote:
>Felipe Ferreri Tonello wrote:
>> On 03/03/16 11:38, Clemens Ladisch wrote:
>>> But in what way was the old state machine not "proper"?
>>
>> Because it didn't reflect all the correct and possible MIDI states
>
>The whole point of the one-byte real-time messages is that they do not
>affect the parsing of the surrounding MIDI stream.  So not making them
>part of the state machine is the proper way of handling them.  (Also
>see the flowchart in appendix A of the spec.)

I really don't get your point. So why do we have a state machine at all? 

>
>> This patch doesn't change any functionality. But the important thing
>> here is that it improves the driver maintainability [...]
>
>Then I won't get in the way of this driver's maintainer.


Clemens, I really value your feedback. I just want to understand what's the 
problem of this patch. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On 03/03/16 11:38, Clemens Ladisch wrote:
>> But in what way was the old state machine not "proper"?
>
> Because it didn't reflect all the correct and possible MIDI states

The whole point of the one-byte real-time messages is that they do not
affect the parsing of the surrounding MIDI stream.  So not making them
part of the state machine is the proper way of handling them.  (Also
see the flowchart in appendix A of the spec.)

> This patch doesn't change any functionality. But the important thing
> here is that it improves the driver maintainability [...]

Then I won't get in the way of this driver's maintainer.


Regards,
Clemens

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On 03/03/16 11:38, Clemens Ladisch wrote:
>> But in what way was the old state machine not "proper"?
>
> Because it didn't reflect all the correct and possible MIDI states

The whole point of the one-byte real-time messages is that they do not
affect the parsing of the surrounding MIDI stream.  So not making them
part of the state machine is the proper way of handling them.  (Also
see the flowchart in appendix A of the spec.)

> This patch doesn't change any functionality. But the important thing
> here is that it improves the driver maintainability [...]

Then I won't get in the way of this driver's maintainer.


Regards,
Clemens

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 03/03/16 11:38, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 02/03/16 21:09, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
 This refactor results in a cleaner state machine code
>>>
>>> It increases the number of states, and now juggles two state variables.
>>> I cannot agree to it being cleaner.
>>
>> Yes, it increases the number of states. That was done in order to
>> actually implement a proper finite state machine with one state at a
>> time and a transition state.
> 
> I know, "clean" is subjective.

Clean is subjective, yes. However, based on our common sense and
experience we can discern on what is clean and what is not. There is
also good literature about the subject that we can always consider.

> But in what way was the old state
> machine not "proper"?

Because it didn't reflect all the correct and possible MIDI states and
there was no transitional state.

> 
> And how is handling two states (port->state and next_state) cleaner?
> As far as I can tell, the requirement for a separate variable comes not
> from any inherent complexity of the state machine itself, but only
> because the transmit_packet function was inlined.

next_state is a transitional state, thus the temporal nature.

This patch doesn't change any functionality. But the important thing
here is that it improves the driver maintainability by making the state
machine cleaner (which is one of the most important pieces of code of
the driver). I call it clean because on each circumstance of each state
it's clear on what is about to happen to the USB request and to the
port's buffers.

I confess I would not spend the time on it just for puritanisms, but I
found myself a hard time while debugging it.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 03/03/16 11:38, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 02/03/16 21:09, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
 This refactor results in a cleaner state machine code
>>>
>>> It increases the number of states, and now juggles two state variables.
>>> I cannot agree to it being cleaner.
>>
>> Yes, it increases the number of states. That was done in order to
>> actually implement a proper finite state machine with one state at a
>> time and a transition state.
> 
> I know, "clean" is subjective.

Clean is subjective, yes. However, based on our common sense and
experience we can discern on what is clean and what is not. There is
also good literature about the subject that we can always consider.

> But in what way was the old state
> machine not "proper"?

Because it didn't reflect all the correct and possible MIDI states and
there was no transitional state.

> 
> And how is handling two states (port->state and next_state) cleaner?
> As far as I can tell, the requirement for a separate variable comes not
> from any inherent complexity of the state machine itself, but only
> because the transmit_packet function was inlined.

next_state is a transitional state, thus the temporal nature.

This patch doesn't change any functionality. But the important thing
here is that it improves the driver maintainability by making the state
machine cleaner (which is one of the most important pieces of code of
the driver). I call it clean because on each circumstance of each state
it's clear on what is about to happen to the USB request and to the
port's buffers.

I confess I would not spend the time on it just for puritanisms, but I
found myself a hard time while debugging it.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On 02/03/16 21:09, Clemens Ladisch wrote:
>> Felipe F. Tonello wrote:
>>> This refactor results in a cleaner state machine code
>>
>> It increases the number of states, and now juggles two state variables.
>> I cannot agree to it being cleaner.
>
> Yes, it increases the number of states. That was done in order to
> actually implement a proper finite state machine with one state at a
> time and a transition state.

I know, "clean" is subjective.  But in what way was the old state
machine not "proper"?

And how is handling two states (port->state and next_state) cleaner?
As far as I can tell, the requirement for a separate variable comes not
from any inherent complexity of the state machine itself, but only
because the transmit_packet function was inlined.


Regards,
Clemens

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On 02/03/16 21:09, Clemens Ladisch wrote:
>> Felipe F. Tonello wrote:
>>> This refactor results in a cleaner state machine code
>>
>> It increases the number of states, and now juggles two state variables.
>> I cannot agree to it being cleaner.
>
> Yes, it increases the number of states. That was done in order to
> actually implement a proper finite state machine with one state at a
> time and a transition state.

I know, "clean" is subjective.  But in what way was the old state
machine not "proper"?

And how is handling two states (port->state and next_state) cleaner?
As far as I can tell, the requirement for a separate variable comes not
from any inherent complexity of the state machine itself, but only
because the transmit_packet function was inlined.


Regards,
Clemens

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 02/03/16 21:09, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor results in a cleaner state machine code
> 
> It increases the number of states, and now juggles two state variables.
> I cannot agree to it being cleaner.

Yes, it increases the number of states. That was done in order to
actually implement a proper finite state machine with one state at a
time and a transition state. The result is a much cleaner MIDI parser
that is easy to maintain and read.

I recommend you to apply the patch yourself (it's on top of Balbi's next
branch) because the patch can be confusing to understand the end result.

> 
>> and as a result fixed a bug when packaging a USB-MIDI packet right after
>> a non-conformant MIDI byte stream.
> 
> I have been unable to determine where exactly the new code behaves
> differently.  Can you show an example?

Sorry, I forgot to remove this comment since your last revision. There
is no bug I could reproduce with the previous parser.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 02/03/16 21:09, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor results in a cleaner state machine code
> 
> It increases the number of states, and now juggles two state variables.
> I cannot agree to it being cleaner.

Yes, it increases the number of states. That was done in order to
actually implement a proper finite state machine with one state at a
time and a transition state. The result is a much cleaner MIDI parser
that is easy to maintain and read.

I recommend you to apply the patch yourself (it's on top of Balbi's next
branch) because the patch can be confusing to understand the end result.

> 
>> and as a result fixed a bug when packaging a USB-MIDI packet right after
>> a non-conformant MIDI byte stream.
> 
> I have been unable to determine where exactly the new code behaves
> differently.  Can you show an example?

Sorry, I forgot to remove this comment since your last revision. There
is no bug I could reproduce with the previous parser.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe F. Tonello wrote:
> This refactor results in a cleaner state machine code

It increases the number of states, and now juggles two state variables.
I cannot agree to it being cleaner.

> and as a result fixed a bug when packaging a USB-MIDI packet right after
> a non-conformant MIDI byte stream.

I have been unable to determine where exactly the new code behaves
differently.  Can you show an example?


Regards,
Clemens

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe F. Tonello wrote:
> This refactor results in a cleaner state machine code

It increases the number of states, and now juggles two state variables.
I cannot agree to it being cleaner.

> and as a result fixed a bug when packaging a USB-MIDI packet right after
> a non-conformant MIDI byte stream.

I have been unable to determine where exactly the new code behaves
differently.  Can you show an example?


Regards,
Clemens

[PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe F. Tonello]

This refactor results in a cleaner state machine code and as a result fixed a
bug when packaging a USB-MIDI packet right after a non-conformant MIDI byte 
stream.

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/gadget/function/f_midi.c | 204 ++-
 1 file changed, 129 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index 84c0ee5ebd1e..3cdb0741f3f8 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -50,6 +50,19 @@ static const char f_midi_longname[] = "MIDI Gadget";
  */
 #define MAX_PORTS 16
 
+/* MIDI message states */
+enum {
+   STATE_INITIAL = 0,  /* pseudo state */
+   STATE_1PARAM,
+   STATE_2PARAM_1,
+   STATE_2PARAM_2,
+   STATE_SYSEX_0,
+   STATE_SYSEX_1,
+   STATE_SYSEX_2,
+   STATE_REAL_TIME,
+   STATE_FINISHED, /* pseudo state */
+};
+
 /*
  * This is a gadget, and the IN/OUT naming is from the host's perspective.
  * USB -> OUT endpoint -> rawmidi
@@ -60,13 +73,6 @@ struct gmidi_in_port {
int active;
uint8_t cable;
uint8_t state;
-#define STATE_UNKNOWN  0
-#define STATE_1PARAM   1
-#define STATE_2PARAM_1 2
-#define STATE_2PARAM_2 3
-#define STATE_SYSEX_0  4
-#define STATE_SYSEX_1  5
-#define STATE_SYSEX_2  6
uint8_t data[2];
 };
 
@@ -400,118 +406,166 @@ static int f_midi_snd_free(struct snd_device *device)
return 0;
 }
 
-static void f_midi_transmit_packet(struct usb_request *req, uint8_t p0,
-   uint8_t p1, uint8_t p2, uint8_t p3)
-{
-   unsigned length = req->length;
-   u8 *buf = (u8 *)req->buf + length;
-
-   buf[0] = p0;
-   buf[1] = p1;
-   buf[2] = p2;
-   buf[3] = p3;
-   req->length = length + 4;
-}
-
 /*
  * Converts MIDI commands to USB MIDI packets.
  */
 static void f_midi_transmit_byte(struct usb_request *req,
 struct gmidi_in_port *port, uint8_t b)
 {
-   uint8_t p0 = port->cable << 4;
+   uint8_t p[4] = { port->cable << 4, 0, 0, 0 };
+   uint8_t next_state = STATE_INITIAL;
+
+   switch (b) {
+   case 0xf8 ... 0xff:
+   /* System Real-Time Messages */
+   p[0] |= 0x0f;
+   p[1] = b;
+   next_state = port->state;
+   port->state = STATE_REAL_TIME;
+   break;
+
+   case 0xf7:
+   /* End of SysEx */
+   switch (port->state) {
+   case STATE_SYSEX_0:
+   p[0] |= 0x05;
+   p[1] = 0xf7;
+   next_state = STATE_FINISHED;
+   break;
+   case STATE_SYSEX_1:
+   p[0] |= 0x06;
+   p[1] = port->data[0];
+   p[2] = 0xf7;
+   next_state = STATE_FINISHED;
+   break;
+   case STATE_SYSEX_2:
+   p[0] |= 0x07;
+   p[1] = port->data[0];
+   p[2] = port->data[1];
+   p[3] = 0xf7;
+   next_state = STATE_FINISHED;
+   break;
+   default:
+   /* Ignore byte */
+   next_state = port->state;
+   port->state = STATE_INITIAL;
+   }
+   break;
 
-   if (b >= 0xf8) {
-   f_midi_transmit_packet(req, p0 | 0x0f, b, 0, 0);
-   } else if (b >= 0xf0) {
+   case 0xf0 ... 0xf6:
+   /* System Common Messages */
+   port->data[0] = port->data[1] = 0;
+   port->state = STATE_INITIAL;
switch (b) {
case 0xf0:
port->data[0] = b;
-   port->state = STATE_SYSEX_1;
+   port->data[1] = 0;
+   next_state = STATE_SYSEX_1;
break;
case 0xf1:
case 0xf3:
port->data[0] = b;
-   port->state = STATE_1PARAM;
+   next_state = STATE_1PARAM;
break;
case 0xf2:
port->data[0] = b;
-   port->state = STATE_2PARAM_1;
+   next_state = STATE_2PARAM_1;
break;
case 0xf4:
case 0xf5:
-   port->state = STATE_UNKNOWN;
+   next_state = STATE_INITIAL;
break;
case 0xf6:
-   f_midi_transmit_packet(req, p0 | 0x05, 0xf6, 0, 0);
-   port->state = STATE_UNKNOWN;
-   break;
-   case 0xf7:
-   switch (port->state) {
-   case 

[PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe F. Tonello]

This refactor results in a cleaner state machine code and as a result fixed a
bug when packaging a USB-MIDI packet right after a non-conformant MIDI byte 
stream.

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/gadget/function/f_midi.c | 204 ++-
 1 file changed, 129 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index 84c0ee5ebd1e..3cdb0741f3f8 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -50,6 +50,19 @@ static const char f_midi_longname[] = "MIDI Gadget";
  */
 #define MAX_PORTS 16
 
+/* MIDI message states */
+enum {
+   STATE_INITIAL = 0,  /* pseudo state */
+   STATE_1PARAM,
+   STATE_2PARAM_1,
+   STATE_2PARAM_2,
+   STATE_SYSEX_0,
+   STATE_SYSEX_1,
+   STATE_SYSEX_2,
+   STATE_REAL_TIME,
+   STATE_FINISHED, /* pseudo state */
+};
+
 /*
  * This is a gadget, and the IN/OUT naming is from the host's perspective.
  * USB -> OUT endpoint -> rawmidi
@@ -60,13 +73,6 @@ struct gmidi_in_port {
int active;
uint8_t cable;
uint8_t state;
-#define STATE_UNKNOWN  0
-#define STATE_1PARAM   1
-#define STATE_2PARAM_1 2
-#define STATE_2PARAM_2 3
-#define STATE_SYSEX_0  4
-#define STATE_SYSEX_1  5
-#define STATE_SYSEX_2  6
uint8_t data[2];
 };
 
@@ -400,118 +406,166 @@ static int f_midi_snd_free(struct snd_device *device)
return 0;
 }
 
-static void f_midi_transmit_packet(struct usb_request *req, uint8_t p0,
-   uint8_t p1, uint8_t p2, uint8_t p3)
-{
-   unsigned length = req->length;
-   u8 *buf = (u8 *)req->buf + length;
-
-   buf[0] = p0;
-   buf[1] = p1;
-   buf[2] = p2;
-   buf[3] = p3;
-   req->length = length + 4;
-}
-
 /*
  * Converts MIDI commands to USB MIDI packets.
  */
 static void f_midi_transmit_byte(struct usb_request *req,
 struct gmidi_in_port *port, uint8_t b)
 {
-   uint8_t p0 = port->cable << 4;
+   uint8_t p[4] = { port->cable << 4, 0, 0, 0 };
+   uint8_t next_state = STATE_INITIAL;
+
+   switch (b) {
+   case 0xf8 ... 0xff:
+   /* System Real-Time Messages */
+   p[0] |= 0x0f;
+   p[1] = b;
+   next_state = port->state;
+   port->state = STATE_REAL_TIME;
+   break;
+
+   case 0xf7:
+   /* End of SysEx */
+   switch (port->state) {
+   case STATE_SYSEX_0:
+   p[0] |= 0x05;
+   p[1] = 0xf7;
+   next_state = STATE_FINISHED;
+   break;
+   case STATE_SYSEX_1:
+   p[0] |= 0x06;
+   p[1] = port->data[0];
+   p[2] = 0xf7;
+   next_state = STATE_FINISHED;
+   break;
+   case STATE_SYSEX_2:
+   p[0] |= 0x07;
+   p[1] = port->data[0];
+   p[2] = port->data[1];
+   p[3] = 0xf7;
+   next_state = STATE_FINISHED;
+   break;
+   default:
+   /* Ignore byte */
+   next_state = port->state;
+   port->state = STATE_INITIAL;
+   }
+   break;
 
-   if (b >= 0xf8) {
-   f_midi_transmit_packet(req, p0 | 0x0f, b, 0, 0);
-   } else if (b >= 0xf0) {
+   case 0xf0 ... 0xf6:
+   /* System Common Messages */
+   port->data[0] = port->data[1] = 0;
+   port->state = STATE_INITIAL;
switch (b) {
case 0xf0:
port->data[0] = b;
-   port->state = STATE_SYSEX_1;
+   port->data[1] = 0;
+   next_state = STATE_SYSEX_1;
break;
case 0xf1:
case 0xf3:
port->data[0] = b;
-   port->state = STATE_1PARAM;
+   next_state = STATE_1PARAM;
break;
case 0xf2:
port->data[0] = b;
-   port->state = STATE_2PARAM_1;
+   next_state = STATE_2PARAM_1;
break;
case 0xf4:
case 0xf5:
-   port->state = STATE_UNKNOWN;
+   next_state = STATE_INITIAL;
break;
case 0xf6:
-   f_midi_transmit_packet(req, p0 | 0x05, 0xf6, 0, 0);
-   port->state = STATE_UNKNOWN;
-   break;
-   case 0xf7:
-   switch (port->state) {
-   case STATE_SYSEX_0:
-