Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a new feature to receive NMEA messages #605

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Conversation

Sempakonka
Copy link
Contributor

@Sempakonka Sempakonka commented Nov 24, 2020

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

This PR introduces a new feature.

⤵️ What is the current behavior?

Currently you can not receive NMEA messages.

🆕 What is the new behavior (if this is a feature change)?

With this PR, you are able to recieve NMEA mesages

Example

StreamSubscription<NmeaMessage> nmeaStream = getNmeaStream()
    .listen((NmeaMessage nmea) {
      // Handle NMEA changes
    });

This stream will continually give NMEA updates

💥 Does this PR introduce a breaking change?

No

🐛 Recommendations for testing

Use the example app ;)

📝 Links to relevant issues/docs

fixes #561

🤔 Checklist before submitting

  • All projects build
  • Follows style guide lines (code style guide)
  • Relevant documentation was updated
  • Rebased onto current develop

@Sempakonka Sempakonka changed the title Issue#516 Added a new feature to receive NMEA messages Nov 24, 2020
@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #605 (818653f) into master (9dc3f8d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #605    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           15         1    -14     
  Lines          258        43   -215     
==========================================
- Hits           258        43   -215     
Impacted Files Coverage Δ
geolocator/lib/geolocator.dart 100.00% <100.00%> (ø)
...terface/lib/src/extensions/integer_extensions.dart
...orm_interface/lib/src/models/location_options.dart
...or_platform_interface/lib/src/models/position.dart
...e/lib/src/errors/invalid_permission_exception.dart
...rc/errors/location_service_disabled_exception.dart
...ce/lib/src/errors/permission_denied_exception.dart
...m_interface/lib/src/enums/location_permission.dart
...face/lib/src/errors/position_update_exception.dart
...e/lib/src/errors/already_subscribed_exception.dart
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dc3f8d...818653f. Read the comment docs.

@@ -1,3 +1,5 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.baseflow.geolocator">

<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be removed. It will probably give a warning which can be suppressed in code.

Permissions are the responsibility of the developers using the plugin in their App and should not be something we determine for them.

Suggested change
<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />

@@ -21,11 +21,17 @@

@Nullable private MethodCallHandlerImpl methodCallHandler;

@Nullable private StreamHandlerImpl streamHandler;
@Nullable
private PositionStreamImpl streamHandler;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you renamed the class StreamHandlerImpl to PositionStreamImpl, this name however suggests that the class is a stream dealing with positions. This is however not the case, the class implements all the logic necessary to create a stream but it is not the stream it self. So I suggest we rename it to PositionStreamHandlerImpl.

Also it would be nice to rename the variable streamHandler to positionStreamHandler making it a bit more semantic and easier to read code for other developers.

Suggested change
private PositionStreamImpl streamHandler;
private PositionStreamHandlerImpl positionStreamHandler;


@Nullable private Registrar pluginRegistrar;
@Nullable
private NmeaStreamImpl nmeaStream;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the above PositionStreamImpl. It would be better to rename this to:

Suggested change
private NmeaStreamImpl nmeaStream;
private NmeaStreamHandlerImpl nmeaStreamHandler;

this.geolocationManager = geolocationManager;
}

public static Map<String, Object> toMap(String n, Long l) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should be made private.

It's intention is to convert the NMEA result into a format that the Flutter EventChannel understands. Only the NmeaStreamHandlerImpl (after you renamed the class) is allowed to send messages on the NMEA EventChannel, meaning other code should be protected from knowing how to map a NMEA object to a map.

