Hi Jano, Hi Jonathan,

Le jeudi 10 octobre 2013 à 13:26 +0200, Jano Svitok a écrit :
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 :)


Of course I will test with patches applied, but possibly I won't be able to do this before a few days.


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

Yes, and I am wondering: had you a try changing the setting while streaming ? Of course, it is fundamentally dependent on the firmware and possibly the one for Profire polls regularly the register. For the Saffire, no changes are accounted for until the message set register is set at the appropriate value.


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.

Yes, there is now duplicated codes but this is matter of time and I agree with you: better to do something which works at first, then move to a more elegant implementation further.


J.

As an additional comment to the Jonathan remarks, the local variable name "volume" in Profire2626::Profire2626EAP::SettingsSection::SettingsSection() is indeed not very explicit: isn't it a switch to connect different outputs to the physical control (and not a value for the volume or anything like), which makes the setting very different to the ones for other devices (until now, essentially Saffire) ?

Regards,

Phil



--
Philippe Carriere <la-page-web-of-phil.contact@orange.fr>