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

Add Room Audio Control to Control4 Integration #87821

Merged
merged 8 commits into from
Mar 19, 2024

Conversation

nalin29
Copy link
Contributor

@nalin29 nalin29 commented Feb 10, 2023

Breaking change

Proposed change

Add Media Player Device to Control4 Integration. This change allows users to operate their room sound system by changing sources, changing volume, muting and turning off the room.

Dependency Diff:

lawtancool/pyControl4@v1.0.0b6...v1.1.0

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)
  • Deprecation (breaking change to happen in the future)
  • 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.
  • 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.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hi @nalin29

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!

@home-assistant
Copy link

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

Code owner commands

Code owners of control4 can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign control4 Removes the current integration label and assignees on the issue, add the integration domain after the command.

@lawtancool
Copy link
Contributor

lawtancool commented Feb 10, 2023

@nalin29 Thanks for this! Is the only reason for the forked dependency this PR? lawtancool/pyControl4#20

If so, we can work to get that merged and avoid the dependency change.

homeassistant/components/control4/director_utils.py Outdated Show resolved Hide resolved
homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft February 10, 2023 05:55
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@nalin29
Copy link
Contributor Author

nalin29 commented Feb 10, 2023

@nalin29 Thanks for this! Is the only reason for the forked dependency this PR? lawtancool/pyControl4#20

If so, we can work to get that merged and avoid the dependency change.

Yep, its only that. If that gets merged then we do not need the forked version of pyControl4.

@nalin29
Copy link
Contributor Author

nalin29 commented Feb 10, 2023

@lawtancool If kdkavanagh who authored the original pull request does not respond promptly I can send a pull request and make any required changes.

@nalin29
Copy link
Contributor Author

nalin29 commented Feb 11, 2023

Note I squashed all previous commits into one

@nalin29
Copy link
Contributor Author

nalin29 commented Feb 11, 2023

@lawtancool I added a new pull request for pycontrol4 with the suggested changes and squashed all commits: lawtancool/pyControl4#21

@nalin29
Copy link
Contributor Author

nalin29 commented Feb 15, 2023

Updated the dependency fixed the code and tested

@nalin29 nalin29 marked this pull request as ready for review February 15, 2023 05:40
@nalin29
Copy link
Contributor Author

nalin29 commented Jul 13, 2023

Thanks for splitting out/sorting out all the preliminary PRs.

Sadly for various reasons I have not been in a position to review PRs recently and won't be able to do so for a few months still.

Is there another HA dev that could be assigned? This should be close to done so if someone else has the time we can get this pulled.

@edenhaus
Copy link
Contributor

Is #90501 a duplicate of this PR?

At least @nalin29 your are the author of both and the changes look similar

@nalin29
Copy link
Contributor Author

nalin29 commented Sep 28, 2023

@edenhaus yes I moved the changes over to here since this was the original PR which required multiple preliminary PRs and is attached to documentation changes should it be merged. This should be the newer version of the changes.

@nalin29
Copy link
Contributor Author

nalin29 commented Sep 28, 2023

@edenhaus rebased to fix conflict

homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/control4/media_player.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft November 23, 2023 08:59
@emontnemery
Copy link
Contributor

Please open a documentation PR to make it clear what is supported: https://www.home-assistant.io/integrations/control4/

@nalin29
Copy link
Contributor Author

nalin29 commented Nov 25, 2023

Please open a documentation PR to make it clear what is supported: https://www.home-assistant.io/integrations/control4/

home-assistant/home-assistant.io#26196

@granvillebarker
Copy link

Guys this control4 integration not getting merged has really been a pain for me. I didn't realize that lawtancool had a much newer version on his site, and implemented fan and climate based on this old outdated stuff in the current version of home assistant. Then I rewrote it all to work with his latest integration on his git respository. Now after reading all this, I don't know if nalin29 has updated lawtancools's repository, or if there are becoming many versions of this integration that are going in different directions, which is not good for anyone.

The whole point of having all this is to prevent this type of stuff from happening, but when latest codes doesn't get merged its not good. SO CAN ALL YOU GUYS GET OFF YOUR COUCHES AND GET THIS DONE!

@nalin29
Copy link
Contributor Author

nalin29 commented Dec 11, 2023

@granvillebarker

I was waiting to try to get this merged beginning of year. Due to unfortunate circumstances the reviewer was unable to help get this merged so it sat stale hence my 3rd party. But I don't have continuous access to the control4 system anymore as I have moved. So any developments from my side will be slow. If you want this merged you can try and pick this up, it's very simple code and I can help if wanted.

@emontnemery
Copy link
Contributor

emontnemery commented Feb 1, 2024

@nalin29 If you want to get the attention of the reviewer, you must hit the "ready for review" button. You replied to some of my review comments, but not all, are you planning to address the remaining ones?

Edit: I see now you mentioned you no longer have access to the Control 4 system

@nalin29 nalin29 marked this pull request as ready for review March 17, 2024 05:04
@nalin29
Copy link
Contributor Author

nalin29 commented Mar 17, 2024

hey @emontnemery . I just got access to the C4 system for a short time. Thought I could bang this revision out and test it. Can you review. There are some other changes needed to allow me to pass pylint due to changes with data coordinator default data type.

nalin29 and others added 2 commits March 18, 2024 17:07
@nalin29
Copy link
Contributor Author

nalin29 commented Mar 18, 2024

@emontnemery made all requested changes please take a look.

If we are ready to merge i had to reopen the doc pull request as it was closed: home-assistant/home-assistant.io#31910

Additionally, there is one more pull request I will open that will address stability since users are reporting to custom implementation that c4 api is unstable recently so i am adding retries to some of the class during setup.

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Thanks, @nalin29 👍

@emontnemery emontnemery merged commit 18ef76a into home-assistant:dev Mar 19, 2024
21 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 2024
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.

7 participants