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

Create zwave-js select platform and discover additional siren values #53018

Merged
merged 11 commits into from Aug 16, 2021

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Jul 14, 2021

Proposed change

This PR adds the select platform to zwave_js and adds two new values for discovery from siren devices - default tone, and default volume level.

One thing to note here is that the siren platform entity uses a volume level between 0-1 but the default volume level number entity uses a volume level between 0-100%. I originally made a separate number entity class to represent the default volume as being between 0 and 1 but the UI component for number entities seems better optimized for larger values. With that being said, I can add that back if it makes sense to be consistent across the two platforms given that these are for the same device.

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
  • 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.
  • 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

Hey there @home-assistant/z-wave, mind taking a look at this pull request as its been labeled with an integration (zwave_js) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@MartinHjelmare
Copy link
Member

Maybe have it be 0-100% for both current and default volume level?

@raman325
Copy link
Contributor Author

raman325 commented Jul 15, 2021

Maybe have it be 0-100% for both current and default volume level?

Right now the siren platform uses cv.small_float. Maybe we should change that to vol.Any(cv.small_float, vol.All(vol.Coerce(int), vol.Range(min=0, max=100))) then leave it up to the integration to choose which? Or should we just make it 0-100 for all?

The reason I chose small_float for this btw is because that's how the media_player does it and it's the only other platform that has volume

@MartinHjelmare
Copy link
Member

Generally I think percent is more intuitive. Let's ask around and hear what others think.

@raman325
Copy link
Contributor Author

Generally I think percent is more intuitive. Let's ask around and hear what others think.

I would agree. I've asked the members group for input, let's see what they say

@raman325
Copy link
Contributor Author

People seem to be split. I looked across all non media player integrations for volume settings and the ones that are most commonly used (alexa and google assistant) use 0-1. There are two integrations that use 0-100 for custom services but that's it.

While I think that 0-100 is more intuitive, I think it's better to be consistent because it's otherwise not intuitive. So I am adjusting the number entity to be 0-1

@Mariusthvdb
Copy link
Contributor

not sure if this is waiting for some more response, but if so, please consider merging so we can go ahead and test. Now that sirens are supported in core, being able to set the volume more intuitively is of utmost priority.. Current situation is very counter intuitive.

my vote would be for percentage too, but in all honesty, could live with the other option too.

@raman325 raman325 requested a review from a team as a code owner August 16, 2021 14:55
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

Dev automation moved this from Needs review to Reviewer approved Aug 16, 2021
@raman325 raman325 merged commit a41ee9e into home-assistant:dev Aug 16, 2021
Dev automation moved this from Reviewer approved to Done Aug 16, 2021
@raman325 raman325 deleted the zwave_js_siren_2 branch August 16, 2021 17:36
@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants