Re: [PATCH 1/2 v3] emulator: add codec negotiation support

[Simon Fels <simon.fels@canonical.com>]

[-- Attachment #1: Type: text/plain, Size: 6658 bytes --]

On 21.10.2015 14:57, Denis Kenzior wrote:
> Hi Simon,
>
> On 10/21/2015 02:42 AM, Simon Fels wrote:
>> Hey Denis,
>>
>>> Please make sure not to go over 80 chars / line.  Even if you need to
>>> indent arguments 2-4 less than what is required by our style guidelines.
>>
>> vim shows me I am at 76 chars with having 4 spaces per tab. So how many
>> spaces do you take per tab to come over 80 chars? 8 spaces?
>>
>
> We use kernel coding style, so yes, 8 spaces.
>
>>>> +
>>>>   #ifdef __cplusplus
>>>>   }
>>>>   #endif
>>>> diff --git a/src/emulator.c b/src/emulator.c
>>>> index 626dec3..e16312e 100644
>>>> --- a/src/emulator.c
>>>> +++ b/src/emulator.c
>>>> @@ -27,6 +27,7 @@
>>>>   #include <string.h>
>>>>   #include <unistd.h>
>>>>   #include <stdbool.h>
>>>> +#include <errno.h>
>>>>
>>>>   #include <glib.h>
>>>>
>>>> @@ -39,6 +40,15 @@
>>>>
>>>>   #define RING_TIMEOUT 3
>>>>
>>>> +#define CVSD_OFFSET 0
>>>> +#define MSBC_OFFSET 1
>>>> +#define CODECS_COUNT (MSBC_OFFSET + 1)
>>>> +
>>>> +struct hfp_codec_info {
>>>> +    unsigned char type;
>>>> +    ofono_bool_t supported;
>>>> +};
>>>> +
>>>>   struct ofono_emulator {
>>>>       struct ofono_atom *atom;
>>>>       enum ofono_emulator_type type;
>>>> @@ -50,6 +60,13 @@ struct ofono_emulator {
>>>>       guint callsetup_source;
>>>>       int pns_id;
>>>>       struct ofono_handsfree_card *card;
>>>> +    struct hfp_codec_info r_codecs[CODECS_COUNT];
>>>> +    unsigned char negotiated_codec;
>>>> +    unsigned char proposed_codec;
>>>> +    guint delay_sco;
>>>
>>> Is this still needed?
>>>
>>>> +    ofono_emulator_codec_negotiation_cb codec_negotiation_cb;
>>>> +    void *codec_negotiation_data;
>>>> +    ofono_bool_t bac_received;
>>>>       bool slc : 1;
>>>>       unsigned int events_mode : 2;
>>>>       bool events_ind : 1;
>>>> @@ -938,6 +955,168 @@ fail:
>>>>       }
>>>>   }
>>>>
>>>> +static void bac_cb(GAtServer *server, GAtServerRequestType type,
>>>> +            GAtResult *result, gpointer user_data)
>>>> +{
>>>> +    struct ofono_emulator *em = user_data;
>>>> +    GAtResultIter iter;
>>>> +    int val;
>>>> +
>>>> +    DBG("");
>>>> +
>>>> +    switch (type) {
>>>> +    case G_AT_SERVER_REQUEST_TYPE_SET:
>>>> +        g_at_result_iter_init(&iter, result);
>>>> +        g_at_result_iter_next(&iter, "");
>>>> +
>>>> +        /*
>>>> +         * CVSD codec is mandatory and must come first.
>>>> +         * See HFP v1.6 4.34.1
>>>> +         */
>>>> +        if (g_at_result_iter_next_number(&iter, &val) == FALSE ||
>>>> +                val != HFP_CODEC_CVSD)
>>>> +            goto fail;
>>>> +
>>>> +        em->bac_received = TRUE;
>>>> +
>>>> +        em->negotiated_codec = 0;
>>>> +        em->r_codecs[CVSD_OFFSET].supported = TRUE;
>>>> +
>>>> +        while (g_at_result_iter_next_number(&iter, &val)) {
>>>> +            switch (val) {
>>>> +            case HFP_CODEC_MSBC:
>>>> +                em->r_codecs[MSBC_OFFSET].supported = TRUE;
>>>> +                break;
>>>> +            default:
>>>> +                DBG("Unsupported HFP codec %d", val);
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
>>>> +
>>>> +        /*
>>>> +         * If we're currently in the process of selecting a codec
>>>> +         * we have to restart that now
>>>> +         */
>>>> +        if (em->proposed_codec)
>>>> +            ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);
>>>> +
>>>> +        break;
>>>> +
>>>> +    default:
>>>> +fail:
>>>> +        DBG("Process AT+BAC failed");
>>>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
>>>
>>> In theory our codec-negotiation procedure might have failed, do we want
>>> to call the callback here as well?
>>>
>>>> +        break;
>>>> +    }
>>>> +}
>>>> +
>>>> +static void finish_codec_negotiation(struct ofono_emulator *em,
>>>> +            int err)
>>>> +{
>>>> +    if (em->codec_negotiation_cb == NULL)
>>>> +        return;
>>>> +
>>>> +    em->codec_negotiation_cb(err, em->codec_negotiation_data);
>>>> +
>>>> +    em->codec_negotiation_cb = NULL;
>>>> +    em->codec_negotiation_data = NULL;
>>>> +}
>>>> +
>>>> +static void connect_sco(struct ofono_emulator *em)
>>>> +{
>>>> +    int err;
>>>> +
>>>> +    DBG("");
>>>> +
>>>> +    em->delay_sco = 0;
>>>
>>> What does this do?
>>>
>>>> +
>>>> +    err = ofono_handsfree_card_connect_sco(em->card);
>>>
>>> ofono_handsfree_card_connect_sco function does not check for em->card
>>> being NULL.
>>>
>>>> +    if (err == 0) {
>>>> +        finish_codec_negotiation(em, 0);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* If we have another codec we can try then lets do that */
>>>> +    if (em->negotiated_codec != HFP_CODEC_CVSD) {
>>>> +        ofono_emulator_start_codec_negotiation(em, HFP_CODEC_CVSD,
>>>> +                            em->codec_negotiation_cb,
>>>> +                            em->codec_negotiation_data);
>>>
>>> Over 80 characters here again.  Indent less if you need to.
>>>
>>> ofono_handsfree_card_connect_sco won't really fail unless the kernel is
>>> not configured properly.  I'm unsure of the practical utility of this
>>> logic.  Would it be simpler to mark the wideband / negotiated codec as
>>> unsupported and simply error out here?
>>
>> This is the fallback to the mandatory codec. According to the spec this
>> one must be supported by both sides so if we have selected a different
>> one before we fallback to what must work but negotiate that first before
>> actually using it. Sure, if we end up here then something in our system
>> is wrong as when we say we support a codec we should be able to use it
>> without further problems. We can go and drop this if you think we don't
>> need it.
>
> The spec says:
> "If an (e)SCO link cannot be established according to the parameters
> required for the selected codec (e.g., basebands negotiation fails for a
> link with re-transmission although a wide band codec has been selected),
> the Codec Connection establishment procedure shall be re-started by the
> AG with the purpose of selecting a codec that can be used. The mandatory
> narrow band Codec (CVSD) shall be used before the AG gives up trying to
> establish a Codec connection."
>
> In practice, the only failure we can detect here is if setsockopt for
> BT_VOICE fails inside apply_codec_settings.  But I'm okay keeping this
> logic...

Ok.

regards,
Simon