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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fallback lg_soundbar sound mode on unknown value #35892

Merged
merged 5 commits into from May 25, 2020

Conversation

teldri
Copy link
Contributor

@teldri teldri commented May 20, 2020

Proposed change

Make the "lg_soundbar" component crash free in case of equaliser settings that the temescal library does not know.

A issue has been created in the temescal repro to fix the unknown equaliser.
google/python-temescal#2

It麓s most likley that LG will extend the equalisers in their soundbar(s) in future again. This fix or something similar should be applied in order to prevent bad user experience in that case.

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

Example entry for configuration.yaml:

# Example configuration.yaml
discovery:

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

@project-bot project-bot bot added this to Needs review in Dev May 20, 2020
@homeassistant
Copy link
Contributor

Hi @teldri,

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!

@teldri
Copy link
Contributor Author

teldri commented May 23, 2020

Hi @homeassistant,

I fixed the cla-error. Do I nned to do something to get this label removed?

@MartinHjelmare MartinHjelmare changed the title fallback if equaliser setting unknown to temescal Fallback lg_soundbar sound mode fallback on unknown value May 23, 2020
@MartinHjelmare MartinHjelmare changed the title Fallback lg_soundbar sound mode fallback on unknown value Fallback lg_soundbar sound mode on unknown value May 23, 2020
Dev automation moved this from Needs review to Review in progress May 23, 2020
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.

This looks like it should be fixed in the library instead.

return temescal.equalisers[self._equaliser]

@property
def sound_mode_list(self):
"""Return the available sound modes."""
modes = []
for equaliser in self._equalisers:
if equaliser >= len(temescal.equalisers):
temescal.equalisers.append("unknown " + str(equaliser))
Copy link
Member

Choose a reason for hiding this comment

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

Entity state properties should not have side effects. Please move this to eg update method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like it should be fixed in the library instead.

I agree that it would be the optimum that it麓s handeled by the library.
But it麓s not the case and changing that would require changes in the lib as well as in this component.

This hacky approch is for sure not the optimum, but I麓m as a user prefer to have a equaliser setting named e.g.: "unknown 18" instead of not beeing able to select the setting at all.
Because the side effect is also that if I select the unknown setting using the lg app or the soundbars remote, in the lovelace dashboard I don麓t see any selected sound mode.

I made a PR in the librarys repro to fix the missing sound mode. If that get麓s accepted I would do another PR to there to handle that problem in the lib it麓s self. It just seems kind if inactive to me and I麓m not expecting too much.
Sorry writing half a book in this comment.

@home-assistant home-assistant deleted a comment from homeassistant May 23, 2020
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.

I think it's ok for now if we add a comment.

Dev automation moved this from Review in progress to Reviewer approved May 25, 2020
@cgarwood cgarwood merged commit 2793e6c into home-assistant:dev May 25, 2020
Dev automation moved this from Reviewer approved to Done May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants