Re: [Line6linux-devel] Status of line6 staging cleanup

[Stefan H. <ste...@gm...>]

On Thu, Nov 15, 2012 at 11:38 PM, Markus Grabner <gr...@ic...> wrote:
> Am Donnerstag, 15. November 2012, 08:16:25 schrieb Stefan Hajnoczi:
>> On Thu, Nov 15, 2012 at 1:11 AM, Markus Grabner <gr...@ic...>
> wrote:
>> > On Wednesday 14 November 2012 08:45:24 Stefan Hajnoczi wrote:
>> >> The sysfs attributes involve MIDI commands and some state.  If we
>> >> leave this to userspace then the kernel driver can focus on PCM and
>> >> MIDI I/O.  The code will become much smaller and simpler because we
>> >> can drop all the peeking into MIDI buffers and the housekeeping that
>> >> goes along with that.
>> >>
>> >> Letting userspace handle MIDI means that line6linux development
>> >> becomes accessible to a wider group of developers - people not
>> >> comfortable with C or driver hacking.  It encourages people to explore
>> >> their devices and contribute code.
>> >
>> > I agree with all of that. Let me point to a document which I started to
>> > write a year ago when we first briefly discussed this:
>> >
>> > https://line6linux.svn.sourceforge.net/svnroot/line6linux/apps/branches/qt
>> > based/doc/design.tex
>> >
>> > The sysfs attributes are ugly, but very convenient for development and
>> > testing. I don't want to remove them without having a similarly convenient
>> > replacement, that's why I started to write some code according to the
>> > ideas
>> > discussed in the abovementioned document. Feel free to comment on this!
>> >
>> > One particular question is how to best control the driver's behaviour
>> > (e.g., the midi masks) from user mode. I can imagine at least two
>> > methods: *) using ioctl() calls
>> > *) defining a virtual MIDI device for communication with the driver
>> >
>> > Are there more options, and what would be considered best practise in this
>> > case?
>>
>> Either ioctl() or sysfs is okay.  ioctl() is very easy for C/C++
>> applications to use if they already have a file descriptor.  For
>> human-readable data sysfs is nicer since it can be accessed easily
>> from a shell session.  The challenge with sysfs is locating the device
>> directory, but this could be done given the ALSA device numbers.
> As far as I remember, it was Linus himself who recommended the special files
> in the Line6 driver to be writable only by root :-), so this would be too
> restrictive for most use cases. If it is accepted practise to define ioctl()
> calls for this purpose, this is probably the best solution once we have user-
> space tools to access Line6 devices. This would replace a command like
>
> echo 4 > /sys/bus/usb/drivers/line6usb/7-2:1.0/midi_mask_transmit
>
> by something like
>
> /usr/bin/line6 -d PODxtPro set midi_mask_transmit 4

I'm not sure what the best practice is, I guess we'll see when
submitting the patches to move the driver out of staging.

The only problem I see with sysfs is that it's not clear to me how to
control file ownership.  Typically we need the 'audio' group to own
the attrs so regular users can read/write them.  sysfs supports
chmod(2)/chown(2) but I don't know whether udev(7) rules can apply
ownership on sysfs attributes.

>> The design document you posted looks sane and well thought out.  I
>> think the main "meat" of the library will be the data tables for each
>> supported device.  Ideally the library code would not hard-code names
>> (e.g. amp1_gain, wah_position) but instead use self-describing data
>> tables.  The advantage here is that developers can easily add new
>> tweak parameters by modifying the data tables.  The only piece of
>> software that needs to understand the parameter names is the GUI, and
>> even there we could try to use a data-driven approach.
>>
>> In other words, the library doesn't need enums for all possible
>> parameters.  Instead you could say line6_set_param(dev,
>> "master_volume", 0x40), which looks up "master_volume" in the device's
>> data table.  That produces a MIDI message (PC, CC, or SysEx).
> I actually prefer symbols over strings for this purpose for several reasons:
> *) a typo in a string doesn't get noticed at compile time
> *) symbols can be used in switch/case statements
> *) comparing an integer variable with a symbolic constant gives cleaner code
> than a string comparison (using the "==" operator instead of calling "strcmp")
> *) an integer is lightweight compared to a string, both in terms of memory
> consumption and performance

Since you have code working today let's go with the enum approach.

>
> The CSV table approach discussed in Section 4.4 of the design document comes
> close to the self-describing data tables you mention above: all relevant
> relations (e.g., the mapping between USB product ids and device names, or
> between device parameters and corresponding MIDI byte codes) are defined and
> edited in CSV files, which are automatically converted to corresponding C++
> code (enums, tables, maps, etc.) during the build process. The application can
> then use these symbols as desired, and consistently adding a new parameter to
> all relevant enum lists, tables, maps, etc., is as easy as adding a single row
> to a CSV table. The created files should contain a prominent notice that they
> were created automatically, with a reference to the input file (this is not
> yet implemented).
>
> The perl script that does all this magic (parse_data.pl) is actually the most
> complex piece of software in the user space code repository at this time :-)

Cool :).  I should try adding POD HD300 support.

>> When the device sends MIDI messages to the host in response to the wah
>> pedal, for example, the library uses the data table to translate from
>> the MIDI message to event_param_changed(dev, "wah_position", 0x7f).
> That's basically how it is (partially) implemented, except that a symbolic
> constant is used instead of a string, and that the parameter id and the value
> are collected in an "Action" class. This is yet missing from the design
> document, the basic idea is to have a (small) set of action types (such as
> "set a continuous parameter value", "set a boolean parameter value", etc.).
> These are device-independent (though certainly not all devices support all
> parameters), and it's the library's responsibility to translate between these
> and the corrsponding device-dependent MIDI byte codes when transmitting or
> receiving data.
>
>> The design document doesn't cover whether to make the library stateful
>> or not.  A stateless library simply transmits and receives events on
>> behalf of the application.  A stateful library keeps a "current value"
>> stored for every parameter in the data table so that the application
>> can retrieve it quickly.  My feeling is that a stateless library is a
>> good thing but there is a risk that applications will duplicate code,
>> so we'd have to watch out if we find ourselves reimplementing the same
>> state code in multiple applications.
> As far as I know it is not possible to query a single parameter value from the
> device (e.g., the current volume), and even if it were, such a query would
> require a host-device-host roundtrip and suffer from a (small) delay. On the
> other hand, whenever changing to a different tone on a POD device, the entire
> settings for the new tone are transmitted to the host (~140 bytes). I
> therefore think that maintaining the current state is useful here (that's
> actually how the driver currently responds to read requests on special files
> representing device parameters, which would also become obsolete in the driver
> when done by a user space library).

In a Qt GUI the widgets already keep state - you can ask a slider for
its current value.  So the application doesn't need the library to
duplicate that state.

BTW I have begun removing the MIDI sysfs attributes from pod.c.  I'll
send patches next week.

>> A small detail that came to mind: transmitting MIDI should be
>> asynchronous just like receiving MIDI is.  The reason for this is that
>> sending large commands (e.g. restoring saved presets) should not block
>> the GUI.
> After the headaches we had with a "true" asynchronous approach in our first
> attempt, I think it's less error-prone to use the select() mechanism provided
> by most UI libraries. In Qt, you can request notification when data are ready
> for reading (or writing data has completed), by connecting signals from the Qt
> device classes with corresponding slots in your application code. It is
> probably sufficient to use non-blocking I/O and react on the completion
> notification (e.g., by displaying a "process completed" dialog). If a progress
> bar should be displayed, a large message should be split into smaller pieces
> (e.g., one per preset) to allow for proper upgrade of the progress bar, but
> the Qt main loop frequently get the opportunity to process user interface
> events such that the GUI remains responsive. I'm not familiar with GTK, but I
> think it's similar there.

I think we're talking about the same thing here.  I mean that the ALSA
MIDI write call should be non-blocking so that we don't block the main
loop.

>
>> Is there code to implement this design?
> Yes, it's under
>
> https://line6linux.svn.sourceforge.net/svnroot/line6linux/apps/branches/qtbased
>
> The name "qtbased" is actually a misnomer, but the idea to clearly separate
> the code related to a particular user interface library (Section 3.3.2 of the
> design document) only gradually evolved while working on this branch, and I
> simply didn't want to rename it again.
>
> Be aware that the code is very experimental, poorly documented, and quite a
> mess. Nevertheless, the line6shell at least displays clear text descriptions
> of events received from the device by making use of the auto-generated tables,
> i.e., without any hard-coded parameter name in the code. Have fun exploring
> the code :-)

Thanks!

Stefan