Sync from DIY #229

ps2 · 2020-08-27T19:31:07Z

https://tidepool.atlassian.net/browse/LOOP-1689

* Update carthage to use g6 hotfix * Bump version for hotfix release * Use CGMBLEKit v3.1

* Updating genstrings, Adding new localizable.strings, adding some new translations to existing languages * cleaning up unused files and adding Vietnamese, Swedish, Danish, Finnish, Japanese, Portuguese (BRA) * typos and minor missing translations. * cleanup unnecessary .strings and typo fixes * Adding Romanian * Italian updates * Danish translations * German touchups * Finnish touchups * Japanese and French touchups * Norwegian, dutch, Portuguese, Romanian, russian, swedish, vietnamese, and chinese finished * polish Co-authored-by: Pete Schwamb <pete@schwamb.net>

* Track cgm device when uploading BG data * Bump carthage revs

Release 2.0

…shold

Set carb cache length to 24 hours.

…olds Include current bg for suspend threshold and dosing thresholds

…into merge-from-tidepool

nhamming

LGTM

nhamming · 2020-09-01T12:05:58Z

Cartfile

-github "LoopKit/LoopKit" "dev"
-github "LoopKit/CGMBLEKit" "dev"
+github "LoopKit/LoopKit" "merge-from-tidepool"
+github "LoopKit/CGMBLEKit" "merge-from-tidepool"
 github "i-schuetz/SwiftCharts" == 0.6.5
-github "LoopKit/dexcom-share-client-swift" "dev"
-github "LoopKit/G4ShareSpy" "dev"
-github "ps2/rileylink_ios" "dev"
+github "LoopKit/dexcom-share-client-swift" "merge-from-tidepool"
+github "LoopKit/G4ShareSpy" "merge-from-tidepool"
+github "ps2/rileylink_ios" "merge-from-tidepool"


[nit] why not dev here?

This is an artifact of me making the tidepool -> diy merge available for DIY testers to try out using the normal build method (non-workspace) that most DIYers use.

Tidepool doesn't use this file in its builds at all, so I'd just treat this as noise for now.

rickpasetto

I've added a few questions I would love to know the answers to before I approve this.

Similar comment as my LoopKit review: I'd feel more comfortable if there were unit tests covering the changes made.

rickpasetto · 2020-09-01T18:27:07Z

Loop/AppDelegate.swift

