Skip to content

Conversation

@marionbarker
Copy link
Collaborator

@marionbarker marionbarker commented Jan 3, 2026

Edit to explain the purpose of this PR:

Purpose

The test for OmniBLE Test suite fails when run using Xcode 26.2.

  • Failed at OmniBLETests/Driver/Comm/message/MessagePacketTests.swift line 52.
  • screenshot for test error is first screenshot below
OmniBLE-test-error-xcode-26

After fixing that error, I decided to try removing some warnings.

After some back and forth with Pete and Joe - we decided to fix just the one error in this PR.

  • Each of the warning commits was reverted.
  • If we want to revisit them later, the commits can be found in this PR, but will be squash merged to avoid messing up dev branch

Warnings Ignored

The hang risk graphic is shown below.

OmniBLE-hang-risk-warning

The warnings displayed for OmniBLE are shown in the screenshot below. These are ignored for the PR.

OmniBLE-warnings

UI bug is Trio and SE phone only

I thought changing the animations lines fixed a UI bug, but (1) it did not affect the bug and (2) the bug is only seen with Trio on an SE phone running iOS 26.

  • The UI glitch is if you put your finger too far to the left before swiping to Deactivate the pod, the card is dismissed
    • Seen only with iOS 26 on an SE phone with Trio when deactivating
    • Not seen with Loop with any test phones available
    • Not seen with Trio using an iPhone 15 running iOS 26 or with an SE running iOS 18

Original comments below

Use GPT embedded in Xcode 26.2 to assist in fixing a test error (new with Xcode 26.2) and removing warnings.

Test

  • ✅ Run OmniBLE test suite - successful once the modification is made
  • ✅ Build and test using a DASH rPi simulator
    • A UI bug that had reappeared with iOS 26 is fixed with the animation update
    • Before this modification, if you swipe the slider too close to the edge of the card, it dismisses the card

Comments

See this comment, after I switched to using claude, instead: #160 (comment)

The QoS inversion is improved but not fixed.

GPT says of the accepted change:

> This reduces the window for QoS inversion while preserving the existing control flow (callers still get a boolean indicating whether storage succeeded). If timeouts show up in logs, it’s a signal to consider a larger refactor to make this storage fully asynchronous with respect to session control flow.

GPT suggests further modifications which were not requested

> If you want, I can also:
> • Plumb an asynchronous completion through store(doses:in:) so callers don’t need to block at all.
> • Evaluate the pumpDelegate queue QoS configuration to align with the session queue and eliminate the inversion at the source.

@marionbarker marionbarker requested review from itsmojo and ps2 January 3, 2026 22:40
@marionbarker
Copy link
Collaborator Author

Compare to OmniKit

When comparing to OmniKit, because I was planning to make similar changes there, I noted that all but one .animation( line was not present in OmniKit.

Perhaps those lines should just be deleted instead of modified.

I did a test build where I removed them from OmniBLE and every screen that used them, worked as expected.

@marionbarker
Copy link
Collaborator Author

Updates

I switched to using claude instead of the free gpt in Xcode.

I reverted the original QoS inversion (from GPT) and replaced it with what I believe is a better fix.

See: commit 5215187

Copy link

@itsmojo itsmojo left a comment

Choose a reason for hiding this comment

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

I am not familiar with using DispatchGroup along with group.{enter,leave,wait}, so I cannot evaluate those changes other than to say it appears reasonable. I would defer to Pete for his thoughts the func store() changes. All the other changes look good to me.

@marionbarker
Copy link
Collaborator Author

@itsmojo and @ps2

This PR now just fixes the one error associated with the Test Suite using Xcode 26.2.

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.

3 participants