search

fluttercommunity / plus_plugins

Flutter Community Plus Plugins
BSD 3-Clause "New" or "Revised" License
1.5k stars 881 forks source link

fix(sensors_plus): corrected iOS magnetometer source #3019

Open Zabadam opened 3 weeks ago

Zabadam commented 3 weeks ago

Obtain magnetometer events from DeviceMotion rather than raw startMagnetometerUpdates(). This also makes the magnetometer onCancel() method now correct.1 Added set showsDeviceMovementDisplay to true for magnetometer.

1 While working on the changes, I noticed the current implementation in onCancel() calls _motionManager.stopDeviceMotionUpdates(). This is interesting, because the correct course is to obtain events from MotionManager, but the current implementation is not. Because it currently calls startMagnetometerUpdates(), the onCancel() method ought to call stopMagnetometerUpdates(). Current users of sensors_plus, after obtaining magnetometer readings, could never have the sensor correctly canceled, to my understanding.

At any rate, the onCancel gets to stay as it is while the source of the events is corrected.

Description

When this package was converted to Swift, a previous fix that corrected magnetometer source to DeviceMotion was reverted to using startMagnetometerUpdates().

This pull aims to restore the calibrated magnetometer readings that users are expecting.

As before, I have no way to test this code (no iOS device), and would appreciate someone's assitance in verifying the functionality.

Related Issues

Checklist

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Zabadam commented 3 weeks ago

I dug into that test file at sensors_plus\example\integration_test\sensors_plus_test.dart and saw this:

Failing test ```dart void main() { IntegrationTestWidgetsFlutterBinding.ensureInitialized(); // NOTE: The accelerometer events are not returned on iOS simulators. testWidgets('Can subscribe to accelerometerEvents and get non-null events', (WidgetTester tester) async { final completer = Completer(); late StreamSubscription subscription; subscription = accelerometerEventStream().listen((AccelerometerEvent event) { completer.complete(event); subscription.cancel(); }); expect(await completer.future, isNotNull); }, skip: !Platform.isAndroid); } ```

So perhaps this test was expected to fail regardless of my changes. (My changes are not tested by subscription = accelerometerEventStream().listen(...) and the comment describes the simulator's downfall.)

Thus it would still be beneficial for someone to manually test this change:

dependencies
  # Testing calibrated magnetometer on iOS (https://github.com/fluttercommunity/plus_plugins/pull/3019)
  sensors_plus:
    git:
      url: https://github.com/Zabadam/plus_plugins.git
      ref: 1e2248d # Branch mag-sensor-ios
      path: packages/sensors_plus/sensors_plus

Edit: It did occur to me that some error may be unhandled due to the inclusion of the showsDeviceMovementDisplay flag set, but I struggled to find any such handling in a comparison with peer zesage/motion_sensors who also indeed sets the flag to true.

vbuberen commented 3 weeks ago

Thanks a lot for your contribution. I will try to test the change by the end of the week.

Zabadam commented 2 weeks ago

It seems that @Silverviql has provided confirmation of corrected functionality. Happy to hear this.

vbuberen commented 6 days ago

Sorry for not getting back in the promised timeline. Will do some additional testing myself today for sure.

vbuberen commented 5 days ago

Finally got to this PR to test it and do some research.

When this package was converted to Swift, a https://github.com/fluttercommunity/plus_plugins/pull/812 source to DeviceMotion was reverted to using startMagnetometerUpdates().

This is not the case. In fact, this change was done on purpose. See https://github.com/fluttercommunity/plus_plugins/pull/2248 and https://github.com/fluttercommunity/plus_plugins/pull/2250

However, I see that the calibrated values provided in your branch are actually the ones users might want to get. Also compared the output with a few apps obtaining magnetometer data and see that with changes from this PR getting same output.

Unfortunately, I see that the problem mentioned in #2250 with userAccelerometer and magnetometer sharing sampling rate is back with these changes, so there we need to find some solution to avoid this issue before releasing changes with this PR.

I will do some additional research in the nearest days to see what we can do here.

Zabadam commented 3 days ago

When this package was converted to Swift, a https://github.com/fluttercommunity/plus_plugins/pull/812 source to DeviceMotion was reverted to using startMagnetometerUpdates().

This is not the case. In fact, this change was done on purpose. See https://github.com/fluttercommunity/plus_plugins/pull/2248 and https://github.com/fluttercommunity/plus_plugins/pull/2250

Better phrasing: "At some point after this package was converted to using Swift . . ." I was gone a while and, without the full research on newer PRs, was trying to reason about a sort of deja-vu feeling. "Did I or didn't I get this corrected last time?" 😅 I never did receive full confirmation of fix. So, thank you for the full timeline.

However, I see that the calibrated values provided in your branch are actually the ones users might want to get. Also compared the output with a few apps obtaining magnetometer data and see that with changes from this PR getting same output.

It seems to be the case overwhelmingly, yes, and it's excellent to hear the results match expectations after this PR.

Unfortunately, I see that the problem mentioned in #2250 with userAccelerometer and magnetometer sharing sampling rate is back with these changes, so there we need to find some solution to avoid this issue before releasing changes with this PR.

I need to familiarize myself with the sampling rate code before I could be of further assistance here.