-
Notifications
You must be signed in to change notification settings - Fork 2.7k
sms bolus from AAPSClient #3507
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
base: dev
Are you sure you want to change the base?
Conversation
9780f97 to
3f837ee
Compare
|
One concern is that AAPSClient doesn't always sync perfectly with AAPS. Both IOB and COB can be wrong due to many different reasons. We should consider adding a warning about this in the confirmation box of the bolus calculator in AAPSClient if enabling this feature since bolusing based on IOB/COB from AAPSClient's bolus calculator will then be more tempting to use when it's more accessible. I don't think there is a way with SMS commands to verify what IOB and COB AAPS really got. It's rare for my part that AAPSClient and AAPS got different IOB/COB, because if sync settings and NS is setup correctly, it's very unlikely to happen. But when looking at threads for support in FB/Discord/Github regarding wrong sync between AAPS and AAPSClient this is a concern I have. @MilosKozak In the OpenAPS log you can click to view in AAPSClient you can see the exact COB from AAPS; regardless if COB in NS / AAPSClient is wrong. Would it be possible to add IOB amount to the openAPS-log also? That way there is one place one could verify that COB/IOB in AAPSClient is correctly synced with AAPS without visiting master phone. |
core/objects/src/main/kotlin/app/aaps/core/objects/wizard/BolusWizard.kt
Outdated
Show resolved
Hide resolved
|
@olorinmaia @MilosKozak what are next steps? |
|
I'm not able to test this as my test-rig don't have sim-cards. One way is to inform about PR on Discord and see if there is any testers there and get more input regarding this PR generally. |
f7d6c31 to
e3341e2
Compare
|
@MilosKozak can your review it? I assume groups of people building dev branches and parents needing features I implemented are disjoint. I asked about feedback on parents channel, didn't get any feedback. I have been testing it myself for some time. |
|
I am testing this on top of beta1 and first use went all right. I will continue to test in the next few days. I think I would have searched the preferences for that in Overview > Buttons, since it is where we make use of them, in AAPSClient. |
|
@emilisev thanks for catching that! See my latest update for fixes, I found few other corner cases. I also moved sms related settings under button menu as you suggested. @olorinmaia I added current IOB to sms request response. I've tested AAPSClient, haven't testes AAPS with updated SMS response yet, but I don't expect any issues there |
|
I can't build it, I get : e: file:AndroidAPS-dev/plugins/main/src/main/kotlin/app/aaps/plugins/main/general/smsCommunicator/SmsCommunicatorPlugin.kt:936:65 Unresolved reference 'plus' for operator '+'. |
03376ef to
66c3ad6
Compare
|
I've pushed compilation fix. Note I didn't test it functionally yet. You are free to do it or you can wait for me. I will send update |
- preferences config update
- TODO: calc wizard doesn't send sms, remove animation
- create calc entry in history if sms was send
- fix this option to work only on aapsclient, only when it's enabledd
- enable phone number verification in settings - fix issues - update AAPS response for SMS bolus request to include IOB
| <string name="partialboluswizard_reset_time">Old glycemia time threshold [min]</string> | ||
| <string name="bolus_constraint_applied_warn">Bolus constraint applied: %1$.2f U to %2$.2f U</string> | ||
| <string name="bolus_recorded_only">Bolus will be recorded only (not delivered by pump)</string> | ||
| <string name="sms_request_notification">SMS command will be send</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"SMS command will be send" seem to be bad grammar.
"SMS command will be sent" or "The SMS command will be sent" is probably better if I understand the context. :)
|
I have been testing it now for one week on two rigs. Since the most recent update, I experienced no problems. I it a great improvement. Saves time and accurate entries of carbs. |
|
@MilosKozak it's ready for your review. |
| AutotuneAdditionalLog("autotune_additional_log", false), | ||
|
|
||
| SmsAllowRemoteCommands("smscommunicator_remotecommandsallowed", false), | ||
| SmsAllowRemoteCommands("smscommunicator_remotecommandsallowed", false, showInApsMode = false, showInPumpControlMode = false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin is intended only for AAPSClient. It sends SMS commands to phone running AAPS. Both AAPS and control app are on the pump controlling phone so presence of that plugin would be confusing.
| @Inject lateinit var processedDeviceStatusData: ProcessedDeviceStatusData | ||
|
|
||
| var timeStamp: Long | ||
| // var phoneNumber: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review https://github.com/nightscout/AndroidAPS/pull/3507/files and cleanup the code
|
|
||
| // SMS COMMUNICATOR | ||
| const val remoteBolusMinDistance = 15 * 60 * 1000L | ||
| const val remoteBolusMinDistance = 5 * 60 * 1000L |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
15 minutes seems to be too much. In my case I've set some max carbs limit for single meal. It's intended to limit bolus to make sure carbs absorption kicks in on time. When meal is bigger than max carb then I split calculations into 2 meal with 5 minutes delay. 5 should also prevent accidental bolus at the same time from multiple caregivers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree. I have the same process in place with the treatment on my two kids. Set bolus limit for safety, leading to mandatory split bolus for regular meals.
|
@MilosKozak can you review again? |
|
|
I have tested it now on 3.3.3.0-dev since it's release without any issues on two rigs. From a practical point of view, being able to track the bolus calculation via bolus calculator in the treatments tab proofs valuable in order to draw conslusions of futher treatments and for general analysis. Correct SMS commands proof to save time, especially since it prevents typos which occur quite often. I furher suggest to extend the funcionalty on pump commands, Connect/Disconnect Pump, Stop/Enable loop. Which I have to use regualrily on sport aktivities like supervising my kids on Karate Trainings, MTB Tours where the setting of temp targets or % of Porfile just doesn't suffice to handle the situation. I am looking forward to have this functionality as well. |
|
Almost three months have passed since my last post, and I can now confirm: still no issues whatsoever. From a practical perspective, the ability to track bolus calculations in the Treatments tab has proven consistently helpful for evaluating therapy decisions and overall analysis. The use of structured SMS commands continues to save time and prevents common input errors. I stand by my recommendation to pull this—at least into dev. If there are any concerns or objections, I’d appreciate hearing them so I can evaluate whether they’re practically relevant and warrant further attention. |
|
Two months later, I can confirm the feature works well and meets expectations. Since it’s been pending here for quite a while, I’d like to discuss potential reasons against implementing it. |
|
@MilosKozak can you review? |
b64509c to
659759c
Compare
|





Implement bolus sms feature in AAPS client, if feature is enabled in menu and target phone number is provided it allows to send SMS directly from insulin/calc wizard dialog instead of "record only" feature. It still requires authorization code to be typed manually in the second message. If calc wizard was used, results will be saved in "treatment" tab history.