phablet-extras

Merge lp://qastaging/~awe/phablet-extras/ofono-gprs-support into lp://qastaging/phablet-extras/ofono

ofono-gprs-support
Merge into ofono

Proposed by Tony Espy on 2013-05-29

Status:	Merged
Approved by:	Ricardo Salveti on 2013-05-31
Approved revision:	66
Merged at revision:	37
Proposed branch:	lp://qastaging/~awe/phablet-extras/ofono-gprs-support
Merge into:	lp://qastaging/phablet-extras/ofono
Diff against target:	2469 lines (+1530/-257) 18 files modified Makefile.am (+2/-0) debian/changelog (+4/-0) drivers/rilmodem/gprs-context.c (+548/-0) drivers/rilmodem/gprs.c (+276/-0) drivers/rilmodem/network-registration.c (+3/-5) drivers/rilmodem/rilmodem.c (+7/-1) drivers/rilmodem/rilmodem.h (+6/-0) drivers/rilmodem/rilutil.c (+309/-88) drivers/rilmodem/rilutil.h (+51/-1) drivers/rilmodem/sim.c (+132/-20) drivers/rilmodem/voicecall.c (+0/-2) gril/gril.c (+64/-94) gril/grilutil.c (+60/-36) gril/grilutil.h (+1/-0) gril/parcel.c (+16/-6) gril/ril_constants.h (+20/-0) plugins/provision.c (+6/-1) plugins/ril.c (+25/-3)
To merge this branch:	bzr merge lp://qastaging/~awe/phablet-extras/ofono-gprs-support
Related bugs:	Link a bug report
Related blueprints:	Telephony Support (Undefined)

Reviewer	Review Type	Date Requested	Status
Ricardo Salveti (community)		2013-05-29	Approve on 2013-05-31
PS Jenkins bot	continuous-integration		Approve on 2013-05-31
Review via email: mp+166294@code.qastaging.launchpad.net

Commit message

[rilmodem] Initial GPRS support.

Description of the change

This change adds basic GPRS support to our ofono/rilmodem code.

New source files in drivers/rilmodem include gprs.c and gprs-context.c.

In addition to the GPRS changes, some re-factoring of the debug logging code is also present in this MP. This includes removal of some of the low-level /gril log calls ( which have been commented out and marked as TODO: make conditional ), and addition of RIL message logging which mimics the logging done by the native RIL code. This latter change has not been 100% implemented yet, however it was critical to getting the GPRS code to work.

This code has been tested on maguro and mako, using an AT&T SIM in the US.

Testing requires installation of the python test scripts bundled with oFono. Sergio is working on packaging these scripts into a stand-alone package. For now they can be copied to the phone ( note, they require the additional package python-dbus ).

To test:

1. Run list-contexts to see the existing contexts.

2. Use activate-context to activate 3g. If multiple contexts exist, you may need to specify a 0-based index to tell oFono to use the correct context.

3. Use list-contexts to see the settings returned by RILD for the data call.

4. Use ifconfig to confirm that the interface has been correctly setup ( rmnet0 or rmnet_usb0 depending on the phone ).

5. On both phones, you'll need to manually configured DNS. My shortcut is just:

# echo "nameserver 8.8.8.8" > /etc/resolv.conf

6. On maguro, the default route is not configured by default; that said, you should see the network route ( use netstat -rn to see the routing table ). To configured the default route, run the following command:

# route add default gw <gateway-address> <iface-name>

6. To disable the connection, run deactivate-context

Note, sometimes it take awhile for GPRS to be attached. If you try to activate the context while !attached, activate-context will return an error. You can use the list-modems command to examine the ConnectionManager properties ( one of which is 'Attached' ).

A couple of other comments:

* No disconnect nor suspend testing has yet been done.

* Data roaming is disabled by default, and is not yet implemented.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-05-29:

PASSED: Continuous integration, rev:55
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/16/
Executed test runs:
SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-raring-armhf-ci/16
deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-raring-armhf-ci/16/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/16/rebuild

review: Approve (continuous-integration)

lp://qastaging/~awe/phablet-extras/ofono-gprs-support updated on 2013-05-29

56. By Tony Espy on 2013-05-29: [rilmodem] Get rid of unused gprs_context_data vendor field.
57. By Tony Espy on 2013-05-29: [rilmodem] Get rid of unused gprs_data vendor field.
58. By Tony Espy on 2013-05-29: [rilmodem] Add comments explaining lack of usage of core gprs suspend/resume/bearer functions.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-05-29:

PASSED: Continuous integration, rev:58
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/17/
Executed test runs:
SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-raring-armhf-ci/17
deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-raring-armhf-ci/17/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/17/rebuild

review: Approve (continuous-integration)

Revision history for this message

Tony Espy (awe) wrote on 2013-05-30:

I just re-factored the DATA_CALL response code based on feedback from cyphermox:

1. The code that calculates netmasks has been moved to rilutil, so that it can be unit-tested ( TODO ).

2. The code now handles multiple IP address and/or gateways returned from RILD in a more graceful manner, in that it splits the addresses, and only hands the first address and gateway to the core ofono code ( which only handles single IP addresses and gateways ).

