Re: [PATCH v2 1/7] hfpmodem: Watch for changes in the selected codec

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Vinicius,

On 04/10/2013 02:37 PM, Vinicius Costa Gomes wrote:
> This patch adds a function to monitor when the AG sends a new codec
> before establishing the SCO connection.
> ---
>   plugins/hfp_hf_bluez5.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
>
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index c63b1a2..efbb209 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -314,9 +314,60 @@ static struct ofono_modem_driver hfp_driver = {
>   	.post_sim	= hfp_post_sim,
>   };
>
> +static void bcs_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct hfp *hfp = user_data;
> +	struct hfp_slc_info *info =&hfp->info;
> +	GAtResultIter iter;
> +	char str[32];
> +	int value;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+BCS:"))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter,&value))
> +		return;
> +
> +	memset(str, 0, sizeof(str));

Why do you need this exactly?

> +
> +	switch (value) {
> +	case HFP_CODEC_MSBC:
> +		if (!ofono_handsfree_audio_has_wideband()) {
> +			/* No wideband speech support, so we only have CVSD */
> +			sprintf(str, "AT+BAC=%d", HFP_CODEC_CVSD);
> +			break;
> +		}
> +
> +		/* fall through */
> +

This fall through and the default statement make this code extremely 
hard to read.  Why don't we move the codec selection logic into the core 
instead?

Then this becomes something like:

if (ofono_handsfree_audio_card_set_codec(card, value) == FALSE) {
	if (ofono_handsfree_audio_has_wideband())
		...
	else
		...

	goto done;
}

sprintf(str, "AT+BCS=%d", value);

done:
....


> +	case HFP_CODEC_CVSD:
> +		/* Confirm the codec */
> +		sprintf(str, "AT+BCS=%d", value);
> +		break;
> +
> +	default:
> +		/* Unsupported codec, re-send our codecs */
> +		if (ofono_handsfree_audio_has_wideband())
> +			sprintf(str, "AT+BAC=%d,%d", HFP_CODEC_CVSD,
> +							HFP_CODEC_MSBC);
> +		else
> +			sprintf(str, "AT+BAC=%d", HFP_CODEC_CVSD);
> +	}
> +
> +	g_at_chat_send(info->chat, str, NULL, NULL, NULL, NULL);
> +}
> +
>   static int hfp16_card_probe(struct ofono_handsfree_card *card,
>   					unsigned int vendor, void *data)
>   {
> +	struct hfp *hfp = data;
> +	struct hfp_slc_info *info =&hfp->info;
> +
> +	g_at_chat_register(info->chat, "+BCS:", bcs_notify, FALSE,
> +								hfp, NULL);
> +

You read my mind, I was going to suggest doing this exactly :)

>   	return 0;
>   }
>

Regards,
-Denis