Re: [PATCH_v0 2/3] huawei: Add modem type detection

[Guillaume Zajac <guillaume.zajac@linux.intel.com>]

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

Hi Marcel,

>> This plugin is now merged with huaweicdma one.
>> Atoms will now be created with GSM or CDMA drivers
>> depending on the result of ATI command.
>> No SIM atom is created for embedded sim from CDMA
>> modems.
>> ---
>>   plugins/huawei.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>>   1 files changed, 69 insertions(+), 8 deletions(-)
>>
>> diff --git a/plugins/huawei.c b/plugins/huawei.c
>> index ae15bf9..a3768c8 100644
>> --- a/plugins/huawei.c
>> +++ b/plugins/huawei.c
>> @@ -51,6 +51,9 @@
>>   #include<ofono/message-waiting.h>
>>   #include<ofono/log.h>
>>
>> +#include<ofono/cdma-netreg.h>
>> +#include<ofono/cdma-connman.h>
>> +
>>   #include<drivers/atmodem/atutil.h>
>>   #include<drivers/atmodem/vendor.h>
>>
>> @@ -66,6 +69,7 @@ enum {
>>   	SIM_STATE_INVALID_CS =		2,
>>   	SIM_STATE_INVALID_PS =		3,
>>   	SIM_STATE_INVALID_PS_AND_CS =	4,
>> +	SIM_STATE_ROMSIM =		240,
>>   	SIM_STATE_NOT_EXISTENT =	255,
>>   };
> I really don't wanna random intermix of determining modem type and
> dealing with CDMA extensions. This should be nicely split into logical
> patches.

Yes, that was my intention this was a v0 for review.
Anyway it seems you did it well ;)