Suggested change
public static Map<String, Object> toMap(String n, Long l) {
private static Map<String, Object> toMap(String n, Long l) {

Comment on lines 82 to 99
public void startNmeaUpdates(Context context, Activity activity, NmeaMessageaClient client,
NmeaChangedCallback nmeaChangedCallback, ErrorCallback errorCallback) {

handlePermissions(
context,
activity,
() -> client.startNmeaUpdates(nmeaChangedCallback, errorCallback),
errorCallback);
}

public void stopNmeaUpdates(NmeaMessageaClient client) {
client.stopNmeaUpdates();
}

public NmeaMessageaClient createNmeaClient(Context context) {
return android.os.Build.VERSION.SDK_INT >= VERSION_CODES.N ? new GnssNmeaMessageClient(context)
: new GpsNmeaMessageClient(context);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to create a whole new class (in it's own sub folder, together with the NmeaMessageaClient, GnssNmeaMessageClient and GpsNmeaMessageClient classes) NmeaManager and put this logic there. It is not really related to location information and currently bloats the GeolocationManager class making it harder to understand what is going on in the GeolocationManager class.

The current implementation violates the Single-responsibility principle which says (more info here):

A class should have one and only one reason to change, meaning that a class should have only one job.

Currently this class has two reasons two change:

  1. When the implementation of requesting location information changes;
  2. When the implementation of requesting NMEA information changes.

I think it would be good to move the handlePermissions method into the PermissionManager class (and make it public). This way we don't have to duplicate this code in the GeolocationManager and NmeaManager class and we also don't need a reference from the NmeaManager class to the GeolocationManager class.

return _nmeaMessageStream;
}

var nmeaStream = nmeaChannel.receiveBroadcastStream();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nmeaStream variable can be made final, once it is initialized it does not change (and is should not change), by making it a final variable we can protect against this.

Suggested change
var nmeaStream = nmeaChannel.receiveBroadcastStream();
final nmeaStream = nmeaChannel.receiveBroadcastStream();


@immutable

///nmea message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should be written as sentences (see here and here for more details). You can also have a look at the Position class for inspiration.


///nmea message
class NmeaMessage {
///nmea message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should be written as sentences (see here and here for more details). You can also have a look at the Position class for inspiration.

///nmea message
NmeaMessage(this.message, this.timestamp);

///message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should be written as sentences (see here and here for more details). You can also have a look at the Position class for inspiration.

///message
final String message;

///timestamp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should be written as sentences (see here and here for more details). You can also have a look at the Position class for inspiration.

Comment on lines 271 to 278
/// StreamSubscription<NmeaMessage> nmeaStream = getNmeaStream()
/// .listen((NmeaMessage nmea) {
/// // Handle NMEA changes
/// });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code example should include static class:

Suggested change
/// StreamSubscription<NmeaMessage> nmeaStream = getNmeaStream()
/// .listen((NmeaMessage nmea) {
/// // Handle NMEA changes
/// });
/// StreamSubscription<NmeaMessage> nmeaStream = Geolocator.getNmeaStream()
/// .listen((NmeaMessage nmea) {
/// // Handle NMEA changes
/// });

Copy link
Member

@mvanbeusekom mvanbeusekom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating your PR. I think the code now looks good but still have some minor requests that should be fixed.

Comment on lines 56 to 64
StreamHandlerImpl streamHandler = new StreamHandlerImpl(geolocatorPlugin.geolocationManager);
PositionStreamHandlerImpl streamHandler = new PositionStreamHandlerImpl(geolocatorPlugin.geolocationManager);
streamHandler.startListening(registrar.context(), registrar.messenger());
streamHandler.setActivity(registrar.activity());

NmeaStreamHandlerImpl nmeaStream = new NmeaStreamHandlerImpl(geolocatorPlugin.nmeaMessageManager);
nmeaStream.startListening(registrar.context(), registrar.messenger());
nmeaStream.setActivity(registrar.activity());
}

@Override
public void onAttachedToEngine(@NonNull FlutterPluginBinding flutterPluginBinding) {
methodCallHandler = new MethodCallHandlerImpl(this.permissionManager, this.geolocationManager);
methodCallHandler.startListening(
flutterPluginBinding.getApplicationContext(), flutterPluginBinding.getBinaryMessenger());
streamHandler = new StreamHandlerImpl(this.geolocationManager);
streamHandler.startListening(
positionStreamHandler = new PositionStreamHandlerImpl(this.geolocationManager);
positionStreamHandler.startListening(
flutterPluginBinding.getApplicationContext(), flutterPluginBinding.getBinaryMessenger());
nmeaStreamHandler = new NmeaStreamHandlerImpl(this.nmeaMessageManager);
nmeaStreamHandler.startListening(
flutterPluginBinding.getApplicationContext(), flutterPluginBinding.getBinaryMessenger());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is time to simplify this block of code and do some more code sharing. I think we can replace the registerWith, onAttachedToEngine and onAttachedToActivity with the following methods:

Improved registerWith

  // This static function is optional and equivalent to onAttachedToEngine. It supports the old
  // pre-Flutter-1.12 Android projects. You are encouraged to continue supporting
  // plugin registration via this function while apps migrate to use the new Android APIs
  // post-flutter-1.12 via https://flutter.dev/go/android-project-migration.
  //
  // It is encouraged to share logic between onAttachedToEngine and registerWith to keep
  // them functionally equivalent. Only one of onAttachedToEngine or registerWith will be called
  // depending on the user's project. onAttachedToEngine or registerWith must both be defined
  // in the same class.
  public static void registerWith(Registrar registrar) {
    GeolocatorPlugin geolocatorPlugin = new GeolocatorPlugin();
    geolocatorPlugin.pluginRegistrar = registrar;
    geolocatorPlugin.configureListeners(registrar.context(), registrar.messenger());
    geolocatorPlugin.setActivity(registrar.activity());
  }

Improved onAttachedToEngine

  @Override
  public void onAttachedToEngine(@NonNull FlutterPluginBinding flutterPluginBinding) {
    configureListeners(flutterPluginBinding.getApplicationContext(), flutterPluginBinding.getBinaryMessenger());
  }

Improved onAttachedToActivity

  @Override
  public void onAttachedToActivity(@NonNull ActivityPluginBinding binding) {
    this.pluginBinding = binding;
    setActivity(binding.getActivity());
  }

Improved onDetachedFromActivity

  @Override
  public void onDetachedFromActivity() {
    setActivity(null);
  }

New method configureListeners

  void configureListeners(Context applicationContext, BinaryMessenger messenger) {
    methodCallHandler =
        new MethodCallHandlerImpl(permissionManager, geolocationManager);
    methodCallHandler.startListening(applicationContext, messenger);

    positionStreamHandler = new PositionStreamHandlerImpl(geolocationManager);
    positionStreamHandler.startListening(applicationContext, messenger);

    nmeaStreamHandler = new NmeaStreamHandlerImpl(nmeaMessageManager);
    nmeaStreamHandler.startListening(applicationContext, messenger);
  }

New method setActivity

  void setActivity(@Nullable Activity activity) {
    if (methodCallHandler != null) {
      methodCallHandler.setActivity(activity);
    }
    if (positionStreamHandler != null) {
      positionStreamHandler.setActivity(activity);
    }
    if (nmeaStreamHandler != null) {
      nmeaStreamHandler.setActivity(activity);
    }
    
    if (activity != null) {
      registerListeners();
    } else {
      deregisterListeners();
    }
  }

This will remove some duplicated code and also solves confusion with variable names.


class NmeaStreamHandlerImpl implements EventChannel.StreamHandler {

private static final String TAG = "NmeaStreamImpl";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename TAG to match class name:

Suggested change
private static final String TAG = "NmeaStreamImpl";
private static final String TAG = "NmeaStreamHandlerImpl";

this.nmeaMessageManager = nmeaMessageManager;
}

private static Map<String, Object> toMap(String n, Long l) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide clear self-explanatory variable names instead of using single characters:

Suggested change
private static Map<String, Object> toMap(String n, Long l) {
private static Map<String, Object> toMap(String message, long timestamp) {

return null;
}

Map<String, Object> Nmea = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local variables should not start with a capital:

Suggested change
Map<String, Object> Nmea = new HashMap<>();
Map<String, Object> nmeaMap = new HashMap<>();

context,
activity,
this.nmeaMessageaClient,
(String n, long l) -> events.success(toMap(n, l)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide clear self-explanatory variable name instead of a single character:

Suggested change
(String n, long l) -> events.success(toMap(n, l)),
(String message, long timestamp) -> events.success(toMap(message, timestamp)),


@SuppressLint("MissingPermission")
@Override
public void onProviderDisabled(String s) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide self-explanatory variable names instead of single character variable names:

Suggested change
public void onProviderDisabled(String s) {
public void onProviderDisabled(String provider) {

Comment on lines 37 to 39
return android.os.Build.VERSION.SDK_INT >= VERSION_CODES.N ? new GnssNmeaMessageClient(context)
: new GpsNmeaMessageClient(context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: formatting can be little bit improved:

Suggested change
return android.os.Build.VERSION.SDK_INT >= VERSION_CODES.N ? new GnssNmeaMessageClient(context)
: new GpsNmeaMessageClient(context);
return android.os.Build.VERSION.SDK_INT >= VERSION_CODES.N
? new GnssNmeaMessageClient(context)
: new GpsNmeaMessageClient(context);

Comment on lines 264 to 266
/// Returns a stream emitting NMEA-0183 sentences when they are received from
/// the GNSS engine. With devices running a Android API level lower than 24
/// NMEA-0183 sentences are received from the GPS engine.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the explanation is very clean but Dart documentation guidelines prescribe the first sentence to be short and compact. So maybe we can update this to:

Suggested change
/// Returns a stream emitting NMEA-0183 sentences when they are received from
/// the GNSS engine. With devices running a Android API level lower than 24
/// NMEA-0183 sentences are received from the GPS engine.
/// Returns a stream emitting NMEA-0183 sentences (Android only, no-op on iOS).
///
/// With devices running a Android API level lower than 24 NMEA-0183 sentences
/// are received from the GPS engine. From API level 24 and up the GNSS engine
/// is used.

Comment on lines +111 to +113
/// Returns a stream emitting NMEA-0183 sentences when they are received from
/// the GNSS engine. With devices running a Android API level lower than 24
/// NMEA-0183 sentences are received from the GPS engine.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the explanation is very clean but Dart documentation guidelines prescribe the first sentence to be short and compact. So maybe we can update this to:

Suggested change
/// Returns a stream emitting NMEA-0183 sentences when they are received from
/// the GNSS engine. With devices running a Android API level lower than 24
/// NMEA-0183 sentences are received from the GPS engine.
/// Returns a stream emitting NMEA-0183 sentences (Android only, no-op on iOS).
///
/// With devices running a Android API level lower than 24 NMEA-0183 sentences
/// are received from the GPS engine. From API level 24 and up the GNSS engine
/// is used.

Comment on lines 25 to 31
EventChannel eventChannel =
EventChannel('flutter.baseflow.com/geolocator_updates');

/// The event channel used to receive [NmeaMessage] updates from the native
/// platform.
@visibleForTesting
EventChannel nmeaChannel = EventChannel('flutter.baseflow.com/nmea_updates');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the implementation on the JAVA side, I think here we should also use clear naming to differentiate between the event channel broadcasting positions and the event channel broadcasting NMEA messages:

Suggested change
EventChannel eventChannel =
EventChannel('flutter.baseflow.com/geolocator_updates');
/// The event channel used to receive [NmeaMessage] updates from the native
/// platform.
@visibleForTesting
EventChannel nmeaChannel = EventChannel('flutter.baseflow.com/nmea_updates');
EventChannel positionEventChannel =
EventChannel('flutter.baseflow.com/geolocator_updates');
/// The event channel used to receive [NmeaMessage] updates from the native
/// platform.
@visibleForTesting
EventChannel nmeaEventChannel = EventChannel('flutter.baseflow.com/nmea_updates');

Sempakonka and others added 5 commits December 1, 2020 12:00
…to issue#516

� Conflicts:
�	geolocator/CHANGELOG.md
�	geolocator/README.md
�	geolocator/example/lib/main.dart
�	geolocator/pubspec.yaml
�	geolocator_platform_interface/CHANGELOG.md
@parcodepie
Copy link

Any news about this feature please? is it ready to be used in Flutter?

@rickcasson rickcasson mentioned this pull request Jan 6, 2022
6 tasks
@Wackymax Wackymax mentioned this pull request Apr 11, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting the number of satellites
4 participants