Implement OpenSky library #92814

joostlek · 2023-05-08T16:35:00Z

Proposed change

So in #92593 we discovered that the old way of implementing was bad. Since the /states/all request we did, fetched all planes all over the world (Last time I ran it on my pc I received 9000+ records). When we had the list, we would calculate for each plane if their distance to the set point was below the set radius. If so, it would fire events and add it to the state of the entity.

Currently as a workaround, the max scan interval is set to once every 15 minutes, to allow our requests (which cost 4 credits, from the 400 you get as anonymous user) to actually work. Since before it ran every 12 seconds or so, and shitting it's pants in the logs since it kept receiving 429 with no valid JSON.

There are 2 other types of users within OpenSky. Users that are authenticated, they get 4000 credits a day, and users who are actively helping the network, they get 8000 per day and they can request their own states from the sensor as much as they'd like (until general rate limiting kicks in ofc).

In the comment of that PR, we also see that people want to add a way to authenticate themselves for OpenSky so they can benefit from more frequent updates. But before we are able to do that, we need to improve our codebase since it's still a yaml integration and all the logic is currently in the integration. Also, since the radius functionality is only a thing that home assistant has, it's heavy in the current implementation, so we need to change this functionality to be less heavy or change the way we configure.

Last weekend I built https://github.com/joostlek/python-opensky, an async library for OpenSky. OpenSky also has their own python library, but we like async, async is good. (And I wanted to get into library development).

So to give a rough sketch of a future for OpenSky:

Decide how we want to configure OpenSky in the future. (Do we want to keep the functionality of an anonymous account? Do we want to add support for active users? How are we going to fix the radius?). This might result in a breaking change.
Add config flow. Since we might have a breaking change, doing it at this point would be the best. Instead of importing the current config, we could have people set the integration to the way they like.
Add data coordinator. AFAIK, we can only dynamically set the update interval in a coordinator. If someone logs in, we don't want to keep polling every 15 minutes, we want to be able to put that to every 60 / 4000 / 24 ~ 30 seconds. (Also another note, are we able to detect other entry's from a single one. If someone has 2 locations they want to track, we want to decrease the polling for both locations.)

The distance method is taken from https://gist.github.com/jtornero/9f3ddabc6a89f8292bb2. There are uncertainties about the license, but I am not an expert on both licensing or maths. So if someone has a better plan, lmk!

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

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

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
I have followed the perfect PR recommendations
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:

Documentation added/updated for www.home-assistant.io

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:

I have reviewed two other open pull requests in this repository.

joostlek · 2023-05-08T16:36:14Z

Also pinging @OzGav, since he replied to the other PR.

joostlek · 2023-05-08T16:38:39Z

Oh and to explain the change in functionality, currently I use the radius to calculate the north, east, south and west edges of the circle, and making it a box. That means that all the diagonal directions are now longer since it's a cube, not a radius

epenet

Please move get_bounding_box and calculate_point to the library.
Please also add a link to the library changelog in the PR description.

epenet · 2023-05-08T19:07:45Z

homeassistant/components/opensky/sensor.py

@@ -70,6 +70,124 @@
 )


+def calculate_point(


Please move calculate_point to the library

epenet · 2023-05-08T19:07:53Z

homeassistant/components/opensky/sensor.py

+    return phi2, lembda2
+
+
+def get_bounding_box(latitude: float, longitude: float, radius: float) -> BoundingBox:


Please move get_bounding_box to the library

joostlek · 2023-05-08T21:24:05Z

@epenet do you agree on the chosen solution direction? Before I move it to the library and we end up not using it.

OzGav · 2023-05-08T22:09:32Z

Just note I have a fork with the bounding box solution and ability to login to increase the api limit

epenet · 2023-05-09T06:08:45Z

@epenet do you agree on the chosen solution direction? Before I move it to the library and we end up not using it.

I am not commenting on library code. I am only commenting on HA code.

joostlek · 2023-05-09T10:14:36Z

@epenet do you agree on the chosen solution direction? Before I move it to the library and we end up not using it.

I am not commenting on library code. I am only commenting on HA code.

Of course of course, but even when moving this code to the library, the HA integration would work different, so I first want to know if that change is a good change before actually implementing it in the library.

joostlek · 2023-05-09T10:15:44Z

Just note I have a fork with the bounding box solution and ability to login to increase the api limit

You mean #91162? I only see the authentication part, but only as a breaking change.

OzGav · 2023-05-09T11:46:11Z

This is my current component that I am using as a custom. https://github.com/OzGav/opensky

It is combination of other PRs that have been rejected as well as the authentication addition that I wrote plus some other guards to try and trap errors due to website unavailability.

joostlek · 2023-05-09T12:16:32Z

This is my current component that I am using as a custom. https://github.com/OzGav/opensky

It is combination of other PRs that have been rejected as well as the authentication addition that I wrote plus some other guards to try and trap errors due to website unavailability.

Right. I wrote https://github.com/joostlek/python-opensky which now also has auth support and is async. Let's try to at least get the integration up to standards and then we can take a look at how we can improve it

OzGav · 2023-05-09T13:28:31Z

Sounds good. There are a bunch of requirements (Architecture Decision Records) that apply to new or updated components that I don't have time to address hence my custom component. The community will very much appreciate it if you can bring this component up to speed to keep it within core.

emontnemery · 2023-05-23T17:33:07Z

@epenet do you agree on the chosen solution direction? Before I move it to the library and we end up not using it.

I am not commenting on library code. I am only commenting on HA code.

Of course of course, but even when moving this code to the library, the HA integration would work different, so I first want to know if that change is a good change before actually implementing it in the library.

First of all, I agree with @epenet that the calculations should be in the library 👍

To answer the question: I don't think there are any concerns with the changes in this PR or with the development path you outline in the PR description, it makes sense.

emontnemery · 2023-05-23T17:44:59Z

Please mark the PR as ready for review when the requested changes have been implemented 👍

joostlek · 2023-05-24T07:20:17Z

@emontnemery Did the changes. Is it preferred to add tests in this PR or follow ups?

homeassistant/components/opensky/sensor.py

I don't have any more concerns about this PR.

emontnemery · 2023-05-24T10:37:39Z

@emontnemery Did the changes. Is it preferred to add tests in this PR or follow ups?

Several small PRs are preferred over fewer large PRs; it's fine to add tests in a follow-up since opensky does not yet have tests 👍

emontnemery

LGTM, thanks @joostlek 👍

…icator * 'dev' of https://github.com/home-assistant/core: (400 commits) Fix todoist state updates (home-assistant#91915) Bump pydeconz to v112 (home-assistant#91924) Fix directv attribute media_position_updated_at (home-assistant#92383) Add a DataUpdateCoordinator to Hydrawise (home-assistant#93223) Add UDP listener in Lightwave (home-assistant#85385) Prevent SensorEntity and RestoreEntity inheritance (home-assistant#88971) Implement OpenSky library (home-assistant#92814) Bump pygti and change the request for the new api version (home-assistant#92283) Fully Kiosk: Truncate long URLs (home-assistant#92347) Enable strict type checks of cloud (home-assistant#92576) Remove legacy translations from electrasmart (home-assistant#93446) Update aioairzone-cloud to v0.1.3 (home-assistant#93443) Add backup location and mount failed repair (home-assistant#93126) Remove unused zwave discovery logic (home-assistant#93436) Bump PySwitchbee to 1.8.0 (home-assistant#92348) remove template deprecated function `device_trigger.py` (home-assistant#93419) Fix reference string in data disk repair (home-assistant#93220) Use SnapshotAssertion in Renault tests (part 2) (home-assistant#92395) Add ability to unload demo integration (home-assistant#92515) Move Twitch constants to separate file (home-assistant#92605) ...

Implement OpenSky library

e60c72d

home-assistant bot added cla-signed dependency integration: opensky Quality Scale: No score labels May 8, 2023

joostlek marked this pull request as draft May 8, 2023 16:38

epenet requested changes May 8, 2023

View reviewed changes

joostlek marked this pull request as ready for review May 20, 2023 11:58

Merge branch 'dev' into opensky_deps

7e7a7d6

emontnemery marked this pull request as draft May 23, 2023 17:44

Move function to library

6234ca6

epenet previously requested changes May 24, 2023

View reviewed changes

homeassistant/components/opensky/sensor.py Outdated Show resolved Hide resolved

homeassistant/components/opensky/sensor.py Outdated Show resolved Hide resolved

homeassistant/components/opensky/sensor.py Outdated Show resolved Hide resolved

joostlek added 2 commits May 24, 2023 11:46

Fix feedback

a56c24c

Merge branch 'dev' into opensky_deps

5dd8280

joostlek marked this pull request as ready for review May 24, 2023 09:58

emontnemery approved these changes May 24, 2023

View reviewed changes

emontnemery merged commit db33aad into home-assistant:dev May 24, 2023
48 checks passed

joostlek deleted the opensky_deps branch May 24, 2023 10:49

joostlek mentioned this pull request May 24, 2023

opensky with Update Error #45453

Closed

github-actions bot locked and limited conversation to collaborators May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement OpenSky library #92814

Implement OpenSky library #92814

joostlek commented May 8, 2023

joostlek commented May 8, 2023

joostlek commented May 8, 2023

epenet left a comment

epenet May 8, 2023

epenet May 8, 2023

joostlek commented May 8, 2023 •

edited

OzGav commented May 8, 2023

epenet commented May 9, 2023

joostlek commented May 9, 2023 •

edited

joostlek commented May 9, 2023

OzGav commented May 9, 2023

joostlek commented May 9, 2023

OzGav commented May 9, 2023

emontnemery commented May 23, 2023 •

edited

emontnemery commented May 23, 2023

joostlek commented May 24, 2023

emontnemery commented May 24, 2023

emontnemery left a comment

		return phi2, lembda2


		def get_bounding_box(latitude: float, longitude: float, radius: float) -> BoundingBox:

Implement OpenSky library #92814

Implement OpenSky library #92814

Conversation

joostlek commented May 8, 2023

Proposed change

Type of change

Additional information

Checklist

joostlek commented May 8, 2023

joostlek commented May 8, 2023

epenet left a comment

Choose a reason for hiding this comment

epenet May 8, 2023

Choose a reason for hiding this comment

epenet May 8, 2023

Choose a reason for hiding this comment

joostlek commented May 8, 2023 • edited

OzGav commented May 8, 2023

epenet commented May 9, 2023

joostlek commented May 9, 2023 • edited

joostlek commented May 9, 2023

OzGav commented May 9, 2023

joostlek commented May 9, 2023

OzGav commented May 9, 2023

emontnemery commented May 23, 2023 • edited

emontnemery commented May 23, 2023

joostlek commented May 24, 2023

emontnemery commented May 24, 2023

emontnemery left a comment

Choose a reason for hiding this comment

joostlek commented May 8, 2023 •

edited

joostlek commented May 9, 2023 •

edited

emontnemery commented May 23, 2023 •

edited