Hi Denis,
On 12:22 Fri 05 Apr, Denis Kenzior wrote:
> Hi Vinicius,
>
> On 04/03/2013 06:24 PM, Vinicius Costa Gomes wrote:
> >---
> > plugins/hfp_hf_bluez5.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 40 insertions(+), 1 deletion(-)
> >
> >diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> >index a6cc156..4c8f412 100644
> >--- a/plugins/hfp_hf_bluez5.c
> >+++ b/plugins/hfp_hf_bluez5.c
> >@@ -67,6 +67,8 @@ struct hfp {
> > struct hfp_slc_info info;
> > DBusMessage *msg;
> > struct ofono_handsfree_card *card;
> >+ ofono_handsfree_card_connect_cb_t cb;
> >+ void *user_data;
>
> Please use the cb_data_new mechanism instead of storing this here.
Somehow I missed this. Yeah, this (and the atutil.h helpers) will simplify the
code a lot.
>
> > };
> >
> > static GDBusClient *bluez = NULL;
> >@@ -324,11 +326,48 @@ static void hfp16_card_remove(struct
> >ofono_handsfree_card *card)
> >
> > }
> >
> >+static void bcc_cb(gboolean ok, GAtResult *result, gpointer user_data)
> >+{
> >+ struct hfp *hfp = user_data;
> >+ struct hfp_slc_info *info =&hfp->info;
> >+
> >+ if (ok)
> >+ CALLBACK_WITH_SUCCESS(hfp->cb, hfp->user_data);
> >+ else
> >+ CALLBACK_WITH_FAILURE(hfp->cb, hfp->user_data);
>
> Please use decode_at_error instead.
>
> e.g. decode_at_error(&error, g_at_result_final_response(result));
> callback(&error, ...);
>
> >+
> >+ g_at_chat_unref(info->chat);
>
> No need to reference count the g_at_chat. This is just needless overhead.
>
> >+}
> >+
> > static void hfp16_card_connect(struct ofono_handsfree_card *card,
> > ofono_handsfree_card_connect_cb_t cb,
> > void *data)
> > {
> >- CALLBACK_WITH_FAILURE(cb, data);
> >+ struct hfp *hfp = ofono_handsfree_card_get_data(card);
> >+ struct hfp_slc_info *info =&hfp->info;
> >+
> >+ if (info->chat == NULL) {
> >+ CALLBACK_WITH_FAILURE(cb, data);
> >+ return;
> >+ }
>
> Can this case even happen?
Not anymore. Will fix.
>
> >+
> >+ if (!(info->hf_features& HFP_HF_FEATURE_CODEC_NEGOTIATION&&
> >+ info->ag_features& HFP_AG_FEATURE_CODEC_NEGOTIATION)) {
> >+ /*
> >+ * If both sides don't support codec negotiation,
> >+ * fallback to direct SCO connection. Calling
> >+ * connect_sco() hands the connection responsability
> >+ * to the core, so no need to call the callback
> >+ */
> >+ ofono_handsfree_card_connect_sco(card);
> >+ return;
> >+ }
>
> Can we reverse this logic to make it more readable?
Sure.
>
> e.g. if (HF has CodecNegotiation and AG has CodecNegotiation) {
> send BCC;
> return;
> }
>
> >+
> >+ info->chat = g_at_chat_ref(info->chat);
>
> No need to take a reference. Remove this part
>
> >+ hfp->cb = cb;
> >+ hfp->user_data = data;
> >+
>
> Use the cb_data_new mechanism here
>
> >+ g_at_chat_send(info->chat, "AT+BCC", NULL, bcc_cb, hfp, NULL);
>
> And make sure to set the destroy callback. There are examples of
> this throughout the drivers/atmodem/* and drivers/hfpmodem/*.
>
> > }
> >
> > static struct ofono_handsfree_card_driver hfp16_hf_driver = {
>
> Regards,
> -Denis
Cheers,
--
Vinicius
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
