Add a Geoclue source that may be able to receive NMEA GGA sentences over a network. This is needed for GSoC 2015 project. https://wiki.gnome.org/Outreach/SummerOfCode/2015/Projects/Ankit_GPSSharing
Created attachment 116488 [details] [review] Add NMEA_SOURCE_PROTOCOL NMEA_SOURCE_PROTOCOL contains protocol that GClueNMEASource will use to receive GGA sentences from the location server.
Created attachment 116489 [details] [review] Create gps-utilites This patch creates gps-utilites module. gps-utilities contains functions for parsing NMEA sentences. These functions are moved from GClueModemGPS.
Created attachment 116490 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from the NMEA source on the network. This will help us to receive location info from different devices.
Created attachment 116491 [details] Dummy NMEA location server
Comment on attachment 116488 [details] [review] Add NMEA_SOURCE_PROTOCOL Review of attachment 116488 [details] [review]: ----------------------------------------------------------------- Good work documenting it. ::: NMEA_SOURCE_PROTOCOL @@ +1,1 @@ > +GCLUE NMEA SOURCE PROTOCOL This would be the generic protocol for sharing of NMEA information and I think this document should describe the protocol in a generic way so it's readable to not only geoclue developers but anyone wanting to write a server app (like the one you are going to write for android). As such you want to name it appropriately too. Having said that, it would be helpful to add geoclue-internal/specific notes at the end describing what different modules do. @@ +1,5 @@ > +GCLUE NMEA SOURCE PROTOCOL > +========================== > + > +GClueNMEASource uses GGA sentences for the transfer of location data from a > +location source to Geoclue. This sentence makes it sound like GClueNMEASource sends data (server). @@ +3,5 @@ > + > +GClueNMEASource uses GGA sentences for the transfer of location data from a > +location source to Geoclue. > + > +Following protocol is followed while data transfer: "Following protocol is followed" sounds really weird. How about simply "Here is how exchange of NMEA data work:"? @@ +6,5 @@ > + > +Following protocol is followed while data transfer: > + > +* Geoclue connects to the location server. > +* Location server then replies with the length of GGA sentence in unsigned It's not a reply since there was no request, just an incoming connection. @@ +7,5 @@ > +Following protocol is followed while data transfer: > + > +* Geoclue connects to the location server. > +* Location server then replies with the length of GGA sentence in unsigned > + 32-bit integer format. you probably want to mention that its in network byte order. @@ +8,5 @@ > + > +* Geoclue connects to the location server. > +* Location server then replies with the length of GGA sentence in unsigned > + 32-bit integer format. > +* After the size of GGA sentence is sent, the server sends GGA sentence itself. Is it guaranteed that server always sends 1 sentence at a time? Seems to be implied here.
Comment on attachment 116489 [details] [review] Create gps-utilites Review of attachment 116489 [details] [review]: ----------------------------------------------------------------- * Creation of the module isn't the important bit but moving of NMEA utility functions so more approprite short log would be: Move NMEA parsing API to own module. Similarly for description. * I'd call it nmea-utils instead. * Please always leave empty lines between paragraphs (commit description is supposed to be mostly normal English, following grammar rules). ::: src/gps-utilities.h @@ +31,5 @@ > +gdouble parse_coordinate_string (const char *coordinate, > + const char *direction); > + > +gdouble parse_altitude_string (const char *altitude, > + const char *unit); * I'll add '_nmea' after 'parse_' in the names of the last two. * Do we need to move all three of these? How about we refactor the code to transform nmea to a location instance, into a separate function and then only move that?
Comment on attachment 116490 [details] [review] Add GClueNMEASource Review of attachment 116490 [details] [review]: ----------------------------------------------------------------- Pretty good as your first module! * the -> a. * help -> enable * different devices -> other devices on the local network with location capabilities, such as smartphones. ::: configure.ac @@ +94,5 @@ > fi > AM_CONDITIONAL([BUILD_MODEM_GPS_SOURCE], [test "$build_modem_gps_source" = "yes"]) > > +# NMEA source > +AC_DEFINE([GCLUE_USE_NMEA_SOURCE], [1], [Build NMEA source?]) This isn't very useful with accompanying AM_CONDITIONAL and configure flag. ::: src/Makefile.am @@ +94,5 @@ > gclue-mozilla.c \ > gclue-location.h \ > gclue-location.c \ > + gclue-nmea-source.h \ > + gclue-nmea-source.c \ You need to handle this similar to other conditionally built source, e.g gclue-modem-gps.c ::: src/gclue-nmea-source.c @@ +54,5 @@ > + > + /* FIXME: I've not yet checked for the NMEA source availability > + * It'd be possible to check once I start working with Avahi. > + * For now it is setting accuracy level as EXACT > + */ The comment doesn't say how accuracy level is connected to avahi. How about "FIXME: Accuracy level is currently hardcoded. Accuracy level will be provided out of band through Avahi to avoid having to connect to server for only fetching this peace of information." @@ +90,5 @@ > + g_clear_error (&error); > + } > + return; > + } > + g_print ("Message was: \"%s\"\n", source->priv->buffer); Please remove (or move to another patch) things only meant for your eyes/use only. Having said that, this one might be better to retain if changed to g_debug and a lightly descriptive sentence than "Message was..". Remember to remove '\n' since g_debug/g_warning/g_message have implicit \n. @@ +116,5 @@ > + accuracy = get_accuracy_from_hdop (hdop); > + > + location = gclue_location_new (latitude, longitude, accuracy); > + if (altitude != GEOCODE_LOCATION_ALTITUDE_UNKNOWN) > + g_object_set (location, "altitude", altitude, NULL); If you act on my suggestion from previous comment about refactoring this code, you'll save a lot of code duplication.. @@ +135,5 @@ > + g_debug ("%s", error->message); > + return; > + } > + > + g_print ("%d",data_size); Please remove. @@ +221,5 @@ > + gpointer user_data) > +{ > + GInputStream * istream = G_INPUT_STREAM (object); > + GClueNMEASource * source = GCLUE_NMEA_SOURCE (user_data); > + GError * error = NULL; Please keep an empty line after variable declarations. @@ +246,5 @@ > + > + g_clear_object (&priv->connection); > + g_clear_object (&priv->client); > + g_free (priv->buffer); > + g_free (priv->cancellable); cancellable is a gobject, you want to use g_clear_object() on it. @@ +330,5 @@ > + if (!base_class->start (source)) > + return FALSE; > + > + priv->client = g_socket_client_new (); > + /* FIXME: This would change as soon as we incorporate Avahi */ What would change? you want to place this comment above (or next to) host_and_port argument below. Also a more helpful/descriptive comment would be: FIXME: Don't hardcode hostname; Use Avahi for autodiscovery. @@ +333,5 @@ > + priv->client = g_socket_client_new (); > + /* FIXME: This would change as soon as we incorporate Avahi */ > + g_socket_client_connect_to_host_async > + (priv->client, > + (gchar*)"localhost", No need for the cast here. @@ +358,5 @@ > + > + istream = g_io_stream_get_input_stream > + (G_IO_STREAM (priv->connection)); > + > + g_cancellable_cancel(priv->cancellable); Coding-style nitpick: Space before '('.
Comment on attachment 116488 [details] [review] Add NMEA_SOURCE_PROTOCOL Review of attachment 116488 [details] [review]: ----------------------------------------------------------------- ::: NMEA_SOURCE_PROTOCOL @@ +8,5 @@ > + > +* Geoclue connects to the location server. > +* Location server then replies with the length of GGA sentence in unsigned > + 32-bit integer format. > +* After the size of GGA sentence is sent, the server sends GGA sentence itself. wouldn't it?
Comment on attachment 116489 [details] [review] Create gps-utilites Review of attachment 116489 [details] [review]: ----------------------------------------------------------------- ::: src/gps-utilities.h @@ +31,5 @@ > +gdouble parse_coordinate_string (const char *coordinate, > + const char *direction); > + > +gdouble parse_altitude_string (const char *altitude, > + const char *unit); How about keeping these three functions in this file and using it for creating a function called gclue_location_from_gga() instead?
Comment on attachment 116488 [details] [review] Add NMEA_SOURCE_PROTOCOL Review of attachment 116488 [details] [review]: ----------------------------------------------------------------- ::: NMEA_SOURCE_PROTOCOL @@ +8,5 @@ > + > +* Geoclue connects to the location server. > +* Location server then replies with the length of GGA sentence in unsigned > + 32-bit integer format. > +* After the size of GGA sentence is sent, the server sends GGA sentence itself. If server has more than 1 than it should send them all at once. Also I think we discussed that server should send all NMEA sentences, not just GGA.
Comment on attachment 116489 [details] [review] Create gps-utilites Review of attachment 116489 [details] [review]: ----------------------------------------------------------------- ::: src/gps-utilities.h @@ +31,5 @@ > +gdouble parse_coordinate_string (const char *coordinate, > + const char *direction); > + > +gdouble parse_altitude_string (const char *altitude, > + const char *unit); You mean gclue_location_new_from_gga(), yeah that sounds good but then we don't need this file at all but instead we can just move the whole code to GClueLocation class.
Created attachment 116634 [details] [review] Add GPS_SHARING_PROTOCOL GPS_SHARING_PROTOCOL lays down specifications for the developers who may be interested in creating applications for sharing location from a GPS device to Geoclue.
Created attachment 116635 [details] [review] Create gclue_location_new_from_gga This patch creates gclue_location_new_from_gga function inside GClueLocation which will be able to construct a GClueLocation object out of a NMEA GGA sentence.
Created attachment 116636 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Created attachment 116637 [details] Dummy NMEA location server
Comment on attachment 116634 [details] [review] Add GPS_SHARING_PROTOCOL Review of attachment 116634 [details] [review]: ----------------------------------------------------------------- ::: GPS_SHARING_PROTOCOL @@ +7,5 @@ > +Service specifications: > + > +* The service must be on the same network as that of Geoclue. > +* Server socket must be used for location service. > +* It must listen at port number 60793 Why? This shouldn't be required if the service is advertised through mDNS. @@ +10,5 @@ > +* Server socket must be used for location service. > +* It must listen at port number 60793 > +* It must use NMEA GGA[1] sentences for the exchange of location data. > +* The service must send length of GGA sentence followed by the GGA sentence > + if new location is available and Geoclue is connected with the server. Again, why is this needed? We probably want to avoid creating a specific protocol and the existing "NMEA to TCP" services should also work.
Comment on attachment 116635 [details] [review] Create gclue_location_new_from_gga Review of attachment 116635 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +285,5 @@ > + return g_ascii_strtod (altitude, NULL); > +} > + > +/** > + * gclue_location_new_from_gga: You should do the parsing in the same place the data is read from the network, as it's highly specialised. If you want to share the parser, this isn't the right place.
Comment on attachment 116636 [details] [review] Add GClueNMEASource Review of attachment 116636 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +49,5 @@ > + > +static void > +refresh_accuracy_level (GClueNMEASource *source) > +{ > + Spurious linefeed here. @@ +86,5 @@ > + g_clear_error (&error); > + } > + return; > + } > + g_message ("Network source sent: \"%s\"", source->priv->buffer); g_debug() is better here. @@ +135,5 @@ > + result, > + &error); > + > + if (error != NULL) { > + g_warning ("%s", error->message); You need to free the error. I would print it as debug as well, as it can happen during normal operation, such as the service becoming unavailable. @@ +179,5 @@ > + g_io_stream_close_finish (iostream, > + result, > + &error); > + if (error != NULL) { > + g_warning ("%s", error->message); Leak and g_debug(). @@ +198,5 @@ > + g_input_stream_close_finish (istream, > + result, > + &error); > + if (error != NULL) { > + g_warning ("%s", error->message); Ditto. @@ +255,5 @@ > +static void > +on_nmea_source_destroyed (gpointer data, > + GObject *where_the_object_was) > +{ > + GClueNMEASource **source = (GClueNMEASource **) data; g_object_add_weak_pointer instead? @@ +299,5 @@ > + return FALSE; > + > + priv->client = g_socket_client_new (); > + > + /* FIXME: Don't hardcode hostname; Use Avahi for auto-discovery. */ Right, and you can get the port from the same place.
Comment on attachment 116635 [details] [review] Create gclue_location_new_from_gga Review of attachment 116635 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +285,5 @@ > + return g_ascii_strtod (altitude, NULL); > +} > + > +/** > + * gclue_location_new_from_gga: Should I create a new class to share parsing functions? It would also be great for further expansion. How does GClueNMEAParser sound? I would also create a method which will tell us the type of NMEA sentence.
Comment on attachment 116636 [details] [review] Add GClueNMEASource Review of attachment 116636 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +255,5 @@ > +static void > +on_nmea_source_destroyed (gpointer data, > + GObject *where_the_object_was) > +{ > + GClueNMEASource **source = (GClueNMEASource **) data; So I should use g_object_unref() here?
Comment on attachment 116634 [details] [review] Add GPS_SHARING_PROTOCOL Review of attachment 116634 [details] [review]: ----------------------------------------------------------------- ::: GPS_SHARING_PROTOCOL @@ +10,5 @@ > +* Server socket must be used for location service. > +* It must listen at port number 60793 > +* It must use NMEA GGA[1] sentences for the exchange of location data. > +* The service must send length of GGA sentence followed by the GGA sentence > + if new location is available and Geoclue is connected with the server. What are those services? I tried once to find some info on those and failed.
Comment on attachment 116635 [details] [review] Create gclue_location_new_from_gga Review of attachment 116635 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +285,5 @@ > + return g_ascii_strtod (altitude, NULL); > +} > + > +/** > + * gclue_location_new_from_gga: I think it's fine in here, maybe just not as a _new() method (since those are reserved to be simple wrappers around g_object_new). Perhaps gclue_location_create_from_gga() would be good.
Comment on attachment 116636 [details] [review] Add GClueNMEASource Review of attachment 116636 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +255,5 @@ > +static void > +on_nmea_source_destroyed (gpointer data, > + GObject *where_the_object_was) > +{ > + GClueNMEASource **source = (GClueNMEASource **) data; Ankit probably just copied from my code so it's my bad that I didn't know/remember of g_object_add_weak_pointer. Ankit, you don't need to have this function here if you use g_object_add_weak_pointer() below. @@ +277,5 @@ > + if (source == NULL) { > + source = g_object_new (GCLUE_TYPE_NMEA_SOURCE, NULL); > + g_object_weak_ref (G_OBJECT (source), > + on_nmea_source_destroyed, > + &source); Here is where you can use g_object_add_weak_pointer() instead. I'll update my own code too for this..
Comment on attachment 116634 [details] [review] Add GPS_SHARING_PROTOCOL Review of attachment 116634 [details] [review]: ----------------------------------------------------------------- ::: GPS_SHARING_PROTOCOL @@ +10,5 @@ > +* Server socket must be used for location service. > +* It must listen at port number 60793 > +* It must use NMEA GGA[1] sentences for the exchange of location data. > +* The service must send length of GGA sentence followed by the GGA sentence > + if new location is available and Geoclue is connected with the server. Something like this Android app: https://play.google.com/store/apps/details?id=com.rbc.gpsbridge&hl=en or this iOS app: https://itunes.apple.com/us/app/nmea-gps/id590868529?mt=8 They don't advertise their host/port/etc. over mDNS, but that's something trivial to test: avahi-publish-service -H 192.168.0.10 "NMEA over TCP" _geoclue._tcp 1024 will tell the local avahi to advertise the geoclue service at 192.168.0.10:1024
Comment on attachment 116634 [details] [review] Add GPS_SHARING_PROTOCOL Review of attachment 116634 [details] [review]: ----------------------------------------------------------------- ::: GPS_SHARING_PROTOCOL @@ +10,5 @@ > +* Server socket must be used for location service. > +* It must listen at port number 60793 > +* It must use NMEA GGA[1] sentences for the exchange of location data. > +* The service must send length of GGA sentence followed by the GGA sentence > + if new location is available and Geoclue is connected with the server. Well we'll have to try them out and trace with tcpdump to see how they are doing it but thinking more about it, we don't need to even transfer the size of buffer about to be sent if we ensure NMEA strings are always null-terminated. Then we can just use g_data_input_stream_read_upto(). Then it's hardly a protocol and don't even need documentation.
(In reply to Zeeshan Ali from comment #25) > Comment on attachment 116634 [details] [review] [review] > Add GPS_SHARING_PROTOCOL > > Review of attachment 116634 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: GPS_SHARING_PROTOCOL > @@ +10,5 @@ > > +* Server socket must be used for location service. > > +* It must listen at port number 60793 > > +* It must use NMEA GGA[1] sentences for the exchange of location data. > > +* The service must send length of GGA sentence followed by the GGA sentence > > + if new location is available and Geoclue is connected with the server. > > Well we'll have to try them out and trace with tcpdump to see how they are > doing it but thinking more about it, we don't need to even transfer the size > of buffer about to be sent if we ensure NMEA strings are always > null-terminated. Each sentence should be CR/LF terminated. See https://en.wikipedia.org/wiki/NMEA_0183 > Then we can just use g_data_input_stream_read_upto(). g_data_input_stream_read_line() ? > Then > it's hardly a protocol and don't even need documentation. The hard part of this is that, on the phone, you need to reconstruct NMEA sentences from an API that doesn't give you that full data. On the Geoclue side, I would probably start from gypsy's parser.
(In reply to Bastien Nocera from comment #26) > The hard part of this is that, on the phone, you need to reconstruct NMEA > sentences from an API that doesn't give you that full data. Android provides NMEA sentences directly from the GPS. https://developer.android.com/reference/android/location/GpsStatus.NmeaListener.html > > On the Geoclue side, I would probably start from gypsy's parser.
Created attachment 116948 [details] [review] Create gclue_location_create_from_gga This patch creates gclue_location_create_from_gga function inside GClueLocation which will be able to create a GClueLocation object out of a NMEA GGA sentence.
Created attachment 116949 [details] [review] Add confg option for NMEA source This patch adds configuration option which will tell us whether we want to use GClueNMEASource or not. GClueNMEASource will be added in the following commit.
Created attachment 116950 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Created attachment 116951 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Created attachment 116952 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Created attachment 116953 [details] [review] Create gclue_location_create_from_gga This patch creates gclue_location_create_from_gga function inside GClueLocation which will be able to create a GClueLocation object out of a NMEA GGA sentence.
Created attachment 116954 [details] [review] Add confg option for NMEA source This patch adds configuration option which will tell us whether we want to use GClueNMEASource or not. GClueNMEASource will be added in the following commit.
Created attachment 116955 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Comment on attachment 116953 [details] [review] Create gclue_location_create_from_gga Review of attachment 116953 [details] [review]: ----------------------------------------------------------------- It more like moves existing code but from description it sounds like entirely new code. ::: src/gclue-location.c @@ +26,4 @@ > #include "gclue-location.h" > #include <math.h> > > +#define INVALID_COORDINATE -G_MAXDOUBLE you want to define this with other GCLUE_ macros in the headerfile.
Comment on attachment 116954 [details] [review] Add confg option for NMEA source Review of attachment 116954 [details] [review]: ----------------------------------------------------------------- This change should come after the source has been added. Obviously, this and following patch will have to be modified. ::: data/geoclue.conf.in @@ +26,4 @@ > # > submit-data=false > > +# Do we want to use Network NMEA source This comment is telling what is obvious from the name of the settings. It should say something like: "Fetch location from NMEA sources on local network?"
Comment on attachment 116955 [details] [review] Add GClueNMEASource Review of attachment 116955 [details] [review]: ----------------------------------------------------------------- If we're going to hardcode the server for now, I'd rather we do so through a configuration option (geoclue.conf file) for now. Otherwise this source is currently not very useful. Also, commit log make no mention of this limitation and sounds like we can already get location from other devices, which we can't with this patch. ::: src/gclue-locator.c @@ +282,4 @@ > GClueLocator *locator = GCLUE_LOCATOR (object); > GClueLocationSource *submit_source = NULL; > GList *node; > + GClueConfig * gconfig = gclue_config_get_singleton (); Redundant space. ::: src/gclue-nmea-source.c @@ +73,5 @@ > + > + message = g_data_input_stream_read_line_finish (distream, > + result, > + &data_size, > + &error); args need to be aligned. @@ +85,5 @@ > + return; > + } > + g_debug ("Network source sent: \"%s\"", message); > + > + location = gclue_location_create_from_gga (message); NULL check missing here. @@ +94,5 @@ > + g_data_input_stream_read_line_async (distream, > + G_PRIORITY_DEFAULT, > + source->priv->cancellable, > + on_read_gga_sentence, > + source); alignment of args. I'd leave finding other cases of misaligned args in this (and other) patches to you now. :) @@ +256,5 @@ > + > + priv->client = g_socket_client_new (); > + g_cancellable_reset (priv->cancellable); > + > + /* FIXME: Don't hardcode hostname; Use Avahi for auto-discovery. */ nor port. @@ +259,5 @@ > + > + /* FIXME: Don't hardcode hostname; Use Avahi for auto-discovery. */ > + g_socket_client_connect_to_host_async > + (priv->client, > + "192.168.1.33", macro for port but not host? :) @@ +261,5 @@ > + g_socket_client_connect_to_host_async > + (priv->client, > + "192.168.1.33", > + GCLUE_NMEA_SERVER_PORT, > + NULL, Why is cancellable NULL? @@ +273,5 @@ > +gclue_nmea_source_stop (GClueLocationSource *source) > +{ > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (source)->priv; > + GClueLocationSourceClass *base_class; > + GInputStream * istream; * Redundant space. * Please use descriptive names whenever possible. In this case: input_stream. @@ +285,5 @@ > + g_return_val_if_fail (G_IS_SOCKET_CONNECTION (priv->connection), TRUE); > + > + > + istream = g_io_stream_get_input_stream > + (G_IO_STREAM (priv->connection)); Please move this just before first use of istream below. @@ +289,5 @@ > + (G_IO_STREAM (priv->connection)); > + > + g_cancellable_cancel (priv->cancellable); > + > + g_input_stream_clear_pending (istream); I don't think we need to do this. closing and unrefing should be enough. @@ +294,5 @@ > + g_input_stream_close_async (istream, > + G_PRIORITY_DEFAULT, > + NULL, > + on_input_stream_close, > + GCLUE_NMEA_SOURCE (source)); To avoid any race-conditions, stopping should be sync. I'd just g_object_unref() it and let GIO take care of closing sockets.
Comment on attachment 116955 [details] [review] Add GClueNMEASource Review of attachment 116955 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +289,5 @@ > + (G_IO_STREAM (priv->connection)); > + > + g_cancellable_cancel (priv->cancellable); > + > + g_input_stream_clear_pending (istream); I wasn't able to close stream without it. It displayed error: Outstanding operations @@ +294,5 @@ > + g_input_stream_close_async (istream, > + G_PRIORITY_DEFAULT, > + NULL, > + on_input_stream_close, > + GCLUE_NMEA_SOURCE (source)); I'll try this.
(In reply to Zeeshan Ali from comment #38) > Comment on attachment 116955 [details] [review] [review] > Add GClueNMEASource > > Review of attachment 116955 [details] [review] [review]: > ----------------------------------------------------------------- > > If we're going to hardcode the server for now, I'd rather we do so through a > configuration option (geoclue.conf file) for now. Otherwise this source is > currently not very useful. Also, commit log make no mention of this > limitation and sounds like we can already get location from other devices, > which we can't with this patch. Very soon we are going to add Avahi, can't we just go with a macro for the host string too? Because once we use Avahi, that configuration option becomes useless.
(In reply to Ankit from comment #40) > (In reply to Zeeshan Ali from comment #38) > > Comment on attachment 116955 [details] [review] [review] [review] > > Add GClueNMEASource > > > > Review of attachment 116955 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > If we're going to hardcode the server for now, I'd rather we do so through a > > configuration option (geoclue.conf file) for now. Otherwise this source is > > currently not very useful. Also, commit log make no mention of this > > limitation and sounds like we can already get location from other devices, > > which we can't with this patch. > > Very soon we are going to add Avahi, can't we just go with a macro for the > host string too? Because once we use Avahi, that configuration option > becomes useless. Oh if that's happening very soon, you should just rebase these patches to make use of that.
Created attachment 117149 [details] [review] Create gclue_location_create_from_gga This patch creates gclue_location_create_from_gga function inside GClueLocation which will be able to create a GClueLocation object out of a NMEA GGA sentence. This function works with the help of functions: get_accuracy_from_hdop, parse_coordinate_string and parse_altitude_string which are moved from gclue-modem-gps.c.
Created attachment 117150 [details] [review] Add Avahi dependency Avahi will be required by GClueNMEASource, which will be added in the coming patches. Avahi will help GClueNMEASource to automatically discover nmea-tcp services on the local network.
Created attachment 117151 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. It uses Avahi for the auto-discovery of nmea-tcp services on local network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Created attachment 117152 [details] [review] Add config option for NMEA source This patch adds configuration option which will tell us whether we want to use GClueNMEASource or not.
Comment on attachment 117149 [details] [review] Create gclue_location_create_from_gga Review of attachment 117149 [details] [review]: ----------------------------------------------------------------- Looks good otherwise. ::: src/gclue-location.c @@ +291,5 @@ > + * > + * Returns: a new #GClueLocation object. Use g_object_unref() when done. > + **/ > +GClueLocation * > +gclue_location_create_from_gga (const char *gga) This function should take an Error * argument and set it appropriately when parsing fails. Feel free to use G_IO_ERROR_INVALID_ARGUMENT error code for parsing error. @@ +299,5 @@ > + gdouble hdop; /* Horizontal Dilution Of Precision */ > + char **parts; > + > + parts = g_strsplit (gga, ",", -1); > + if (g_strv_length (parts) < 14 ) { redundant space after '14'. ::: src/gclue-modem-gps.c @@ -21,4 @@ > > #include <stdlib.h> > #include <glib.h> > -#include <string.h> Since this is not moved to gclue-location.h, I suspect this change is unrelated to the patch itself (i-e this declaration wasn't needed anyway?).
Comment on attachment 117150 [details] [review] Add Avahi dependency Review of attachment 117150 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +57,5 @@ > gio-unix-2.0 >= $GLIB_MIN_VERSION > json-glib-1.0 >= $JSON_GLIB_MIN_VERSION > libsoup-2.4 >= $LIBSOUP_MIN_VERSION > + avahi-client >= $AVAHI_MIN_VERSION > + avahi-glib >= $AVAHI_MIN_VERSION 1. You are checking for these twice (two additional PKG_CHECK_MODULES calls below), you only need to check for them once. If you add it here (which is were it should be if we wanted an unconditional dependency), the cflags and library flags will get added to GEOCLUE_CFLAGS and GEOCLUE_LIBS. 2. We actually should make build of NMEA source optional so you want to add a configure option to be able to disable the source. See usage of AC_ARG_ENABLE macro for options to disable out sources for example. @@ +111,5 @@ > +# Check for avahi-client > +PKG_CHECK_MODULES(AvahiClient, avahi-client >= $AVAHI_MIN_VERSION) > + > +# Check for avahi-glib > +PKG_CHECK_MODULES(AvahiGlib, avahi-glib >= $AVAHI_MIN_VERSION) * You don't need two calls (and therefore 4 generated flags here). On call can do multiple checks (as you see above for main deps check). * Please capitalize the first argument so generated flags marcos are all caps, as per general conventions. I know you followed the ModemManager example here but that itself is a mistake.
Comment on attachment 117149 [details] [review] Create gclue_location_create_from_gga Review of attachment 117149 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-modem-gps.c @@ -21,4 @@ > > #include <stdlib.h> > #include <glib.h> > -#include <string.h> Yeah, you are right. At first I thought that it was being used by parsing functions. But, that was all GLIB.
Comment on attachment 117151 [details] [review] Add GClueNMEASource Review of attachment 117151 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +35,5 @@ > +#include <avahi-common/malloc.h> > +#include <avahi-common/error.h> > + > +#include <avahi-glib/glib-watch.h> > +#include <avahi-glib/glib-malloc.h> Do we really need to include all these header individually? Isn't there some common header that we can include, like for all libraries? @@ +46,5 @@ > + GCancellable *cancellable; > + > + AvahiClient *avahi_client; > + > + GList *services; I don't think we want to use multiple services but just the first one we find (or comes online). Geoclue could get apps really confused if it starts to report high accuracy locations from two different devices at the same time. @@ +55,5 @@ > +struct AvahiServiceInfo { > + AvahiIfIndex interface; > + AvahiProtocol protocol; > + char *name, *type, *domain; > +}; All the info can go to the private struct if we limit ourself to only 1 service. @@ +68,5 @@ > +gclue_nmea_source_stop (GClueLocationSource *source); > +static void > +on_connection_to_location_server (GObject *object, > + GAsyncResult *result, > + gpointer user_data); with other callbacks of async functions you have defined the before their usage but for this one you declared first and defined after its usage. I'd stick with practice of defining async callback before their usage to keep things consistent. @@ +110,5 @@ > + "available-accuracy-level", &accuracy, > + NULL); > + > + return accuracy; > +} You don't need this getter. We already have gclue_location_source_get_available_accuracy_level(). @@ +114,5 @@ > +} > + > +static void > +resolve_callback (AvahiServiceResolver *service_resolver, > + AVAHI_GCC_UNUSED AvahiIfIndex interface, You want to use G_GNUC_UNUSED instead and you want to place it after the argument name: AvahiIfIndex interface G_GNUC_UNUSED, @@ +130,5 @@ > +{ > + g_return_if_fail (service_resolver != NULL); > + > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (source)->priv; > + char a[AVAHI_ADDRESS_STR_MAX], *t; I prefer descriptive variable names, except for well known names like 'i' for integer iterator. @@ +138,5 @@ > + > + case AVAHI_RESOLVER_FAILURE: > + errorstr = avahi_strerror > + (avahi_client_errno (avahi_service_resolver_get_client > + (service_resolver))); you could make this look much prettier by use of variables. :) General guidelines from me would be to prefer variables over nested calls. @@ +153,5 @@ > + case AVAHI_RESOLVER_FOUND: > + g_warning ("Service '%s' of type '%s' in domain '%s':", > + name, > + type, > + domain); Why warn about this? @@ +172,5 @@ > + !!(flags & AVAHI_LOOKUP_RESULT_LOCAL), > + !!(flags & AVAHI_LOOKUP_RESULT_OUR_OWN), > + !!(flags & AVAHI_LOOKUP_RESULT_WIDE_AREA), > + !!(flags & AVAHI_LOOKUP_RESULT_MULTICAST), > + !!(flags & AVAHI_LOOKUP_RESULT_CACHED)); We really dont need to provide so many details. Avahi provides tools to debug avahi-specific issues. @@ +215,5 @@ > + const char *name, > + const char *type, > + const char *domain, > + AVAHI_GCC_UNUSED AvahiLookupResultFlags flags, > + void *source) you want to declare this param as 'data' or 'user_data' and then declare a typed source and do a casted assignenment to it. See other sources for example of how to do this. `git grep user_data` will help you find all examples. :) @@ +304,5 @@ > + service->domain = g_strdup (domain); > + > + item = g_list_find_custom (priv->services, > + service, > + list_comparator); Instead of having to keep and duplicate all the info and then comparing, you could just have a GHashTable and have the name (if name is not unique, you could concatenate all the info to make a uniq id yourself). @@ +430,5 @@ > + g_clear_object (&priv->connection); > + g_clear_object (&priv->client); > + g_clear_object (&priv->cancellable); > + avahi_client_free (priv->avahi_client); > + g_list_free (priv->services); You're leaking the contents. @@ +474,5 @@ > + source, > + &error); > + > + if (priv->avahi_client == NULL) { > + g_warning ("Failed to create AvahiClient: %s", AvahiClient is a programmer detail. Just say "Failed to connect to avahi service: %s". @@ +526,5 @@ > + GClueLocationSourceClass *base_class; > + GClueNMEASourcePrivate *priv; > + AvahiServiceResolver *service_resolver; > + AvahiServiceInfo *service; > + const gchar *errorstr; Only declare those variables here that are going to be used by main and/or multiple sub-blocks. All other declarations go to the top of the block where they are needed. @@ +554,5 @@ > + service->domain, > + AVAHI_PROTO_UNSPEC, > + 0, > + resolve_callback, > + source); What all this resolving about? Seems to me you find service in twice. Since we are always looking for services, i wouldn't expect any avahi stuff in start/stop but only connecting/disconnection etc to an already found service. @@ +558,5 @@ > + source); > + > + if (service_resolver == NULL) { > + errorstr = avahi_strerror > + (avahi_client_errno (priv->avahi_client)); would use another variable for errno. @@ +590,5 @@ > + (G_IO_STREAM (priv->connection)); > + > + g_cancellable_cancel (priv->cancellable); > + > + g_object_unref (input_stream); g_io_stream_get_input_stream() clearly states that input_stream ref returned is not yours and should not be unrefed by you.
Comment on attachment 117152 [details] [review] Add config option for NMEA source Review of attachment 117152 [details] [review]: ----------------------------------------------------------------- ::: data/geoclue.conf.in @@ +26,5 @@ > # > submit-data=false > > +# Fetch location from NMEA sources on local network? > +use-nmea-source=true why is this part of wifi section? It shouldn't be. ::: src/gclue-locator.c @@ +282,4 @@ > GClueLocator *locator = GCLUE_LOCATOR (object); > GClueLocationSource *submit_source = NULL; > GList *node; > + GClueConfig *gconfig = gclue_config_get_singleton (); would look slightly prettier if we move this before GList line.
Comment on attachment 117151 [details] [review] Add GClueNMEASource Review of attachment 117151 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +225,5 @@ > + GClueAccuracyLevel accuracy; > + AvahiServiceInfo *service; > + const gchar *errorstr; > + char **parts; > + GList *item; Same comment about declarations. @@ +260,5 @@ > + name); > + break; > + } > + > + accuracy = GCLUE_ACCURACY_LEVEL_NONE; I'd initialize this at declaration. @@ +272,5 @@ > + } else if (g_strcmp0 (parts[3], "city") == 0) { > + accuracy = GCLUE_ACCURACY_LEVEL_CITY; > + } else if (g_strcmp0 (parts[3], "country") == 0) { > + accuracy = GCLUE_ACCURACY_LEVEL_COUNTRY; > + } As I was telling you on IM, you want to make use of g_enum* API here. I even pointed you to examples. :)
Comment on attachment 117151 [details] [review] Add GClueNMEASource Review of attachment 117151 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +554,5 @@ > + service->domain, > + AVAHI_PROTO_UNSPEC, > + 0, > + resolve_callback, > + source); Ah, bastien explained me about resolving and now i understand this. So yeah i think we should resolve in here.
Comment on attachment 117151 [details] [review] Add GClueNMEASource Review of attachment 117151 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +35,5 @@ > +#include <avahi-common/malloc.h> > +#include <avahi-common/error.h> > + > +#include <avahi-glib/glib-watch.h> > +#include <avahi-glib/glib-malloc.h> I don't think there is any common header. But, I'll still see if I could find one. @@ +46,5 @@ > + GCancellable *cancellable; > + > + AvahiClient *avahi_client; > + > + GList *services; Avahi triggers an event for Service new or removed. We can't just recheck for all the services available in the network without creating a new AvahiServiceBrowser. So, the cases where we have 2 services, only 1st one gets stored and 2nd one goes unaccounted; now if the 1st service stops there would be no way for Geoclue to find the 2nd service. Unless, of course if it stop and start again. And no, not all services will work at once. Only the service with highest accuracy gets picked. That I'm doing on line #278. @@ +68,5 @@ > +gclue_nmea_source_stop (GClueLocationSource *source); > +static void > +on_connection_to_location_server (GObject *object, > + GAsyncResult *result, > + gpointer user_data); I'll fix it. @@ +153,5 @@ > + case AVAHI_RESOLVER_FOUND: > + g_warning ("Service '%s' of type '%s' in domain '%s':", > + name, > + type, > + domain); Aah... mistake. @@ +304,5 @@ > + service->domain = g_strdup (domain); > + > + item = g_list_find_custom (priv->services, > + service, > + list_comparator); I'm also using list to keep the services sorted by their accuracy too. @@ +590,5 @@ > + (G_IO_STREAM (priv->connection)); > + > + g_cancellable_cancel (priv->cancellable); > + > + g_object_unref (input_stream); So should I then unref connection only to unref this input stream and disconnect from server?
Comment on attachment 117151 [details] [review] Add GClueNMEASource Review of attachment 117151 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +46,5 @@ > + GCancellable *cancellable; > + > + AvahiClient *avahi_client; > + > + GList *services; Thanks for explaining. Yeah I realized only later what you were doing. I think a comment here and/or at _start() would be helpful saying we keep list of all services but only use the most accurate one available. @@ +71,5 @@ > + GAsyncResult *result, > + gpointer user_data); > + > +static gint > +list_comparator (gconstpointer a, compare_avahi_services might be a better name. @@ +105,5 @@ > +static GClueAccuracyLevel > +get_accuracy_level (GClueNMEASource *source) > +{ > + GClueAccuracyLevel accuracy; > + g_object_get (G_OBJECT (source), I prefer a blank line after declarations. @@ +134,5 @@ > + char a[AVAHI_ADDRESS_STR_MAX], *t; > + const gchar *errorstr; > + > + switch (event) { > + redundant line. @@ +282,5 @@ > + priv->services = g_list_prepend (priv->services, > + service); > + } else { > + priv->services = g_list_append (priv->services, > + service); * this puts the new service at the end of the list, regardless of how it's accuracy compares to other services so your list won't be sorted. I'd recommend you simply prepend (since it's more efficient than append) the new service and then call g_list_sort() on it. After which, you can refresh your accuracy level. * What happens if new service is discovered that has better accuracy then previously known and active sources, and source is already active? @@ +304,5 @@ > + service->domain = g_strdup (domain); > + > + item = g_list_find_custom (priv->services, > + service, > + list_comparator); Ah ok, probably list is fine then. You want to fix the argument alignment here though. @@ +327,5 @@ > + g_debug ("(Browser) REMOVE: service '%s' " > + "of type '%s' in domain '%s'", > + name, > + type, > + domain); Similarly here, what if service becomes unavailable while source is active? @@ +590,5 @@ > + (G_IO_STREAM (priv->connection)); > + > + g_cancellable_cancel (priv->cancellable); > + > + g_object_unref (input_stream); I recall you said that you were having trouble if you let connection take care of disconnecting on loosing it's last ref so maybe you want to disconnect explicitly but you are not supposed to unref the input_stream since you don't own the reference to it.
Comment on attachment 117151 [details] [review] Add GClueNMEASource Review of attachment 117151 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +282,5 @@ > + priv->services = g_list_prepend (priv->services, > + service); > + } else { > + priv->services = g_list_append (priv->services, > + service); * I was thinking that I left this anomaly. How about g_list_insert_sorted() instead? Also, I forgot to update accuracy level when some device disconnects. * Once the service resolver has started (inside _start()), no matter what happens to the list, the older service will get resolved and connected to. @@ +327,5 @@ > + g_debug ("(Browser) REMOVE: service '%s' " > + "of type '%s' in domain '%s'", > + name, > + type, > + domain); Service will get removed from the list and Geoclue will stop getting NMEA sentences. This is the case when Network service has stopped.
Comment on attachment 117151 [details] [review] Add GClueNMEASource Review of attachment 117151 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +282,5 @@ > + priv->services = g_list_prepend (priv->services, > + service); > + } else { > + priv->services = g_list_append (priv->services, > + service); * g_list_insert_sorted() use sounds good to me. I forgot that exists. * I think you need to handle that situation: If new service has better accuracy, switch to using that. For now I'd be fine with just adding a FIXME comment before 'break' below about this. @@ +327,5 @@ > + g_debug ("(Browser) REMOVE: service '%s' " > + "of type '%s' in domain '%s'", > + name, > + type, > + domain); well we need to react to that case and start using another service (if available). Also we should stop reading from the socket etc.
Created attachment 117391 [details] [review] Create gclue_location_create_from_gga This patch creates gclue_location_create_from_gga function inside GClueLocation which will be able to create a GClueLocation object out of a NMEA GGA sentence. This function works with the help of functions: get_accuracy_from_hdop, parse_coordinate_string and parse_altitude_string which are moved from gclue-modem-gps.c.
Created attachment 117392 [details] [review] Add Avahi dependency Avahi will be required by GClueNMEASource, which will be added in the coming patches. Avahi will help GClueNMEASource to automatically discover nmea-tcp services on the local network.
Created attachment 117393 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. It uses Avahi for the auto-discovery of nmea-tcp services on local network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Created attachment 117394 [details] [review] Add config option for NMEA source This patch adds configuration option which will tell us whether we want to use GClueNMEASource or not.
Comment on attachment 117391 [details] [review] Create gclue_location_create_from_gga Review of attachment 117391 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +293,5 @@ > + * > + * Returns: a new #GClueLocation object. Use g_object_unref() when done. > + **/ > +GClueLocation * > +gclue_location_create_from_gga (const char *gga, GIOErrorEnum *error) Not GIOErrorEnum, a 'GError **'. See other functions in the code for example of error throwing functions, such as gclue_3g_create_submit_query(). ::: src/gclue-modem-gps.c @@ -21,4 @@ > > #include <stdlib.h> > #include <glib.h> > -#include <string.h> this is still here. :(
Comment on attachment 117392 [details] [review] Add Avahi dependency Review of attachment 117392 [details] [review]: ----------------------------------------------------------------- Almost there. ::: configure.ac @@ +119,5 @@ > +AM_CONDITIONAL([BUILD_NMEA_SOURCE], [test "$build_nmea_source" = "yes"]) > + > +if test "$build_nmea_source" = "yes"; then > + # Check for avahi-client and avahi-glib > + PKG_CHECK_MODULES(AVAHI, [ Today we require avahi only, tomorrow we might add/remove deps so better name it against source itself.
Comment on attachment 117391 [details] [review] Create gclue_location_create_from_gga Review of attachment 117391 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-modem-gps.c @@ -21,4 @@ > > #include <stdlib.h> > #include <glib.h> > -#include <string.h> So, this is not to be removed?
Comment on attachment 117393 [details] [review] Add GClueNMEASource Review of attachment 117393 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +35,5 @@ > +#include <avahi-common/malloc.h> > +#include <avahi-common/error.h> > + > +#include <avahi-glib/glib-watch.h> > +#include <avahi-glib/glib-malloc.h> So I assume you failed to find common header for avahi-client and/or avahi-glib and you have tested that each of these headers are needed? @@ +46,5 @@ > + GCancellable *cancellable; > + > + AvahiClient *avahi_client; > + > + GList *services; I asked for a comment here and/or at _start(). I don't see it anywhere. :( @@ +56,5 @@ > + guint16 port; > + GClueAccuracyLevel accuracy; > +}; > + > +typedef struct AvahiServiceInfo AvahiServiceInfo; * you want to move it below G_DEFINE_TYPE and and function declarations (the _start/_stop just below). * you can simplify this by: typedef struct { ... } AvahiServiceInfo; @@ +78,5 @@ > + return g_strcmp0 (first->identifier, second->identifier); > +} > + > +static gint > +compare_avahi_services_for_sort (gconstpointer a, * You want to use singular form (service). * The suffix should be about what kind of comparison is going to take place rather than what they'll be used for. compare_avahi_service_by_name() compare_avahi_service_by_accuracy() @@ +90,5 @@ > + return second->accuracy - first->accuracy; > +} > + > +static void > +destroy_avahi_services_list (gpointer data) this function only destroy/frees an AvahiServiceInfo where as the name suggests it destroys a list of them. Please name it avahi_service_free and place it just after associated struct as per conventions. @@ +92,5 @@ > + > +static void > +destroy_avahi_services_list (gpointer data) > +{ > + AvahiServiceInfo *service = (AvahiServiceInfo *) data; Blank line after declaration please. @@ +123,5 @@ > + AvahiStringList *txt, > + AvahiLookupResultFlags flags, > + void *user_data G_GNUC_UNUSED) > +{ > + g_return_if_fail (service_resolver != NULL); Declarations are by convention always on the top. @@ +130,5 @@ > + AvahiServiceInfo *service; > + AvahiStringList *node; > + GEnumClass *enum_class; > + GEnumValue *enum_value; > + const gchar *errorstr; At least these last 3 seem to be only used one one of the block below so you should declare them there. @@ +131,5 @@ > + AvahiStringList *node; > + GEnumClass *enum_class; > + GEnumValue *enum_value; > + const gchar *errorstr; > + guint number_of_services; n_services would be a short enough name, but still obvious meaning. It's actually a general convention for "number_of_". @@ +135,5 @@ > + guint number_of_services; > + char **parts; > + > + AvahiClient *avahi_client = avahi_service_resolver_get_client > + (service_resolver); * Please keep delcaration together wither others. You can inialize still in here if that seems better.. * Is this used in both branches below? If not, please move it inside the block too. @@ +168,5 @@ > + g_warning ("No `accuracy` key inside TXT record"); > + break; > + } > + > + parts = g_strsplit ((char *)node->text, "=", -1); Space after "(char *)" please. @@ +181,5 @@ > + g_type_class_unref (enum_class); > + > + if (enum_value == NULL) { > + g_warning ("Error in parsing TXT record for accuracy"); > + service->accuracy = GCLUE_ACCURACY_LEVEL_NONE; I don't think this should be fatal but rather we should assume EXACT accuracy if none is specified. @@ +193,5 @@ > + service, > + compare_avahi_services_for_sort); > + > + service = (AvahiServiceInfo *) g_list_nth_data (priv->services, > + 0); g_list_nth_data is an overkill for first member, just do priv->services->data. @@ +195,5 @@ > + > + service = (AvahiServiceInfo *) g_list_nth_data (priv->services, > + 0); > + > + if (service == NULL) { This is impossible scenario since you just added a list and all this code is synchronous so don't bother handling it. @@ +200,5 @@ > + break; > + } > + > + /* FIXME: If a new service with better accuracy comes in, > + connect to it (if GClueNMEASource is running). */ * You either don't want to add new (higher accuracy) service when source is already running or you want to handle that here already. Otherwise, you'd have the inconsistent state of service in use not being the same as the first one in the list, which might translate to some weird hard to debug bugs.. * You want to put the closing "*/" on it's own line, as per conventions of multiline comments. * You don't always need to refer to the source with it's full name. :) In here, you can just say 'source'. @@ +218,5 @@ > +client_callback (AvahiClient *avahi_client, > + AvahiClientState state, > + void *user_data G_GNUC_UNUSED) > +{ > + g_return_if_fail (avahi_client != NULL); Same comment about declarations first, here as well. @@ +220,5 @@ > + void *user_data G_GNUC_UNUSED) > +{ > + g_return_if_fail (avahi_client != NULL); > + > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (user_data)->priv; You are using the user_data here so you don't want to mark it as UNUSED in signature. Did you compile this and compiler didn't even warn? @@ +241,5 @@ > + const char *domain, > + AvahiLookupResultFlags flags G_GNUC_UNUSED, > + void *user_data) > +{ > + g_return_if_fail (service_browser != NULL); same here. Declarations first. @@ +246,5 @@ > + > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (user_data)->priv; > + AvahiServiceInfo *service; > + AvahiServiceResolver *service_resolver; > + guint number_of_services = 0; n_services @@ +248,5 @@ > + AvahiServiceInfo *service; > + AvahiServiceResolver *service_resolver; > + guint number_of_services = 0; > + const gchar *errorstr; > + GList *item; I pointed this particular variable/context in previous review. Please try to ensure all review comments are addressed properly before submitting new versions. Reviewing is harder than it seems. :) @@ +273,5 @@ > + 0, > + resolve_callback, > + user_data); > + > + if (service_resolver == NULL) { AFAICT the resolver works async so is this possible scenario? @@ +310,5 @@ > + type, > + domain); > + > + service = (AvahiServiceInfo *) g_list_nth_data (priv->services, > + 0); Here also you can use priv->services->data. @@ +314,5 @@ > + 0); > + > + if (service == NULL) { > + set_accuracy_level (GCLUE_NMEA_SOURCE (user_data), > + GCLUE_ACCURACY_LEVEL_NONE); args misaligned. @@ +315,5 @@ > + > + if (service == NULL) { > + set_accuracy_level (GCLUE_NMEA_SOURCE (user_data), > + GCLUE_ACCURACY_LEVEL_NONE); > + break; empty line above break please. @@ +327,5 @@ > + case AVAHI_BROWSER_ALL_FOR_NOW: > + case AVAHI_BROWSER_CACHE_EXHAUSTED: > + g_warning ("(Browser) %s", > + event == AVAHI_BROWSER_CACHE_EXHAUSTED ? > + "CACHE_EXHAUSTED" : "ALL_FOR_NOW"); * you want to align this with previous line. * This will be shown to humans, not used as variables so "cache exhausted" would be more appropriate. Don't know what "ALL_FOR_NOW" is about but I'm sure you can think of better wording for that on your own. :) @@ +333,5 @@ > + } > +} > + > +static void > +on_read_gga_sentence (GObject *object, This function assumes that it's going to be GGA sentence, even the name of this function. You don't want to do that and check if it's GGA sentence or not. If not, you just ignore it for now. @@ +343,5 @@ > + GError *error = NULL; > + GClueLocation *location; > + gsize data_size = 0 ; > + gchar *message; > + GIOErrorEnum gga_parse_error; You don't need a specia error here. You want to re-use same 'error' throughout the function. @@ +356,5 @@ > + if (error != NULL) { > + g_warning ("%s", error->message); > + g_clear_error (&error); > + } > + return; Empty line before return please. @@ +362,5 @@ > + g_debug ("Network source sent: \"%s\"", message); > + > + location = gclue_location_create_from_gga (message, &gga_parse_error); > + > + if (gga_parse_error != G_IO_ERROR_INVALID_ARGUMENT) { GError usage is wrong. You have a good example in your own code just above on how GError works. @@ +391,5 @@ > + result, > + &error); > + > + if (error != NULL) { > + g_debug ("%s", error->message); I'd emit a warning here. @@ +393,5 @@ > + > + if (error != NULL) { > + g_debug ("%s", error->message); > + g_clear_error (&error); > + return; Emtpy line before return please. @@ +398,5 @@ > + } > + > + input_stream = g_io_stream_get_input_stream > + (G_IO_STREAM (source->priv->connection)); > + data_input_stream = g_data_input_stream_new (input_stream); Where do you free this data_input_stream? @@ +447,5 @@ > + int error; > + > + source->priv = G_TYPE_INSTANCE_GET_PRIVATE ((source), > + GCLUE_TYPE_NMEA_SOURCE, > + GClueNMEASourcePrivate); misaligned args. @@ +564,5 @@ > + &error); > + if (error != NULL) { > + g_warning ("Error in closing socket connection"); > + return FALSE; > + } Don't you need to unref client and connection here?
Comment on attachment 117391 [details] [review] Create gclue_location_create_from_gga Review of attachment 117391 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-modem-gps.c @@ -21,4 @@ > > #include <stdlib.h> > #include <glib.h> > -#include <string.h> No, as I said in review of previous version, this is not part of this logical change so at least it shouldn't be in this patch.
Comment on attachment 117394 [details] [review] Add config option for NMEA source Review of attachment 117394 [details] [review]: ----------------------------------------------------------------- ::: data/geoclue.conf.in @@ +7,4 @@ > # separated by a ';'. > whitelist=@demo_agent@gnome-shell > > +# NMEASource configuration options This file is meant to be editted by admins and advanced users, so you want to call it "Network NMEA source". @@ +7,5 @@ > # separated by a ';'. > whitelist=@demo_agent@gnome-shell > > +# NMEASource configuration options > +[networksource] * "source" is not import part here. * Use "-" to separate parts of name. Lets call it 'network-nmea'. @@ +10,5 @@ > +# NMEASource configuration options > +[networksource] > + > +# Fetch location from NMEA sources on local network? > +use-nmea-source=true This can then just become 'enable' since rest is obvious from context. ::: src/gclue-config.c @@ +242,5 @@ > + "networksource", > + "use-nmea-source", > + &error); > + if (error != NULL) { > + g_debug ("Failed to get config wifi/use-nmea-source: %s", This still says 'wifi'.
Comment on attachment 117393 [details] [review] Add GClueNMEASource Review of attachment 117393 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +181,5 @@ > + g_type_class_unref (enum_class); > + > + if (enum_value == NULL) { > + g_warning ("Error in parsing TXT record for accuracy"); > + service->accuracy = GCLUE_ACCURACY_LEVEL_NONE; But why EXACT? As far as we know, this could be a low accuracy device which may block our FOR-SURE high accuracy device. @@ +200,5 @@ > + break; > + } > + > + /* FIXME: If a new service with better accuracy comes in, > + connect to it (if GClueNMEASource is running). */ But when the client again connects to the geoclue for the next time, it will automatically take the better source. @@ +220,5 @@ > + void *user_data G_GNUC_UNUSED) > +{ > + g_return_if_fail (avahi_client != NULL); > + > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (user_data)->priv; yeah, no warnings. @@ +273,5 @@ > + 0, > + resolve_callback, > + user_data); > + > + if (service_resolver == NULL) { Avahi sample code checks for this. @@ +327,5 @@ > + case AVAHI_BROWSER_ALL_FOR_NOW: > + case AVAHI_BROWSER_CACHE_EXHAUSTED: > + g_warning ("(Browser) %s", > + event == AVAHI_BROWSER_CACHE_EXHAUSTED ? > + "CACHE_EXHAUSTED" : "ALL_FOR_NOW"); This is insignificant to humans, shall I remove it? @@ +333,5 @@ > + } > +} > + > +static void > +on_read_gga_sentence (GObject *object, How can I check if it's GGA or not?
Comment on attachment 117393 [details] [review] Add GClueNMEASource Review of attachment 117393 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +181,5 @@ > + g_type_class_unref (enum_class); > + > + if (enum_value == NULL) { > + g_warning ("Error in parsing TXT record for accuracy"); > + service->accuracy = GCLUE_ACCURACY_LEVEL_NONE; It could be, yes but this is mainly for sharing of GPS device and hence it's very unlikely. Also typically you'd only have 1 (or at most 2) devices on a network sharing their GPS. @@ +200,5 @@ > + break; > + } > + > + /* FIXME: If a new service with better accuracy comes in, > + connect to it (if GClueNMEASource is running). */ True although that doesn't help with the possible issues with this inconsistency. What you could do is to keep a pointer to current service in use (which is NULL when inactive) and rename 'services' field to 'all_services' so that it's hard for any developer to get confused and introduce any possible issues with this inconsistency when hacking on this code. @@ +273,5 @@ > + 0, > + resolve_callback, > + user_data); > + > + if (service_resolver == NULL) { oh ok, that is really weird and bad API IMO but I'll ask on the channel and let you know if devs tell me sample code is wrong. @@ +327,5 @@ > + case AVAHI_BROWSER_ALL_FOR_NOW: > + case AVAHI_BROWSER_CACHE_EXHAUSTED: > + g_warning ("(Browser) %s", > + event == AVAHI_BROWSER_CACHE_EXHAUSTED ? > + "CACHE_EXHAUSTED" : "ALL_FOR_NOW"); I don't think so. At least a g_debug would be in order here. Otherwise, we'll get bugs like "NMEA sharing is not working for me" and we'd have no way of knowing whats going on if this is the issue. @@ +333,5 @@ > + } > +} > + > +static void > +on_read_gga_sentence (GObject *object, By looking at the header of NMEA sentence string?
Created attachment 117450 [details] [review] Create gclue_location_create_from_gga This patch creates gclue_location_create_from_gga function inside GClueLocation which will be able to create a GClueLocation object out of a NMEA GGA sentence. This function works with the help of functions: get_accuracy_from_hdop, parse_coordinate_string and parse_altitude_string which are moved from gclue-modem-gps.c.
Created attachment 117451 [details] [review] Add Avahi dependency Avahi will be required by GClueNMEASource, which will be added in the coming patches. Avahi will help GClueNMEASource to automatically discover nmea-tcp services on the local network.
Created attachment 117452 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. It uses Avahi for the auto-discovery of nmea-tcp services on local network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Created attachment 117453 [details] [review] Add config option for NMEA source This patch adds configuration option which will tell us whether we want to use GClueNMEASource or not.
Comment on attachment 117450 [details] [review] Create gclue_location_create_from_gga Review of attachment 117450 [details] [review]: ----------------------------------------------------------------- almost there. ::: src/gclue-location.c @@ +285,5 @@ > + > +/** > + * gclue_location_create_from_gga: > + * @gga NMEA GGA sentence > + * @error a GIOErrorEnum, it's value is set to G_IO_ERROR_INVALID_ARGUMENT You forgot to update this doc string.
Comment on attachment 117451 [details] [review] Add Avahi dependency Review of attachment 117451 [details] [review]: ----------------------------------------------------------------- You were so close :) ::: configure.ac @@ +119,5 @@ > +AM_CONDITIONAL([BUILD_NMEA_SOURCE], [test "$build_nmea_source" = "yes"]) > + > +if test "$build_nmea_source" = "yes"; then > + # Check for avahi-client and avahi-glib > + PKG_CHECK_MODULES(NMEASOURCE, [ NMEA_SOURCE please, to keep it consistent.
Comment on attachment 117452 [details] [review] Add GClueNMEASource Review of attachment 117452 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +33,5 @@ > +#include <avahi-common/malloc.h> > +#include <avahi-common/error.h> > +#include <avahi-glib/glib-watch.h> > + > +typedef struct AvahiServiceInfo AvahiServiceInfo; you didn't quite exacty do what I told you to and i don't know why you simply moved this typedef over here at the top. @@ +44,5 @@ > + GCancellable *cancellable; > + > + AvahiClient *avahi_client; > + > + AvahiServiceInfo *service; Let's call it current_service or active_service please. :) @@ +46,5 @@ > + AvahiClient *avahi_client; > + > + AvahiServiceInfo *service; > + > + /* List of all services but only use the most accurate one available. */ most accurate one is used. @@ +57,5 @@ > +gclue_nmea_source_start (GClueLocationSource *source); > +static gboolean > +gclue_nmea_source_stop (GClueLocationSource *source); > + > +struct AvahiServiceInfo{ space before { @@ +71,5 @@ > + AvahiServiceInfo *service = (AvahiServiceInfo *) data; > + > + avahi_free (service->identifier); > + avahi_free (service->host_name); > + avahi_free (service); * AvahiServiceInfo is not an avahi type nor it's allocated by avahi api, neither is host_name and identifier so you don't use avahi_free for anything here. For AvahiServiceInfo you want to make use of g_slice*() API. You can find example use of that through-out the geoclue source code. To attempt to ensure g_slice is used to allocate the struct, you probably also want to add a avahi_service_new(). Since you set all fields just after allocation of the structure, you probably want to accept them as arguments and set the fields in the avahi_service_new() function (including duplicating identifier and hostname strings). * Do use g_free0() to ensure you don't try to free a NULL pointer. @@ +124,5 @@ > + AvahiLookupResultFlags flags, > + void *user_data) > +{ > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (user_data)->priv; > + const gchar *errorstr; we used 'char' type everywhere and you yourself too in this patch, so lets just stick to 'char' everywhere, they are completely equivalent anyway. @@ +126,5 @@ > +{ > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (user_data)->priv; > + const gchar *errorstr; > + > + g_return_if_fail (service_resolver != NULL); Can this be NULL when this callback is called? I seriously doubt so. @@ +144,5 @@ > + > + break; > + } > + > + case AVAHI_RESOLVER_FOUND: { There is lots of LOC here, so would prefer if you break most (if not all) of it into another function, add_new_service(). @@ +156,5 @@ > + g_debug ("Service %s:%u resolved", > + host_name, > + port); > + > + service = avahi_new (AvahiServiceInfo, 1); As pointed out above, you want to make use of g_slice*(). @@ +166,5 @@ > + node = avahi_string_list_find (txt, "accuracy"); > + > + if (node == NULL) { > + g_warning ("No `accuracy` key inside TXT record"); > + break; * Empty line before break please. Could you ensure that about all break and return statements? * We had a back and fourth about the assertion that we shouldn't ignore the service if it's missing accuracy info. @@ +196,5 @@ > + > + service = (AvahiServiceInfo *) priv->all_services->data; > + > + /* FIXME: If a new service with better accuracy comes in, > + connect to it (if source is running). you also want a '*" at the beginning of this line, as per general conventions about mult-line comments. @@ +206,5 @@ > + > + g_debug ("No. of _nmea-0183._tcp services %u\n", > + n_services); > + } > + } misaligned. @@ +243,5 @@ > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (user_data)->priv; > + AvahiServiceInfo *service; > + const gchar *errorstr; > + > + g_return_if_fail (service_browser != NULL); Same here, is callback every called on NULL? @@ +281,5 @@ > + > + break; > + } > + > + case AVAHI_BROWSER_REMOVE: { similarly here, probably best to add another function, remove_service(). @@ +302,5 @@ > + g_debug ("No. of _nmea-0183._tcp services %u\n", > + n_services); > + > + avahi_free (service->identifier); > + avahi_free (service); you should be using the _free() function you defined above. @@ +303,5 @@ > + n_services); > + > + avahi_free (service->identifier); > + avahi_free (service); > + service = NULL; you always set this to NULL and then have some code below that only executes if it's non-NULL. @@ +306,5 @@ > + avahi_free (service); > + service = NULL; > + > + g_debug ("Avahi service browser state: REMOVE; service '%s' " > + "of type '%s' in domain '%s'", Pretty sure you can come up with some friendlier string. :) @@ +311,5 @@ > + name, > + type, > + domain); > + > + if (priv->all_services != NULL){ space before { @@ +323,5 @@ > + break; > + } > + > + set_accuracy_level (GCLUE_NMEA_SOURCE (user_data), > + service->accuracy); I think you should instead just add a refresh_accuracy_level() (similar to one you see in Web source) which sets the accuracy to accuracy of first element in the list, if it's not the same as current accuracy. @@ +366,5 @@ > + return; > + } > + g_debug ("Network source sent: \"%s\"", message); > + > + if (g_str_has_prefix (message, "$GPGGA")) { Since non-GGA sentence isn't currently entertained, I'd you rather just do: if (!g_str_has_prefix (message, "$GPGGA")) { /* FIXME: Handle other useful NMEA sentences too */ g_debug ("Ignoring non-GGA sentence from NMEA source"); goto READ_NEXT_LINE; } // GGA handling code goes here. ... READ_NEXT_LINE: g_data_input... @@ +378,5 @@ > + (GCLUE_LOCATION_SOURCE (source), location); > + } > + } else { > + g_warning ("The data sent was not in the form of " > + "NMEA GGA sentence"); It's not at all a problem if remote sends other NMEA sentences, we just don't handle them at the moment. @@ +420,5 @@ > + G_PRIORITY_DEFAULT, > + source->priv->cancellable, > + on_read_gga_sentence, > + source); > + g_clear_object (&data_input_stream); No need to ensure clearing of local variables (especially just before their scope ends), just use plain old g_object_unref() @@ +494,5 @@ > + 0, > + browse_callback, > + source); > + > + errorstr = avahi_strerror (avahi_client_errno (priv->avahi_client)); Why was this taken out of the if block below? We only want to do this if there was an error. You want to instead move the errorstr declaration inside the if block and make use of error variable too perhaps?.
Comment on attachment 117452 [details] [review] Add GClueNMEASource Review of attachment 117452 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +33,5 @@ > +#include <avahi-common/malloc.h> > +#include <avahi-common/error.h> > +#include <avahi-glib/glib-watch.h> > + > +typedef struct AvahiServiceInfo AvahiServiceInfo; It's because I'm using AvahiServiceInfo inside _GClueNMEASourcePrivate so it had to be declared before _GClueNMEASourcePrivate. @@ +126,5 @@ > +{ > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (user_data)->priv; > + const gchar *errorstr; > + > + g_return_if_fail (service_resolver != NULL); Same case, sample code checks for it. @@ +206,5 @@ > + > + g_debug ("No. of _nmea-0183._tcp services %u\n", > + n_services); > + } > + } It's because case and switch are on same indent level. What can be done of it? @@ +303,5 @@ > + n_services); > + > + avahi_free (service->identifier); > + avahi_free (service); > + service = NULL; It is being set to the first element of all_services.
Comment on attachment 117450 [details] [review] Create gclue_location_create_from_gga Review of attachment 117450 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +223,5 @@ > + return 300; > +} > + > +static gdouble > +parse_coordinate_string (const char *coordinate, It would be good, for this function to: - show an example of a well-formed string it can parse - explain which sections we're parsing in the code - make the function non-static (but still not exported by the library) and add a unit test for it This goes for pretty much every parsing function as complicated as this one. @@ +284,5 @@ > +} > + > +/** > + * gclue_location_create_from_gga: > + * @gga NMEA GGA sentence @gga: NMEA GGA sentence @@ +285,5 @@ > + > +/** > + * gclue_location_create_from_gga: > + * @gga NMEA GGA sentence > + * @error a GIOErrorEnum, it's value is set to G_IO_ERROR_INVALID_ARGUMENT And it's missing a ":" separating the name from the description @@ +286,5 @@ > +/** > + * gclue_location_create_from_gga: > + * @gga NMEA GGA sentence > + * @error a GIOErrorEnum, it's value is set to G_IO_ERROR_INVALID_ARGUMENT > + * whenever the gga sentence can't be parsed properly. @gga @@ +309,5 @@ > + "Invalid NMEA GGA sentence"); > + goto out; > + } > + > + /* For syntax of GGA senentences: sentences
Comment on attachment 117451 [details] [review] Add Avahi dependency Review of attachment 117451 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +110,5 @@ > +AC_ARG_ENABLE(nmea-source, > + AS_HELP_STRING([--disable-nmea-source], [Disable network NMEA source (requires avahi-client and avahi-glib)]), > + [build_nmea_source=$enableval], [build_nmea_source=yes]) > + > +if test "$build_nmea_source" = "yes"; then The usual way to do this is: if test x$build_nmea_source = xyes; then ... So as to avoid syntax errors when $build_nmea_source is an empty string. @@ +115,5 @@ > + AC_DEFINE([GCLUE_USE_NMEA_SOURCE], [1], [Build network NMEA source?]) > +else > + AC_DEFINE([GCLUE_USE_NMEA_SOURCE], [0], [Build network NMEA source?]) > +fi > +AM_CONDITIONAL([BUILD_NMEA_SOURCE], [test "$build_nmea_source" = "yes"]) And again. @@ +117,5 @@ > + AC_DEFINE([GCLUE_USE_NMEA_SOURCE], [0], [Build network NMEA source?]) > +fi > +AM_CONDITIONAL([BUILD_NMEA_SOURCE], [test "$build_nmea_source" = "yes"]) > + > +if test "$build_nmea_source" = "yes"; then And again.
Comment on attachment 117452 [details] [review] Add GClueNMEASource Review of attachment 117452 [details] [review]: ----------------------------------------------------------------- ::: src/Makefile.am @@ +127,5 @@ > libgeoclue_la_SOURCES += gclue-modem-gps.c gclue-modem-gps.h > endif > > +if BUILD_NMEA_SOURCE > +libgeoclue_la_SOURCES += gclue-nmea-source.h gclue-nmea-source.c Those 2 also need to go in EXTRA_DIST ::: src/gclue-locator.c @@ +302,5 @@ > locator->priv->sources = g_list_append (locator->priv->sources, gps); > submit_source = GCLUE_LOCATION_SOURCE (gps); > #endif > +#if GCLUE_USE_NMEA_SOURCE > + GClueNMEASource *nmea = gclue_nmea_source_get_singleton (); You might be defining a new variable in the middle of a block. Either split up the variable definition, and put it up top the function, or start a new block with "{" "}" ::: src/gclue-nmea-source.c @@ +71,5 @@ > + AvahiServiceInfo *service = (AvahiServiceInfo *) data; > + > + avahi_free (service->identifier); > + avahi_free (service->host_name); > + avahi_free (service); g_free(NULL) works just fine (it does nothing). @@ +75,5 @@ > + avahi_free (service); > +} > + > +static gint > +compare_avahi_service_by_name (gconstpointer a, _by_identifier ? @@ +95,5 @@ > + > + first = (AvahiServiceInfo *) a; > + second = (AvahiServiceInfo *) b; > + > + return second->accuracy - first->accuracy; This isn't quite correct, as the cast to gint when you get out of the function will round the value, so you'd get wrong answers for differences < 1. So: diff = second->accuracy - first->accuracy; if (diff == 0.0) return 0; return diff > 0 ? 1 : -1; @@ +169,5 @@ > + g_warning ("No `accuracy` key inside TXT record"); > + break; > + } > + > + parts = g_strsplit ((char *) node->text, "=", -1); You want to use avahi_string_list_get_pair() here. @@ +181,5 @@ > + enum_value = g_enum_get_value_by_nick (enum_class, parts[1]); > + g_type_class_unref (enum_class); > + > + if (enum_value == NULL) { > + g_warning ("Error in parsing TXT record for accuracy"); Correct this warning to accurately represent what failed. @@ +206,5 @@ > + > + g_debug ("No. of _nmea-0183._tcp services %u\n", > + n_services); > + } > + } Move the code from within the case to a separate function, as Zeeshan mentioned above. @@ +543,5 @@ > + > + /* The service with the highest accuracy will be stored in the beginning > + of the list. > + */ > + if (priv->all_services != NULL){ Probably missing a space after the parenthesis here.
Comment on attachment 117453 [details] [review] Add config option for NMEA source Review of attachment 117453 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-locator.c @@ +305,4 @@ > submit_source = GCLUE_LOCATION_SOURCE (gps); > #endif > #if GCLUE_USE_NMEA_SOURCE > + if (gclue_config_get_enable_nmea_source (gconfig)) { Ha! Here's the block I was asking about in an earlier patch. Then use "{" to start your block in your earlier patch, and this hunk will be a one-liner.
Comment on attachment 117452 [details] [review] Add GClueNMEASource Review of attachment 117452 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-nmea-source.c @@ +33,5 @@ > +#include <avahi-common/malloc.h> > +#include <avahi-common/error.h> > +#include <avahi-glib/glib-watch.h> > + > +typedef struct AvahiServiceInfo AvahiServiceInfo; ah ok. I missed that fact. :) @@ +71,5 @@ > + AvahiServiceInfo *service = (AvahiServiceInfo *) data; > + > + avahi_free (service->identifier); > + avahi_free (service->host_name); > + avahi_free (service); hmm.. there is no g_free0() actually. I don't know why I thought there is a separate g_free() that's NULL resilient.. @@ +126,5 @@ > +{ > + GClueNMEASourcePrivate *priv = GCLUE_NMEA_SOURCE (user_data)->priv; > + const gchar *errorstr; > + > + g_return_if_fail (service_resolver != NULL); Let's not blindly follow samples. Could you ask on avahi chat and/or mailing list? Bonus points if you can check the source code of avahi too. @@ +303,5 @@ > + n_services); > + > + avahi_free (service->identifier); > + avahi_free (service); > + service = NULL; Ah i see now. Please move this assignment to an else block of the 'if' statement below where its set.
Comment on attachment 117450 [details] [review] Create gclue_location_create_from_gga Review of attachment 117450 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +223,5 @@ > + return 300; > +} > + > +static gdouble > +parse_coordinate_string (const char *coordinate, in this patch, Ankit is simply moving this function from another module. I don't think all these improvement ideas should be part of this patch or even this bug.
Comment on attachment 117451 [details] [review] Add Avahi dependency Review of attachment 117451 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +110,5 @@ > +AC_ARG_ENABLE(nmea-source, > + AS_HELP_STRING([--disable-nmea-source], [Disable network NMEA source (requires avahi-client and avahi-glib)]), > + [build_nmea_source=$enableval], [build_nmea_source=yes]) > + > +if test "$build_nmea_source" = "yes"; then Good point, I'll update existing code too. Blame Ryan Lortie. :)
Created attachment 117733 [details] [review] Create gclue_location_create_from_gga This patch creates gclue_location_create_from_gga function inside GClueLocation which will be able to create a GClueLocation object out of a NMEA GGA sentence. This function works with the help of functions: get_accuracy_from_hdop, parse_coordinate_string and parse_altitude_string which are moved from gclue-modem-gps.c.
Created attachment 117734 [details] [review] Add Avahi dependency Avahi will be required by GClueNMEASource, which will be added in the coming patches. Avahi will help GClueNMEASource to automatically discover nmea-tcp services on the local network.
Created attachment 117735 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. It uses Avahi for the auto-discovery of nmea-tcp services on local network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Created attachment 117736 [details] [review] Add config option for NMEA source This patch adds configuration option which will tell us whether we want to use GClueNMEASource or not.
Created attachment 117755 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. It uses Avahi for the auto-discovery of nmea-tcp services on local network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones.
Created attachment 117756 [details] [review] Add config option for NMEA source This patch adds configuration option which will tell us whether we want to use GClueNMEASource or not.
Created attachment 117857 [details] [review] Add gclue_location_create_from_gga() Add gclue_location_create_from_gga() the creates a GClueLocation object out of an NMEA GGA sentence. This function works with the help of get_accuracy_from_hdop(), parse_coordinate_string() and parse_altitude_string() that are moved from gclue-modem-gps.c.
Created attachment 117858 [details] [review] Add GClueNMEASource GClueNMEASource receives NMEA GGA sentences from a NMEA source on the network. It uses Avahi for the auto-discovery of nmea-tcp services on local network. This will enable us to receive location info from other devices on the local network with location capabilities, such as smartphones. This patch add dependency on avahi-glib and avahi-client.
Created attachment 117859 [details] [review] Add config option for NMEA source Add configuration option to enable/disable Network NMEA source. Default is enabled.
I re-worked you patches a bit. Some issues still: 1. I'm not getting location at all from your app on my phone. :( I see the location icon in system try (on the phone) going solid (not blinking anymore) and "Location set by GPS" notification so I'm pretty sure location is fetched by the phone but somehow it's not getting to geoclue. 2. * warnings on geoclue console on client disconnect: (geoclue:25452): Geoclue-WARNING **: Error when receiving message (geoclue:25452): Geoclue-WARNING **: Operation was cancelled Cancellation is done by us so G_IO_ERROR_CLOSED should be ignored.
Oh and i see that GeoclueShare keeping working when in the background (which is really good thing) but it really should provide a notification in system tray that it's running/working so that users don't forget it running and drain the life out of the battery in minutes.
Created attachment 117863 [details] [review] Add GClueNMEASource Fixed the warnings on client disconnecting.
Created attachment 117864 [details] [review] Add config option for NMEA source Rebased.
Attachment 117857 [details] pushed as 22b6274 - Add gclue_location_create_from_gga() Attachment 117863 [details] pushed as 9e04fe7 - Add GClueNMEASource Attachment 117864 [details] pushed as 118ffd3 - Add config option for NMEA source
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.