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

Dynamic icons causing way too many requests #3404

Closed
TheTrueRandom opened this issue May 18, 2023 · 34 comments · Fixed by #3456
Closed

Dynamic icons causing way too many requests #3404

TheTrueRandom opened this issue May 18, 2023 · 34 comments · Fixed by #3456
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@TheTrueRandom
Copy link

TheTrueRandom commented May 18, 2023

Actual behaviour

Each time a value is updated (with a new value which is not cached) a request is sent to openhab to load the icon. This causes incredible many request in some cases:
I am using a sitemap to show power information, this includes: grid power, PV power, battery power, power of devices, etc. The values are updated 1-2 times per second, resulting in about 10 updates per second distributed on those items. This looks incredible nice when it is actually updating. However on the app I can often see icons are flickering and also item updates are stuck until manual refresh. This github issue is for the icons, for the items not updating I still need to analyze why this is happening. In the android studio app inspection I can see that incredible many requests are done to resolve the same icon (poweroutlet, which I am using for all those items).

Generally I have the feeling the app is not really optimized when values are updated multiple times a second (as the UI gets stuck quite often), which is a pity as this looks incredible nice when it actually works.

Screenshot_2023-05-18_23-57-30

Expected behaviour

Icons should be cached more aggressively, I'm not sure how an optimal solution could look like but maybe considering something like this

  • poweroutlet icon is not even dynamic in openhab, why is it requested every time?
  • I'm not sure how dynamic icons work, but I'm quite sure issuing one request every time something changes is very inefficient. Maybe all icon states could be retrieved at once in the beginning. And this could even be persisted in the app and openhab only asked/subscribed for changes to the icons
  • on the screenshot you can see the items are updated with many digits, even though on the UI no digits are rendered (as specified in the openhab item file), this seems to be ignored for the image cache. But even if it would not be ignored, there are incredible many different values as seen on the screenshot

Steps to reproduce

  1. create an item of type number
  2. define a sitemap with the item and poweroutlet icon
  3. create a script to update the item multiple times a second, slightly changing the values (like on the screenshot)
  4. notice for every change a request is sent to openhab to fetch the icon

(app is connected to local url)

Can you reproduce the issue in demo mode?

Environment data

Client

  • Android version: 13.0
  • Device model: Pixel XL
  • App version 3.6.1
  • Build flavor play store and from github on an emulator.
  • Device language: english

Server

  • Server version: openhab 3.4.4 release build
  • Reverse Proxy:
  • Authentication method: None
@TheTrueRandom TheTrueRandom added the bug Indicates an unexpected problem or unintended behavior label May 18, 2023
@maniac103
Copy link
Contributor

I'm not sure how dynamic icons work, but I'm quite sure issuing one request every time something changes is very inefficient.

They work by sending the item state as query parameter. Whether or not that request will yield a different or the same icon is something not known to the app, only to the server.
Looking at your request log, the only area of improvement I can think of is formatting the state in the query parameter using the format in the state description. I assume you use 0 or 1 decimal digits for displaying those powers?

@TheTrueRandom
Copy link
Author

TheTrueRandom commented May 19, 2023

@maniac103 maybe it should be something the app could know then? We could add an api to openhab-core to get this information. For example the server could return the number ranges for which a given icon are valid ("0-1000: icon1, 1001-5000: icon2"), so the android app doesn't need to request an icon of a range already loaded.
Even 0 or 1 decimal digits won't solve the issue as the values are anything between -5000 and +10000 varying very often. This easily results in 1000 requests when opening the sitemap for just a few seconds.

Also I found the issue that items are not updating at all is not related to the android app but to the eventstream of openhab-core. /sitemaps/events at some point just stalls and returns nothing. Created an issue for that

@mueller-ma
Copy link
Member

Looking at your request log, the only area of improvement I can think of is formatting the state in the query parameter using the format in the state description.

👍

Anything else requires some server side changes and I remember a similar issue years ago that was closed as "won't fix". I can't find it right now, though.

@TheTrueRandom
Copy link
Author

But why don't fix it properly? This issue is worth to solve it properly as it easily generates a lot of traffic and probably for many people (most likely unknowingly) it will also cause more unnecessary mobile data usage which is quite bothering.

@rkoshak
Copy link

rkoshak commented May 22, 2023

But why don't fix it properly?

