8000 Evaluate AirVisual interval on reboot by jugla · Pull Request #48392 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Evaluate AirVisual interval on reboot #48392

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Mar 30, 2021
Merged

Evaluate AirVisual interval on reboot #48392

merged 19 commits into from
Mar 30, 2021

Conversation

jugla
Copy link
Contributor
@jugla jugla commented Mar 27, 2021 •
edited
Loading

Proposed change

Upon HA initialisation, all AirVisual coordinators are initialized in asynchroneous way : they can be initialised at same time and so don't see each other. The server request interval is then not correct.

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@jugla jugla changed the title Reevaluate interval on reboot (and periodically) AirVisual : reevaluate interval on reboot (and periodically) Mar 27, 2021
@MartinHjelmare MartinHjelmare changed the title AirVisual : reevaluate interval on reboot (and periodically) Reevaluate AirVisual interval on reboot (and periodically) Mar 27, 2021
bachya
bachya requested changes Mar 27, 2021
View reviewed changes
@jugla jugla requested a review from bachya March 28, 2021 23:02
@bachya
Copy link
Contributor
bachya commented Mar 29, 2021

I'm still a bit unclear on why this is necessary. We re-level the API call rate when we add a new instance:

core/homeassistant/components/airvisual/__init__.py

Lines 246 to 248 in 03b7b13

async_sync_geo_coordinator_update_intervals(
hass, config_entry.data[CONF_API_KEY]
)

...or remove an instance:

core/homeassistant/components/airvisual/__init__.py

Lines 352 to 354 in 03b7b13

async_sync_geo_coordinator_update_intervals(
hass, config_entry.data[CONF_API_KEY]
)

If we haven't added or removed an instance, why would we need to consider re-leveling with every API call?

@jugla
Copy link
Contributor Author
jugla commented Mar 29, 2021 •
edited
Loading

In fact, when HA restarts, it reloads all instance in an asynchrenous way. This triggers the issue.
In order the see the problem here after a scenario :

  • Imagine 2 cities, let's say A and B.
  • At first time, no city are declared
  • then user declare one city A ==> Airvisual create the instance , no other city exist (interval between 2 requests is then not re-evaluated)
  • then user declare the other city B ==> Airvisual create the instance , see that an other instance exists, and updates the interval between 2 requests [The interval function sees only A and not A+B ==> this is false]
  • a trigger force HA to be restarted (at a software update for instance)
  • both A & B are already declared in HA
  • HA restart at the same time the instance A & B
  • at same time (i.e. in parallel), the instance A & B are taken into account by Airvisual
  • A & B perform the same action at same time
  • At this moment Airvisual has not stored A & B, and so when A or B asks if there is another instance, the gloval variable is empty
  • so no need to re-evaluate the interval between 2 requests
  • Then A&B instanciation are started

==> in this scenario A&B are still with the default value 5minutes wheras is shall be higher

@jugla
Copy link
Contributor Author
jugla commented Mar 29, 2021

That's why I put a re-evaluation on instanciation update.
Perhaps, the interval computation can be added AFTER storage of Airvisual instance

@jugla
Copy link
Contributor Author
jugla commented Mar 29, 2021

Great it works by moving the time of interval computing during the instanciation.
I will look for the remove (I think there is an error)

@jugla jugla changed the title Reevaluate AirVisual interval on reboot (and periodically) Evaluate AirVisual interval on reboot Mar 29, 2021
@jugla
Copy link
Contributor Author
jugla commented Mar 29, 2021

The pull was performed without pylint issue, this time.
I think the last pull shows why there is (was ;-) ) an issue at startup

bachya
bachya reviewed Mar 30, 2021
View reviewed changes
homeassistant/components/airvisual/__init__.py
@@ -343,10 +345,7 @@ async def async_unload_entry(hass, config_entry):
remove_listener = hass.data[DOMAIN][DATA_LISTENER].pop(config_entry.entry_id)
remove_listener()

if (
Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, great catch! If someone added a name-based entry, this would not have happened previously, which is definitely a bug. 👍🏻

bachya
bachya approved these changes Mar 30, 2021
View reviewed changes
Copy link
Contributor
@bachya bachya left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@bachya bachya merged commit 76d9c62 into home-assistant:dev Mar 30, 2021
@jugla
Copy link
Contributor Author
jugla commented Mar 30, 2021

Your welcome! Thank you !

@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bachya bachya bachya approved these changes

Assignees

@bachya bachya

Labels
bugfix cla-signed integration: airvisual small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

airvisual assessment of interval between 2 requests : missing trigger
3 participants
@jugla @bachya @homeassistant
0