Hi Jonathan

On Thu, Oct 10, 2013 at 1:07 PM, Jonathan Woithe <jwoithe@just42.net> wrote:
Assuming these can be addressed (and none of them are particularly onerous)
I would be happy to commit this.  However, before doing so it would be good
if someone with a different DICE device could test to make completely
certain there are no side effects for them.  There shouldn't be, but it's
better to be safe than sorry.
 
No problem. I briefly tested it with my profire 2626, I will post later FW version. I am pretty confident,
since the changes should affect only this one type of the card, nevertheless all testing is good :)

> +Profire2626::Profire2626EAP::Profire2626EAP(Dice::Device & dev) : Dice::EAP(dev) {
> +}
> +

The usual FFADO code style would have the { on a line by itself.

The same comment applies for readApplicationReg() and writeApplicationReg().
 
I will fix the formatting and whitespace and post another patch.

> diff --git a/libffado/src/dice/maudio/profire_2626.h b/libffado/src/dice/maudio/profire_2626.h
> :
> @@ -46,12 +52,10 @@ public:
>
>      bool canChangeNickname() { return false; }
>
> -private:

Why as the private directive been removed?  Oh hang on, this is probably
because of additional details you inserted above it.
 
the Switch class needs to call readApplicationReg() and writeApplicationReg() of Profire2626EAP, so
I had to make the Profire2626EAP public. I could use friend declaration, I just don't like it (the friend keyword).
 
> @@ -63,7 +67,38 @@ private:
> :
> +       /**
> +        * @brief A standard-switch for boolean.
> +        *
> +        * If you don't like True and False for the labels, subclass and return your own.
> +        * \internal copy&paste from focusrite_eap.h
> +        */
> +        class Switch : public Control::Boolean
> +        {
> :

This seems fair, but I wonder whether a boolean control like this might
prove useful for other devices in time.  In other words, would it be best to
implement it in the generic dbus control layer (along with Element and so
on)?

This code was copied and simplified from Focusrite code (that's how I noticed those excess m_'s :) ). In the Focusrite case,
there's one more operation when setting value (set message or something).

The class is a Boolean control that read/writes its data from/to application space in DICE EAP config.

I expect that for remaining controls more code/classes will be copied. It may be beneficial to move the common code to Dice::EAP class.
My plan was to first make something that works, and then move code around if needed.
I'm aware that since I don't have the other devices, the operation is risky.

J.