Merge lp:~thomas-voss/platform-api/retry-loading-of-gps-hal into lp:platform-api

[Thomas Voß]

Fix lp:1573168.

Fix lp:1573168.

[Simon Fels (morphis)]

LGTM

[Simon Fels (morphis)]

review: Approve

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

See inline comment.

review: Needs Information

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Still crashing for me, unfortunately :-/

review: Needs Fixing

[lp:~thomas-voss/platform-api/retry-loading-of-gps-hal]

321. By Thomas Voß on 2016-05-19

Wait for 10 seconds in total, sampling roughly 5 times per second if the HAL is ready, yet.

322. By Thomas Voß on 2016-05-20

Make sure that the gnss_sv_status_callback is not NULL as the HAL implementation does not
check if the callback is NULL.

323. By Thomas Voß on 2016-05-20

Merge with remote branch.

324. By Thomas Voß on 2016-05-20

Fix typo.

325. By Thomas Voß on 2016-05-20

Place callback functions in anonymous namespace.

326. By Thomas Voß on 2016-05-20

Clean up scope of callback function definition.

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM, working without crashing now.

review: Approve

=== modified file 'android/hybris/ubuntu_application_gps_for_hybris.cpp'
--- android/hybris/ubuntu_application_gps_for_hybris.cpp	2016-02-02 09:04:07 +0000
+++ android/hybris/ubuntu_application_gps_for_hybris.cpp	2016-05-20 12:05:12 +0000
@@ -23,6 +23,8 @@
 #include <hardware/gps.h>
 #include <hardware_legacy/power.h>
 
+#include <unistd.h>
+
 #define WAKE_LOCK_NAME  "U_HARDWARE_GPS"
 
 struct UHardwareGps_
@@ -78,78 +80,74 @@
 namespace
 {
 UHardwareGps hybris_gps_instance = NULL;
-}
-
-static void location_callback(GpsLocation* location)
-{
-    if (!hybris_gps_instance)
-        return;
-
-    hybris_gps_instance->location_cb(
-        reinterpret_cast<UHardwareGpsLocation*>(location),
-        hybris_gps_instance->context);
-}
-
-static void status_callback(GpsStatus* status)
-{
-    if (!hybris_gps_instance)
-        return;
-
-    hybris_gps_instance->status_cb(status->status, hybris_gps_instance->context);
-}
-
-static void sv_status_callback(GpsSvStatus* sv_status)
-{
-    if (!hybris_gps_instance)
-        return;
-
-    hybris_gps_instance->sv_status_cb(
+
+namespace cb
+{
+static void location(GpsLocation* location)
+{
+    if (hybris_gps_instance && hybris_gps_instance->location_cb)
+        hybris_gps_instance->location_cb(
+            reinterpret_cast<UHardwareGpsLocation*>(location),
+            hybris_gps_instance->context);
+}
+
+static void status(GpsStatus* status)
+{
+    if (hybris_gps_instance && hybris_gps_instance->status_cb)
+        hybris_gps_instance->status_cb(status->status, hybris_gps_instance->context);
+}
+
+static void sv_status(GpsSvStatus* sv_status)
+{
+    if (hybris_gps_instance && hybris_gps_instance->sv_status_cb)
+        hybris_gps_instance->sv_status_cb(
             reinterpret_cast<UHardwareGpsSvStatus*>(sv_status),
             hybris_gps_instance->context);
 }
 
-static void nmea_callback(GpsUtcTime timestamp, const char* nmea, int length)
-{
-    if (!hybris_gps_instance)
-        return;
-
-    hybris_gps_instance->nmea_cb(timestamp, nmea, length, hybris_gps_instance->context);
-}
-
-static void set_capabilities_callback(uint32_t capabilities)
-{
-    if (!hybris_gps_instance)
-        return;
-
-    hybris_gps_instance->set_capabilities_cb(capabilities, hybris_gps_instance->context);
-}
-
-static void acquire_wakelock_callback()
+#ifdef BOARD_HAS_GNSS_STATUS_CALLBACK
+static void gnss_sv_status(GnssSvStatus*)
+{
+    // Empty on purpose. We do not expose status information about
+    // other satellites networks, yet.
+}
+#endif
+
+static void nmea(GpsUtcTime timestamp, const char* nmea, int length)
+{
+    if (hybris_gps_instance && hybris_gps_instance->nmea_cb)
+        hybris_gps_instance->nmea_cb(timestamp, nmea, length, hybris_gps_instance->context);
+}
+
+static void set_capabilities(uint32_t capabilities)
+{
+    if (hybris_gps_instance && hybris_gps_instance->set_capabilities_cb)
+        hybris_gps_instance->set_capabilities_cb(capabilities, hybris_gps_instance->context);
+}
+
+static void acquire_wakelock()
 {
     acquire_wake_lock(PARTIAL_WAKE_LOCK, WAKE_LOCK_NAME);
 }
 
-static void release_wakelock_callback()
+static void release_wakelock()
 {
     release_wake_lock(WAKE_LOCK_NAME);
 }
 
-static void request_utc_time_callback()
+static void request_utc_time()
 {
-    if (!hybris_gps_instance)
-        return;
-
-    hybris_gps_instance->request_utc_time_cb(hybris_gps_instance->context);
+    if (hybris_gps_instance && hybris_gps_instance->request_utc_time_cb)
+        hybris_gps_instance->request_utc_time_cb(hybris_gps_instance->context);
 }
 
-
 typedef struct 
 {
     void (*func)(void *);
     void *arg;
 } FuncAndArg;
 
-static void * thread_start_wrapper(void* arg)
+static void* thread_start_wrapper(void* arg)
 {
     FuncAndArg *func_and_arg = reinterpret_cast<FuncAndArg*>(arg);
     func_and_arg->func(func_and_arg->arg);
@@ -157,7 +155,7 @@
     return NULL;
 }
 
-static pthread_t create_thread_callback(const char* name, void (*start)(void *), void* arg)
+static pthread_t create_thread(const char* name, void (*start)(void *), void* arg)
 {
     pthread_t thread;
 
@@ -169,92 +167,82 @@
     return thread;
 }
 
-GpsCallbacks gps_callbacks =
+GpsCallbacks gps =
 {
     sizeof(GpsCallbacks),
-    location_callback,
-    status_callback,
-    sv_status_callback,
+    cb::location,
+    cb::status,
+    cb::sv_status,
 #ifdef BOARD_HAS_GNSS_STATUS_CALLBACK
-    NULL,
+    cb::gnss_sv_status,
 #endif
-    nmea_callback,
-    set_capabilities_callback,
-    acquire_wakelock_callback,
-    release_wakelock_callback,
-    create_thread_callback,
-    request_utc_time_callback,
+    cb::nmea,
+    cb::set_capabilities,
+    cb::acquire_wakelock,
+    cb::release_wakelock,
+    cb::create_thread,
+    cb::request_utc_time,
 };
 
-static void xtra_download_request_callback()
+static void xtra_download_request()
 {
-    if (hybris_gps_instance)
+    if (hybris_gps_instance && hybris_gps_instance->xtra_download_request_cb)
         hybris_gps_instance->xtra_download_request_cb(hybris_gps_instance->context);
 }
 
-GpsXtraCallbacks gps_xtra_callbacks =
+GpsXtraCallbacks gps_xtra =
 {
-    xtra_download_request_callback,
-    create_thread_callback,
+    cb::xtra_download_request,
+    cb::create_thread,
 };
 
-static void agps_status_cb(AGpsStatus* agps_status)
+static void agps_status(AGpsStatus* agps_status)
 {
-    if (!hybris_gps_instance)
-        return;
-
-    /*
-    uint32_t ipaddr;
-    // ipaddr field was not included in original AGpsStatus
-    if (agps_status->size >= sizeof(AGpsStatus))
-        ipaddr = agps_status->ipaddr;
-    else
-        ipaddr = 0xFFFFFFFF;
-    */
-
-    hybris_gps_instance->agps_status_cb(
-        reinterpret_cast<UHardwareGpsAGpsStatus*>(agps_status), hybris_gps_instance->context);
+    if (hybris_gps_instance && hybris_gps_instance->agps_status_cb)
+        hybris_gps_instance->agps_status_cb(
+            reinterpret_cast<UHardwareGpsAGpsStatus*>(agps_status), hybris_gps_instance->context);
 }
 
-AGpsCallbacks agps_callbacks =
+AGpsCallbacks agps =
 {
-    agps_status_cb,
-    create_thread_callback,
+    cb::agps_status,
+    cb::create_thread
 };
 
-static void gps_ni_notify_cb(GpsNiNotification *notification)
+static void gps_ni_notify(GpsNiNotification *notification)
 {
-    if (hybris_gps_instance)
+    if (hybris_gps_instance && hybris_gps_instance->gps_ni_notify_cb)
         hybris_gps_instance->gps_ni_notify_cb(
             reinterpret_cast<UHardwareGpsNiNotification*>(notification),
             hybris_gps_instance->context);
 }
 
-GpsNiCallbacks gps_ni_callbacks =
+GpsNiCallbacks gps_ni =
 {
-    gps_ni_notify_cb,
-    create_thread_callback,
+    cb::gps_ni_notify,
+    cb::create_thread,
 };
 
 static void agps_request_set_id(uint32_t flags)
 {
-    if (hybris_gps_instance)
+    if (hybris_gps_instance && hybris_gps_instance->request_setid_cb)
         hybris_gps_instance->request_setid_cb(flags, hybris_gps_instance->context);
 }
 
 static void agps_request_ref_location(uint32_t flags)
 {
-    if (hybris_gps_instance)
+    if (hybris_gps_instance && hybris_gps_instance->request_refloc_cb)
         hybris_gps_instance->request_refloc_cb(flags, hybris_gps_instance->context);
 }
 
-AGpsRilCallbacks agps_ril_callbacks =
+AGpsRilCallbacks agps_ril =
 {
-    agps_request_set_id,
-    agps_request_ref_location,
-    create_thread_callback,
+    cb::agps_request_set_id,
+    cb::agps_request_ref_location,
+    cb::create_thread,
 };
-
+}
+}
 
 UHardwareGps_::UHardwareGps_(UHardwareGpsParams* params)
     : gps_interface(NULL),
@@ -276,57 +264,56 @@
       request_refloc_cb(params->request_refloc_cb),
       context(params->context)
 {
+   
+}
+
+UHardwareGps_::~UHardwareGps_()
+{
+    if (gps_interface)
+        gps_interface->cleanup();
+}
+
+bool UHardwareGps_::init()
+{
     int err;
     hw_module_t* module;
 
     err = hw_get_module(GPS_HARDWARE_MODULE_ID, (hw_module_t const**)&module);
-    if (err == 0)
-    {
-        hw_device_t* device;
-        err = module->methods->open(module, GPS_HARDWARE_MODULE_ID, &device);
-        if (err == 0)
-        {
-            gps_device_t* gps_device = (gps_device_t *)device;
-            gps_interface = gps_device->get_gps_interface(gps_device);
-        }
-    }
-    if (gps_interface)
-    {
-        gps_xtra_interface =
+
+    if (err != 0) return false;
+
+    hw_device_t* device;
+    err = module->methods->open(module, GPS_HARDWARE_MODULE_ID, &device);
+
+    if (err != 0) return false;
+
+    gps_device_t* gps_device = (gps_device_t *)device;
+    gps_interface = gps_device->get_gps_interface(gps_device);
+
+    if (not gps_interface) return false;
+    if (gps_interface->init(&cb::gps) != 0) return false;
+    
+    gps_xtra_interface =
             (const GpsXtraInterface*)gps_interface->get_extension(GPS_XTRA_INTERFACE);
-        agps_interface =
+    agps_interface =
             (const AGpsInterface*)gps_interface->get_extension(AGPS_INTERFACE);
-        gps_ni_interface =
+    gps_ni_interface =
             (const GpsNiInterface*)gps_interface->get_extension(GPS_NI_INTERFACE);
-        gps_debug_interface =
+    gps_debug_interface =
             (const GpsDebugInterface*)gps_interface->get_extension(GPS_DEBUG_INTERFACE);
-        agps_ril_interface =
+    agps_ril_interface =
             (const AGpsRilInterface*)gps_interface->get_extension(AGPS_RIL_INTERFACE);
-    }
-}
-
-UHardwareGps_::~UHardwareGps_()
-{
-    if (gps_interface)
-        gps_interface->cleanup();
-}
-
-bool UHardwareGps_::init()
-{
-    // fail if the main interface fails to initialize
-    if (!gps_interface || gps_interface->init(&gps_callbacks) != 0)
-        return false;
-
+    
     // if XTRA initialization fails we will disable it by gps_Xtra_interface to null,
     // but continue to allow the rest of the GPS interface to work.
-    if (gps_xtra_interface && gps_xtra_interface->init(&gps_xtra_callbacks) != 0)
+    if (gps_xtra_interface && gps_xtra_interface->init(&cb::gps_xtra) != 0)
         gps_xtra_interface = NULL;
     if (agps_interface)
-        agps_interface->init(&agps_callbacks);
+        agps_interface->init(&cb::agps);
     if (gps_ni_interface)
-        gps_ni_interface->init(&gps_ni_callbacks);
+        gps_ni_interface->init(&cb::gps_ni);
     if (agps_ril_interface)
-        agps_ril_interface->init(&agps_ril_callbacks);
+        agps_ril_interface->init(&cb::agps_ril);
 
     return true;
 }
@@ -430,13 +417,19 @@
     UHardwareGps u_hardware_gps = new UHardwareGps_(params);
     hybris_gps_instance = u_hardware_gps;
 
-    if (!u_hardware_gps->init())
-    {
-        delete u_hardware_gps;
-        u_hardware_gps = NULL;
-    }
+    // Try ten times to initialize the GPS HAL interface,
+    // sleeping for 200ms per iteration in case of issues.
+    for (unsigned int i = 0; i < 50; i++)
+        if (u_hardware_gps->init())
+            return hybris_gps_instance = u_hardware_gps;
+        else
+            // Sleep for some time and leave some time for the system
+            // to finish initialization.
+            ::usleep(200 * 1000);
 
-    return u_hardware_gps;
+    // This is the error case, as we did not succeed in initializing the GPS interface.
+    delete u_hardware_gps;
+    return hybris_gps_instance;
 }
 
 void u_hardware_gps_delete(UHardwareGps handle)