3. The code now strips IP address prefixes, as NM doesn't handle them properly.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-05-30:

PASSED: Continuous integration, rev:59
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/19/
Executed test runs:
SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-raring-armhf-ci/19
deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-raring-armhf-ci/19/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/19/rebuild

review: Approve (continuous-integration)

Revision history for this message

Ricardo Salveti (rsalveti) wrote on 2013-05-30:

Download full text (3.7 KiB)

91 +static char printBuf[PRINTBUF_SIZE];

574 +static char printBuf[PRINTBUF_SIZE];

910 +static char printBuf[PRINTBUF_SIZE];

2390 +static char printBuf[PRINTBUF_SIZE];

Do we really need a global string? Would avoid calling clear.

1467 + * $AOSP/hardware/ril/libril/ril.cpp
1468 + */
1469 +#define startRequest sprintf(printBuf, "(")
1470 +#define closeRequest sprintf(printBuf, "%s)", printBuf)
1471 +#define printRequest(token, req) \
1472 + ofono_debug("[%04d]> %s %s", token, ril_request_id_to_string(req), printBuf)
1473 +
1474 +#define startResponse sprintf(printBuf, "%s {", printBuf)
1475 +#define closeResponse sprintf(printBuf, "%s}", printBuf)
1476 +#define printResponse ofono_debug("%s", printBuf)
1477 +
1478 +#define clearPrintBuf printBuf[0] = 0
1479 +#define removeLastChar printBuf[strlen(printBuf)-1] = 0
1480 +#define appendPrintBuf(x...) sprintf(printBuf, x)

91 +static char printBuf[PRINTBUF_SIZE];

Coding style here might be an issue when pushing this code upstream. I'd guess it'd be better to use print_buf instead.

