Add config flow to the Onkyo component and change to local push #59518
Conversation
|
I just thought of some more changes to increase the quality scale for the integration, which I'll soon add to this PR. But it would be great if someone could already start reviewing this. |
At this point the builds are failing, please make sure the builds pass. |
|
I changed the max volume option from an integration option to a number entity per device. I use a
|
|
@klaashoekstra94 Thanks so much for your work in extending Onkyo support, this is very exciting stuff. Could I very humbly ask for 1 small (I believe) addition? My AVR supports the Whole House listening mode which I believe is not included in your mappings list, is this something that you could add? I see the option in Onkyo-EISCP and I've tested it successfully which gives me hope this may be possible in your implementation. Thank you again |
|
@Chumba I added the whole house listening mode. Initially I had not implemented it since it does not seem to be working on my AVR, but if it works on yours we should add it. Now let's hope this PR gets merged soon! |
|
Hey @frenck, I was wondering, when can I expect some progress on this PR? Thanks in advance! |
|
I don't think the PR description matches the actual PR anymore. I suggest instead of trying to pile all of these changes into one PR, you split out the config flow addition, the library switch, and the addition of sound mode into 3 separate PRs. I can almost guarantee you will get a lot faster turnaround on those. So much is changing in this PR that it's hard to keep track of everything. |
frenck
added
the
config-flow
klaashoekstra94
force-pushed
the
onkyo-config-flow
branch
from
87850fe to
463fbe5
Compare
klaashoekstra94
changed the title
|
@raman325 I agree that the PR has gotten quite big, but @frenck already made some initial review comments that I resolved. Also I would greatly appreciate it if this could stay a single PR, instead of having to split everything apart again, given the time I already put into this. Also, by keeping this in a single PR, the breaking changes are minimal. Let's see what happens! |
klaashoekstra94
force-pushed
the
onkyo-config-flow
branch
from
463fbe5 to
729c9f1
Compare
klaashoekstra94
force-pushed
the
onkyo-config-flow
branch
from
729c9f1 to
46c87b2
Compare
|
Hey @frenck, I'd still like to get this into Home Assistant :) Can you maybe review this, or do you need anything else from me? |
klaashoekstra94
force-pushed
the
onkyo-config-flow
branch
2 times, most recently
from
46c87b2 to
f3769b6
Compare
|
@frenck I'm still trying to keep this PR up to date, would be great if it can be reviewed. Also I see @MartinHjelmare added this to Needs review, does that mean it will be reviewed? |
|
Be patient, thanks 👍 |
klaashoekstra94
force-pushed
the
onkyo-config-flow
branch
from
7232ed7 to
4d49cc3
Compare
klaashoekstra94
force-pushed
the
onkyo-config-flow
branch
from
be7be7e to
5dd9ab2
Compare
There was a problem hiding this comment.
A few things I have spotted.
Mainly my recommendation is that this PR is bigger than it needs to be.
Some of the changes could be done in a preliminary or follow-up PR.
See https://developers.home-assistant.io/docs/review-process#creating-the-perfect-pr
| @@ -0,0 +1,81 @@ | |||
| { | |||
There was a problem hiding this comment.
This file is no longer required - please remove it.
| """Platform for sensor integration.""" | ||
| from __future__ import annotations |
There was a problem hiding this comment.
I find it very confusing that you are adding a new platform as part of config-flow implementation.
I think it should have been kept for a follow-up PR.
| "loggers": ["eiscp"], | ||
| "requirements": ["onkyo-eiscp==1.2.7"] | ||
| "requirements": ["pyeiscp==0.0.7"] |
There was a problem hiding this comment.
Changing the library could be done as a preliminary PR, no?
| @@ -223,6 +223,7 @@ homeassistant.components.number.* | |||
| homeassistant.components.nut.* | |||
| homeassistant.components.oncue.* | |||
| homeassistant.components.onewire.* | |||
| homeassistant.components.onkyo.* | |||
There was a problem hiding this comment.
It's very confusing that strict-typing is implemented at the same time as a config-flow PR.
Can that not be a separate/preliminary PR?
| } | ||
|
|
||
|
|
||
| def get_mock_connection(host, name, identifier): |
There was a problem hiding this comment.
It is recommended that config_flow tests do not hit the integration internals.
You should look at adding mock_setup_entry in a local conftest.py file.
| def get_mock_connection(host, name, identifier): | |
| pytestmark = pytest.mark.usefixtures("mock_setup_entry") | |
| def get_mock_connection(host, name, identifier): |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
@epenet Sorry for the late response, work is busy at the moment so I didn't have the time yet to look into your review. I actually wanted to do the switch to the other library as a preliminary PR, but it was rejected because I was not allowed to add functionality to a component that didn't have a config flow yet, is that not the case anymore? Because that's why I also added the config flow in here, as it would be a lot of work to first make a config flow and then change the library to the async one, meaning I would have to do a big part of the config flow again. |
Please read https://developers.home-assistant.io/docs/review-process/: the smaller the PR, the better it is. Config-flow PRs are always very big and take a while to get reviewed, so whatever you can do as a preliminary PR or as a follow-up PR is beneficial. I think this PR has been in the queue for 18 months because it is too big... The main consideration about the library bump and the config flow is this: can the library be changed whilst keeping the existing configuration schema and not adding any new functionnality? |
|
The onkyo failures caused by the last Home Assistant core release (#47918) reminded me of this and that the onkyo integration really needs a refresh. I wonder if we can actually get this over the line - I've been using a version of this since 2021 when it first appeared and it is much more responsive than the official Home Assistant one. Or @klaashoekstra94 maybe we just cut our losses and move it to HACs? |
|
@foxy82 I would prefer having this directly in HA instead of using HACS, but I'm afraid that would mean that I have to split this PR into multiple smaller ones. Maybe I can use PR #48640 as a starting point but make sure that the YAML platform schema doesn't change at first. That way I could at least get the async library in first. Still that would mean a major overhaul to add the config flow in a second PR, making that one possibly almost just as big as this one. So I would still prefer if this one could get merged. |
|
I started to look at adding the config flow to the current Home Assistant version. It needs tests and the strings aren't working properly but it's an initial version that works. I based it hugely on this pr in the hope that if we need to merge it first rebasing this on to it shouldn't be too hard. Testing it has reminded me how unresponsive the current library used in Home Assistant is. It is so bad. I'll try to push my work so for later. |
|
Here is the start of the work to add config_flow to the existing component in case we can't get this PR merged. |
|
Let me know if you need any help testing this. |
|
I too would be interested in assisting in any way helpful here as I have a TX-NR636 I have for nearly 4 years used with the ancient/standard Onkyo integration. Before that/and overlapping with my HA work I did a bit of code tweaking using ToddGreenfeild's HomeBridge implementation. Unfortunately I'm not clear non what's required to integrate a pre-release component in HomeAssistant. Given the comments about migration above, I didn't mind recreating the integration/configuration and entities if still required to use given migration issues. Thanks for any pointers or guidance. While not totally clueless as a former Engineer but not very familiar with the internals of HomeAssistant though willing to learn/help. --T |
| ] | ||
|
|
||
|
|
||
| class OptionsFlowHandler(config_entries.OptionsFlow): |
There was a problem hiding this comment.
From what I can see having these at the level forces the user to use the same input sources across all of their receivers.
I don't think it allows the following to happen:
Receiver 1 - I want to import Game, TV and rename Game to "PlayStation"
Receiver 2 - I want to import Game, CD, TV, Aux and rename Game to "XBox"
There was a problem hiding this comment.
In the current implementation, you have to run the config flow for each receiver/connection. So you can set different inputs per receiver. If there are multiple zones they are added in the same config, but I don't think it's possible to set different mappings for a zone anyways
I would like to know as well, since it seems like this is going on for a long time. Also it would be nice to test if the issues are actually gone. Is it even possible to just alter the any part of HA manually? I guess this has to be done after every update of HA which would be fine to me if it works better like that. |
@taw123 and @dawiinci I've added it as a custom HACs repo. If you setup HACS then go HACS -> 3 Dots -> Custom Repositories. |
|
Seems to work, thanks. However, if the power is switched off, the receiver still says it is on. Is there a way to update the state manually? Switching it off after power is gone doesn't update. It doesn't even update after a restart which the original does. Unfortunately, after a restart the device is missing completely, if it is offline. Reverting back to original fixes this behavior. |
Raise these here: https://github.com/foxy82/hacs-onkyo-repository/issues - so the PR stays on track. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Hi there, Klaas thank you so much for your work implementing this modernised Onkyo integration, I’m really looking forward to it being merged. To assist with the review process, I tried to look over the code, but not being a developer, I fear I would do more harm than good. But, for about 6 months I had been testing this code as a manually created custom component and it was working great. Then around May of this year presumably after changes to Core, the integration would no longer load with the error: AttributeError: 'ConfigEntries' object has no attribute 'async_setup_platforms'2023-09-14 22:35:22.595 ERROR (MainThread) [homeassistant.config_entries] Error setting up entry Living Room Receiver for onkyo So it looks like it may need some changes to get going again, but (from an non-dev’s perspective) it looks really solid. I have also recorded some observations I had made when it was working, that I would be happy to share, if they would be of any value. Franck I just wanted to flick you a quick message to thank you very much for your previous reviews on this PR. In addition to that I really appreciate all of your work to Home Assistant such a great platform! It looks like Klaas has been hoping to get another review from you. When you get back from your vacation and are caught up with the chaos of returning, would you please consider casting your expertise over this PR? I really want this to comes across as a polite and genuine request. I know there are quite a few users that would really benefit from an update to the Onkyo integration and I didn’t want this PR to get closed off by the bot. Thanks for reading and your consideration! With respect and kindness, |
There was a problem hiding this comment.
While its already been said in #59518 (comment) Please break this up into smaller chunks. It should be at minimum 2 PRs (3 if the new library is mostly compatible with the old one). This PR won't be able to move forward with the size and scope of the review required as its simply too large to review effectively.
- Add the config flow
- Switch out the library
- Switch to local push
|
Because there hasn't been any activity on this PR for quite some time now, I've decided to close it for being stale. Feel free to re-open this PR when you are ready to pick up work on it again 👍 ../Frenck |
Breaking change
Rename service
media_player.onkyo_select_hdmi_outputtoonkyo.select_hdmi_outputProposed change
This is a follow-up of the closed PR #48640.
This PR adds a config flow for the Onkyo component. The component
now uses the pyeiscp library to make it local push instead of polling.
This PR also includes setting the sound mode on supported devices.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: