[pulseaudio-discuss] [PATCH next v0 05/11] bluetooth: Refactor parse_audio_property() to support more properties

[Mikel Astiz]

Hi Tanu,

On Sat, Dec 8, 2012 at 2:40 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Thu, 2012-12-06 at 15:55 +0100, Mikel Astiz wrote:
>> @@ -475,8 +475,20 @@ static int parse_audio_property(pa_bluetooth_discovery *u, int *state, DBusMessa
>>              dbus_message_iter_get_basic(&variant_i, &value);
>>
>>              if (pa_streq(key, "State")) {
>> -                *state = pa_bt_audio_state_from_string(value);
>> +                pa_bt_audio_state_t state = pa_bt_audio_state_from_string(value);
>> +
>>                  pa_log_debug("dbus: property 'State' changed to value '%s'", value);
>> +
>> +                if (state == PA_BT_AUDIO_STATE_INVALID)
>> +                    return -1;
>> +
>> +                /* A hackish p == PROFILE_OFF is used to represent org.bluez.Audio */
>> +                if (p == PROFILE_OFF) {
>> +                    d->audio_state = state;
>> +                    break;
>> +                }
>
> I don't like this hack. See below for a suggestion for an alternative
> solution.
>
>> +
>> +                d->profile_state[p] = state;
>>              }
>>
>>              break;
>> @@ -609,16 +621,13 @@ static void get_properties_reply(DBusPendingCall *pending, void *userdata) {
>>                      goto finish;
>>
>>              } else if (dbus_message_has_interface(p->message, "org.bluez.Audio")) {
>> -                if (parse_audio_property(y, &d->audio_state, &dict_i) < 0)
>> +                if (parse_audio_property(d, PROFILE_OFF, &dict_i) < 0)
>>                      goto finish;
>>
>>              } else if (pa_bt_audio_profile_from_interface(dbus_message_get_interface(p->message), &profile)) {
>> -                pa_bt_audio_state_t state;
>> -
>> -                if (parse_audio_property(y, &state, &dict_i) < 0)
>> +                if (parse_audio_property(d, profile, &dict_i) < 0)
>>                      goto finish;
>>
>> -                d->profile_state[profile] = state;
>>              }
>>          }
>
> We could have an enum for all BlueZ interfaces (or just for the audio
> interfaces, but I think it's cleaner to cover all interfaces).

I'd rather avoid enumerating org.bluez.Audio in this enum type, since
it will probably disappear in the future. I'd expect that this enum
type evolves into representing Bluetooth profiles/roles, where
org.bluez.Audio would not fit.

I think trying to list D-Bus interfaces is not a very good idea since
the list will change significantly across different versions of BlueZ
(obviously I' referring to BlueZ 5 in this case). Instead, the API in
bluetooth-util should provide a nice abstraction that the modules can
benefit from, but without necessarily mapping 1:1 to BlueZ's D-Bus
API.

In a similar way, I'd like to propose that PROFILE_OFF (and some other
future card profiles like PROFILE_AUTO) would be in a different enum,
therefore having two different types to enumerate (a) bluetooth
profiles/roles and (b) card profiles. I don't have a proposal for this
yet but I'd rather avoid representing org.bluez.Audio in the list.

> get_properties_reply() could then convert the interface string to an
> enum value, and pass that value instead of a profile to
> parse_audio_property().
>
> Alternatively, the interface enum could be avoided by passing the
> interface string to parse_audio_property(), but that would result in
> more string comparisons.

I'll send v1 following this proposal, passing the string to
parse_audio_property() and taking care of org.bluez.Audio there. This
is definitely better than what I proposed in v0.

Cheers,
Mikel