Fix and improvment of Swiss Hydrological Data component by Bouni · Pull Request #17166 · home-assistant/core

Bouni · 2018-10-05T14:53:45Z

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:

Documentation added/updated in home-assistant.io

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 · 2018-10-05T14:53:46Z

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 · 2018-10-05T18:43:56Z

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:

Got back to the scrape approach which was used in the first place and I provided to some users.
Get the XML file and publish the content by ourself as RESTFul API
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 · 2018-10-08T05:50:17Z

@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 · 2018-10-08T09:12:41Z

@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?

Bouni · 2018-10-11T13:26:26Z

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 · 2018-10-13T08:36:22Z

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

Bouni · 2018-10-15T06:02:26Z

@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 · 2018-10-20T18:36:46Z

@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 · 2018-10-22T14:48:49Z

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 · 2018-11-05T20:43:33Z

What exactly do you mean by "simple client" ?

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

Bouni · 2018-11-06T07:15:24Z

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.

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 · 2018-11-06T09:38:01Z

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

Nice

fabaff · 2018-11-06T10:03:23Z

homeassistant/components/sensor/swiss_hydrological_data.py

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


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

fabaff · 2018-11-06T10:07:32Z

homeassistant/components/sensor/swiss_hydrological_data.py

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


How about adding the station details? Location, etc.

fabaff · 2018-11-06T10:12:06Z

homeassistant/components/sensor/swiss_hydrological_data.py

+        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()


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.

fabaff · 2018-11-06T10:15:11Z

homeassistant/components/sensor/swiss_hydrological_data.py

 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([


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

fabaff · 2018-11-06T10:17:09Z

homeassistant/components/sensor/swiss_hydrological_data.py

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


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

fabaff · 2018-11-06T10:17:38Z

homeassistant/components/sensor/swiss_hydrological_data.py

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


Can be just return.

houndci-bot · 2018-11-06T10:25:40Z

homeassistant/components/sensor/swiss_hydrological_data.py

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


line too long (92 > 79 characters)

houndci-bot · 2018-11-06T10:25:40Z

homeassistant/components/sensor/swiss_hydrological_data.py

+            "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"],


line too long (93 > 79 characters)

houndci-bot · 2018-11-06T10:25:41Z

homeassistant/components/sensor/swiss_hydrological_data.py

        """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"]


line too long (80 > 79 characters)

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

There seems no other way than between the brackets here.

I tried several ways and I always get lint errors, so I used this "trick":
https://github.com/home-assistant/home-assistant/blob/0dd43bb424ef846f18cc33d8dd28873fcf38f452/homeassistant/components/sensor/swiss_hydrological_data.py#L138-L142

Bouni · 2018-11-07T07:51:38Z

homeassistant/components/sensor/swiss_hydrological_data.py

+    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)])


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

Bouni · 2018-11-07T07:52:21Z

homeassistant/components/sensor/swiss_hydrological_data.py

-            self._measurement,
+            self._station,
+            self._condition,
            self._value)


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

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

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!?

Bouni · 2018-11-07T07:57:02Z

homeassistant/components/sensor/swiss_hydrological_data.py

+            "ATTR_UPDATE":
+            self.hydro_data.data["parameters"][self._condition]["datetime"],
            "ATTR_ATTRIBUTION": CONF_ATTRIBUTION
        }


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)

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).

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

Bouni · 2018-11-07T13:56:11Z

💩 I think I've messed someing up

…ng the website

- 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

Looks good to me 🐦

swissuser31415 · 2021-05-19T07:11:04Z

platform: swiss_hydrological_data
station: 6600
monitored_conditions:
- temperature

Any ideas? The 6600 station does exist according to the website and delivers the temperature data.

frenck · 2021-05-19T07:14:14Z

@swissuser31415 this is a pull request from 2018, please don't bump old/closed/merged/handled PRs.

If you need support, please see our website on where to go:

https://www.home-assistant.io/help

Thanks! 👍

homeassistant added platform: sensor.swiss_hydrological_data cla-needed labels Oct 5, 2018

ghost added the in progress label Oct 5, 2018

homeassistant added cla-signed and removed cla-needed labels Oct 5, 2018

Bouni mentioned this pull request Oct 5, 2018

Update Swiss Hydrological Data documentation home-assistant/home-assistant.io#6535

Merged

2 tasks

bachya added the Hacktoberfest label Oct 9, 2018

fabaff reviewed Nov 6, 2018

View reviewed changes

houndci-bot reviewed Nov 6, 2018

View reviewed changes

Bouni commented Nov 7, 2018

View reviewed changes

Bouni requested review from Danielhiversen, amelchio, bachya, cdce8p, dgomes and emlove as code owners November 7, 2018 13:53

Bouni requested review from a team, Kane610, kellerza, rytilahti, scop and syssi as code owners November 7, 2018 13:53

Bouni requested a review from a team November 7, 2018 13:53

Bouni requested a review from a team as a code owner November 7, 2018 13:53

Bouni and others added 5 commits November 7, 2018 15:17

Fix and improvment of Swiss Hydrological Data component

5a22a88

changed component to get data from a REST API rather than from crawli…

0f41596

…ng the website

fixed several issues and lint errors

cf35b41

Fix and improvment of Swiss Hydrological Data component

0dd43bb

Minor changes

46871ef

- 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

ghost assigned fabaff Nov 11, 2018

fabaff approved these changes Nov 11, 2018

View reviewed changes

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

ghost removed the in progress label Nov 11, 2018

balloob mentioned this pull request Nov 29, 2018

0.83 #18776

Merged

ghost added integration: swiss_hydrological_data and removed platform: sensor.swiss_hydrological_data labels Mar 21, 2019

home-assistant locked and limited conversation to collaborators May 19, 2021

Uh oh!

Conversation

Bouni commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

homeassistant commented Oct 5, 2018

Uh oh!

fabaff commented Oct 5, 2018

Uh oh!

Bouni commented Oct 8, 2018

Uh oh!

Bouni commented Oct 8, 2018

Uh oh!

Bouni commented Oct 11, 2018

Uh oh!

fabaff commented Oct 13, 2018

Uh oh!

Bouni commented Oct 15, 2018

Uh oh!

Bouni commented Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bouni commented Oct 22, 2018

Uh oh!

fabaff commented Nov 5, 2018

Uh oh!

Bouni commented Nov 6, 2018

Uh oh!

fabaff commented Nov 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabaff Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bouni commented Nov 7, 2018

Uh oh!

fabaff left a comment

Choose a reason for hiding this comment

Uh oh!

swissuser31415 commented May 19, 2021

Uh oh!

Bouni commented Oct 5, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

Bouni commented Oct 20, 2018 •

edited

Loading

fabaff Nov 8, 2018 •

edited

Loading