-
-
Notifications
You must be signed in to change notification settings - Fork 972
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
fix(sensors_plus): corrected iOS magnetometer source #3019
base: main
Are you sure you want to change the base?
Conversation
I dug into that test file at Failing testvoid 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<AccelerometerEvent>();
late StreamSubscription<AccelerometerEvent> 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 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 |
Thanks a lot for your contribution. I will try to test the change by the end of the week. |
It seems that @Silverviql has provided confirmation of corrected functionality. Happy to hear this. |
Sorry for not getting back in the promised timeline. |
…netometerUpdates(). This also makes the magnetometer onCancel() method now correct. Added set 'showsDeviceMovementDisplay' to true for magnetometer.
Finally got to this PR to test it and do some research.
This is not the case. In fact, this change was done on purpose. See #2248 and #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 I will do some additional research in the nearest days to see what we can do here. |
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.
It seems to be the case overwhelmingly, yes, and it's excellent to hear the results match expectations after this PR.
I need to familiarize myself with the sampling rate code before I could be of further assistance here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and the values are correct on IOS now.
This comment has been minimized.
This comment has been minimized.
This is not the place for a bug report. Open an issue or provide info in the existing one |
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 callsstartMagnetometerUpdates()
, theonCancel()
method ought to callstopMagnetometerUpdates()
. 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 usingstartMagnetometerUpdates()
.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
CHANGELOG.md
nor the plugin version inpubspec.yaml
files.flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
!
in the title as explained in Conventional Commits).