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

Add Trio-Specific App Group for Isolated Data Storage #314

Merged
merged 10 commits into from
Jul 16, 2024

Conversation

dnzxy
Copy link
Contributor

@dnzxy dnzxy commented Jun 16, 2024

This pull request addresses the issue of shared app group data conflicts (addresses and solves #313).
It introduced a Trio-specific app group: group.org.nightscout.YOUR-TEAMID.trio.trio-app-group.

Changes:

  • Add new user-defined build setting APP_GROUP_ID with value group.$(BUNDLE_IDENTIFIER).trio-app-group to Trio scheme.
  • Update the xcconfig file to removeAPP_GROUP_ID as its now set via build settings.
  • Modify testflight.md to document the new Trio app group setup.

Benefits:

  • Enhanced data integrity and security.
  • Clear separation of app data, reducing risks of conflicts.

Implications:

  • Users will not lose a pod or other paired devices.
  • No impact on Xcode builders for fresh downloads.
  • For Xcode builders with existing downloads, users may need to click on their build targets (will show a provisioning profile error) to trigger a refresh of app group profiles
  • New Browser builders need to create and use the new Trio-specific app group instead of LoopGroup.
  • Existing Browser builders need to create a new app group, use that instead of LoopGroup and subsequently run the 3. Create Certificate and 4. Build Trio actions.
  • Xcode builders transitioning to Browser build may want to follow the instructions in the new section Optional: Description Modification in testflight.md to rename the display name of the app group on Apple Developer for more clarity.

Breaking Changes:

  • Adopting this change will break current compatibility of Trio with xDrip4iOS and GlucoseDirect as CGM sources
  • This PR will stay in draft mode for the time being until the maintainers of xDrip4iOS have had the chance to make small changes to accommodate the changed app group and give users the functionality to choose between apps (i.e. app groups) to share data with
  • xDrip4iOS will add a new "Loop Share" workflow with its upcoming v5.3 release. There's a pending staging PR (see xdripswift#551) opened now.
  • GlucoseDirect seems to no longer be maintained; the project could be forked and slightly adapted if the need arises.

@bjornoleh bjornoleh linked an issue Jun 16, 2024 that may be closed by this pull request
@bjornoleh bjornoleh added this to the Trio 1.0 release milestone Jun 16, 2024
@dnzxy dnzxy marked this pull request as ready for review July 1, 2024 19:33
@marionbarker marionbarker self-requested a review July 2, 2024 02:51
@marionbarker
Copy link
Contributor

See testing done with xDrip4iOS at this comment:

I will test this PR after the inadvertent CGMBLEKit change is removed.

@Sjoerd-Bo3 Sjoerd-Bo3 deleted the branch nightscout:dev July 2, 2024 14:00
@Sjoerd-Bo3 Sjoerd-Bo3 closed this Jul 2, 2024
@Sjoerd-Bo3 Sjoerd-Bo3 reopened this Jul 2, 2024
@Sjoerd-Bo3 Sjoerd-Bo3 changed the base branch from alpha to dev July 2, 2024 14:04
@Sjoerd-Bo3 Sjoerd-Bo3 added enhancement New feature or request Pre 1.0 labels Jul 9, 2024
@marionbarker
Copy link
Contributor

Summary

  • Successful test
  • Trio reads glucose from xDrip4iOS with group set to Trio group

Test Details

xDrip4iOS Configuration

xDrip4iOS version 5.3.1 was released today.
User can choose between Loop or Trio app group for the shared app groups feature.

  • build xDrip4iOS 5.3.1 on test phone
  • selected Trio app group for sharing CGM data (under Developer Settings on the Settings screen)
  • configure xDrip4iOS to read from personal Nightscout site (because readings are very different from the test NS site Trio is currently using)

Trio Configuration

  • iPhone SE, running iOS 17.5.1, attached to rPi DASH simulator
  • updated to most recent version of dev (commit bd56601)
  • applied the patch from this PR to the code
    • Please see update notes below
  • selected xDrip4iOS as CGM source
  • wait for glucose to update from new source (xDrip4iOS)
  • observed that green loop continues both unlocked and locke.

Update notes:

  • when building using Mac-Xcode, after this code change, this procedure should make your life easier
  • internet connection is required
  • clean the build folder, close and reopen the workspace
  • tap on the FreeAPS target and select Signing & Capabilities
  • If you need to log into to Apple to update your group, do so
  • Even if you don't need to log into Apple, you might need to tap on each target one at a time and let it automatically update the App Groups that is listed from Loop to Trio app group.

If you then return to dev without this PR, you need to repeat the steps under Update notes to restore the app group to Loop.

Copy link
Contributor

@marionbarker marionbarker left a comment

Choose a reason for hiding this comment

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

Sorry - I failed to notice the testflight.md change has an issue. This needs to be updated before this can be merged

fastlane/testflight.md Outdated Show resolved Hide resolved
fastlane/testflight.md Show resolved Hide resolved
@marionbarker
Copy link
Contributor

marionbarker commented Jul 10, 2024

This PR cannot be tested using the patch method. (I think this is a case where git push --force-with-lease . . . is warranted. Then adding .patch to the PR would probably allow a git apply to work.

However, the dnzxy:trio-app-group branch is now fully update wrt dev, so I can switch to that branch and test that way.

I will test the Build Action following along in the modified testflight.md and report back.

@marionbarker
Copy link
Contributor

Still many issues with testflight.md. Sent information to @dnzxy via PM. Tested the Browser Build and that works. It's just the direction that need to be updated.

@dnzxy dnzxy force-pushed the trio-app-group branch 7 times, most recently from 7086028 to 74ba595 Compare July 10, 2024 18:06
@dnzxy
Copy link
Contributor Author

dnzxy commented Jul 10, 2024

Still many issues with testflight.md. Sent information to @dnzxy via PM. Tested the Browser Build and that works. It's just the direction that need to be updated.

Updated now again.

fastlane/testflight.md Outdated Show resolved Hide resolved
fastlane/testflight.md Outdated Show resolved Hide resolved
fastlane/testflight.md Show resolved Hide resolved
Copy link
Contributor

@marionbarker marionbarker left a comment

Choose a reason for hiding this comment

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

I wonder if we should add a comment to this section indicating it is required moving from before and after Trio App Group was merged to Trio. We can remove all beta-tester info a month or two after the release.

## Add App Group to Bundle Identifiers

> This step is required for first-time builders using GitHub Actions (Browser Build).

> If you previously built using a Mac with Xcode you can skip ahead to [Create Trio App in App Store Connect](#create-trio-app-in-app-store-connect).

fastlane/testflight.md Outdated Show resolved Hide resolved
fastlane/testflight.md Outdated Show resolved Hide resolved
@dnzxy
Copy link
Contributor Author

dnzxy commented Jul 11, 2024

I wonder if we should add a comment to this section indicating it is required moving from before and after Trio App Group was merged to Trio. We can remove all beta-tester info a month or two after the release.

I've addressed the other points and have added a hint around app group and beta testing

> If you have previously built Trio as a beta tester (between May 15th, 2024, and today), you will already have an app group (`Loop App Group`) created and configured for your bundle identifiers. In this case, please *do not* skip this section; you are required to create the `Trio App Group` and configure it for your identifiers, as described below.

Let me know what you think. Admittedly, I'd rather not spend much more time on updating testflight.md but invest it in getting the docs to be the go-to place for instructions.

@dnzxy
Copy link
Contributor Author

dnzxy commented Jul 11, 2024

For whoever gives the final ✅ and merges this: I'd suggest to merge this with squash+merge; no need to bloat up commit history with merge commits of upstream alpha and dev and updates to a single markdown file.

Copy link
Contributor

@marionbarker marionbarker left a comment

Choose a reason for hiding this comment

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

I approve this version of testflight.md (the instructions for building).

I previously tested the modifications that use the Trio App group instead of the Loop App group extensively. It works for Mac-Xcode build and for Browser Build and works with xDrip4iOS 5.3.1 release.

@MikePlante1
Copy link
Contributor

I just submitted an Issue report to xDrip4iOS after doing some testing for this PR. I'll poke at it some more tonight to hopefully figure out a solution that allows both Trio & Loop/iAPS to use xDrip4iOS as a heartbeat.

Copy link
Contributor

@MikePlante1 MikePlante1 left a comment

Choose a reason for hiding this comment

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

Not sure why heartbeat didn’t work for me before, but it’s working now, so all good.

‼️ This merge needs to be coordinated with a message in Discord as well as the approval of PR54 to the Docs. ‼️

Copy link
Contributor

@t1dude t1dude left a comment

Choose a reason for hiding this comment

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

I have reviewed all comments and changes. Looks good.

@MikePlante1 MikePlante1 merged commit 9672da2 into nightscout:dev Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Pre 1.0
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Trio specific App Group
6 participants