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

fix(sensors_plus): iOS calibrated magnetometer #812

Merged
merged 12 commits into from
Sep 27, 2022
Merged

fix(sensors_plus): iOS calibrated magnetometer #812

merged 12 commits into from
Sep 27, 2022

Conversation

Zabadam
Copy link
Contributor

@Zabadam Zabadam commented Mar 31, 2022

Description

iOS: Corrects magnetometer implementation, returning calibrated values from DeviceMotion sensor rather than raw sensor samples.

As the user acceleration implementation already employs this calibrated DeviceMotion sensor, it seems like an appropriate solution to acquiring compensated magnetometer data as well.

melos run format made changes to a huge number of files across all packages. I discarded all changes outside of packages\sensors_plus\.

The focus of this PR is the magnetometer implementation at the end of sensors_plus\sensors_plus\ios\Classes\FLTSensorsPlusPlugin.m.

I kindly request verification of this proposed solution, specifically as I have no means to test it myself without an iOS device, and also because I am uncertain of my Objective-C code.

Related Issues

#781 Magnetometer values are off

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated the version in pubspec.yaml and CHANGELOG.md.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR [for sensors_plus].
  • I read and followed the Flutter Style Guide.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

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

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change, considering the previous output was technically not the expected output. Though if a developer worked around this gap previously in their code, this change may require some refactoring: 1.3.0 -> 1.4.0.

Added necessary Android, iOS, and web API for magnetometer sensor.

Adds new `MagnetometerEvent` class
(and fixes small typo in constructor of other `FooEvent` classes).

Adds new `Stream<MagnetometerEvent>` called `magnetometerEvents`.
- Adds necessary Android, iOS, and web API for magnetometer sensor.

- Adds new `MagnetometerEvent` class.

- Adds new `Stream<MagnetometerEvent>` called `magnetometerEvents`.

(This commit compared to an earlier one reverts any changes that were made that were unrelated to added magnetometer support.)
"Contructs" -> "Constructs"
&
"premission" -> "permission"
Attempting to pass PR tests.
As described in #781 the current implementation of **magnetometer** sensor data acquisition for iOS does not use calibrated values evaluated by iOS's `DeviceMotion` sensor, but rather the raw samples straight from the sensor.

As the **user acceleration** implementation already employs this *calibrated* `DeviceMotion` sensor, it seems like an appropriate solution to acquiring compensated magnetometer data as well.

