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

[Markus G. <gr...@ic...>]

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

> 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

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 :-)

> 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).

> 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.

> 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 :-)

	Kind regards,
		Markus