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

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

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

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

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

Please fix debian/changelog...

At the very least, are you keeping track of the precise patches applied? Is it mostly concentrated into some file additions and few actual code changes?

Since this has been all done flatly into an ofono branch, it's going to be very difficult/painful to get the changes out and into patches unless someone has been keeping track of what aspects have been changed.

@Mathieu

I fixed the changelog, thanks for the catch!

As for patches:

1. This work is being done on a fresh, flattened version of the raring ofono branch ( itself based on ofono 1.12 ).

2. As oFono hasn't really been used in a default Ubuntu build to-date, tracking Precise patches has not been a priority.

3. Please review the telephony blueprint, as it has a work item for splitting out the oFono/RILD plugin code into a separate project, which will make it easier to deal with any required patches to the oFono core. For the sake of agility, I purposely chose to use a flattened bzr tree, as trying to do heavy development using patches is royal PITA. bzr tracks the changes made on top of the flattened source tree, so getting changes out into patches shouldn't be a problem if/when done.

4. We also will need a plan to deal with the fact that an oFono install on a desktop/laptop may be subtly different than a build on a phone. Again, this is a post Raring work item.

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

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

273 + cb(&error, -1, -1, -1, NULL, EF_STATUS_INVALIDATED, cbd->data);
274 + return;

292 + cb(&error, -1, -1, -1, NULL, EF_STATUS_INVALIDATED, cbd->data);
293 + return;

Would prefer if both could use 'goto error' as well. The extra debug message can be printed right after the response result (and you can handle the free at the goto as well).

299 + if (response[0] == 0x62) {
300 + ok = sim_parse_3g_get_response(response, strlen((gchar *) response), &flen, &rlen,
301 + &str, access, NULL);
302 +
303 + file_status = EF_STATUS_VALID;
304 + } else
305 + ok = sim_parse_2g_get_response(response, strlen((gchar *) response), &flen, &rlen,
306 + &str, access, &file_status);
307 +
308 + g_free(response);
309 + }

Mind breaking this down in less columns? 80 should be a good target.

386 + decode_ril_error(&error, "FAIL");
387 + cb(&error, NULL, 0, cbd->data);

396 + decode_ril_error(&error, "FAIL");
397 + cb(&error, NULL, 0, cbd->data);

Could be handled in a single goto.

537 + DBG("g_ril_send failed...");

Mind removing this entry? Not generally used at the other calls, and the log will already point a failure when sending the command anyway.

When testing I also got quite a few GENERIC_FAILURE, not sure if they are critical though:

root@localhost:~# ofonod -d -n
ofonod[1023]: oFono version 1.12
ofonod[1023]: src/plugin.c:__ofono_plugin_init()
ofonod[1023]: plugins/push-notification.c:push_notification_init()
ofonod[1023]: plugins/smart-messaging.c:smart_messaging_init()
ofonod[1023]: src/cdma-provision.c:ofono_cdma_provision_driver_register() driver: 0xa76b0 name: CDMA provisioning
ofonod[1023]: src/gprs-provision.c:ofono_gprs_provision_driver_register() driver: 0xa7684 name: Provisioning
ofonod[1023]: src/cdma-voicecall.c:ofono_cdma_voicecall_driver_register() driver: 0xa762c, name: cdmamodem
ofonod[1023]: src/modem.c:ofono_devinfo_driver_register() driver: 0xa7654, name: cdmamodem
ofonod[1023]: src/cdma-connman.c:ofono_cdma_connman_driver_register() driver: 0xa7670, name: cdmamodem
ofonod[1023]: src/modem.c:ofono_modem_driver_register() driver: 0xa7594, name: phonesim
ofonod[1023]: src/modem.c:ofono_modem_driver_register() driver: 0xa75c4, name: localhfp
ofonod[1023]: src/gprs.c:ofono_gprs_context_driver_register() driver: 0xa757c, name: phonesim
ofonod[1023]: src/ctm.c:ofono_ctm_driver_register() driver: 0xa7568, name: phonesim
ofonod[1023]: plugins/phonesim.c:parse_config() filename /etc/ofono/phonesim.conf
ofonod[1023]: src/voicecall.c:ofono_voicecall_driver_register() driver: 0xa7484, name: hfpmodem
ofonod[1023]: src/modem.c:ofono_devinfo_driver_register() driver: 0xa7528, name: hfpmodem
ofonod[1023]: src/network.c:ofono_netreg_driver_register() driver: 0xa74dc, name: hfpmodem
ofonod[1023]: src/call-volume.c:ofono_call_volume_driver_register() driver: 0xa7510, name: hfpmodem
ofonod[1023]: src/handsfree.c:ofono_handsfree_driver_register() driver: 0xa7554, name: hfpmodem
ofonod[1023]: src/voicecall.c:ofono_voicecall_driver_register() driver: 0xa72e0, name: atmodem
ofonod[1023]: src/modem.c:ofono_devinfo_driver_register() driver: 0xa7378, name: atmodem
ofonod[1023]: src/call-barring.c:ofono_call_barring_driver_register() driver: 0xa7330, name...