-
-    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
+
+    private var isAfterFirstUnlock: Bool {


Yay!

[super-nit] it always felt a teeny bit weird to me that this was computed variable with a side-effect of writing a file. IMHO I'd change it to

private func isAfterFirstUnlock() -> Bool {

rickpasetto · 2020-09-01T18:29:36Z

Loop/AppDelegate.swift

+            let fileURL = documentDirectory.appendingPathComponent("protection.test")
+            guard fileManager.fileExists(atPath: fileURL.path) else {
+                let contents = Data("unimportant".utf8)
+                try? contents.write(to: fileURL, options: .completeFileProtectionUntilFirstUserAuthentication)


IIRC, I think in my work around this I replaced this try? with a try. Seems safe enough to do...WDYT?

Yea, I don't think this works. This entire function is to see if we have access to the local file system (thus indicating after first unlock).

Unless the let documentDirectory = try fileManager.url throws when it is locked (and I don't think it does), then the rest won't work. The fileExists call would return false (by definition, because we don't have access to the local file system before first unlock), the guard clause would be executed, the try? contents.write would do nothing (because of the try?) and it would return true. We'd need to change the try? to try to make this work.

Furthermore, I'd change let contents = try? Data to just try and let it throw. And then update the log.error message in the catch to be more accurate.

If let documentDirectory = try fileManager.url does throw when it is locked, then the rest of the function (and writing/reading the file) is unnecessary.

@darinkrauss are you saying you don't think my suggestion works, or you think this whole code change won't work?

I'm not 100% sure I agree you can guarantee that fileExists will fail because the file system is unavailable. See the following note in the docs:

It’s far better to attempt an operation (such as loading a file or creating a directory), check for errors, and handle those errors gracefully than it is to try to figure out ahead of time whether the operation will succeed.

Therefore, that's why I suggest try instead of try? when writing to the file. Otherwise any potential error/exception thrown (admittedly very unlikely, but we never know across versions of iOS) will be ignored.

(P.S. Way back when I was working on this, I am pretty sure I tested this code extensively and it worked...)

Sorry I wasn't more clear. Yes, your fix is what will make it work. That is, changing the try? to try.

The file existence check here does work before first unlock; this makes sense as individual files can have different access levels applied to them. The logic here is known to be working, and very hard to test; it requires specific minor versions of iOS that I don't know if we can actually install anymore. So I am loathe to make any changes.

@rickpasetto, you were not able to reproduce actually launching Loop before first unlock, iirc, so I don't know how you could've tested any changes to this.

The way it's written right now is that it will tend to launch if there are unexpected changes in iOS behavior. I think this is what we want. The preventative measure is in place and works if we see the same behavior that we saw in some versions of iOS 11 and early versions of 12. If we see other behavior, we'll have to deal with that. We have word from Apple that launching via bluetooth before unlock was a bug, and this code addresses that situation specifically.

Huh, well, I don't see how it would logically, but perhaps it is throwing some non-Swift exception from fileExists or write, but then why would the Swift catch actually catch it? The difficulty I have is with try? contents.write in that it just silently fails. 🤷‍♂️

@ps2 At some point, I was. It was months ago now, though. I just don't recall...maybe I had tested on iOS 12? 🤷‍♂️

At any rate, I agree that this is "bulletproofing" code. I see no reason why changing the try? to a try is a problem, but this is not something I'll lie down on the tracks for, so if you feel strongly about not changing it I'd yield.

The try? contents.write isn't where we're checking for before/after first unlock; that's the code that is run if we're in the first run of the app, and we decide to create the file for later use. If it fails, there's not much we can do, and we should continue on launching the app.

If that try fails, we can log an error if it fails (indeed, there's already a catch for this). If we ever see it, we would know the behavior changed (e.g. an iOS version change modified the behavior somehow).

rickpasetto · 2020-09-01T18:41:33Z

Loop/Managers/DeviceDataManager.swift

        let documentsDirectory = fileManager.urls(for: .documentDirectory, in: .userDomainMask).first!
-        deviceLog = PersistentDeviceLog(storageFile: documentsDirectory.appendingPathComponent("DeviceLog.sqlite"), maxEntryAge: localCacheDuration)
+        let deviceLogDirectory = documentsDirectory.appendingPathComponent("DeviceLog")
+        deviceLog = PersistentDeviceLog(storageFile: deviceLogDirectory.appendingPathComponent("Storage.sqlite"), maxEntryAge: localCacheDuration)


After this change goes in, doing an Issue Report won't return any (old) device logs. (That is, this effectively starts the logging over). Is that ok?

For Tidepool, yes, since we do not have any live users. For DIY, this is the way it has always been, since the change was made before any DIY code was released.

rickpasetto · 2020-09-01T18:53:31Z

Loop/Managers/DoseMath.swift


-            let percentEffected = 1 - model.percentEffectRemaining(at: time)
+            // For 0 <= time <= effectDelay, assume a small amount effected. This will result in large unit recommendation rather than no recommendation at all.
+            let percentEffected = Swift.max(.ulpOfOne, 1 - model.percentEffectRemaining(at: time))


Is there an associated unit test for this change?

The tests in DoseMath did change for this, and cover this code.

rickpasetto · 2020-09-01T18:54:55Z

Loop/Managers/LoopDataManager.swift

        let tempBasal = predictedGlucose.recommendedTempBasal(
            to: glucoseTargetRange,
-            at: now(),
+            at: predictedGlucose[0].startDate,


Hm... this looks like a significant change to me. Is there an explanation I can read? A unit test?

Here is the original PR for this fix: LoopKit#1235. There is also some discussion in the linked issue.

The change aligns LoopDataManager more with what was being tested as well. The tests for recommendedTempBasal almost exclusively use the date of the first predicted glucose (which is current bg).

Loop/Views/NotificationsCriticalAlertPermissionsViewModel.swift

darinkrauss

Mostly nits, a few questions, but some concerns for you to look at. Setting as "Request changes" since there may be some required, but let me know otherwise.

LoopCore/NSUserDefaults.swift

Loop.xcodeproj/project.pbxproj

Loop/AppDelegate.swift

darinkrauss · 2020-09-04T02:13:42Z

Loop/AppDelegate.swift

+            let documentDirectory = try fileManager.url(for: .documentDirectory, in: .userDomainMask, appropriateFor:nil, create:false)
+            let fileURL = documentDirectory.appendingPathComponent("protection.test")
+            guard fileManager.fileExists(atPath: fileURL.path) else {
+                let contents = Data("unimportant".utf8)


[nit] Write empty data?

Not sure what you're asking here.

Sorry, rather than writing the string "unimportant" just write Data(). No big deal, though.

darinkrauss · 2020-09-04T02:28:52Z

Loop/AppDelegate.swift

+            let fileURL = documentDirectory.appendingPathComponent("protection.test")
+            guard fileManager.fileExists(atPath: fileURL.path) else {
+                let contents = Data("unimportant".utf8)
+                try? contents.write(to: fileURL, options: .completeFileProtectionUntilFirstUserAuthentication)


Yea, I don't think this works. This entire function is to see if we have access to the local file system (thus indicating after first unlock).

Unless the let documentDirectory = try fileManager.url throws when it is locked (and I don't think it does), then the rest won't work. The fileExists call would return false (by definition, because we don't have access to the local file system before first unlock), the guard clause would be executed, the try? contents.write would do nothing (because of the try?) and it would return true. We'd need to change the try? to try to make this work.

Furthermore, I'd change let contents = try? Data to just try and let it throw. And then update the log.error message in the catch to be more accurate.

If let documentDirectory = try fileManager.url does throw when it is locked, then the rest of the function (and writing/reading the file) is unnecessary.

DoseMathTests/DoseMathTests.swift

darinkrauss · 2020-09-08T20:46:49Z

Loop.xcodeproj/project.pbxproj

 			runOnlyForDeploymentPostprocessing = 0;
 			shellPath = /bin/sh;
-			shellScript = "if [ -f $PROJECT_DIR/.gitmodules ]; then\n    echo \"Skipping checkout due to presence of .gitmodules file\"\n    if [ $ACTION = \"install\" ]; then\n        echo \"You're installing: Make sure to keep all submodules up-to-date and run carthage build after changes.\"\n    fi\nelse\n    echo \"Bootstrapping carthage dependencies\"\n    unset LLVM_TARGET_TRIPLE_SUFFIX\n    if ! cmp -s Cartfile.Resolved Carthage/Cartfile.resolved; then\n        time /usr/local/bin/carthage bootstrap --project-directory \"$SRCROOT\" --cache-builds --verbose\n        cp Cartfile.resolved Carthage\n    else\n        echo \"Carthage: not bootstrapping\"\n    fi\nfi\n";
+			shellScript = "if [ -f $PROJECT_DIR/.gitmodules ]; then\n    echo \"Skipping checkout due to presence of .gitmodules file\"\n    if [ $ACTION = \"install\" ]; then\n        echo \"You're installing: Make sure to keep all submodules up-to-date and run carthage build after changes.\"\n    fi\nelse\n    echo \"Bootstrapping carthage dependencies\"\n    unset LLVM_TARGET_TRIPLE_SUFFIX\n    if ! cmp -s Cartfile.Resolved Carthage/Cartfile.resolved; then\n        time /usr/local/bin/carthage bootstrap --project-directory \"$SRCROOT\" --cache-builds\n        cp Cartfile.resolved Carthage\n    else\n        echo \"Carthage: not bootstrapping\"\n    fi\nfi\n";


@rickpasetto Do we need the --verbose flag on Carthage bootstrap for Circle CI to not timeout? I forgot why the flag was added in the first place.

@darinkrauss IIRC this was put here for TravisCI, not CircleCI. CircleCI's timeout is configurable (and set for 30 min). That said, I don't mind leaving this in. If you feel strongly, we can remove it in a later PR.

This PR removes it, and I'm good with that. 👍

I think I was having problems with Travis thinking the log was too big. 🙄

LoopCore/NSUserDefaults.swift

darinkrauss

LGTM! 🚀

… from CoreData

rickpasetto

LGTM

ps2 and others added 30 commits July 18, 2019 16:57

G6 hotfix (LoopKit#1014)

eecc718

* Update carthage to use g6 hotfix * Bump version for hotfix release * Use CGMBLEKit v3.1

info.plist fix for BT for ios13 (LoopKit#1124)

5df0b4c

Bump carthage revs

ac31e0b

Pause launch if launching before first unlock (LoopKit#1198)

d22b837

Bump version for dev to 1.10.3 (LoopKit#1204)

650ffd4

One !

94d3279

Remove override presets cell in settings

d0eec98

Revert dev team change

a098d6a

Bump carthage revs

2c47962

Bump carthage revs

0a26361

Bump cartfile revs

fd22ca0

Bump carthage revs

dd69222

clear development team signing (LoopKit#1216)

be121ca

Bump carthage revs

f0cc33c

Remove LoopCore's Localizable.strings from Learn app

caf87ed

Track cgm device when uploading BG data (LoopKit#1222)

6144a00

* Track cgm device when uploading BG data * Bump carthage revs

Use release versions of LoopKit, CGMBLEKit, rileylink_ios

db9b05a

Merge remote-tracking branch 'origin/master' into dev

f324a51

Remove extra NSBluetoothAlwaysUsageDescription key

e623b4b

Merge pull request LoopKit#1223 from LoopKit/dev

a03629d

Release 2.0

Bump xcconfig version

b04a8ab

Bump Loop version to 2.1 for dev

8eae264

Increase history interval for carb cache

71b4f41

Include current bg when considering suspend threshold and dosing thre…

0a19eff

…shold

Include insulin model delay in computation of dose

012aa23

Fix dose math tests

c45caaa

Merge pull request LoopKit#1234 from LoopKit/carb-cache-length

169b429

Set carb cache length to 24 hours.

Merge pull request LoopKit#1235 from LoopKit/current-bg-dosing-thresh…

b944488

…olds Include current bg for suspend threshold and dosing thresholds

add delay to insulin model presets

40ae183

ps2 added 11 commits August 7, 2020 11:55

Do not carthage build verbose

478a524

Clean up travis yaml file

d886ecd

Add carthage cache

6e8f053

Just build carthage deps to populate cache

6ec3570

Build full now that cache is populated

cfc70d1

Fix tests

3ab9693

Split build into stages to avoid timeouts; no cache

733924f

Add cache back

1d8387e

Merge remote-tracking branch 'tidepool/dev' into merge-from-tidepool

b46835a

Merge fixes

677df92

Merge branch 'merge-from-tidepool' of https://github.com/LoopKit/Loop …

50256dc

…into merge-from-tidepool

ps2 changed the title ~~Ps2/loop 1689/diy sync~~ Sync from DIY Aug 27, 2020

Use current BG in suspend threshold evaluation

73d9158

ps2 requested review from darinkrauss, nhamming and rickpasetto August 31, 2020 19:29

nhamming approved these changes Sep 1, 2020

View reviewed changes

rickpasetto reviewed Sep 1, 2020

View reviewed changes

Merge remote-tracking branch 'tidepool/dev' into ps2/LOOP-1689/diy-sync

e9eb822

darinkrauss requested changes Sep 4, 2020

View reviewed changes

ps2 added 2 commits September 7, 2020 12:50

Merge remote-tracking branch 'tidepool/dev' into ps2/LOOP-1689/diy-sync

ebc47c1

Updates from reviews

77b679b

ps2 requested review from darinkrauss and rickpasetto September 8, 2020 18:32

darinkrauss reviewed Sep 8, 2020

View reviewed changes

darinkrauss approved these changes Sep 8, 2020

View reviewed changes

Create DeviceLog directory if it doesn't exist to prevent noisy error…

1d9791a

… from CoreData

rickpasetto approved these changes Sep 9, 2020

View reviewed changes

ps2 merged commit 74bafb9 into dev Sep 10, 2020

ps2 deleted the ps2/LOOP-1689/diy-sync branch October 20, 2020 17:32


		func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {

		private var isAfterFirstUnlock: Bool {

Sync from DIY #229

Sync from DIY #229

Uh oh!

Conversation

ps2 commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nhamming left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickpasetto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickpasetto Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ps2 Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darinkrauss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ps2 commented Aug 27, 2020 •

edited

Loading

rickpasetto Sep 8, 2020 •

edited

Loading

ps2 Sep 8, 2020 •

edited

Loading