Hi Denis, list,
Sorry for my late response, was traveling. I'll address all the
comments and fix the delayed register this week. Thanks again.
On 19 December 2016 at 17:59, Denis Kenzior <denk...@gmail.com> wrote:
> Hi Djalal,
>
> On 12/16/2016 04:05 AM, Djalal Harouni wrote:
>>
>> This adds support for +UCGED mode 2 and for now we collect only
>> EARFCN, EBAND and CQI. Later we will follow up with +UCGED mode 3.
>>
>> Some ublox modems supports +UCGED mode 2 and others only +UCGED mode 3
>> depending on the firmware.
>> ---
>> drivers/ubloxmodem/netmon.c | 210
>> +++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 206 insertions(+), 4 deletions(-)
>>
>
> This looks good. Just a couple of small nitpicks:
>
>> diff --git a/drivers/ubloxmodem/netmon.c b/drivers/ubloxmodem/netmon.c
>> index 0749078..6fc3bad 100644
>> --- a/drivers/ubloxmodem/netmon.c
>> +++ b/drivers/ubloxmodem/netmon.c
>> @@ -46,6 +46,20 @@
>>
>> static const char *cops_prefix[] = { "+COPS:", NULL };
>> static const char *cesq_prefix[] = { "+CESQ:", NULL };
>> +static const char *ucged_prefix[] = { "+UCGED:", NULL };
>> +
>> +/*
>> + * Generic function that should point to number or hexstring result
>> + * parsers. Result should be returned as an int.
>> + */
>> +typedef gboolean (*ucged_g_at_result_iter_next)(GAtResultIter *iter,
>> + int *result);
>> +
>
>
> Maybe ucged_parse_field_t? However, I'm not sure about the function
> signature here. g_at_result_iter_next_hexstring returns a guint8 * and
> gsize value.
>
>> +/* Field from +UCGED response */
>> +typedef struct ublox_ucged_value {
>> + int offset; /* Offset at response */
>> + ucged_g_at_result_iter_next ucged_g_at_result_iter_next_field;
>
>
> The name is unnecessarily long. Maybe parse_field?
>
>
>> +} ublox_ucged_value;
>>
>> struct netmon_driver_data {
>> GAtChat *chat;
>> @@ -67,6 +81,10 @@ struct req_cb_data {
>> int rsrp; /* CESQ: Reference Signal Received Power */
>> int ecn0; /* CESQ: Received Energy Ratio */
>> int rsrq; /* CESQ: Reference Signal Received Quality */
>> +
>> + int earfcn; /* UCGED: E-UTRA ARFCN */
>> + int eband; /* UCGED: E-UTRA Band */
>> + int cqi; /* UCGED: Channel Quality Indicator */
>> };
>>
>> /*
>> @@ -102,6 +120,7 @@ static inline struct req_cb_data
>> *req_cb_data_new0(void *cb, void *data,
>> ret->cb = cb;
>> ret->data = data;
>> ret->netmon = user;
>> +
>> ret->rxlev = -1;
>> ret->ber = -1;
>> ret->rscp = -1;
>> @@ -109,6 +128,10 @@ static inline struct req_cb_data
>> *req_cb_data_new0(void *cb, void *data,
>> ret->ecn0 = -1;
>> ret->rsrq = -1;
>>
>> + ret->earfcn = -1;
>> + ret->eband = -1;
>> + ret->cqi = -1;
>> +
>> return ret;
>> }
>>
>> @@ -157,11 +180,165 @@ static void ublox_netmon_finish_success(struct
>> req_cb_data *cbd)
>> OFONO_NETMON_INFO_ECN0, cbd->ecn0,
>> OFONO_NETMON_INFO_RSRQ, cbd->rsrq,
>> OFONO_NETMON_INFO_RSRP, cbd->rsrp,
>> + OFONO_NETMON_INFO_EARFCN,
>> cbd->earfcn,
>> + OFONO_NETMON_INFO_EBAND,
>> cbd->eband,
>> + OFONO_NETMON_INFO_CQI, cbd->cqi,
>> OFONO_NETMON_INFO_INVALID);
>>
>> CALLBACK_WITH_SUCCESS(cbd->cb, cbd->data);
>> }
>>
>> +static void ucged_parse_4g(struct req_cb_data *cbd, GAtResultIter *iter)
>> +{
>> + /* Offset of values in returned response */
>> + enum ucged_mode2_rat4_offsets {
>> + UCGED_EARFCN = 0,
>> + UCGED_EBAND = 1,
>> + UCGED_CQI = 15,
>> + };
>> +
>> + ublox_ucged_value ucged_values[] = {
>> + { UCGED_EARFCN, g_at_result_iter_next_number },
>> + { UCGED_EBAND, g_at_result_iter_next_number },
>> + { UCGED_CQI, g_at_result_iter_next_number },
>> + { -1, NULL }, /* End of Array */
>> + };
>> +
>> + gboolean ok;
>> + unsigned i;
>> + ublox_ucged_value *cur;
>> +
>> + /* Skip to next */
>> + ok = g_at_result_iter_next(iter, NULL);
>> + if (!ok) {
>> + DBG(" UCGED:response: failed to go to next line ");
>> + return;
>> + }
>> +
>> + i = 0;
>> + cur = &ucged_values[0];
>> + while (cur->offset >= 0) {
>> + int number;
>> +
>
>
> If you declare number as unsigned int you won't need the ugly cast below.
>
>
>> + /* Skip values that we do not want */
>> + while (i != (unsigned) cur->offset) {
>> + ok = g_at_result_iter_skip_next(iter);
>> + if (!ok) {
>> + DBG(" UCGED: error skip idx: %d ", i);
>> + return;
>> + }
>> + i++;
>> + }
>> +
>> + ok = cur->ucged_g_at_result_iter_next_field(iter,
>> &number);
>> + if (!ok) {
>> + DBG(" UCGED: error parsing at idx: %d ", i);
>> + return;
>> + }
>> +
>> + switch (i) {
>> + case UCGED_EARFCN:
>> + cbd->earfcn = number != 65535 ? number :
>> cbd->earfcn;
>> + break;
>> + case UCGED_EBAND:
>> + cbd->eband = number != 255 ? number : cbd->eband;
>> + break;
>> + case UCGED_CQI:
>> + cbd->cqi = number != 255 ? number : cbd->cqi;
>> + break;
>> + }
>> +
>> + i++;
>> + cur++;
>> + }
>> +
>> + /*
>> + * Sample output for +UCGED MODE 2 will be:
>> + *
>> + * UCGED: MODE 2 RAT = 4G EARFCN = 1300
>> + * UCGED: MODE 2 RAT = 4G EBAND = 3
>> + * UCGED: MODE 2 RAT = 4G CQI = -1
>
>
> When I asked for sample output, I meant from the modem, not from this
> function
>
>
>> + *
>> + * Check the networkmonitor-api documentation and the related
>> + * references for mode details.
>> + */
>> + DBG(" UCGED: MODE 2 RAT = 4G EARFCN = %d ", cbd->earfcn);
>> + DBG(" UCGED: MODE 2 RAT = 4G EBAND = %d ", cbd->eband);
>> + DBG(" UCGED: MODE 2 RAT = 4G CQI = %d ", cbd->cqi);
>> +}
>> +
>> +static void ucged_collect_mode2_data(struct req_cb_data *cbd,
>> + GAtResultIter *iter)
>> +{
>> + int rat;
>> + gboolean ok;
>> +
>> + ok = g_at_result_iter_next(iter, NULL);
>> + if (!ok) {
>> + DBG(" UCGED:response: failed to go to next line ");
>> + return;
>> + }
>> +
>> + ok = g_at_result_iter_next_number(iter, &rat);
>> + if (!ok) {
>> + DBG(" UCGED: error parsing 'RAT' ");
>> + return;
>> + }
>> +
>> + /* For now we support only RAT 4 */
>> + if (rat != 4) {
>> + DBG(" UCGED: 'RAT' is %d, nothing to do. ", rat);
>> + return;
>> + }
>> +
>> + DBG(" UCGED: 'RAT' is %d ", rat);
>> + ucged_parse_4g(cbd, iter);
>> +}
>> +
>> +static void ucged_query_mode2_cb(gboolean ok, GAtResult *result,
>> + gpointer user_data)
>> +{
>> + struct req_cb_data *cbd = user_data;
>> + struct ofono_error error;
>> + GAtResultIter iter;
>> + int mode;
>> +
>> + DBG("ok %d", ok);
>> +
>> + decode_at_error(&error, g_at_result_final_response(result));
>> +
>> + if (!ok) {
>> + DBG(" UCGED: failed ");
>> + goto out;
>> + }
>> +
>> + g_at_result_iter_init(&iter, result);
>> +
>> + if (!g_at_result_iter_next(&iter, "+UCGED:")) {
>> + DBG(" UCGED: no result ");
>> + goto out;
>> + }
>> +
>> + ok = g_at_result_iter_next_number(&iter, &mode);
>> + if (!ok) {
>> + DBG(" UCGED: error parsing 'mode' ");
>> + goto out;
>> + }
>> +
>> + DBG(" UCGED: report mode is %d ", mode);
>> +
>> + /* mode 2 collect +UCGED data */
>> + if (mode == 2)
>> + ucged_collect_mode2_data(cbd, &iter);
>> +
>> + /*
>> + * We never fail at this point we always send what we collected so
>> + * far
>> + */
>> +out:
>> + ublox_netmon_finish_success(cbd);
>> +}
>> +
>> static void cesq_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> {
>> enum cesq_ofono_netmon_info {
>> @@ -175,6 +352,8 @@ static void cesq_cb(gboolean ok, GAtResult *result,
>> gpointer user_data)
>> };
>>
>> struct req_cb_data *cbd = user_data;
>> + struct ofono_netmon *nm = cbd->netmon;
>> + struct netmon_driver_data *nmd = ofono_netmon_get_data(nm);
>> struct ofono_error error;
>> GAtResultIter iter;
>> int idx, number;
>> @@ -233,10 +412,17 @@ static void cesq_cb(gboolean ok, GAtResult *result,
>> gpointer user_data)
>> DBG(" RSRQ %d ", cbd->rsrq);
>> DBG(" RSRP %d ", cbd->rsrp);
>>
>> - /*
>> - * We never fail at this point we always send what we collected so
>> - * far
>> - */
>> + cbd = req_cb_data_ref(cbd);
>> + if (g_at_chat_send(nmd->chat, "AT+UCGED?", NULL,
>> + ucged_query_mode2_cb, cbd, req_cb_data_unref) ==
>> 0) {
>> + /* We never fail, we always send what we collected so far
>> */
>> + ofono_error("%s: send +UCGED failed", __func__);
>> + req_cb_data_unref(cbd);
>> + goto out;
>> + }
>> +
>> + return;
>> +
>> out:
>> ublox_netmon_finish_success(cbd);
>> }
>> @@ -320,6 +506,22 @@ static int ublox_netmon_probe(struct ofono_netmon
>> *netmon,
>>
>> g_idle_add(ublox_delayed_register, netmon);
>>
>
> I still think we should remove ublox_delayed_register and move the
> ofono_netmon_register call into the UCGED=2 callback below.
>
>> + /*
>> + * We set +UCGED=2 mode as early as possible so we are able to
>> handle
>> + * the following:
>> + * 1) In case the modem supports only +UCGED mode 2 then we have
>> + * already set the appropriate mode and we can query the
>> information
>> + * later.
>> + * 2) In case the modem supports only +UCGED mode 3, then to set
>> mode 3
>> + * the modem has to be first in +UCGED mode 2. Setting the mode
>> here
>> + * allows later to query the information and on errors we
>> fallback
>> + * to mode 3.
>> + *
>> + * This should handle current firmware versions of ublox modems.
>> + */
>> + g_at_chat_send(nmd->chat, "AT+UCGED=2", ucged_prefix,
>> + NULL, NULL, NULL);
>> +
>> return 0;
>> }
>>
>>
>
> Regards,
> -Denis
_______________________________________________
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono
