Make chrome communicate with gpsd through libgps/shared memory

content/browser/geolocation/libgps_wrapper_linux.cc

Index: content/browser/geolocation/libgps_wrapper_linux.cc
diff --git a/content/browser/geolocation/libgps_wrapper_linux.cc b/content/browser/geolocation/libgps_wrapper_linux.cc
index b0b231543825cc920ba38e036cd52b9c71ab86dd..73416508a2fcaa597b841b937e619fa55b825480 100644
--- a/content/browser/geolocation/libgps_wrapper_linux.cc
+++ b/content/browser/geolocation/libgps_wrapper_linux.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/geolocation/libgps_wrapper_linux.h"
 
+#include <math.h>
 #include <dlfcn.h>
 #include <errno.h>
 
@@ -11,25 +12,39 @@
 #include "base/stringprintf.h"
 #include "content/common/geoposition.h"
 
-namespace {
-// Attempts to load dynamic library named |lib| and initialize the required
-// function pointers according to |mode|. Returns ownership a new instance
-// of the library loader class, or NULL on failure.
-// TODO(joth): This is a hang-over from when we dynamically two different
-// versions of libgps and chose between them. Now we could remove at least one
-// layer of wrapper class and directly #include gps.h in this cc file.
-// See http://crbug.com/98132 and http://crbug.com/99177
-LibGpsLibraryWrapper* TryToOpen(const char* lib) {
-  void* dl_handle = dlopen(lib, RTLD_LAZY);
+LibGps::LibGps(void* dl_handle,
+               gps_open_fn gps_open,
+               gps_close_fn gps_close,
+               gps_read_fn gps_read)
+    : dl_handle_(dl_handle),
+      gps_open_(gps_open),
+      gps_close_(gps_close),
+      gps_read_(gps_read),
+      is_open_(false) {
+  DCHECK(gps_open_);
+  DCHECK(gps_close_);
+  DCHECK(gps_read_);
+}
+
+LibGps::~LibGps() {
+  Stop();
+  if (dl_handle_) {
+    const int err = dlclose(dl_handle_);
+    CHECK_EQ(0, err) << "Error closing dl handle: " << err;

[joth, 2011/11/11 18:40:40]

nit: use DCHECK_EQ

[Yufeng Shen (Slow to review), 2011/11/11 21:11:35]

Done.

+  }
+}
+
+LibGps* LibGps::New() {
+  void* dl_handle = dlopen(LIBGPS_VERSION, RTLD_LAZY);
   if (!dl_handle) {
-    VLOG(1) << "Could not open " << lib << ": " << dlerror();
+    LOG(WARNING) << "Could not open " << LIBGPS_VERSION << ": " << dlerror();

[joth, 2011/11/11 18:40:40]

this is only log warning for Chrome OS.

DLOG(WARNING) might actually make a good compromise here.

[Yufeng Shen (Slow to review), 2011/11/11 21:11:35]

Done.

     return NULL;
   }
-  VLOG(1) << "Loaded " << lib;
+
+  LOG(INFO) << "Loaded " << LIBGPS_VERSION;

[joth, 2011/11/11 18:40:40]

again DLOG (or DVLOG).

chrome is trying to discourage production logging for 'normal' flows

[Yufeng Shen (Slow to review), 2011/11/11 21:11:35]

Done.

 
   #define DECLARE_FN_POINTER(function)                                   \
-    LibGpsLibraryWrapper::function##_fn function;                        \
-    function = reinterpret_cast<LibGpsLibraryWrapper::function##_fn>(    \
+    function##_fn function = reinterpret_cast<function##_fn>(            \
         dlsym(dl_handle, #function));                                    \
     if (!function) {                                                     \
       LOG(WARNING) << "libgps " << #function << " error: " << dlerror(); \

[joth, 2011/11/11 18:40:40]

ditto, and throughout the file

[Yufeng Shen (Slow to review), 2011/11/11 21:11:35]

Done.

@@ -38,87 +53,57 @@ LibGpsLibraryWrapper* TryToOpen(const char* lib) {
     }
   DECLARE_FN_POINTER(gps_open);
   DECLARE_FN_POINTER(gps_close);
-  DECLARE_FN_POINTER(gps_poll);
-  DECLARE_FN_POINTER(gps_stream);
-  DECLARE_FN_POINTER(gps_waiting);
+  DECLARE_FN_POINTER(gps_read);
+  // We don't use gps_shm_read() directly, just to make sure that libgps has
+  // the shared memory support.
+  typedef int (*gps_shm_read_fn)(struct gps_data_t*);
+  DECLARE_FN_POINTER(gps_shm_read);
   #undef DECLARE_FN_POINTER
 
-  return new LibGpsLibraryWrapper(dl_handle,
-                                  gps_open,
-                                  gps_close,
-                                  gps_poll,
-                                  gps_stream,
-                                  gps_waiting);
-}
-} // namespace
-
-LibGps::LibGps(LibGpsLibraryWrapper* dl_wrapper)
-    : library_(dl_wrapper) {
-  DCHECK(dl_wrapper != NULL);
-}
-
-LibGps::~LibGps() {
-}
-
-LibGps* LibGps::New() {
-  LibGpsLibraryWrapper* wrapper;
-  wrapper = TryToOpen("libgps.so.19");
-  if (wrapper)
-    return NewV294(wrapper);
-  wrapper = TryToOpen("libgps.so");
-  if (wrapper)
-    return NewV294(wrapper);
-  return NULL;
+  return new LibGps(dl_handle, gps_open, gps_close, gps_read);
 }
 
 bool LibGps::Start() {
-  if (library().is_open())
+  if (is_open_)
     return true;
+
   errno = 0;
-  static int fail_count = 0;
-  if (!library().open(NULL, NULL)) {
+  if (gps_open_(GPSD_SHARED_MEMORY, 0, &gps_data_) != 0) {

[joth, 2011/11/11 18:40:40]

this is still applied for desktop linux?

I'm still not convinced about using shared mem only on desktop linux. it could
easily break and cause chrome to crash if user has an otherwise benignly
mis-configured system (i.e. 2 versions of libgps.so installed, and the 'wrong'
version of gpsd is running).
at least with the socket interface we'd just get json parse error at worse, not
a crash.

options for the linux build:
- disable it all
- set to "localhost"
- get it from a command-line switch (defaulting to one of the above)

[Yufeng Shen (Slow to review), 2011/11/11 21:11:35]

Drop the desktop linux for now

On 2011/11/11 18:40:40, joth wrote:

     // See gps.h NL_NOxxx for definition of gps_open() error numbers.
-    LOG_IF(WARNING, 0 == fail_count++) << "gps_open() failed: " << errno;
-    return false;
-  }
-  fail_count = 0;
-  if (!StartStreaming()) {
-    VLOG(1) << "StartStreaming failed";
-    library().close();
+    LOG(WARNING) << "gps_open() failed " << errno;
     return false;
   }
+
+  is_open_ = true;
   return true;
 }
-
 void LibGps::Stop() {
-  library().close();
+  if (is_open_)
+    gps_close_(&gps_data_);
+  is_open_ = false;
 }
 
-bool LibGps::Poll() {
-  last_error_ = "no data received from gpsd";
-  while (DataWaiting()) {
-    int error = library().poll();
-    if (error) {
-      last_error_ = base::StringPrintf("poll() returned %d", error);
-      Stop();
+bool LibGps::Read(Geoposition* position) {
+  DCHECK(position);
+  position->error_code = Geoposition::ERROR_CODE_POSITION_UNAVAILABLE;
+  if (!is_open_) {
+      LOG(WARNING) << "No gpsd connection";
+      position->error_message = "No gpsd connection";
       return false;
-    }
-    last_error_.clear();
   }
-  return last_error_.empty();
-}
 
-bool LibGps::GetPosition(Geoposition* position) {
-  DCHECK(position);
-  position->error_code = Geoposition::ERROR_CODE_POSITION_UNAVAILABLE;
-  if (!library().is_open()) {
-    position->error_message = "No gpsd connection";
-    return false;
+  if (gps_read_(&gps_data_) < 0) {
+      LOG(WARNING) << "gps_read() fails";
+      position->error_message = "gps_read() fails";
+      return false;
   }
+
   if (!GetPositionIfFixed(position)) {
-    position->error_message = last_error_;
-    return false;
+      LOG(WARNING) << "No fixed position";
+      position->error_message = "No fixed position";
+      return false;
   }
+
   position->error_code = Geoposition::ERROR_CODE_NONE;
   position->timestamp = base::Time::Now();
   if (!position->IsValidFix()) {
@@ -135,67 +120,41 @@ bool LibGps::GetPosition(Geoposition* position) {
   return true;
 }
 
-LibGpsLibraryWrapper::LibGpsLibraryWrapper(void* dl_handle,
-                                           gps_open_fn gps_open,
-                                           gps_close_fn gps_close,
-                                           gps_poll_fn gps_poll,
-                                           gps_stream_fn gps_stream,
-                                           gps_waiting_fn gps_waiting)
-    : dl_handle_(dl_handle),
-      gps_open_(gps_open),
-      gps_close_(gps_close),
-      gps_poll_(gps_poll),
-      gps_stream_(gps_stream),
-      gps_waiting_(gps_waiting),
-      gps_data_(NULL) {
-}
-
-LibGpsLibraryWrapper::~LibGpsLibraryWrapper() {
-  close();
-  if (dl_handle_) {
-    const int err = dlclose(dl_handle_);
-    CHECK_EQ(0, err) << "Error closing dl handle: " << err;
+bool LibGps::GetPositionIfFixed(Geoposition* position) {
+  DCHECK(position);
+  if (gps_data_.status == STATUS_NO_FIX) {
+    DLOG(WARNING) << "Status_NO_FIX";

[joth, 2011/11/11 18:40:40]

this looks like it will be very noisy. DVLOG(1) or even 2.

(are you familiar with using --v-module to enable verbose logging?)

[Yufeng Shen (Slow to review), 2011/11/11 21:11:35]

done

trying to get myself familiar with that

On 2011/11/11 18:40:40, joth wrote:

+    return false;
   }
-}
-
-bool LibGpsLibraryWrapper::open(const char* host, const char* port) {
-  DCHECK(!gps_data_) << "libgps already opened";
-  DCHECK(gps_open_);
-  gps_data_ = gps_open_(host, port);
-  return is_open();
-}
 
-void LibGpsLibraryWrapper::close() {
-  if (is_open()) {
-    DCHECK(gps_close_);
-    gps_close_(gps_data_);
-    gps_data_ = NULL;
+  if (isnan(gps_data_.fix.latitude) || isnan(gps_data_.fix.longitude)) {
+    DLOG(WARNING) << "No valid lat/lon value";
+    return false;
   }
-}
-
-int LibGpsLibraryWrapper::poll() {
-  DCHECK(is_open());
-  DCHECK(gps_poll_);
-  return gps_poll_(gps_data_);
-}
-
-int LibGpsLibraryWrapper::stream(int flags) {
-  DCHECK(is_open());
-  DCHECK(gps_stream_);
-  return gps_stream_(gps_data_, flags, NULL);
-}
 
-bool LibGpsLibraryWrapper::waiting() {
-  DCHECK(is_open());
-  DCHECK(gps_waiting_);
-  return gps_waiting_(gps_data_);
-}
+  position->latitude = gps_data_.fix.latitude;
+  position->longitude = gps_data_.fix.longitude;
+
+  if (!isnan(gps_data_.fix.epx) && !isnan(gps_data_.fix.epy)) {

[joth, 2011/11/11 18:40:40]

nit: i guess either epx or epy would probably do if the other were nan, but I
doubt that's a practical problem? if so no need to handle that

[Yufeng Shen (Slow to review), 2011/11/11 21:11:35]

Done.

+    position->accuracy = std::max(gps_data_.fix.epx, gps_data_.fix.epy);
+  } else {
+    // TODO(joth): Fixme. This is a workaround for http://crbug.com/99326
+    // (miletus) : With NMEA protocol, only sentences GPGBS (in NMEA 3.0) and
+    // PGRME (Garmin specific) send out accuracy data fix.epx and fix.epy.
+    // So expecting that accuracy is not valid most of the time.

[joth, 2011/11/11 18:40:40]

maybe this comment should go onto the bug report. I'm not quite clear, does this
mean it's expected that accuracy will come and go from one second to the next?
if so we should code around it e.g. remember the accuracy when we get one, and
inject that in subsequent polls

[Yufeng Shen (Slow to review), 2011/11/11 21:11:35]

It means with most devices we won't get fix.epx and fix.epy
information. I updated http://crbug.com/99326   with the details.

On 2011/11/11 18:40:40, joth wrote:

+    DLOG(WARNING) << "libgps reported accuracy NaN, forcing to zero";
+    position->accuracy = 0;
+  }
 
-const gps_data_t& LibGpsLibraryWrapper::data() const {
-  DCHECK(is_open());
-  return *gps_data_;
-}
+  if (gps_data_.fix.mode == MODE_3D && !isnan(gps_data_.fix.altitude)) {
+    position->altitude = gps_data_.fix.altitude;
+    if (!isnan(gps_data_.fix.epv))
+      position->altitude_accuracy = gps_data_.fix.epv;
+  }
 
-bool LibGpsLibraryWrapper::is_open() const {
-  return gps_data_ != NULL;
+  if (!isnan(gps_data_.fix.track))
+    position->heading = gps_data_.fix.track;
+  if (!isnan(gps_data_.fix.speed))
+    position->speed = gps_data_.fix.speed;
+  return true;
 }