Skip to content

Add buttons to Ring chime devices to play ding and motion chimes#71370

Merged
balloob merged 4 commits intohome-assistant:devfrom
grablair:ring-add-play-chime-controls
May 5, 2022
Merged

Add buttons to Ring chime devices to play ding and motion chimes#71370
balloob merged 4 commits intohome-assistant:devfrom
grablair:ring-add-play-chime-controls

Conversation

@grablair
Copy link
Copy Markdown
Contributor

@grablair grablair commented May 5, 2022

Proposed change

The Ring Chime products are great little noisemakers for notifications. There are some people (myself included) who may want to play these sounds outside of the context of a doorbell.

This adds two buttons per Chime device, one for playing each kind of chime notification (ding, motion).

Type of change

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

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass @grablair: (only ran tests associated with Ring)
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @balloob, mind taking a look at this pull request as it has been labeled with an integration (ring) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@grablair grablair force-pushed the ring-add-play-chime-controls branch from ee44c0a to 3e9c2f3 Compare May 5, 2022 20:20
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @grablair,

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!

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Great contribution! Left a few tiny cleanup comments.

grablair and others added 2 commits May 5, 2022 15:34
Co-authored-by: Paulus Schoutsen <paulus@home-assistant.io>
@grablair grablair force-pushed the ring-add-play-chime-controls branch from 59a38b1 to 809e0bd Compare May 5, 2022 22:48
@grablair
Copy link
Copy Markdown
Contributor Author

grablair commented May 5, 2022

Thanks @balloob! Happy to be helping out. :)

Addressed those comments and fixed some style issues; awaiting approval checks again.

@home-assistant home-assistant deleted a comment from homeassistant May 5, 2022
@balloob balloob merged commit c22cf3b into home-assistant:dev May 5, 2022
@balloob
Copy link
Copy Markdown
Member

balloob commented May 5, 2022

Merged, great first contribution. It will be in tonights nightly 👍

@elupus
Copy link
Copy Markdown
Contributor

elupus commented May 6, 2022

Shouldn't this have used the siren (https://www.home-assistant.io/integrations/siren/) platform? While I do know it's a bit messy with lack of ability to play a specific signal from UI, it works from services

@balloob
Copy link
Copy Markdown
Member

balloob commented May 6, 2022

That's a good point @elupus, this should have been a siren. I forgot about that. @grablair could you open a new PR to migrate it?

@grablair
Copy link
Copy Markdown
Contributor Author

grablair commented May 6, 2022

Happy to! Sorry for the miss; I have only been working with Home Assistant for a week or two.


Are sirens required to continue playing until turned off? Or is it appropriate for sirens to play one sound and then turn themselves off? My assumption was the former, but please correct me if I am wrong. It isn't clear from the docs.

Partially based on this assumption, I felt like a button was more appropriate for a few reasons:

  1. AFAIK, there is no way to stop the test sound on a Ring Chime device, so if the siren is turned off, it will complete the sound that is playing. Not a big deal, but not a great experience (since the sounds can be 7 seconds long).
  2. We cannot tell the Ring Chime devices to loop, so if my assumption of the siren being on until turned off is accurate, we will have to just continue to issue the "play sound" call, which is fine and doable, with one caveat:
  3. There is no way for us to programmatically determine the length of a given Chime sound, or whether one is currently playing, and Amazon/Ring could change that sound from device iteration to device iteration, so hardcoding the lengths seems like the wrong move, too.

I felt like a button was very clear to the user, given these limitations: it isn't stoppable, and it will only happen once (and you can make it work with the button.press service).

Happy to make the change to the siren if someone can fill me in on the best practices with these device limitations in mind! :)

@elupus
Copy link
Copy Markdown
Contributor

elupus commented May 6, 2022

It's fine for a siren to play once at the moment. Though i do see the problem/ambiguity. Still siren is the correct place for it. It might be that we need to adjust how the siren platform work a bit later.

@grablair
Copy link
Copy Markdown
Contributor Author

grablair commented May 6, 2022

ACK, thanks! I will try and get that PR submitted over the next week or so.

Personally, I think sirens and chimes could be disambiguated into two platforms, but that's definitely a discussion for another place and time.

@github-actions github-actions bot locked and limited conversation to collaborators May 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants