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.

Sign up for GitHub

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

Jump to bottom

Added refresh rate for picture entity card #5466

Closed
wants to merge 13 commits into from
Closed

Added refresh rate for picture entity card #5466

wants to merge 13 commits into from

Conversation

kkellner
Copy link

@kkellner kkellner commented Apr 6, 2020 •
edited

Proposed change

The hui-image used in picture-entity card has a hard-coded refresh rate of 10 seconds. To provide more flexibility in the image refresh rate a new user-configurable attribute has been added to the picture-entity card.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

For picture-entity card add a refresh_rate property. This is optional and defaults to the original behavior of 10 seconds if the attribute is not defined.

aspect_ratio: 70%
entity: camera.blueiris_cam24
show_name: false
show_state: false
type: picture-entity
refresh_rate: 2

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@homeassistant
Copy link
Contributor

Hi @kkellner,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@kkellner kkellner mentioned this pull request Apr 6, 2020
Closed
8 tasks
@homeassistant
Copy link
Contributor

Hi @kkellner,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@kkellner kkellner changed the title Added refresh rate for picture entity Added refresh rate for picture entity card Apr 6, 2020
bramkragten
bramkragten reviewed Apr 6, 2020
View reviewed changes
src/panels/lovelace/components/hui-image.ts Outdated
Comment on lines 178 to 180
Number(this.refreshRate ? this.refreshRate : DEFAULT_REFRESH_RATE) *
1000
);
Copy link
Member

Choose a reason for hiding this comment

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

Don't do the Number conversion, the values are numbers

bramkragten
bramkragten reviewed Apr 6, 2020
View reviewed changes
src/panels/lovelace/components/hui-image.ts Outdated
@@ -40,6 +40,8 @@ export class HuiImage extends LitElement {

@property() public aspectRatio?: string;

@property() public refreshRate?: string;
Copy link
Member

Choose a reason for hiding this comment

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

This should be a number, not a string

Copy link
Author

Choose a reason for hiding this comment

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

I originally coded it with "number", however the value was never saved. Maybe I missed somewhere I needed to change.

bramkragten
bramkragten reviewed Apr 6, 2020
View reviewed changes
src/panels/lovelace/components/hui-image.ts Outdated
@@ -208,9 +211,13 @@ export class HuiImage extends LitElement {
return;
}

const cacheTime =
(Number(this.refreshRate ? this.refreshRate : DEFAULT_REFRESH_RATE) - 1) *
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

bramkragten
bramkragten reviewed Apr 6, 2020
View reviewed changes
src/panels/lovelace/editor/config-elements/hui-picture-entity-card-editor.ts
@@ -36,6 +36,7 @@ const cardConfigStruct = struct({
camera_image: "string?",
camera_view: "string?",
aspect_ratio: "string?",
refresh_rate: "string?",
Copy link
Member

Choose a reason for hiding this comment

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

number

bramkragten
bramkragten reviewed Apr 6, 2020
View reviewed changes
translations/ca.json Outdated
@@ -2178,6 +2178,7 @@
},
"generic": {
"aspect_ratio": "Relació d'aspecte",
"refresh_rate": "Velocitat d’actualització (segons)",
Copy link
Member

Choose a reason for hiding this comment

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

Translations are handled in Localize, you should not add them here.

Copy link
Author

Choose a reason for hiding this comment

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

This is my first time contributing to HA -- I didn't find any other finds that contained the translations. When you say "handled in Localize", what do you mean? Is there a different git repo for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

src\translations\en.json

This is the only place that it needs to be added

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I get it now. Thanks for pointing that out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the other translation

Copy link
Author

Choose a reason for hiding this comment

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

Removed

@bramkragten
Copy link
Member

I don't think this is a good idea, a user can flood his Home Assistant now. We have camera_view: live for this.

@kkellner
Copy link
Author

kkellner commented Apr 6, 2020

The live view on my setup has a 7-12 second delay between showing on browser and what is "really live". I am using Blue Iris -- when I go directly to the web interface of Blue Iris its immediate. Not sure if there is buffering occuring in HA. However, the image view (auto) was instant. I agree that using low image refresh rates (e.g., 1 second) should be used with caution.

kkellner and others added 6 commits April 6, 2020 17:23
@kkellner @bramkragten
Update src/panels/lovelace/components/hui-image.ts
609d515 
Co-Authored-By: Bram Kragten <mail@bramkragten.nl>
@kkellner @bramkragten
Update src/panels/lovelace/editor/config-elements/hui-picture-entity-…
68caf4d 
…card-editor.ts

Co-Authored-By: Bram Kragten <mail@bramkragten.nl>
@kkellner @bramkragten
Update src/panels/lovelace/editor/config-elements/hui-picture-entity-…
098e114 
…card-editor.ts

Co-Authored-By: Bram Kragten <mail@bramkragten.nl>
@kkellner @bramkragten
Update src/panels/lovelace/elements/types.ts
e6799e1 
Co-Authored-By: Bram Kragten <mail@bramkragten.nl>
@kkellner @bramkragten
Update src/panels/lovelace/editor/config-elements/hui-picture-entity-…
4ce774a 
…card-editor.ts

Co-Authored-By: Bram Kragten <mail@bramkragten.nl>
@kkellner @bramkragten
Update src/panels/lovelace/elements/hui-image-element.ts
ad9e7db 
Co-Authored-By: Bram Kragten <mail@bramkragten.nl>
@kkellner
Copy link
Author

kkellner commented Apr 6, 2020

Changed everything to number and it is working. The only thing that wasn't working well is when, for example, the user changed the default of 10 to 5, the user would press the delete key twice to delete "10" before entering "5". When the field is empty it resulted in the field getting initialized back to 10. I changed the getter to return NaN instead of 10 when value is not specified by user which then uses the internal default of 10. Since this is an optional field, this seemed acceptable.

NaN Change

zsarnett
zsarnett reviewed Apr 9, 2020
View reviewed changes
translations/ca.json Outdated
@@ -2164,6 +2164,7 @@
"name": "Filtre d'entitats"
},
"entity": {
"description": "La targeta entitat mostra una visualització ràpida dels estats d'una o més entitats.",
Copy link
Contributor

Choose a reason for hiding this comment

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

All translations will need removed except for src/translations/en.json

Copy link
Author

Choose a reason for hiding this comment

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

I believe I fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good now

@zsarnett
Copy link
Contributor

zsarnett commented May 4, 2020

Maybe we can limit this to being 10 seconds or greater? If the user makes it lower than 10 it is changed to being 10

@kkellner
Copy link
Author

kkellner commented May 4, 2020

@zsarnett The original use case was to set the update interval to lower then 10 seconds as the live streaming from Blue Iris that flows through HA has a 7+ second delay. So the work around was to refresh a static image every 3 seconds. Maybe we can do some type of warning if the value is less than 10 seconds to inform user that the performance may be impacted.

@zsarnett
Copy link
Contributor

zsarnett commented May 4, 2020

Are you able to test and see if that work? Refreshing at a 3 second delay? IS HA now more in sync?

I know I see a delay with RTSP but I am not sure the delay is HA or just the RTSP

@kkellner
Copy link
Author

kkellner commented May 4, 2020

I can have a 1 second refresh rate on a modern laptop and it keeps up, but the target is a Fire Tablet which is slow, so backed off to 3 seconds which seems to work well.

As for live streaming delay. I can see that there is a 3 second delay coming from Blue Iris on the type of stream that is requested (m3u8 MIME type application/vnd.apple.mpegurl). I think its because it has to transcode to get this format. The additional delay seems to happen between between HA getting the stream and the browser getting the steaming bytes. This is something I want to dig in more to see if the native "video/mpeg" content type can be used from Blue Iris as this is much faster -- but that's a different issue.

@bramkragten
Copy link
Member

@kkellner I still don't think this is a good idea, if we would add this, it should not be in the UI editor as it is too advanced and could break systems.

Would it be an idea to create a custom card for this?

@kkellner
Copy link
Author

@bramkragten It doesn't seem like a refresh rate is too advanced of an option. Are you thinking that users will change to a value that is too low that HA would not be able to keep up? I was trying to avoid creating another card type in order to keep the functionality together and the code in sync with what is already provided. If you think this isn't going to workout, you can toss the change.

@nagyrobi
Copy link
Contributor

nagyrobi commented Jun 5, 2020

3 seconds is perfect here. 10 seconds too slow.
I was looking for a way to speed up.

@lock lock bot locked and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bramkragten bramkragten bramkragten left review comments

@zsarnett zsarnett zsarnett left review comments

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kkellner @homeassistant @bramkragten @zsarnett @nagyrobi