>
>> @@ -79,6 +83,7 @@ struct huawei_data {
>>   	struct cb_data *online_cbd;
>>   	const char *offline_command;
>>   	gboolean voice;
>> +	gboolean gsm;
> So just calling this gsm is a bit to short. Same goes for voice. That
> was a mistake and I fixed that upstream now to rename it to have_voice.
>
>>   };
>>
>>   static int huawei_probe(struct ofono_modem *modem)
>> @@ -405,7 +410,7 @@ static void rfswitch_support(gboolean ok, GAtResult *result, gpointer user_data)
>>   	struct ofono_modem *modem = user_data;
>>   	struct huawei_data *data = ofono_modem_get_data(modem);
>>
>> -	if (!ok)
>> +	if (!ok || !data->gsm)
>>   		data->offline_command = "AT+CFUN=5";
>>   	else
>>   		data->offline_command = "AT+CFUN=7";
> This should be a separate patch as well. I think the important part is
> to first enhance the plugin to detect GSM. And then on-top of that
> handle the difference between GSM and CDMA.
>
>> @@ -414,6 +419,44 @@ static void rfswitch_support(gboolean ok, GAtResult *result, gpointer user_data)
>>   					cfun_enable, modem, NULL);
>>   }
>>
>> +static gboolean parse_ati_result(GAtResult *result)
>> +{
>> +	GAtResultIter iter;
>> +	const char *line;
>> +	int num = g_at_result_num_response_lines(result);
>> +	int i;
>> +
>> +	g_at_result_iter_init(&iter, result);
>> +
>> +	for (i = 0; i<  num; i++) {
>> +		g_at_result_iter_next(&iter, NULL);
>> +		line = g_at_result_iter_raw_line(&iter);
>> +		if (g_str_has_prefix(line, "+GCAP"))
>> +			return (g_strrstr(line, "+CGSM") != NULL);
> This parsing is more complex than it needs to be. It also parses the
> string too many times. Especially since GAtChat does that all for you
> already anyway.
>
> Instead of posting an example here, I just pushed a patch so you can see
> how easily this can be done.

Right, I see this is easier do to how I you submitted it :)
However, I have a dongle EC1261 that is bugged because:
         - when I send ATI, the capabilities line is returned with 
"+GCAP +GCAP:" prefix
         - when I send AT+GCAP, the prefix is "+GCAP:"

Unless the manufacturer fixes this issue, we won't be able to support 
this dongle...

>> +	}
>> +
>> +	/* By default we consider modem is GSM type */
>> +	return TRUE;
>> +}
>> +
>> +static void ati_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +	struct ofono_modem *modem = user_data;
>> +	struct huawei_data *data = ofono_modem_get_data(modem);
>> +
>> +	/* By default we consider modem is GSM type */
>> +	if (!ok)
>> +		data->gsm = TRUE;
>> +	else
>> +		data->gsm = parse_ati_result(result);
> I don't like this. We should use have_gsm and have_cdma and not randomly
> fallback to one of them. If for some reason ATI does not show any of
> them, we should also not do anything either.
>
> More important I do wanna get bug reports if ATI does not carry +GCAP or
> has some weird capabilities inside.
>
>> +
>> +	data->sim_state = SIM_STATE_NOT_EXISTENT;
>> +
>> +	g_at_chat_send(data->pcui, "AT^RFSWITCH=?", rfswitch_prefix,
>> +					rfswitch_support, modem, NULL);
>> +}
>> +
>> +
>>   static GAtChat *open_device(struct ofono_modem *modem,
>>   				const char *key, char *debug)
>>   {
>> @@ -472,10 +515,7 @@ static int huawei_enable(struct ofono_modem *modem)
>>   	g_at_chat_send(data->modem, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
>>   	g_at_chat_send(data->pcui, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
>>
>> -	data->sim_state = SIM_STATE_NOT_EXISTENT;
> Leave this sim_state assignment at this location. No point in moving
> this higher up.
>
>> -
>> -	g_at_chat_send(data->pcui, "AT^RFSWITCH=?", rfswitch_prefix,
>> -					rfswitch_support, modem, NULL);
>> +	g_at_chat_send(data->pcui, "ATI", NULL, ati_cb, modem, NULL);
>>
>>   	return -EINPROGRESS;
>>   }
>> @@ -555,6 +595,7 @@ static void sysinfo_online_cb(gboolean ok, GAtResult *result,
>>   	case SIM_STATE_INVALID_CS:
>>   	case SIM_STATE_INVALID_PS:
>>   	case SIM_STATE_INVALID_PS_AND_CS:
>> +	case SIM_STATE_ROMSIM:
>>   		CALLBACK_WITH_SUCCESS(cb, data->online_cbd->data);
> This should be a separate patch together with the state addition.
>
>>   		goto done;
>>   	}
>> @@ -650,13 +691,21 @@ static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
>>   static void huawei_pre_sim(struct ofono_modem *modem)
>>   {
>>   	struct huawei_data *data = ofono_modem_get_data(modem);
>> -	struct ofono_sim *sim;
>> +	struct ofono_sim *sim = NULL;
> Please don't do that. I prefer not assigning variables with NULL here.
> It is better to change the logic around.
>
>>
>>   	DBG("%p", modem);
>>
>> -	ofono_devinfo_create(modem, 0, "atmodem", data->pcui);
>> -	sim = ofono_sim_create(modem, OFONO_VENDOR_HUAWEI,
>> +	if (data->gsm == TRUE) {
>> +		ofono_devinfo_create(modem, 0, "atmodem", data->pcui);
>> +		sim = ofono_sim_create(modem, OFONO_VENDOR_HUAWEI,
>>   					"atmodem", data->pcui);
>> +	} else {
>> +		ofono_devinfo_create(modem, 0, "cdmamodem", data->pcui);
>> +		/* Create sim atom only if sim is not embedded */
>> +		if (data->sim_state != SIM_STATE_ROMSIM)
>> +			sim = ofono_sim_create(modem, OFONO_VENDOR_HUAWEI,
>> +						"atmodem", data->pcui);
> I am really not sure that it is a good idea to just use the GSM SIM atom
> here. The EF structure will be different and we might cause more harm
> than doing any good in assuming that we get any proper EF fields.
>
> This is clearly the part where we need detailed information from Huawei
> on how this is suppose to work. And how this is suppose to be done for
> CDMA in the first place anyway.

I agree, maybe Deng Yin An could ask Huawei how they (plan to) support 
R-UIM in their modem e.g.
- Do they have some commands to read UIM file system
- And what does ROMSIM consist in (differences against R-UIM)?

>> +	}
>>
>>   	if (sim&&  data->have_sim == TRUE)
>>   		ofono_sim_inserted_notify(sim, TRUE);
>> @@ -670,6 +719,9 @@ static void huawei_post_sim(struct ofono_modem *modem)
>>
>>   	DBG("%p", modem);
>>
>> +	if (data->gsm == FALSE)
>> +		return;
>> +
>>   	if (data->voice == TRUE) {
>>   		ofono_voicecall_create(modem, 0, "huaweimodem", data->pcui);
>>   		ofono_audio_settings_create(modem, 0,
>> @@ -695,6 +747,15 @@ static void huawei_post_online(struct ofono_modem *modem)
>>
>>   	DBG("%p", modem);
>>
>> +	if (data->gsm == FALSE) {
>> +		ofono_cdma_netreg_create(modem, 0, "huaweicdmamodem",
>> +						data->pcui);
>> +
>> +		ofono_cdma_connman_create(modem, OFONO_VENDOR_HUAWEI,
>> +						"cdmamodem", data->modem);
>> +		return;
>> +	}
>> +
>>   	ofono_netreg_create(modem, OFONO_VENDOR_HUAWEI, "atmodem", data->pcui);
>>
>>   	ofono_cbs_create(modem, OFONO_VENDOR_QUALCOMM_MSM,
> Lets do if (data->have_gsm ... and if (data->have_cdma ...) separate
> here. I rather no degrade GSM to second class citizen.
>
> Regards
>
> Marce

Kind regards,
Guillaume