Things I can see:

  • the proposed fix is not in this repo, or at the very least it first needs to be implemented in core
  • the proposed fix would break some use existing cases
    • changes made to Item/Icon configs require a reload to see in the UI
    • dynamically generated icons (e.g. current weather conditions) would no longer work at all as they are often implemented (i.e. downloading and saving the current icon from the weather provider to a single png/svg file as it changes)
  • Your case is fairly far outside the data rates OH has been designed to support. That's not a vote against changes to make it so that OH can support them, but not at the cost of breaking use cases for existing users.

@TheTrueRandom
Copy link
Author

TheTrueRandom commented May 26, 2023

I agree that it needs a change to core. However I don't agree that it's a breaking change. It won't break anything. Following things are proposed:

  • some icons are not dynamic, like the poweroutlet icon, they could even be compiled with the android app and don't need a http request.
  • for dynamic icons the valid range could be requested from core (needs a change to core), those ranges can be stored and subscribed for changes.
  • use the item value representation for icon requests, this is also not a breaking change, rather the current implementation is a bug. If openhab item state is 700.23408W, but my sitemap shows 700W, the request has to go for 700W.
  • I would assume the core developers of the android app could have many more ideas if think about it.

Seems like my usecase is specific, which I think is a bit unfortunate. Openhab is way more pretty with fast updating items and there are many usecases for that.

@maniac103
Copy link
Contributor

some icons are not dynamic, like the poweroutlet icon, they could even be compiled with the android app and don't need a http request.

That's not true. The default poweroutlet icon is static, but there's nothing hindering the user from adding additional icons that make it dynamic. Because of this, compiling icons into the app also is not an option.

for dynamic icons the valid range could be requested from core (needs a change to core), those ranges can be stored and subscribed for changes.

That's probably worth a feature request in core.

use the item value representation for icon requests, this is also not a breaking change, rather the current implementation is a bug

I already acknowledged this above.

I would assume the core developers of the android app could have many more ideas if think about it.

Now you're saying me and @mueller-ma are just too lazy to properly think about the problem?

@TheTrueRandom
Copy link
Author

Now you're saying me and @mueller-ma are just too lazy to properly think about the problem?

That's you saying that.

Other option could be to add an option to the android app to disable dynamic icon loading, maybe only for the remote server. On the local network it doesn't matter so much (besides the icons flickering - disappearing and appearing again), but connecting to the remote server it matters.
I measured the data consumption a bit: Having a sitemap with 1-3 changes per second causes 1MB of network usage per minute. So opening this sitemap every day for 3 minutes consumes 100MB of mobile data a month, 85% of it is for the icon loading.

@mueller-ma
Copy link
Member

You can enable the data saver mode to disable dynamic icons. But be aware that other data-heavy widgets won't work as well, e.g. images.

I'm not saying that it shouldn't be changed something on the server side, but in this case it's getting a lot better when the formatting is applied to the icon URL. And please note that icons are cached forever until you press "Clear icon cache" in the settings.

@TheTrueRandom
Copy link
Author

@mueller-ma ah I wasn't aware of the data-saver, that's good, will try it. Also wasn't aware about that it's cached forever. How does the caching work as I thought only the server knows if there is a change? In my case will also be quite some to cache (>10000 states) What would be your suggestion for that usecase? Having Grid/PV power refreshing 1-2 times a second, values usually varying between -5000W and 10000W.

@lolodomo
Copy link
Contributor

lolodomo commented Jul 14, 2023

I believe we could add a new property to the sitemap widget, something like "iconIgnoreState". If present, the sitemap UI will build an icon URL without the item state and the cache will then be used whatever the item state. @maniac103 @mueller-ma : WDYT ?

There is something similar in MainUI, even if I am not 100% sure of the current status of this option in OH4.

@mueller-ma
Copy link
Member

How does the caching work as I thought only the server knows if there is a change?

When you change icons you have to clear the cache manually in the app settings.

There is something similar in MainUI, even if I am not 100% sure of the current status of this option in OH4.

I think keeping it similar to MainUI is a good idea and a new parameter is probably the least amount of work to implement it. Another idea would be some API that returns all icon states on the server, e.g. 0-10, 11-20, 21-30, and so on. But that requires some parsing on the client.

At least for the Label Card I haven't seen such an option, though:
grafik

@lolodomo
Copy link
Contributor

@mueller-ma : it is a parameter for the default list widget.
It is enabled by default only for certain item types.
openhab/openhab-webui#1874

@mueller-ma
Copy link
Member

For consistency with Main UI a new Sitemap parameter iconUseState should be added
I'm not sure about the default value, though: Should it default to true for 'Contact', 'Dimmer', 'Rollershutter' and 'Switch' and false otherwise? This would lead to a situation where someone with openHAB server 3 (or unpatched version 4) and the patched Android app has dynamic icons disabled for some Item types and is unable to enable them. Also existing Sitemaps would need to be migrated.
Maybe set the default to true for all Item types?

@lolodomo
Copy link
Contributor

lolodomo commented Jul 26, 2023

By the way, this will be a parameter for sitemap elements. I am not in favor to hardcode something about item type in each sitemap UI.
I agree that it should be ON by default for backward compatibility. But in this case, we need to reverse its name to disable the default, calling it iconIgnoreState for example.

@lolodomo
Copy link
Contributor

Finally, I think we can add a parameter iconUseState accepting true or false as possible values and with true as default.
I will prepare a PR to update the sitemap syntax.

@dilyanpalauzov
Copy link
Contributor

dilyanpalauzov commented Jul 29, 2023

I suggest instead staticIcon. This shall be the opposite of “Dynamic icon” and shall not have true or false value. Just its presence implies that the icon is static. This results more concise syntax.

@lolodomo
Copy link
Contributor

I suggest instead staticIcon. This shall be the opposite of “Dynamic icon” and shall not have true or false value. Just it presence implies that the icon is static. This results more concise syntax.

My PR was almost ready with iconUseState but your proposal with staticIcon is fine for me and I even prefer it.
Is it ok for you @mueller-ma ?

lolodomo added a commit to lolodomo/openhab-core that referenced this issue Jul 30, 2023
Added to the following sitemap elements: text, input, switch, slider,
selection, setpoint, colorpicker and default.

When set, the UI should not provide the item state when requestiong the
OH icon and should not request again the icon when the item state is
updated.
The default is to use the item state for dynamic icon.

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
lolodomo added a commit to lolodomo/openhab-core that referenced this issue Jul 31, 2023
When set, the UI should not provide the item state when requestiong the
OH icon and should not request again the icon when the item state is
updated.

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
lolodomo added a commit to lolodomo/openhab-webui that referenced this issue Jul 31, 2023
When set on a sitemap element, Basic UI ignores the item state when
initially requesting the OH icon and do not request it again when the
item state is updated.

Depends on openhab/openhab-core#3735

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo
Copy link
Contributor

It is now implemented in core framework and in Basic UI. We just need to wait for review & merge.

@mueller-ma
Copy link
Member

Is it ok for you @mueller-ma ?

I'm fine with that as well.

@kaikreuzer
Copy link
Member

My PR was almost ready with iconUseState but your proposal with staticIcon is fine for me and I even prefer it.

Imho this makes the code in openhab/openhab-core#3735 not consistant. icon being an String parameter and staticIcon being a Boolean is unintuitive. Could we go more along with Java naming conventions, e.g. isStaticIcon, just like the method is called as well?

@dilyanpalauzov
Copy link
Contributor

My understanding is that staticIcon has in the sitemap no value, so from users’s perspective it is not boolean. Just its presence implies no dynamic icon. A non backward compatible approach would be to repleace icon= with both staticIcon=name or dynamicIcon=name.

@kaikreuzer
Copy link
Member

so from users’s perspective it is not boolean.

Hm, well, it is a keyword whose presence corresponds to a boolean param.
Still, imho to the user it feels natural to start typing staticIcon=socket, which can be an easy trap for people not knowing that it is a boolean and not a string param. So anything that is more intuitive might be preferable.

@dilyanpalauzov
Copy link
Contributor

So anything that is more intuitive might be preferable.

For instance?

@lolodomo
Copy link
Contributor

lolodomo commented Aug 8, 2023

Still, imho to the user it feels natural to start typing staticIcon=socket,

Ok, that is possible and it keeps backward compatibility.
But internally (REST API), I will keep the additional boolean to not have two different fields that can contain an icon reference and to be backward compatible..
I will update my PR.

@mueller-ma
Copy link
Member

@lolodomo What @kaikreuzer wrote sounds to me more like: If the parameter is called staticIcon it feels strange that there's no icon name after it.
I'd avoid having two parameters to configure an item (icon=foo and staticIcon=foo).

@lolodomo
Copy link
Contributor

lolodomo commented Aug 9, 2023

@lolodomo What @kaikreuzer wrote sounds to me more like: If the parameter is called staticIcon it feels strange that there's no icon name after it.

That is also what I understand.

I'd avoid having two parameters to configure an item (icon=foo and staticIcon=foo).

But having both is what @kaikreuzer propose!
I can try in the syntax to avoid having both defined for the same element.

@dilyanpalauzov
Copy link
Contributor

How about icon=file meanimg dynamic icon, as it is now. And staticIcon=file, without the mutually exclusive icon=file, meaning static icon?

@maniac103
Copy link
Contributor

without the mutually exclusive icon=file,

I think the 'mutually exclusive' part is what @lolodomo referred to in

I can try in the syntax to avoid having both defined for the same element.

I agree this would be the most elegant solution.

lolodomo added a commit to lolodomo/openhab-core that referenced this issue Aug 9, 2023
When set, the UI should not provide the item state when requestiong the
OH icon and should not request again the icon when the item state is
updated.

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
lolodomo added a commit to lolodomo/openhab-webui that referenced this issue Aug 9, 2023
When set on a sitemap element, Basic UI ignores the item state when
initially requesting the OH icon and do not request it again when the
item state is updated.

Depends on openhab/openhab-core#3735

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo
Copy link
Contributor

lolodomo commented Aug 9, 2023

Ok, PR is changed. icon and staticIcon are exclusive for the same sitemap element. You can use either icon or staticIcon to define the icon.

When the icon is implicitly defined by the sitemap element because not set neither on item nor on sitemap element, you will have to define it to make it static. Imagine this element:

Switch item=toto

and no category is set on item.
The icon is implicitly "switch" and will be dynamic.
If you want to make your icon static, you will have to set:

Switch item=toto staticIcon="switch"

@dilyanpalauzov
Copy link
Contributor

If you want to make your icon static, you will have to set:

Switch item=toto staticIcon="switch"

I do not understand this. Why is it not sufficient to write `Switch staticIcon="switch" ?

@lolodomo
Copy link
Contributor

lolodomo commented Aug 9, 2023

Because you have to link your sitemap element to an item !
Maybe the item parameter is optional for a switch element, I don't remember. By the way, having a UI element linked to nothing is just useless.
But strangely, your question seems to have no link with the current discussion with icon...

@dilyanpalauzov
Copy link
Contributor

Yes, you are correct. You wrote item=, but I read icon=

kaikreuzer pushed a commit to openhab/openhab-core that referenced this issue Aug 13, 2023
When set, the UI should not provide the item state when requestiong the
OH icon and should not request again the icon when the item state is
updated.

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo
Copy link
Contributor

@openhab/android-maintainers : core PR has just been merged. This can now be handled in the Android app.

maniac103 added a commit to maniac103/openhab.android that referenced this issue Aug 31, 2023
If set, icon state should not be included when fetching the icon for a
given widget.

See openhab#3404

Signed-off-by: Danny Baumann <[email protected]>
maniac103 added a commit to maniac103/openhab.android that referenced this issue Aug 31, 2023
kaikreuzer pushed a commit to openhab/openhab-webui that referenced this issue Sep 9, 2023
When set on a sitemap element, Basic UI ignores the item state when
initially requesting the OH icon and do not request it again when the
item state is updated.

Depends on openhab/openhab-core#3735

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
stefan-hoehn pushed a commit to stefan-hoehn/openhab-webui that referenced this issue Sep 23, 2023
When set on a sitemap element, Basic UI ignores the item state when
initially requesting the OH icon and do not request it again when the
item state is updated.

Depends on openhab/openhab-core#3735

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Stefan Höhn <[email protected]>
JustinGeorgi pushed a commit to JustinGeorgi/openhab-webui that referenced this issue Sep 24, 2023
When set on a sitemap element, Basic UI ignores the item state when
initially requesting the OH icon and do not request it again when the
item state is updated.

Depends on openhab/openhab-core#3735

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: jgeorgi <[email protected]>
JustinGeorgi pushed a commit to JustinGeorgi/openhab-webui that referenced this issue Sep 24, 2023
When set on a sitemap element, Basic UI ignores the item state when
initially requesting the OH icon and do not request it again when the
item state is updated.

Depends on openhab/openhab-core#3735

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: jgeorgi <[email protected]>
digitaldan pushed a commit to digitaldan/openhab-webui that referenced this issue Sep 24, 2023
When set on a sitemap element, Basic UI ignores the item state when
initially requesting the OH icon and do not request it again when the
item state is updated.

Depends on openhab/openhab-core#3735

Related to openhab/openhab-android#3404

Signed-off-by: Laurent Garnier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants