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

Fix and improvment of Swiss Hydrological Data component #17166

Merged
merged 5 commits into from Nov 11, 2018

Conversation

@Bouni
Contributor

Bouni commented Oct 5, 2018

Description:

Fix for the not working Swiss Hydrological Data component.

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: swiss_hydrological_data
    station: 2143
    name: Rhein Rekingen
    measurements:
      - temperature
      - level
      - discharge
      - min_temperature
      - min_level
      - min_discharge
      - max_temperature
      - max_level
      - max_discharge
      - mean_temperature
      - mean_level
      - mean_discharge

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@homeassistant

This comment has been minimized.

homeassistant commented Oct 5, 2018

Hi @Bouni,

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!

@fabaff

This comment has been minimized.

Member

fabaff commented Oct 5, 2018

The issue with the proposed solution is that every instance is going to download around 800 kB of data per hour (approx. 30-40 kB per file) when all sensors are activate but only one value per file is of interest. Thus we are wasting a lot of resources.

When I wrote the integration the FOEN agreed on a automatic approach to get the XML file with the data from ftp.hydrodata.ch. Now the access requires credentials which made our integration non-functional.

I see three way to fix it:

  1. Got back to the scrape approach which was used in the first place and I provided to some users.
  2. Get the XML file and publish the content by ourself as RESTFul API
  3. Team-up with the guys from https://aare.guru. Last time we spoke they were interested in sharing the data because they have access to the XML file.
@Bouni

This comment has been minimized.

Contributor

Bouni commented Oct 8, 2018

@fabaff
Totally agree with you! To be honest I wasn't aware of the size of the CSV files!
I'll take a look into your suggestions and update the pull request as soon as I've found a viable solution!

@Bouni

This comment has been minimized.

Contributor

Bouni commented Oct 8, 2018

@fabaff
I've got in touch with FEON to ask them about the HTTP Interface the seem to offer.

  • They no longer provide the HTTP nor the anonymous interface
  • They switched to HTTPS and a per user account
  • They don't alow to use their server as backend and limit the request rate to once every 10 minutes.
  • They told me that they will change their XML format in 2020 but will be backwards compatible.
  • They normally give no access to private persons but just to specialist agencies.

Anyway, the signaled that if we could have our own backend (as you've suggested) we could get an account for that.

What exactly did you mean by "publish the content by ourself as RESTFul API" where would that service be hosted? Its obviously not a big deal writing the interpreter and REST API, but who is going to host it? Or are there already other components relying on such an HA provided API which could be implemented for this component as well?

@bachya bachya added the Hacktoberfest label Oct 9, 2018

@Bouni

This comment has been minimized.

Contributor

Bouni commented Oct 11, 2018

Just a quick update:
I've already done writing an API for this. I'll fetch the XML every 10 minutes and provide its data through a REST API. We then just need to fetch the necessary data.
I'm willing to host the API for now and hope that the load on my server does not get to intense.

@fabaff

This comment has been minimized.

Member

fabaff commented Oct 13, 2018

If you give me access to the API then I can create a simple client which we then can use for this sensor.

@Bouni

This comment has been minimized.

Contributor

Bouni commented Oct 15, 2018

@fabaff I still wait for the credentials, at the moment the API just respondes data from a single static file they gave me for reference. I'll ping you as soon as the API is live!

@Bouni

This comment has been minimized.

Contributor

Bouni commented Oct 20, 2018

@fabaff I think I'm done with my API but I still wait for the login data from FEON.
You can have a look at the API (with a static xml at the moment): https://swisshydroapi.bouni.de/

If you give me access to the API then I can create a simple client which we then can use for this sensor.

What exactly do you mean by "simple client" ?

@Bouni

This comment has been minimized.

Contributor

Bouni commented Oct 22, 2018

Just got an E-Mail, my account at FEON will be ready by 5th of November.
I've no idea what takes that long toset up another account, but thats how it works I assume 🙉

@fabaff

This comment has been minimized.

Member

fabaff commented Nov 5, 2018

What exactly do you mean by "simple client" ?

A module that is performing the requests and process the received data.

@Bouni

This comment has been minimized.

Contributor

Bouni commented Nov 6, 2018

I've already done that with this https://pypi.org/project/swisshydrodata/
Yesterday in the evening I got my access data from FEON for their data service and since then my API is up and running.
grafik

I would like to test the component for a few days to see if there are any problems and then update my pull request. Is there anything you want me to do / to test before I update the PR?

@fabaff

This comment has been minimized.

Member

fabaff commented Nov 6, 2018

I've already done that with this https://pypi.org/project/swisshydrodata/

Nice

attributes[ATTR_LOCATION] = self.data.measurings['location']
attributes[ATTR_UPDATE] = self.data.measurings['update_time']
attributes[ATTR_ATTRIBUTION] = CONF_ATTRIBUTION
return attributes

This comment has been minimized.

@fabaff

fabaff Nov 6, 2018

Member

This is a breaking change which needs an entry in the release notes.

attributes[ATTR_ATTRIBUTION] = CONF_ATTRIBUTION
return attributes
return {
ATTR_ATTRIBUTION: CONF_ATTRIBUTION

This comment has been minimized.

@fabaff

fabaff Nov 6, 2018

Member

How about adding the station details? Location, etc.

data["max_discharge"] = shd.get_max_discharge()
data["mean_temperature"] = shd.get_mean_temperature()
data["mean_level"] = shd.get_mean_level()
data["mean_discharge"] = shd.get_mean_discharge()

This comment has been minimized.

@fabaff

fabaff Nov 6, 2018

Member

I'm not so sure that this is the right approach resource-wise. There will be a dozen requests for every update to get the data instead of one that is sharing the data.

You want to change the sensor style (single sensors instead of attributes), thus you should only get the data, e. g., for mean_temperature that's needed and not everything that is available. BTW, there is a chance (home-assistant/architecture#100) that it needs to be updated/changed again soon.

MIN_TIME_BETWEEN_UPDATES = timedelta(minutes=30)
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_STATION): vol.Coerce(int),
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Required(CONF_MEASUREMENTS): vol.Any([

This comment has been minimized.

@fabaff

fabaff Nov 6, 2018

Member

That's a breaking change. Please check the documentation for the validation to check that it's a list.

_LOGGER.error("The URL is not accessible")
data = HydrologicalData(station)
response = requests.get(

This comment has been minimized.

@fabaff

fabaff Nov 6, 2018

Member

Should be tested with swisshydrodata as shd should be created here anyway and passed to HydrologicalData.

timeout=5
)
if response.status_code != 200:
_LOGGER.error("The given station does not exist: %s", station)
return False

This comment has been minimized.

@fabaff

fabaff Nov 6, 2018

Member

Can be just return.

if self.hydro_data.data is None:
self._state = STATE_UNKNOWN
else:
self._state = self.hydro_data.data["parameters"][self._measurement][self._value]

This comment has been minimized.

@houndci-bot

houndci-bot Nov 6, 2018

line too long (92 > 79 characters)

"ATTR_WATER_BODY": self.hydro_data.data["water-body-name"],
"ATTR_WATER_BODY_TYPE": self.hydro_data.data["water-body-type"],
"ATTR_STATION": self.hydro_data.data["name"],
"ATTR_UPDATE": self.hydro_data.data["parameters"][self._measurement]["datetime"],

This comment has been minimized.

@houndci-bot

houndci-bot Nov 6, 2018

line too long (93 > 79 characters)

@property
def unit_of_measurement(self):
"""Return the unit of measurement of this entity, if any."""
if self._state is not STATE_UNKNOWN:
return self._unit_of_measurement
return self.hydro_data.data["parameters"][self._measurement]["unit"]

This comment has been minimized.

@houndci-bot

houndci-bot Nov 6, 2018

line too long (80 > 79 characters)

This comment has been minimized.

@Bouni

Bouni Nov 6, 2018

Contributor

@fabaff Whats the pythonic way for breaking lines like this so that I meet the hounds requirements?

This comment has been minimized.

@fabaff

fabaff Nov 8, 2018

Member

There seems no other way than between the brackets here.

This comment has been minimized.

@Bouni

Bouni Nov 8, 2018

Contributor

I tried several ways and I always get lint errors, so I used this "trick":

data = self.hydro_data.data
if data is None:
self._state = STATE_UNKNOWN
else:
self._state = data["parameters"][self._condition][self._value]

vol.Required(CONF_MONITORED_CONDITIONS): vol.Schema({
"temperature": vol.All(cv.ensure_list, [vol.In(CONDITIONS)]),
"level": vol.All(cv.ensure_list, [vol.In(CONDITIONS)]),
"discharge": vol.All(cv.ensure_list, [vol.In(CONDITIONS)])

This comment has been minimized.

@Bouni

Bouni Nov 7, 2018

Contributor

@fabaff Is this what you meant by ensuring that the conditions are a list?

self.hydro_data.data["name"],
self._measurement,
self._station,
self._condition,
self._value)

This comment has been minimized.

@Bouni

Bouni Nov 7, 2018

Contributor

@fabaff What do you think about the way I name the sensors? Is that a good way?

This comment has been minimized.

@fabaff

fabaff Nov 8, 2018

Member

I would go with the water body for the name by default.

This comment has been minimized.

@Bouni

Bouni Nov 8, 2018

Contributor

The "problem" is that the waterbody name is something I get from the API and if its down for some reason I cannot know the waterbody name when setting up the entities. Thats why I went for the station ID. The user still can set a friendly_name, right!?

return {
"ATTR_WATER_BODY": self.hydro_data.data["water-body-name"],
"ATTR_WATER_BODY_TYPE": self.hydro_data.data["water-body-type"],
"ATTR_STATION": self.hydro_data.data["name"],
"ATTR_UPDATE": self.hydro_data.data["parameters"][self._measurement]["datetime"],
"ATTR_UPDATE":
self.hydro_data.data["parameters"][self._condition]["datetime"],
"ATTR_ATTRIBUTION": CONF_ATTRIBUTION
}

This comment has been minimized.

@Bouni

Bouni Nov 7, 2018

Contributor

I'm not sure whats the prefered way to set attributes and I'm not very happy with this, especially how they show up in HA (Lovelace UI)
grafik

This comment has been minimized.

@fabaff

fabaff Nov 8, 2018

Member

You removed the definition of all constants (line 33-42) and made the key strings in the dict. You need to add the missing one (like ATTR_WATER_BODY).

This comment has been minimized.

@Bouni

Bouni Nov 8, 2018

Contributor

Ok, got that, I'll fix it today if I find some spare time.

@Bouni Bouni requested review from Kane610, kellerza, rytilahti, scop, syssi and home-assistant/core as code owners Nov 7, 2018

@Bouni

This comment has been minimized.

Contributor

Bouni commented Nov 7, 2018

💩 I think I've messed someing up

@Bouni Bouni force-pushed the Bouni:fix-swiss-hydrological-data branch from af7fea9 to 0dd43bb Nov 7, 2018

Minor changes
- Simplify the sensor configuration (expose value as attributes rather than sensor)
- Make the setup fail if station is not available
- Add unique ID
- Prepare for config flow
@fabaff

fabaff approved these changes Nov 11, 2018

Looks good to me 🐦

@fabaff fabaff merged commit 372470f into home-assistant:dev Nov 11, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0008%) to 93.043%
Details

@wafflebot wafflebot bot removed the in progress label Nov 11, 2018

sqldiablo added a commit to sqldiablo/home-assistant that referenced this pull request Nov 12, 2018

Fix and improvment of Swiss Hydrological Data component (home-assista…
…nt#17166)

* Fix and improvment of Swiss Hydrological Data component

* changed component to get data from a REST API rather than from crawling the website

* fixed several issues and lint errors

* Fix and improvment of Swiss Hydrological Data component

* Minor changes

- Simplify the sensor configuration (expose value as attributes rather than sensor)
- Make the setup fail if station is not available
- Add unique ID
- Prepare for config flow

sqldiablo added a commit to sqldiablo/home-assistant that referenced this pull request Nov 12, 2018

Fix and improvment of Swiss Hydrological Data component (home-assista…
…nt#17166)

* Fix and improvment of Swiss Hydrological Data component

* changed component to get data from a REST API rather than from crawling the website

* fixed several issues and lint errors

* Fix and improvment of Swiss Hydrological Data component

* Minor changes

- Simplify the sensor configuration (expose value as attributes rather than sensor)
- Make the setup fail if station is not available
- Add unique ID
- Prepare for config flow

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Fix and improvment of Swiss Hydrological Data component (home-assista…
…nt#17166)

* Fix and improvment of Swiss Hydrological Data component

* changed component to get data from a REST API rather than from crawling the website

* fixed several issues and lint errors

* Fix and improvment of Swiss Hydrological Data component

* Minor changes

- Simplify the sensor configuration (expose value as attributes rather than sensor)
- Make the setup fail if station is not available
- Add unique ID
- Prepare for config flow

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment