Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1348422 > unrolled thread

[PATCH 0/5] MIDI USB Gadget improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] MIDI USB Gadget improvements "Felipe F. Tonello" <eu@felipetonello.com> - 2016-03-02 20:40 +0100
    [PATCH 5/5] usb: gadget: f_midi: updated copyright "Felipe F. Tonello" <eu@felipetonello.com> - 2016-03-02 20:40 +0100
      Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright Felipe Balbi <balbi@kernel.org> - 2016-03-04 08:20 +0100
        Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-04 19:50 +0100
          Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright Felipe Balbi <balbi@kernel.org> - 2016-03-07 08:40 +0100
            Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-07 10:30 +0100
    [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes "Felipe F. Tonello" <eu@felipetonello.com> - 2016-03-02 20:40 +0100
      Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Felipe Balbi <balbi@kernel.org> - 2016-03-04 08:20 +0100
      Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Michal Nazarewicz <mina86@mina86.com> - 2016-03-04 20:20 +0100
        Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-04 21:20 +0100
          Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Michal Nazarewicz <mina86@mina86.com> - 2016-03-05 17:30 +0100
            Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Greg KH <greg@kroah.com> - 2016-03-05 20:40 +0100
              Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-06 01:00 +0100
                Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Greg KH <greg@kroah.com> - 2016-03-06 04:10 +0100
            Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-06 01:00 +0100
              Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Felipe Balbi <balbi@kernel.org> - 2016-03-07 08:40 +0100
                Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-07 10:40 +0100
                  Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Felipe Balbi <balbi@kernel.org> - 2016-03-08 08:50 +0100
    [PATCH 1/5] usb: gadget: f_midi: refactor state machine "Felipe F. Tonello" <eu@felipetonello.com> - 2016-03-02 20:50 +0100
      Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Clemens Ladisch <clemens@ladisch.de> - 2016-03-02 22:20 +0100
        Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-03 10:00 +0100
          Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Clemens Ladisch <clemens@ladisch.de> - 2016-03-03 12:40 +0100
            Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-03 17:30 +0100
              Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Clemens Ladisch <clemens@ladisch.de> - 2016-03-04 10:40 +0100
                Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-04 19:50 +0100
                  Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Clemens Ladisch <clemens@ladisch.de> - 2016-03-04 19:50 +0100
    Re: [PATCH 0/5] MIDI USB Gadget improvements Felipe Balbi <balbi@kernel.org> - 2016-03-04 08:20 +0100
      Re: [PATCH 0/5] MIDI USB Gadget improvements Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-04 19:50 +0100

Page 2 of 2 — ← Prev page 1 [2]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

Hi Clemens, 

On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch <clemens@ladisch.de> 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.

[toc] | [prev] | [next] | [standalone]

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

Felipe Ferreri Tonello wrote:
> On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch <clemens@ladisch.de> 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

[toc] | [prev] | [next] | [standalone]

#1349910

[Multipart message — attachments visible in raw view] — view raw

Hi,

"Felipe F. Tonello" <eu@felipetonello.com> writes:
> [ text/plain ]
> Patches are pretty much self-described.
>
> Patch 1 is revised from comments.

you really need to describe what you changed. This also should have v2
on subject line.

I guess it's too late to get this in v4.6 merge window as I'm already
applying the last few patches and plan to send a pull request in a few
minutes.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1350516

Hi Balbi, 

On March 4, 2016 7:11:30 AM GMT+00:00, Felipe Balbi <balbi@kernel.org> wrote:
>
>Hi,
>
>"Felipe F. Tonello" <eu@felipetonello.com> writes:
>> [ text/plain ]
>> Patches are pretty much self-described.
>>
>> Patch 1 is revised from comments.
>
>you really need to describe what you changed. This also should have v2
>on subject line.

Right. I didn't in this case because I sent this patch previously a while ago right before you changed employer. 

>
>I guess it's too late to get this in v4.6 merge window as I'm already
>applying the last few patches and plan to send a pull request in a few
>minutes.

That's fine I won't be able to rework the comments before Monday anyway. 

Thanks, 
Felipe 

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

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web