Skip to content

Conversation

@bjorkert
Copy link
Member

@bjorkert bjorkert commented Oct 6, 2025

This PR adds two fields to the Nightscout upload so clients like LoopFollow can display a correction recommendation and respect the pump’s bolus step:
• openaps.recommendedBolus — the treatment view equivalent recommended correction bolus, for no new carbs.
• pump.bolusIncrement — the smallest supported bolus volume from the active pump.

@bjorkert
Copy link
Member Author

bjorkert commented Oct 6, 2025

CleanShot 2025-10-06 at 13 00 21

@bjorkert
Copy link
Member Author

I made some updates to this PR, see below:

  • Removed NightscoutManager injection from APSManager to eliminate the circular dependency.
  • NightscoutManager now directly @injects BolusCalculationManager and APSManager (in stead of the ad-hoc resolve()).
  • Deleted Nightscout upload calls from APSManager loop, Nightscout is already updated via Core Data subscriptions. This avoids a second, redundant upload path that could potentially create duplicates.
  • The uploaded bolusIncrement sourced from preferences instead of pumpmanager.

@bjorkert
Copy link
Member Author

My son is using this branch, it has been running without issues. Currently developing the LoopFollow side of this.

@bjorkert bjorkert marked this pull request as draft October 20, 2025 14:08
@bjorkert
Copy link
Member Author

Doing some refactoring and re-implementing upload triggering from APSManager to catch situations where the upload based on data fails, but in a way that prevent duplicate uploads. Changing this PR to a draft while working/testing.

@bjorkert
Copy link
Member Author

bjorkert commented Oct 20, 2025

Pulled the Nightscout subscriber logic into BaseNightscoutManager+Subscribers.swift and added lane-based “kick” pipelines (2s throttle) to avoid double uploads. APSManager now requests uploads via requestNightscoutUpload (carbs/pumpHistory/overrides/tempTargets).

I'll resume testing and will open up this for review again if/when I am happy with the tests.

@bjorkert bjorkert marked this pull request as ready for review October 31, 2025 12:48
Copy link
Contributor

@dnzxy dnzxy left a comment

Choose a reason for hiding this comment

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

I'll approve this based on in-vivo testing on my end and in-vivo testing with LFxTrio on @bjorkert 's end with his son's setup.

Jonas and I have discussed this PR extensively via DM over the last ~2 weeks.
Are we okay with the naming and changes here? Functionality-wise it's okay, but it is a bigger change, so asking you folks, too, @marv-out @kingst .

Copy link

@kingst kingst left a comment

Choose a reason for hiding this comment

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

Overall this LGTM. I didn't test it, my review is just from reading the code. I have one optional suggestion and one question but in general this PR is ready to go.

extension BaseNightscoutManager {
/// Call once from init. Hooks up:
/// 1) external upload requests (NotificationCenter)
/// 2) Core Data change triggers → kicks per lane
Copy link

Choose a reason for hiding this comment

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

[minor / optional] I haven't seen the phrases kick or lane, is this a standard nomenclature for Combine workflows? I think I understand what it means, but maybe lanes are classes / objects and kicks are sends? I'm not 100% sure so this comment is optional

private let queue = DispatchQueue(label: "BaseNightscoutManager.queue", qos: .background)
private var coreDataPublisher: AnyPublisher<Set<NSManagedObjectID>, Never>?
private var subscriptions = Set<AnyCancellable>()
let queue = DispatchQueue(label: "BaseNightscoutManager.queue", qos: .utility)
Copy link

Choose a reason for hiding this comment

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

Why did you change the qos, was there priority inversion?

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