`melos run format` made changes to a huge number of files across all packages. I discarded all changes outside of `packages\sensors_plus\`.

The focus of this PR is  the magnetometer implementation at the end of `sensors_plus\sensors_plus\ios\Classes\FLTSensorsPlusPlugin.m`.
@Zabadam
Copy link
Contributor Author

Zabadam commented Mar 31, 2022

Force-push amended some formatting and analysis discrepancies between local melos output and the checks performed here.

@Zabadam Zabadam marked this pull request as ready for review March 31, 2022 06:14
@Zabadam
Copy link
Contributor Author

Zabadam commented Mar 31, 2022

I have realized it would have been cleaner to create a brand-new branch instead of updating my last development branch--less commits. I am still learning!

Copy link
Member

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

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

Thanks @Zabadam LGTM, any chance you can fix the merge conflicts?

@miquelbeltran miquelbeltran self-assigned this Apr 11, 2022
@vbuberen
Copy link
Collaborator

@Zabadam Maybe you have missed last message, but could you fix merge conflicts in this PR?

@Zabadam
Copy link
Contributor Author

Zabadam commented May 16, 2022

Oh, I definitely missed that! I could indeed do as requested, but I must note that the original Issue raised that this PR was meant to rectify was allegedly not fixed by this PR.
I did not get word back from that user if they had fully cleaned their cache when testing the change.

That said, these changes are probably still in the best interest of the package, as they match other similar implementations of iOS sensor data with Flutter (i.e. using DeviceMotion).

While prior merge conflict was resolved via github.com, this push ran true melos format command.

Abbreviations were made to comment to accommodate the smaller formatted space.
@Zabadam
Copy link
Contributor Author

Zabadam commented Sep 24, 2022

At long last, this PR is truly ready. Thank you for your patience.

Side-note: I do have more hopes/visions for this package. Other similar packages have more features but less platform support.
A true major bump would bring the remaining platforms as well as expected features like setting sampling rate and checking for sensor availability.

@miquelbeltran
Copy link
Member

I am checking this PR locally and it doesn't build for me, the errors I get are:

Xcode's output:
↳
    Writing result bundle at path:
    	/var/folders/l0/ry6db4m546166m22b1ytssm40000gn/T/flutter_tools.OnfG04/flutter_ios_build_temp_dirB5HNBV/temporary_xcresult_bundle

    /Users/miquel/dev/projects/fluttercommunity/plus_plugins/packages/sensors_plus/sensors_plus/ios/Classes/FLTSensorsPlusPlugin.m:95:24: warning: this old-style function definition is not preceded by a prototype
    [-Wstrict-prototypes]
    void _initMotionManager() {
                           ^
    /Users/miquel/dev/projects/fluttercommunity/plus_plugins/packages/sensors_plus/sensors_plus/ios/Classes/FLTSensorsPlusPlugin.m:224:7: error: no visible @interface for 'CMMotionManager' declares the selector
    'startDeviceMotionUpdatesUsingReferenceFrame:ToQueue:withHandler:'
          startDeviceMotionUpdatesUsingReferenceFrame:
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1 warning and 1 error generated.
    /Users/miquel/dev/projects/fluttercommunity/plus_plugins/packages/sensors_plus/sensors_plus/ios/Classes/FLTSensorsPlusPlugin.m:95:24: warning: this old-style function definition is not preceded by a prototype
    [-Wstrict-prototypes]
    void _initMotionManager() {
                           ^
    /Users/miquel/dev/projects/fluttercommunity/plus_plugins/packages/sensors_plus/sensors_plus/ios/Classes/FLTSensorsPlusPlugin.m:224:7: error: no visible @interface for 'CMMotionManager' declares the selector
    'startDeviceMotionUpdatesUsingReferenceFrame:ToQueue:withHandler:'
          startDeviceMotionUpdatesUsingReferenceFrame:
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1 warning and 1 error generated.
    note: Building targets in dependency order
    warning: Run script build phase 'Run Script' will be run during every build because it does not specify any outputs. To address this warning, either add output dependencies to the script phase, or configure it
    to run in every build by unchecking "Based on dependency analysis" in the script phase. (in target 'Runner' from project 'Runner')
    warning: Run script build phase 'Thin Binary' will be run during every build because it does not specify any outputs. To address this warning, either add output dependencies to the script phase, or configure it
    to run in every build by unchecking "Based on dependency analysis" in the script phase. (in target 'Runner' from project 'Runner')

    Result bundle written to path:
    	/var/folders/l0/ry6db4m546166m22b1ytssm40000gn/T/flutter_tools.OnfG04/flutter_ios_build_temp_dirB5HNBV/temporary_xcresult_bundle


ARC Semantic Issue (Xcode): No visible @interface for 'CMMotionManager' declares the selector 'startDeviceMotionUpdatesUsingReferenceFrame:ToQueue:withHandler:'
/Users/miquel/dev/projects/fluttercommunity/plus_plugins/packages/sensors_plus/sensors_plus/ios/Classes/FLTSensorsPlusPlugin.m:223:6


Could not build the application for the simulator.
Error launching application on iPhone 14.

(seems that CI iOS build job didn't catch this up, so we need to review that as well)

@Zabadam
Copy link
Contributor Author

Zabadam commented Sep 26, 2022

I am checking this PR locally and it doesn't build for me, the errors I get are:

Thank you so much for helping me test and verify. I am especially grateful as I really have no means to check these fixes myself.

plus_plugins/packages/sensors_plus/sensors_plus/ios/Classes/FLTSensorsPlusPlugin.m:95:24:
    warning: this old-style function definition is not preceded by a prototype
        void _initMotionManager() {

This portion of code receives a warning, but the work was done before this PR. [Note 1] To the best that I can ascertain through my research, it seems in 2021 an XCode/iOS update started flagging empty parameter lists.

What makes this an "old-style function?"
So the solution there is likely either void _initMotionManager(void) {} or void _initMotionManager {}.
Would the former correct the "old-style function" while the latter is a modern style function?

plus_plugins/packages/sensors_plus/sensors_plus/ios/Classes/FLTSensorsPlusPlugin.m:224:7: error: no visible @interface for 'CMMotionManager' declares the selector
    'startDeviceMotionUpdatesUsingReferenceFrame:ToQueue:withHandler:'
          startDeviceMotionUpdatesUsingReferenceFrame:

I was initially stumped but I think it is pretty easy. I believe ToQueue should be toQueue and thus startDeviceMotionUpdatesUsingReferenceFrame:toQueue:withHandler:

As a benefit, pondering this led me to better understand how these sensors are activated and fused in iOS.
Initially I thought using the calibrated magnetic field from DeviceMotion would solve the Issue of invalid readings. [Note 2]

Today I realized the different CMAttitudeReferenceFrames, when employed directly through method call or indirectly by default, will impact the sensor selection; as seen at the bottom of that documentation link where the constants are defined.
Imagine referring to DeviceMotion's magneticField when it hasn't even turned on your magnetometer!


[Note 1]: This bug allowed me to catch something else: a missing reference to if (_isCleanUp) {return;} in my new magnetometer approach. Easy fix.

[Note 2]: I read an excellent comment on StackOverflow about the different ways to obtain magnetometer data in iOS.
In summary, my original solution used Core Motion's CMMagnetometer, a non-useful metric as the device itself heavily influences its values. My proposed solution here, based on other iOS/Flutter magnetometer plugins, uses Core Motion's CMDeviceMotion and its CMCalibratedMagneticField. However:

Core Location's CLHeading.[x|y|z] is the most vulnerable (responsive) to local external fields, whether moving or static relative to the device.

An interesting find. As CLHeading removes device bias just as CMDeviceMotion but is more vulnerable to external fields, it would seem to give a measurement with better "resolution" in terms of what a supposed user is evaluating.

Ultimately it depends on what the user actually intends to do with the geomagnetic data. Stud finder? Probably CLHeading, funny enough. Compass? Definitely CLHeading, but is has more specific properties for that functionality. General measurement of natural geomagnetic influence? Probably CMDeviceMotion.magneticField (the CalibratedMagneticField).

Furthermore how these values compare to Android's implementation is mystery to me.

Zabadam and others added 2 commits September 26, 2022 15:27
Also altering reference frame from `XArbitraryCorrectedZVertical` to `XMagneticNorthZVertical` which may or may not impact the effectiveness of the magnetometer calibration.
@miquelbeltran
Copy link
Member

taking a look at this at the moment, I want to give it a quick run on my phone but I have to deal with some iOS shenanigans with updates and so :)

I merged the current main branch because we incorporated ios specific build steps to catch any compilation issues

@miquelbeltran
Copy link
Member

alright, I was able to give it a run and saw no issue, so I see no reason to not to merge and release.

Thanks for your work!

@miquelbeltran miquelbeltran merged commit 69c4e46 into fluttercommunity:main Sep 27, 2022
@Zabadam Zabadam deleted the zaba branch September 27, 2022 15:56
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.

None yet

3 participants