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

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

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

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?

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

>> +        return;
>> +    }
>> +
>> +    finish_codec_negotiation(em, -EIO);
>> +}
>> +
>> +static void bcs_cb(GAtServer *server, GAtServerRequestType type,
>> +            GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_emulator *em = user_data;
>> +    GAtResultIter iter;
>> +    int val;
>> +
>> +    switch (type) {
>> +    case G_AT_SERVER_REQUEST_TYPE_SET:
>> +        g_at_result_iter_init(&iter, result);
>> +        g_at_result_iter_next(&iter, "");
>> +
>> +        if (!g_at_result_iter_next_number(&iter, &val))
>> +            break;
>> +
>> +        if (em->proposed_codec != val) {
>> +            em->proposed_codec = 0;
>> +            break;
>> +        }
>> +
>> +        em->proposed_codec = 0;
>> +        em->negotiated_codec = val;
>> +
>> +        DBG("negotiated codec %d", val);
>> +
>> +        if (em->card != NULL)
>> +            ofono_handsfree_card_set_codec(em->card,
>> +                            em->negotiated_codec);
>> +
>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
>> +
>> +        connect_sco(em);
>> +
>> +        return;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    finish_codec_negotiation(em, -EIO);
>> +
>> +    g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
>> +}
>> +
>> +static void bcc_cb(GAtServer *server, GAtServerRequestType type,
>> +            GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_emulator *em = user_data;
>> +
>> +    switch (type) {
>> +    case G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY:
>> +
>
> No need for an empty line here.
>
>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
>> +
>> +        if (!em->negotiated_codec) {
>> +            ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);
>
> Over 80 characters / line here.
>
> It seems we would have a potential race condition here.  Lets say HF
> sends AT+BCC, we call ofono_emulator_start_codec_negotiation.  Right
> after, the AG calls HandsfreeCard.Connect.  This will result in two +BCS
> unsolicited notifications being sent out.
>
> We might want to put in a guard where we don't send another +BCS
> notification until the HF has sent us back a +BCS or +BAC command. Also,
> in the case of HF initiated connections, it probably makes sense to
> reject any immediately subsequent attempts to establish the audio
> connection by the AG side with an -EALREADY.

Good point. Let me add proper guards.

>> +            return;
>> +        }
>> +
>> +        connect_sco(em);
>> +
>> +        return;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
>> +}
>> +
>>   static void emulator_add_indicator(struct ofono_emulator *em, const
>> char* name,
>>                       int min, int max, int dflt,
>>                       gboolean mandatory)
>> @@ -1047,6 +1226,9 @@ void ofono_emulator_register(struct
>> ofono_emulator *em, int fd)
>>           g_at_server_register(em->server, "+BIA", bia_cb, em, NULL);
>>           g_at_server_register(em->server, "+BIND", bind_cb, em, NULL);
>>           g_at_server_register(em->server, "+BIEV", biev_cb, em, NULL);
>> +        g_at_server_register(em->server, "+BAC", bac_cb, em, NULL);
>> +        g_at_server_register(em->server, "+BCC", bcc_cb, em, NULL);
>> +        g_at_server_register(em->server, "+BCS", bcs_cb, em, NULL);
>>       }
>>
>>       __ofono_atom_register(em->atom, emulator_unregister);
>> @@ -1101,6 +1283,7 @@ struct ofono_emulator
>> *ofono_emulator_create(struct ofono_modem *modem,
>>       em->l_features |= HFP_AG_FEATURE_ENHANCED_CALL_CONTROL;
>>       em->l_features |= HFP_AG_FEATURE_EXTENDED_RES_CODE;
>>       em->l_features |= HFP_AG_FEATURE_HF_INDICATORS;
>> +    em->l_features |= HFP_AG_FEATURE_CODEC_NEGOTIATION;
>>       em->events_mode = 3;    /* default mode is forwarding events */
>>       em->cmee_mode = 0;    /* CME ERROR disabled by default */
>>
>> @@ -1476,3 +1659,68 @@ void ofono_emulator_set_handsfree_card(struct
>> ofono_emulator *em,
>>
>>       em->card = card;
>>   }
>> +
>> +static unsigned char select_codec(struct ofono_emulator *em)
>> +{
>> +    if (ofono_handsfree_audio_has_wideband() &&
>> +            em->r_codecs[MSBC_OFFSET].supported)
>> +        return HFP_CODEC_MSBC;
>> +
>> +    /* CVSD is mandatory for both sides */
>> +    return HFP_CODEC_CVSD;
>> +}
>> +
>> +int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em,
>> +                        unsigned char codec,
>
> What is the theoretical purpose of this codec parameter?  Is the intent
> to only use it for the fallback use case in connect_sco() above?  If so,
> then we probably need to handle it some other way and remove this
> parameter from the public API.

Yes, that was the reason. Will drop that.

regards,
Simon