114 + if (message->buf_len < 36) {
115 + DBG("Parcel is less then minimum DataCallResponseV6 size!");

Missing "decode_ril_error(&error, "FAIL");".

126 + /* TODO: make conditional */
127 + appendPrintBuf("[%04d]< %s",
128 + message->serial_no,
129 + ril_request_id_to_string(message->req));
130 + startResponse;
131 + /* TODO: make conditional */

Mind moving this code a bit bellow so it can be near the rest of the append/close/print logic? Otherwise it's hard to track which message it's actually covering (in case of another message setup later).

135 + /* RIL version */
136 + version = parcel_r_int32(&rilp);
137 +
138 + /* Size of call list */
139 + /* TODO: What if there's more than 1 call in the list?? */
140 + num = parcel_r_int32(&rilp);

Isn't the reply just a RIL_Data_Call_Response_v6 struct? If so, mind adding a comment saying that the version and num arguments were not described correctly by the Android ril.h file?

175 + if (status != 0) {
176 + DBG("Reply failure; status %d", status);
177 + gcd->state = STATE_IDLE;
178 + goto error;
179 + }

203 + if (ip_addrs[0] == NULL) {
204 + DBG("Invalid IP address field returned: %s", raw_ip_addrs);
205 + goto error;
206 + }

217 + if (split_ip_addr[0] == NULL) {
218 + ofono_error("Invalid IP address; can't strip prefix: %s",
219 + ip_addrs[0]);
220 + goto error;
221 + }

230 + if (gateways[0] == NULL) {
231 + DBG("Invalid gateways field returned: %s", raw_gws);
232 + goto error;
233 + }

Don't you also need to update error.error here?

Also, in case of error, we're not releasing any of the strings allocated with parcel_r_string (your code just free them in case it all works).

266 + guchar profile[5] = { 0x00 };

Not used.

329 + DBG("Invalid protocol");

Mind doing a better error message here? Otherwise it will be hard to get from the ofono log.

334 + appendPrintBuf("%s %s,%s,%s,%s,%s,%s,%s",
335 + printBuf,
336 + tech,
337 + DATA_PROFILE_DEFAULT,
338 + ctx->apn,
339 + ctx->username,
340 + ctx->password,
341 + CHAP_PAP_OK,
342 + protocol);
343 +
344 + ret = g_ril_send(gcd->ril,
345 + request,
346 + rilp.data,
347 + rilp.size,
348 + ril_setup_data_call_cb, cbd, g_...

91	+static char printBuf[PRINTBUF_SIZE];

574	+static char printBuf[PRINTBUF_SIZE];

910	+static char printBuf[PRINTBUF_SIZE];

2390	+static char printBuf[PRINTBUF_SIZE];

Do we really need a global string? Would avoid calling clear.

1467	+ * $AOSP/hardware/ril/libril/ril.cpp
1468	+ */
1469	+#define startRequest sprintf(printBuf, "(")
1470	+#define closeRequest sprintf(printBuf, "%s)", printBuf)
1471	+#define printRequest(token, req) \
1472	+ ofono_debug("[%04d]> %s %s", token, ril_request_id_to_string(req), printBuf)
1473	+
1474	+#define startResponse sprintf(printBuf, "%s {", printBuf)
1475	+#define closeResponse sprintf(printBuf, "%s}", printBuf)
1476	+#define printResponse ofono_debug("%s", printBuf)
1477	+
1478	+#define clearPrintBuf printBuf[0] = 0
1479	+#define removeLastChar printBuf[strlen(printBuf)-1] = 0
1480	+#define appendPrintBuf(x...) sprintf(printBuf, x)

91	+static char printBuf[PRINTBUF_SIZE];

Coding style here might be an issue when pushing this code upstream. I'd guess it'd be better to use print_buf instead.

114	+ if (message->buf_len < 36) {
115	+ DBG("Parcel is less then minimum DataCallResponseV6 size!");

Missing "decode_ril_error(&error, "FAIL");".

126	+ /* TODO: make conditional */
127	+ appendPrintBuf("[%04d]< %s",
128	+ message->serial_no,
129	+ ril_request_id_to_string(message->req));
130	+ startResponse;
131	+ /* TODO: make conditional */

135	+ /* RIL version */
136	+ version = parcel_r_int32(&rilp);
137	+
138	+ /* Size of call list */
139	+ /* TODO: What if there's more than 1 call in the list?? */
140	+ num = parcel_r_int32(&rilp);

Isn't the reply just a RIL_Data_Call_Response_v6 struct? If so, mind adding a comment saying that the version and num arguments were not described correctly by the Android ril.h file?

175	+ if (status != 0) {
176	+ DBG("Reply failure; status %d", status);
177	+ gcd->state = STATE_IDLE;
178	+ goto error;
179	+ }

203	+ if (ip_addrs[0] == NULL) {
204	+ DBG("Invalid IP address field returned: %s", raw_ip_addrs);
205	+ goto error;
206	+ }

217	+ if (split_ip_addr[0] == NULL) {
218	+ ofono_error("Invalid IP address; can't strip prefix: %s",
219	+ ip_addrs[0]);
220	+ goto error;
221	+ }

230	+ if (gateways[0] == NULL) {
231	+ DBG("Invalid gateways field returned: %s", raw_gws);
232	+ goto error;
233	+ }

Don't you also need to update error.error here?

Also, in case of error, we're not releasing any of the strings allocated with parcel_r_string (your code just free them in case it all works).

266	+ guchar profile[5] = { 0x00 };

Not used.

329	+ DBG("Invalid protocol");

Mind doing a better error message here? Otherwise it will be hard to get from the ofono log.

334	+ appendPrintBuf("%s %s,%s,%s,%s,%s,%s,%s",
335	+ printBuf,
336	+ tech,
337	+ DATA_PROFILE_DEFAULT,
338	+ ctx->apn,
339	+ ctx->username,
340	+ ctx->password,
341	+ CHAP_PAP_OK,
342	+ protocol);
343	+
344	+ ret = g_ril_send(gcd->ril,
345	+ request,
346	+ rilp.data,
347	+ rilp.size,
348	+ ril_setup_data_call_cb, cbd, g_free);
349	+
350	+ /* TODO: make conditional */
351	+ closeRequest;
352	+ printRequest(ret, request);
353	+ /* TODO: make conditional */

Mind containing all the printbuf related function calls together? Easier to read and understand.

375	+ appendPrintBuf("[%04d]< %s",
376	+ message->serial_no,
377	+ ril_request_id_to_string(message->req));

Why just the append without any clear, response or print?

720	+ if (cb)
721	+ cb(&error, status, cbd->data);
722	+ return;
723	+
724	+error:
725	+ if (cb)
726	+ cb(&error, -1, cbd->data);

Could probably simplify by setting up status in case of an error.

lp://qastaging/~awe/phablet-extras/ofono-gprs-support updated on 2013-05-31

59. By Tony Espy on 2013-05-30: [rilmodem] Move DATA_CALL_LIST code from gprs to gprs-context.
60. By Tony Espy on 2013-05-30: [rilmodem] Re-merge from trunk.
61. By Tony Espy on 2013-05-30: [rilmodem] Added code to detect disconnects based on updated DATA_CALL_LIST.
62. By Tony Espy on 2013-05-30: Re-merge from trunk
63. By Tony Espy on 2013-05-31: [rilmodem] Re-factored debug trace code, and some error logic based on review comments.

Revision history for this message

Ricardo Salveti (rsalveti) wrote on 2013-05-31:

30 - [ Tony Espy ]

Don't need to change previous changelog entries.

Rest looks fine, will test it more and happrove it tomorrow.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-05-31:

PASSED: Continuous integration, rev:63
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/21/
Executed test runs:
SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-raring-armhf-ci/21
deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-raring-armhf-ci/21/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/21/rebuild

review: Approve (continuous-integration)

Revision history for this message

Tony Espy (awe) wrote on 2013-05-31:

Fixed the changelog.

I've tested on maguro and mako and data, voice and SMS all work as advertised.

Still little to no mileage on the disconnect code unfortunately.

Also seeing problems on both systems with deactivate-context, including a segfault ( looks like a day-one bug ) after deactivating and then trying to send an SMS from the device.

lp://qastaging/~awe/phablet-extras/ofono-gprs-support updated on 2013-05-31