From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============7803025849179798011==" MIME-Version: 1.0 From: Simon Fels Subject: Re: [PATCH 1/2 v3] emulator: add codec negotiation support Date: Wed, 21 Oct 2015 09:42:38 +0200 Message-ID: <562741EE.7010901@canonical.com> In-Reply-To: <5626FEC5.3040003@gmail.com> List-Id: To: ofono@ofono.org --===============7803025849179798011== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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 >> #include >> #include >> +#include >> >> #include >> >> @@ -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 =3D 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) =3D=3D FALSE || >> + val !=3D HFP_CODEC_CVSD) >> + goto fail; >> + >> + em->bac_received =3D TRUE; >> + >> + em->negotiated_codec =3D 0; >> + em->r_codecs[CVSD_OFFSET].supported =3D TRUE; >> + >> + while (g_at_result_iter_next_number(&iter, &val)) { >> + switch (val) { >> + case HFP_CODEC_MSBC: >> + em->r_codecs[MSBC_OFFSET].supported =3D 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 =3D=3D NULL) >> + return; >> + >> + em->codec_negotiation_cb(err, em->codec_negotiation_data); >> + >> + em->codec_negotiation_cb =3D NULL; >> + em->codec_negotiation_data =3D NULL; >> +} >> + >> +static void connect_sco(struct ofono_emulator *em) >> +{ >> + int err; >> + >> + DBG(""); >> + >> + em->delay_sco =3D 0; > > What does this do? > >> + >> + err =3D ofono_handsfree_card_connect_sco(em->card); > > ofono_handsfree_card_connect_sco function does not check for em->card > being NULL. > >> + if (err =3D=3D 0) { >> + finish_codec_negotiation(em, 0); >> + return; >> + } >> + >> + /* If we have another codec we can try then lets do that */ >> + if (em->negotiated_codec !=3D 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 =3D 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 !=3D val) { >> + em->proposed_codec =3D 0; >> + break; >> + } >> + >> + em->proposed_codec =3D 0; >> + em->negotiated_codec =3D val; >> + >> + DBG("negotiated codec %d", val); >> + >> + if (em->card !=3D 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 =3D 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 |=3D HFP_AG_FEATURE_ENHANCED_CALL_CONTROL; >> em->l_features |=3D HFP_AG_FEATURE_EXTENDED_RES_CODE; >> em->l_features |=3D HFP_AG_FEATURE_HF_INDICATORS; >> + em->l_features |=3D HFP_AG_FEATURE_CODEC_NEGOTIATION; >> em->events_mode =3D 3; /* default mode is forwarding events */ >> em->cmee_mode =3D 0; /* CME ERROR disabled by default */ >> >> @@ -1476,3 +1659,68 @@ void ofono_emulator_set_handsfree_card(struct >> ofono_emulator *em, >> >> em->card =3D 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 --===============7803025849179798011==--