Re: [PATCH v2] quectel: EC21 needs aux channel to be the first mux channel

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Lars,

On 5/28/20 4:32 AM, poeschel(a)lemonage.de wrote:
> From: Lars Poeschel <poeschel@lemonage.de>
> 
> The Quectel EC21 does only work correctly, if the mux channel used for
> aux is the first mux channel. It does only put it's URC messages in the
> first mux channel, so this has to be the aux channel in our case.
> To be flexible on the mux order we introduce an array here, that
> contains the initialization data in it's needed order and is then simply
> applied by a for-loop. Initialization of this array is done after we
> queried the modem model.
> ---
>   plugins/quectel.c | 72 ++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 58 insertions(+), 14 deletions(-)
> 

So this looks mostly reasonable, but see below:

> diff --git a/plugins/quectel.c b/plugins/quectel.c
> index 043d39f9..1cad35d6 100644
> --- a/plugins/quectel.c
> +++ b/plugins/quectel.c
> @@ -78,6 +78,21 @@ static const uint8_t gsm0710_terminate[] = {
>   	0xf9, /* close flag */
>   };
>   
> +enum mux_type {
> +	QUECTEL_MUX_TYPE_AUX = 0,
> +	QUECTEL_MUX_TYPE_MODEM,
> +	QUECTEL_MUX_TYPE_MAX,
> +};
> +
> +struct mux_initialization_data {
> +	enum mux_type mux_type;
> +	char *chat_debug;
> +	const char *n_gsm_key;
> +	const char *n_gsm_value;
> +};
> +
> +static struct mux_initialization_data mux_order[QUECTEL_MUX_TYPE_MAX];
> +

So the issue with this is that this driver now no-longer supports 
multiple modems of the same type active simultaneously (since all 
instances would be trying to write / read from this location).

A better approach would be to use two such variables, i.e. 
mux_order_ec21 and mux_order_default and then either store a pointer to 
the one to use inside quectel_data, or programmatically by looking up 
based on the quectel_data::model.

Alternatively, storing mux_order in quectel_data itself is also an option.

>   enum quectel_model {
>   	QUECTEL_UNKNOWN,
>   	QUECTEL_UC15,
> @@ -1014,6 +1037,17 @@ static void cgmm_cb(int ok, GAtResult *result, void *user_data)
>   		return;
>   	}
>   
> +	mux_order[0] = (struct mux_initialization_data)
> +			{ QUECTEL_MUX_TYPE_MODEM,
> +			"Modem: ",
> +			"Modem",
> +			"/dev/gsmtty1"};
> +	mux_order[1] = (struct mux_initialization_data)
> +			{ QUECTEL_MUX_TYPE_AUX,
> +			"Aux: ",
> +			"Aux",
> +			"/dev/gsmtty2"};
> +

This would then move into the static initializer above...

>   	if (strcmp(model, "UC15") == 0) {
>   		DBG("%p model UC15", modem);
>   		data->vendor = OFONO_VENDOR_QUECTEL;
> @@ -1030,6 +1064,16 @@ static void cgmm_cb(int ok, GAtResult *result, void *user_data)
>   		DBG("%p model EC21", modem);
>   		data->vendor = OFONO_VENDOR_QUECTEL;
>   		data->model = QUECTEL_EC21;
> +		mux_order[0] = (struct mux_initialization_data)
> +				{ QUECTEL_MUX_TYPE_AUX,
> +				"Aux: ",
> +				"Aux",
> +				"/dev/gsmtty1"};
> +		mux_order[1] = (struct mux_initialization_data)
> +				{ QUECTEL_MUX_TYPE_MODEM,
> +				"Modem: ",
> +				"Modem",
> +				"/dev/gsmtty2"};
>   	} else {
>   		ofono_warn("%p unknown model: '%s'", modem, model);
>   		data->vendor = OFONO_VENDOR_QUECTEL;
> 

Regards,
-Denis