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

Implement relative volume adjustments for the command_volume_level notification command #4056

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marazmarci
Copy link
Contributor

@marazmarci marazmarci commented Dec 15, 2023

Summary

This PR enhances the command_volume_level notification command to support both absolute and relative volume adjustments. Introducing a boolean parameter, relative_volume, allows users to specify whether the provided value is an absolute volume level or a relative adjustment to the current volume. Omitting or setting the parameter to false preserves the original absolute volume behavior.

The motivation behind these changes is to make changing the volume as responsible as possible. When I tried to set up an automation for a Zigbee music controller remote to control music playback on an Android device through HA using notification commands, I encountered some difficulties: the volume sensors don't update instantly, so if I wanted to handle multiple successive volume change actions from the remote, these steps were required in the automation:

  1. read the current value of the volume sensor
  2. calculate the new volume by adding or subtracting from it, based on whether the Volume Up or Down button was pressed on the remote
  3. fire the command_volume_level notification command with the new absolute volume
  4. since the volume sensor doesn't update automatically, update the volume sensor's value by firing the command_update_sensors notification command, so a potential successive volume change can be handled with the same steps.

And the last step made it pretty slow. It can be much faster if the automation doesn't have to wait for the volume sensor to be updated.

This is how the YAML of a service call using this functionality looks like:

service: notify.mobile_app_<device_name>
data:
  message: command_volume_level
  data:
    media_stream: music_stream
    relative_volume / relative / adjust / delta: true # <- which one do you like better?
    command: -2 # this makes the volume decrease 2 steps
  • The PR is marked as a draft because I'm not sure which parameter name is the best. I let the maintainers decide it during the review.

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

  • I'll open the doc update PR when we have the final parameter name ⏳

@marazmarci marazmarci marked this pull request as draft December 15, 2023 12:44
@@ -136,6 +136,7 @@ class MessagingManager @Inject constructor(
const val HIGH_ACCURACY_UPDATE_INTERVAL = "high_accuracy_update_interval"
const val PACKAGE_NAME = "package_name"
const val CONFIRMATION = "confirmation"
const val RELATIVE_VOLUME = "relative_volume"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which parameter name do you think is the best?

  • relative_volume
  • relative
  • adjust
  • delta

@dshokouhi
Copy link
Member

What is the use case behind this change? I thought that relative was part of setStreamVolume but it seems we are doing something different here? Does this match another integration or something that I am missing?

@marazmarci
Copy link
Contributor Author

marazmarci commented Dec 20, 2023

What is the use case behind this change?

Sorry, I forgot to write down the most important part in the description 😅 I extended the Summary section, please take a look.

I thought that relative was part of setStreamVolume but it seems we are doing something different here?

No, setStreamVolume uses an absolute volume level.

Does this match another integration or something that I am missing?

No, I didn't base my implementation on another integration, I just went after my intuition when making these changes. But I'm not exactly sure what you meant with this 😄

@dshokouhi
Copy link
Member

ok it does make more sense but I am honestly still not sure how many other users would find it useful and if a more generic approach to the issue is better to consider.

Do you think a better fix would be to just update the sensors anytime we get this command called? That would solve your issue of not having an immediate update after the command was received as mentioned in point 4.

@marazmarci
Copy link
Contributor Author

I have to admit that I have another PR prepared that can provide an alternative and more general "solution". It will make the volume sensors update instantly by subscribing to a broadcast Intent sent by the Android system. (Here's the code, I just need to test it on Wear OS before opening a PR.

This will certainly make the UX better in my use case because the automation doesn't have to send a command_update_sensors before waiting for the volume sensor to update, so the roundtrip time of an additional notification command can be spared.

However, I can imagine other users also wanting to set up automations to control media playback on their Android devices, including +/- volume control, and in that case, they'll run into the same limitation as me, and their +/- volume control can never be 100% reliable.

@dshokouhi
Copy link
Member

I think updating the volume sensors immediately will be greatly appreciated by all users who use those sensors :) in all honesty I would've added that when the sensors were added but I missed this intent when adding them 😅

@dshokouhi
Copy link
Member

Hey @marazmarci did you still want to add this feature? I know we added instant updates in a different PR but given we just accepted another relative PR this one may still have some value?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants