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

Adds integration for Plaato Airlock #23727

Merged
merged 16 commits into from
Jun 18, 2019
Merged

Conversation

JohNan
Copy link
Contributor

@JohNan JohNan commented May 6, 2019

Description:

Adds a new integration for the Plaato Airlock. Plaato Airlock is a tool for beer brewers that wants a unique insight in the fermentation process. It will create sensors for the data that is pushed to a webhook from the airlock.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#9398

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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 (example).
  • New dependencies have been added to requirements in the manifest (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@homeassistant
Copy link
Contributor

Hi @JohNan,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@awarecan
Copy link
Contributor

awarecan commented May 8, 2019

Please remove plaato/.translations/sv.json from the PR. The only language resource json we accepted is en.json. The translation should go through our L10N process, see dev doc for details. The new resource will be upload to Lokalise after the PR merged.

@JohNan JohNan force-pushed the plaato-airlock branch 2 times, most recently from d115cca to 135945e Compare May 15, 2019 07:47
@JohNan
Copy link
Contributor Author

JohNan commented May 15, 2019

Rebased on latest dev and fixed things for config flow.

@JohNan
Copy link
Contributor Author

JohNan commented May 26, 2019

Any chance someone could look at this PR?

.coveragerc Show resolved Hide resolved
async_add_entities(entities, True)
else:
for entity in devices[device_id]:
entity.async_schedule_update_ha_state()
Copy link
Member

Choose a reason for hiding this comment

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

Pass True so that it will call the async_update method and fetch the latest data.

Suggested change
entity.async_schedule_update_ha_state()
entity.async_schedule_update_ha_state(True)

Copy link
Member

Choose a reason for hiding this comment

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

Alternative, since the data is just fetched from a dictionary stored in memory during update, you could have all properties fetch the data directly from that dictionary instead of storing it on the entity instance. In that case you can skip True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you are suggesting would mean that I should move things from async_update to the state property? Something like this?

    @property
    def state(self):
        """Return the state of the sensor."""
         sensors = self.get_sensors()
        if sensors is False:
            _LOGGER.debug("Device with name %s has no sensors.", self.name)
            return 0

        if self._type == ATTR_ABV:
            return round(sensors.get(self._type), 2)
        elif self._type == ATTR_TEMP:
              return round(sensors.get(self._type), 1)
        elif self._type == ATTR_CO2_VOLUME:
            return round(sensors.get(self._type), 2)
        else:
            return sensors.get(self._type)

Copy link
Member

@balloob balloob Jun 14, 2019

Choose a reason for hiding this comment

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

yeah. Just know that you cannot do any I/O inside properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

Choose a reason for hiding this comment

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

It's not safe to call entity.async_schedule_update_ha_state directly in non polling platforms.

For polling platforms we can assume that the interval between polls is long enough so that all new entities will have been added to home assistant before the next update, via poll, is done and calls this method.

For non polling platforms we can't assume this. The next update for new entities might come immediately after the first one, before they have been added to home assistant. Then this call will error.

The safe approach in this case is to use our dispatch helper and send a signal to the entity to have it call async_schedule_update_ha_state from inside itself. The signal should be connected in async_added_to_hass which will guard from the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank @MartinHjelmare I will look into this. You don't happen to have any examples where this behavior is used? So I can learn how to do it.

Copy link
Member

@MartinHjelmare MartinHjelmare Jun 18, 2019

Choose a reason for hiding this comment

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

Something like this:
https://github.com/home-assistant/home-assistant/blob/f382be4c15ef32f557ba6d40bb01c084c5cbc692/homeassistant/components/aftership/sensor.py#L145-L153

In our case we might not even need an extra method that does the update call. We can connect async_schedule_update_ha_state directly. So replace self.force_update with self.async_schedule_update_ha_state.

Send the signal to update like so:
https://github.com/home-assistant/home-assistant/blob/f382be4c15ef32f557ba6d40bb01c084c5cbc692/homeassistant/components/aftership/sensor.py#L85

Copy link
Contributor Author

@JohNan JohNan Jun 19, 2019

Choose a reason for hiding this comment

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

@MartinHjelmare Thank you. I'll open up a new PR for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartinHjelmare PR with the changes you requested: #24627

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Awesome!

@balloob
Copy link
Member

balloob commented Jun 18, 2019

Tests failing because dev was temporarily broken. Merging it ! 🎉 🍺

@balloob balloob merged commit 266b3bc into home-assistant:dev Jun 18, 2019
@JohNan
Copy link
Contributor Author

JohNan commented Jun 18, 2019

@balloob Thanks for that fix, review and finally merge! 🍺

@balloob balloob mentioned this pull request Jun 26, 2019
alandtse pushed a commit to alandtse/home-assistant that referenced this pull request Oct 12, 2019
* Adds integration for Plaato Airlock

* Updates codeowners and coveragerc

* Fixes lint errors

* Fixers lint check error

* Removed sv translation file

* Adds en translation file

* Sets config flow to true in manifest

* Moves config flow and domain to seperate files

* Fixes lint errors

* Runs hassfest to regenerate config_flows.py

* Adds should poll property and fixes for loop

* Only log a warning when webhook data was broken

* Fixes static test failure

* Moves state update from async_update to state prop

* Unsubscribes the dispatch signal listener

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

Successfully merging this pull request may close these issues.

5 participants