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 Ipv4 connection type sensor #134

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

karlsvenssonn
Copy link
Contributor

Add sensor for ipv4 connection type.

Good to check if 4G router is connected to WAN or 4G

I'm in no means an expert, this is my first change to a custom_components. It seems to work but I might have done something incorrect.

Also includes swedish translation

@AlexandrErohin
Copy link
Owner

AlexandrErohin commented Jan 9, 2025

@karlsvenssonn Hi. Thank you for your PR. Unfortunately, it brakes compatibilities with routers that doesnt have ipv4 status

Could you add swedish translation in a different PR please? In this PR we will think what to do with ipv4 status

@karlsvenssonn
Copy link
Contributor Author

Yes, I can fix that. I think I have a solution to exclude it for routers that don't have ipv4_status as well.

Give me some days to come up with a proposal for solution.

Will create a new PR for Swedish meanwhile.

@karlsvenssonn
Copy link
Contributor Author

Latest changes should make it compatible with routers without ipv4_status.
Home Assistant sensor "IPv4 Connection Type" will be unavailable if router is incompatible with ipv4_status.

One thing to consider is if it's always ipoe_1_d when the router is connected with WAN port, might be different for different models?

Might be better to change to ipv4_status: "4G" if ipv4_status.wan_ipv4_conntype == "4G" else "WAN"
But need to find the exact connType when connected via 4G/LTE

@AlexandrErohin
Copy link
Owner

@karlsvenssonn Is there a way to make new sensor disabled for those who doesn't have this option? It would be better then set Unavailable

@karlsvenssonn
Copy link
Contributor Author

karlsvenssonn commented Jan 10, 2025

Yes, the sensor is now disabled by default.
EDIT: WAIT, I was to quick, let me change

@karlsvenssonn
Copy link
Contributor Author

@AlexandrErohin Take a look at latest changes, the sensor should Only be enabled if get_ipv4_status exists.

@AlexandrErohin
Copy link
Owner

@karlsvenssonn Have you test these changes?
P.S Could you fix flake8 errors please? https://github.com/AlexandrErohin/home-assistant-tplink-router/actions/runs/12710127864/job/35430562654?pr=134

@karlsvenssonn
Copy link
Contributor Author

@AlexandrErohin I have tested with my MR600 router, but when simulating a router without ipv4 I got some problems.
I will but this PR into draft and ping you when ready.
Thanks!

@karlsvenssonn karlsvenssonn marked this pull request as draft January 10, 2025 13:56
@karlsvenssonn
Copy link
Contributor Author

@AlexandrErohin I'm now happy with the functionality. If the router client supports get_ipv4_status the sensor will be created. If the router does not support it it won't be created. This keeps the integration clean without having a bunch on disabled entities.
I have tested the functionality like this:

I have removed the get_ipv4_status method from tplinkrouterc6u package locally to simulate a router that does not support it. -> Sensor not created.

@karlsvenssonn karlsvenssonn marked this pull request as ready for review January 13, 2025 13:43
@karlsvenssonn
Copy link
Contributor Author

@AlexandrErohin Good suggestions, i have separated status sensors and ipv4 sensors. I decided to do a similar setup for ipv4_sensors, this makes it easy to add additional sensors from ipv4_status if wanted.

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

Successfully merging this pull request may close these issues.

2 participants