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 Asus Router integration #86084

Closed
wants to merge 14 commits into from
Closed

Conversation

Vaskivskyi
Copy link
Contributor

@Vaskivskyi Vaskivskyi commented Jan 17, 2023

Proposed change

Hello,

I would like to suggest implementing a new integration Asus Router (domain asus_router), providing the possibility to control Asus wireless devices (including routers, access points and mesh nodes) using the native Asus HTTP(S) API. This is the way the Web UI for the devices work as well as the official Android and iOS apps. As a result, if implemented, the integration potentially can support all the features supported by devices.

Currently, HA Core already has ASUSWRT integration for Asus devices, which allows control using SSH/Telnet protocols. But both these ways are not working well with the newer device models.

The proposed integration is based on the custom AsusRouter integration already been used by HA users for almost one year. Recently I posted a Feature request on the Community Forum with the idea to implement it into HA Core. Since it got some popularity and I did not find anything against the proposal, here is the first PR.

This PR contains the base for the integration to be useful for users with support of the Sensor platform and 2 sensors (CPU and RAM usage). If accepted, the plan for the next PRs is to add other platforms as well as more actual sensors for the Sensor platform. Also, currently, only tests for the configuration flow are provided (as required), but I would like to implement other tests as well.

I am looking forward to your comments and suggestions on the code to make it better and more suitable for HA Core.

Also, there is a bit more on why the integration should be separate from the currently existing ASUSWRT in the abovementioned feature request.

Sincerely,
Yevhenii

Update 1: Added link to the PR for documentation
Update 2: Changelog for the dependency bump between the initial and the last commit: Vaskivskyi/asusrouter@0.18.1...0.18.2

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:

Copy link
Contributor

@Lash-L Lash-L left a comment

Choose a reason for hiding this comment

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

I am definitely no expert - but I wanted to give reading someone elses PR a try, looking through I didn't see anything that stuck out to me and my limited experience. I just had one note!

As well, it may be helpful to link the changelog for the asusrouter package in your PR description

@Vaskivskyi
Copy link
Contributor Author

@Lash-L,

Thanks for your review and your comments! I have added the changelog for the dependency bump between the initial commit of this PR and the last one. Unfortunately, I cannot add the whole changelog for the library from the beginning of its existence, since it is just a couple of weeks before the 1-year anniversary. That would be a long log.

@RenierM26
Copy link
Contributor

Any reason why there should be two Asus integrations on Home Assistant? Why not open this pull request to update the existing integration?

@Vaskivskyi
Copy link
Contributor Author

Hey, @RenierM26. Thanks for your comments. I have made changes to the code to implement the suggestions.

As for why there should be 2 Asus integrations, I have described it in detail in the Feature request mentioned in the PR. Please, check it out

@vpathuis
Copy link
Contributor

vpathuis commented Feb 4, 2023

I'm sure I'm doing something wrong, but this just gives me two disabled entities, for my RT-AX88U. If I enables these, they are unavailable. Tried SSL and non-SSL.
image

The Asuswrt integration is working fine for me.

@Vaskivskyi
Copy link
Contributor Author

Hey, @vpathuis,

Yes, both the entities - CPU and RAM usage are created as disabled. But after the user enabled them, they should be working
image

Do you mind checking whether there are any errors/warnings in the log?

If not, maybe you can also check, whether debug log is showing everything as expected:

Example debug log
2023-02-05 08:17:13.612 DEBUG (MainThread) [asusrouter.connection] Login successful on port 8443: {'Model_Name': 'RT-AX88U', 'AiHOMEAPILevel': '22', 'Httpd_AiHome_Ver': '0'}
2023-02-05 08:17:13.612 DEBUG (MainThread) [asusrouter.asusrouter] Identifying the device
2023-02-05 08:17:14.908 DEBUG (MainThread) [asusrouter.asusrouter] Identity collected
2023-02-05 08:17:14.977 DEBUG (MainThread) [homeassistant.components.asus_router.bridge] Available `cpu` sensors: ['total_total', 'total_used', 'total_usage', '1_total', '1_used', '1_usage', '2_total', '2_used', '2_usage', '3_total', '3_used', '3_usage', '4_total', '4_used', '4_usage']
2023-02-05 08:17:14.977 DEBUG (MainThread) [homeassistant.components.asus_router.router] Coordinator initialized for `cpu`. Update interval: `0:00:30`
2023-02-05 08:17:14.977 DEBUG (MainThread) [homeassistant.components.asus_router.router] Finished fetching cpu data in 0.000 seconds (success: True)
2023-02-05 08:17:14.977 DEBUG (MainThread) [homeassistant.components.asus_router.router] Coordinator initialized for `ram`. Update interval: `0:00:30`
2023-02-05 08:17:14.977 DEBUG (MainThread) [homeassistant.components.asus_router.router] Finished fetching ram data in 0.000 seconds (success: True)
2023-02-05 08:17:14.979 DEBUG (MainThread) [homeassistant.components.asus_router.router] Finished fetching cpu data in 0.000 seconds (success: True)
2023-02-05 08:17:14.979 DEBUG (MainThread) [homeassistant.components.asus_router.router] Finished fetching ram data in 0.000 seconds (success: True)

Thanks in advance!

@vpathuis
Copy link
Contributor

vpathuis commented Feb 5, 2023

Yes, both the entities - CPU and RAM usage are created as disabled. But after the user enabled them, they should be working !

Both entities work now. Needed some time I guess, or perhaps a restart.
image

I certainly can appreciate the effort to make an integration based on the official API and I'm looking forward to a fully functional ASUS-integration. Code looks fine to me, but I'm no expert reviewer.

@Vaskivskyi
Copy link
Contributor Author

A question. After Translation files removed from Core repository, do I update PR immediately or do I wait for a proper review?

It's not obvious and I don't want to change something in case a review is already in progress accidentally

In any case, other than that, the PR is ready for review

@Vaskivskyi
Copy link
Contributor Author

The PR is updated to fulfil the recent HA changes (manifest sorting, removal of translations and EntityCategory move) and is again ready for review

@RenierM26
Copy link
Contributor

Hi @Vaskivskyi,

In general, the hass team avoids multiple integrations. (Difficult to maintain and support)

They will most likely ask you to rebase on the existing integration. I know it's effort but it's possible to make it work with both the HTTP API as well as SSL (for legacy devices - don't know if it's worth keeping for a hand full of old devices though.).

Just my 2c.

@vpathuis
Copy link
Contributor

Hi @Vaskivskyi,

In general, the hass team avoids multiple integrations. (Difficult to maintain and support)

They will most likely ask you to rebase on the existing integration. I know it's effort but it's possible to make it work with both the HTTP API as well as SSL (for legacy devices - don't know if it's worth keeping for a hand full of old devices though.).

Just my 2c.

Tbh I wouldn't agree with such an approach at all. The extra effort would be pure waste, especially since you end up cleaning up the old stuff anyway. I'd rather see a clean and fast path forward to the API version without any legacy to hold you back. I think the Z-wave JS approach was exacly that, and with good reason.

@Vaskivskyi
Copy link
Contributor Author

🏇🏻 Meanwhile, the feature request to add Asus Router to HA has received 100 votes on the HA Community forum

@Vaskivskyi
Copy link
Contributor Author

The latest commits:

  • Fixed ruff check
  • Bumped asusrouter library to 0.20.3 to fix dependency conflict for the aiohttp (which is now 3.8.4 in Core and was strictly fixed to 3.8.1 in the library). The full difference with the version from previous commits is here: Vaskivskyi/asusrouter@0.18.2...0.20.3. The library bump implies other changes which do not influence the Asus Router component functionality in this PR

@ollo69
Copy link
Contributor

ollo69 commented Apr 11, 2023

Let me just make some consideration on this PR and all the others that involve AsusWRT integration on which I'm code owner.
Original AsusWRT integration have some issue with new Asus firmware / router, partially fixed with some improvement in original AioAsusWrt library based on telnet / ssh. Because with new firmware Asus released new API based on HTTP, new library based on new API becomes available.

On May 2022 I created PR #71899 with the scope to extend the AsusWRT integration with the new HTTP protocol. This integration was ignored for many months by HA teams (I was thinking because it's to big), so I put that PR in draft and create the new PR #84152 with the scope to partially implement the previous one to give the ability to integrate new libraries in the original integration. In the meantime I also created a custom integration that is a copy of the first PR and that I'm using from months without any issue.

Honestly I don't really like the idea to have 2 integrations for the same device, this will probably create a lot of confusion to the users. And the old integration cannot be simply removed, because old routers that do not implement HTTP API still need that version.

But I really think that have new HTTP protocol available for new Asus routers is important, a well working device tracker is one of the most important part of Home Automation. So I hope that HA Teams will take one of this PRs in charge to better support one of te most used router.

Copy link

@Ndamix-Store Ndamix-Store left a comment

Choose a reason for hiding this comment

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

Asus Router integration

@ScottG489
Copy link
Contributor

First I'd like to say that I used the HACS version of this integration and I really liked it. The config flow is one of the most complex I've seen and it provided a lot of helpful information from my router about my network.

I can't speak for this core implementation, but I had some serious performance issues with the custom component. I ended up running the profiler service in HA and it was clear it was this integration that was causing the issues, and even more clear running the profiler after disabling this integration.

I haven't had time to write up a detailed report, but I'd be happy to send over the profiler information.

Otherwise, I'd be cautious of the performance impact of this integration as it's the only integration I've ever had to disable due to performance reasons.

@bieniu
Copy link
Member

bieniu commented Jun 25, 2023

Having two integrations for the same device brand in core is not a good idea. Users shouldn't have to decide which integration is better.
@ollo69 Do I understand correctly that AsusWrt after implementing http(s) will have more or less the same capabilities as Asus Router?
@Vaskivskyi Am I right that Asus Router integrations works with firmware 380 and newer?

@ollo69
Copy link
Contributor

ollo69 commented Jun 25, 2023

Do I understand correctly that AsusWrt after implementing http(s) will have more or less the same capabilities as Asus Router?

Yes, it will use both old SSH library (for compatibility with old router / firmware) and HTTP for new router / firmware. I just put PR #71899 in draft because I didn't receive any feedback for long time and I completed development as custom component here, but I can reopen and complete that PR based on custom component if you think that this approach is the right one.

@bieniu
Copy link
Member

bieniu commented Jun 25, 2023

I tested both custom integrations... and both work well and offer similar features. Asus Router creates more entities but the config flow is not user friendly. Hard to say which integration is better.

@ollo69 I think that splitting this change into smaller steps is much better approach. I will look at the first step #84152 this week.

@Vaskivskyi
Copy link
Contributor Author

Am I right that Asus Router integrations works with firmware 380 and newer?

Yes, that's correct. Actually it works with all the versions of Asus FW since the implementation of the native API (so, released in the last 12 or so years). This includes some of the 380.xx versions, even though not all of them

The same way I agree that having 2 integrations is not the best solution, I would like to point a problem of having a single integration with which user can have completely different set of sensors and controls depending on what they selected as a connection interface

@Vaskivskyi
Copy link
Contributor Author

First I'd like to say that I used the HACS version of this integration and I really liked it. The config flow is one of the most complex I've seen and it provided a lot of helpful information from my router about my network.

I can't speak for this core implementation, but I had some serious performance issues with the custom component. I ended up running the profiler service in HA and it was clear it was this integration that was causing the issues, and even more clear running the profiler after disabling this integration.

I haven't had time to write up a detailed report, but I'd be happy to send over the profiler information.

Otherwise, I'd be cautious of the performance impact of this integration as it's the only integration I've ever had to disable due to performance reasons.

@ScottG489, as I have asked you on the Community forum, it would be nice to get from you at least some information about the device you are using (model and FW version), as well as some other details about the device settings.

AsusWRT firmware has some problems which might provide performance issue on the integration side - e.g. with hanging the connection sessions instead of closing or reporting them.

In order to help with your issue (and any other issue with AsusRouter custom integration), I need to be able to diagnose and troubleshoot which exactly part is not working properly

@ollo69
Copy link
Contributor

ollo69 commented Jun 25, 2023

I think that splitting this change into smaller steps is much better approach. I will look at the first step #84152 this week.

Fine, I will rebase #84152 shortly.

I would like to point a problem of having a single integration with which user can have completely different set of sensors and controls depending on what they selected as a connection interface

That's the scope of PR #84152. We could work together on next PRs and base library to obtain the best results.

@bieniu
Copy link
Member

bieniu commented Jun 25, 2023

@Vaskivskyi I appreciate your hard work on this integration and I understand your frustrations if this PR is not merged, probably users who use it as a custom one won't be happy either.
Maybe you will consider working with @ollo69 on developing asuswrt integration, which is already in the core?

@Vaskivskyi
Copy link
Contributor Author

We definitely can try that. And then we'll see how it goes

@edenhaus
Copy link
Contributor

edenhaus commented Aug 9, 2023

It is possible to have two integrations for the same brand.
The user must then select which one he wants to add:

When adding a config entry for Ikea, the user will be asked to decide which integration to add:
grafik

@Vaskivskyi Is it possible to detect which communication protocol should be used?
If yes I would suggest implementing it into the current one and during the config flow decide automatically which protocol should be used, completely transparent for the user. Imo at the end, the user wants to get the data into HA and does not matter, which protocol is used or what do you think?

Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Nevertheless, if you want to add the https communication as own integration, please rebase and fix the merge requests.

After that, I can start with a review.

@home-assistant
Copy link

home-assistant bot commented Aug 9, 2023

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.

@home-assistant home-assistant bot marked this pull request as draft August 9, 2023 07:14
@ollo69
Copy link
Contributor

ollo69 commented Aug 9, 2023

@edenhaus

There is a already PR #95720 this. Only one solution should be followed, not a problem for me which one, but we already discussed about this.

@edenhaus
Copy link
Contributor

edenhaus commented Aug 9, 2023

@Vaskivskyi If I understood it correctly, only #95720 should be merged, which combines both protocols, correct?
If yes, please close this PR

@vpathuis
Copy link
Contributor

vpathuis commented Sep 9, 2023

I've been running this integration as a custom component since Feb and I only now noticed that the trackers don't seem to work as expected. I'm running AsusWRT in parallel and for AsusWRT, I get things like this (for an Android smartphone), which is correct:
image

But for AsusRouter it is just this:
image
There are no errors, as for as I can see.

Copy link

github-actions bot commented Dec 8, 2023

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.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Dec 8, 2023
@Vaskivskyi Vaskivskyi closed this Dec 14, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2023
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.

9 participants