nightscout / AndroidAPS

Opensource automated insulin delivery system (closed loop)
https://wiki.aaps.app
GNU Affero General Public License v3.0
724 stars 1.76k forks source link

Too long SMS reply messages #2257

Open justmara opened 1 year ago

justmara commented 1 year ago

Currently SMS communicator has 2 major problems with replies:

  1. It adds activePlugin.activePump.shortStatus(true) to every successful reply
  2. It is forced to send successful replies to every number in allowed numbers list.

For some pumps this is not an issue - we had no any pump status in reply messages while were using Combo pump. But for some it is. For example, this is reply message in Russian locale with Omnipod Dash pump:

Болюс 0,80ед. подан успешно
Предыдущее соединение: 0 мин. назад
Предыдущии болюс: 0,80 @ 12:04
Временн 0,00U/h @11:41 24/120'
Резервуар: 44ед

It contains 142 characters, so it is split onto 3 SMS messages (SMS can contain up to 70 non-ascii characters), and those 3 SMS are sent to each parent. So every successful reply takes 6 SMS messages to be served. Thats expensive :)

MilosKozak commented 1 year ago

you were not participating when we last time discussed that I'm trying to reduce amount of setting in app to avoid xDrip syndrome (settings for eveything and nobody knows the meaning). both things you describe have good reason

SMS was never meant as a replacement of common care but as a supplement where some SMS doesn't matter But I understand there can be special cases where it may not fit perfectly

justmara commented 1 year ago

trying to reduce amount of setting in app

Some people wants simplicity, some wants more control and customization. Maybe its time to make a 'simple/advanced' settings switch, hiding all the extended options under the 'advanced' mode so regular users don't get headache while advanced ones don't loose their control?

you were not participating when we last time discussed that I'm trying to reduce amount of setting in app to avoid xDrip syndrome (settings for eveything and nobody knows the meaning).

These options are located in particular plugin's settings and have pretty obvious meaning. They help reducing the spam AAPS generates and money drain it (ocassionally) adds. This is much less an issue when using english locale, but translated versions suffer of it much more because of technical limitations of SMS protocol.

settings for eveything and nobody knows the meaning

xDrip's main problem is setting's organization. Some seems really randomly placed in different subsections, some ssections duplicated but still contain different settings...

  • See status and last bolus time especially when comm fails

Theoretically this can be helpful, but in reality noone cares. Useless bunch of strings that literally costs some money. All that info is in NS and can be seen in nsclient.

  • To let know to all caregivers that something already happened

Sender must receive report, I agree. But the rest can see it in nsclient/treatments. There is no real requirement for this. It is an option not everyone really requires.

SMS was never meant as a replacement of common care but as a supplement where some SMS doesn't matter But I understand there can be special cases where it may not fit perfectly

SMS IS a common care for pumpcontrol mode, for example. Some parents use pumpcontrol as a (literally) remote control for pump, so SMS communicator is a daily option for those.

MilosKozak commented 1 year ago

on your place i'd change translation to use ascii characters, if SMS are expensive in your country. in europe and most other countries are free or cheap

justmara commented 1 year ago

on your place i'd change translation to use ascii characters

You cant only change SMS message's localization - it switches with whole application only. And when people switch app UI language - they do it for a reason, don't you think?

in europe and most other countries are free or cheap

Some people struggles paying 5$/month for nightscout. SMS messages are (mostly) cheap, but usually comes in packages. I were surprised this month that my kid's aaps has eaten whole 500 sms package only because of switching to Dash (Combo pump were not sending its status for some reason). SMS preboluses are pre pretty common while kid is at school and mother wants to give AAPS some help without setting it overaggressive mode.

It is not a best practice to dictate people what they must to do and how they must do it, instead of simply giving'em an instrument that covers their needs. This code does not change any of current (default) behaviour, but adds an option to customize it to some special (pretty reasonable) needs.

MilosKozak commented 1 year ago

It is not a best practice to dictate people what they must to do and how they must do it, instead of simply giving'em an instrument that covers their needs. This code does not change any of current (default) behaviour, but adds an option to customize it to some special (pretty reasonable) needs.

I had similiar opinion when i started. Now not anymore

MilosKozak commented 1 year ago

You cant only change SMS message's localization - it switches with whole application only. And when people switch app UI language - they do it for a reason, don't you think?

You can just change translations in Crowdin and keep app in your favorite language

MilosKozak commented 1 year ago

well you are a special case. You try understand the code, improve it etc ...... most people are blind. If you give them option to bypass some safety feature (which you mean "do it only when you know what you are doing" they interpret it as "there is an option and because it's available it's always save to check/uncheck this checbox" And if you hide it under engineering they just receive hint on FB: "sure, it's easy. Just turn on engineering mode and uncheck it" without any evaluation of risk

justmara commented 1 year ago

You can just change translations in Crowdin and keep app in your favorite language

No I cant. Because any translation will fall over that 70 characters limit. Any message substitution will lead to different behaviour because of locale change (emptying status message template for translation is a bad option - someone will sill want it).

If you give them option to bypass some safety feature

Whats the safety behind that pump status addition? It does not add any safety. It is only informational bloat that duplicates information already available in nsclient/ns. It does not trigger any additional safety check or so. Simple pointless metadata attached to informational message (report itself).

I had similiar opinion when i started. Now not anymore

I clearly understand the point of 'simplicity is the king' when talking about users of the application/service. But AAPS is not built as a simple get-and-run solution. Imho we can expect some degree of rationality from users. You cant stop people from doing stupid things. The more you try to guard'em - the more they treat you as a mindless tyrant and develop more exotic ways to do their stupid stuff :) And that could lead to even more dangerous scenarios than you could even imagine.