Using raring image from http://cdimage.ubuntu.com/ubuntu-touch-preview/daily-preinstalled/20130422/ on Mako. Didn't yet test on maguro.

Cleaned up error handling/extraneous comments per review.

Also, turns out the SIM IO response is returned from RILD as a Parcel-based ASCII hexadecimal string which needs to be converted to a byte string ( eg. "00AF" --> 0x00AF ).

Re-tested on latest Raring build on maguro. Incoming/Outgoing phone calls and SMS messages worked as expected.

No testing on mako, due to that fact that I don't have a valid microSIM. I'm going to get my ATT SIM cutdown this afternoon.

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

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

Looks like we have our first instance of device-specific RIL behavior. After testing the latest Raring build on my Nexus4, I'm seeing failures reading the IMSI ( Subscriber Identity ) and SIM IO calls.

Turns out the CM 10.1 source tree includes a QualcommSharedRIL.java class which looks like it overrides the IMSI and SIM IO logic. We'll need to discuss how we implement a similar mechanism.

I would hold off on further review until we can discuss further.

Turns out that the mako RILD is Qualcomm-based, whereas maguro is Samsung-based.

The Qualcomm RIL code is sensitive to file paths ( ie. directory specifier ) that need to be specified in addition to the file ID. The Samsung RIL code doesn't really care and treats file paths as optional.

When the Raring oFono merge was done, I removed code that I'd written to handle paths and had made an incorrect assumption that since file path was added as a parameter to all of the file IO operations in the sim_driver, that the oFono core would correctly specify paths for each of it's file operations. This wasn't the case.

So, the latest code now uses the oFono ef_db to look up file paths ( which then have to be converted to ASCII hex ).

I also added support to grab the UMTS/GSM application ID (AID) and app type ( should always be USIM ) from the the SIM status reply. AID is another optional parameter for SIM file IO, so if correctly read from the SIM, it will be passed to all file IO calls.

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

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

Just some minor comments.

170 + /* Minimum length of SIM_IO_v6 is 12:

Here the struct parsed is actually RIL_SIM_IO_Response instead of SIM_IO_v6.

269 + if (aid_str && strlen(aid_str)) {

Why the extra check with strlen here? Parcel will return null in case it's invalid.

770 + if (app.app_id && strlen(app.app_id))
771 + sd->app_id = app.app_id;

Same here.

+ } else if (fileid == 0x2FE2 || fileid == 0x2FE0) {

Shouldn't 0x2FE2 be SIM_EF_ICCID_FILEID? It'd also be good to have a const for 0x2FE0.

474 + guchar file_status = 0x01;

If initialized with EF_STATUS_VALID (would be nice to replace it with the defined const), you don't need to set it to valid later (line 517).

Rest of the code looks good, we just need to improve the parcel handling and the power-on conditions (for emergency call, as you added as TODO).

Tested with mako an maguro using latest raring-based build. Old functionality still working as expected.

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

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