aex351 / home-assistant-neerslag-card

Display Buienalarm and/or Buienradar data in a graph for Home Assistant.
30 stars 3 forks source link

Sensor code improvements/questions #1

Open denilsonsa opened 3 years ago

denilsonsa commented 3 years ago

Hi! I have a few suggestions/questions:

  1. Any reason why using command_line instead of rest?
    • I assume buienalarm would work fine with REST, as it returns a nicely formatted JSON.
    • I also believe plaintext would work fine with REST as well. By looking at the code, if neither json_attributes nor value_template is passed, then it looks like the raw response is stored in the template.
  2. Any reason why the main data is inside the data attribute of the entity? Why not in the main entity state?
  3. You can use {{ state_attr('zone.home', 'latitude') | round(3) }} and {{ state_attr('zone.home', 'longitude') | round(3) }} to automatically populate lat/lon from HA configuration.
  4. For the c parameter, you can use either {{ as_timestamp(now()} }} or {{ range(0,999999999)|random }}
denilsonsa commented 3 years ago

Oh, I see why you store everything in an attribute…

  - platform: rest
    name: Buienalarm regen data
    resource_template: https://cdn-secure.buienalarm.nl/api/3.4/forecast.php?lat={{ state_attr('zone.home', 'latitude') | round(3) }}&lon={{ state_attr('zone.home', 'longitude') | round(3) }}&region=nl&unit=mm%2Fu&c={{ as_timestamp(now()) }}
    scan_interval: 300
  - platform: rest
    name: Buienradar regen data
    resource_template: https://gpsgadget.buienradar.nl/data/raintext?lat={{ state_attr('zone.home', 'latitude') | round(3) }}&lon={{ state_attr('zone.home', 'longitude') | round(3) }}&c={{ as_timestamp(now()) }}
    scan_interval: 300

State max length is 255 characters.

That's why you store things as an attribute. But since that requires json_attributes key, you had to use custom Python code to wrap everything into a trivial JSON.

Still, using templates for the lat/lon and for the random might make the code simpler. Or even building a custom integration component to be published on HACS.

aex351 commented 3 years ago

The rest sensor has a limitation of 255 characters that can be stored in the state. This is something different than an attribute. Attributes don't have this limitation. Unfortunately there is no easy solution to split the rest data up in to attributes. The usage of json_attributes is limited, very specific and only works when the rest data is pure json. For this reason I had to use the command_line sensor.

In regards to the usage of the location as set in zone.home. I felt making this default or adding it to the readme might add to the complexity of setting up these custom sensors. Especially when users have not set zone.home. Currently the readme basically says: do a find and replace. Which is a pretty straightforward task. I can add a note that they can also use zone.home.

For the c parameter I like the improvement idea as it minimizes the Python code. The import random can then be removed.

Ultimately I would like to have these custom sensors converted in to a custom integration. That would simplify the installation proces and usage for users.

denilsonsa commented 3 years ago

One small comment: according to the Zone docs, “if no configuration is given, the zone integration will create a zone for home. This zone will use location provided in the configuration.yaml. Well, the text sounds a bit outdated, by my understanding is the zone.home is always available.

This is my current configuration:

  - platform: command_line
    command: python3 -c 'import requests; print("{\"data\":" + requests.get("https://cdn-secure.buienalarm.nl/api/3.4/forecast.php?lat={{ state_attr("zone.home", "latitude") | round(3) }}&lon={{ state_attr("zone.home", "longitude") | round(3) }}&region=nl&unit=mm%2Fu&c={{ as_timestamp(now()) }}").text.replace("\r\n"," ") + "}")'
    name: Buienalarm regen data
    json_attributes:
      - data
    value_template: 'last_changed: {{states.sensor.buienalarm_regen_data.last_changed}}'
    scan_interval: 300
  - platform: command_line
    command: python3 -c 'import requests; print("{\"data\":\"" + requests.get("https://gpsgadget.buienradar.nl/data/raintext?lat={{ state_attr("zone.home", "latitude") | round(3) }}&lon={{ state_attr("zone.home", "longitude") | round(3) }}&region=nl&unit=mm%2Fu&c={{ as_timestamp(now()) }}").text.replace("\r\n"," ") + "\"}")'
    name: Buienradar regen data
    json_attributes:
      - data
    value_template: 'last_changed: {{states.sensor.buienradar_regen_data.last_changed}}'
    scan_interval: 300

I'm not sure if value_template is useful. It feels like it only serves to generate useless entries to the record/history integration; so a static text (or the HTTP response code) could likely be more useful and less noisy.

Also, I changed the double-quotes around the Python program to single-quotes. That's because in bash (or sh) single-quoted text doesn't have any special meaning, while double-quoted text can have variables and maybe escape codes.

aex351 commented 3 years ago

zone.home will still require a latitude and longitude to be configured for it to work properly with these custom sensors. These values won't be auto populated. Therefore for these custom sensors, one way or another, users will need to have the latitude and longitude configured. For zone.home this can be done through the interface or configuration.yaml (legacy).

The value_template that sets the state of the custom sensor with the last_changed value is so you can check when the last update took place. It is not mandatory.

I see in your current configuration you changed the value of the c variable to {{ as_timestamp(now()) }}. This actually creates something like 0000000000.0000. Which contains a dot. While is shouldn't cause any problems it might be nicer to use {{ (range(0,99999) | random) * 99999 }}.

groenmarsmannetje commented 3 years ago

I have already setup my home location and standard buienradar weather and sensor integration. I really doubt why a user should need to setup his own additional sensor in configuration file with a command line interface. I will keep track of this custom integration, but for me it is just an early release with couple of improvements. I really like the graph, that already looks great !

Romkabouter commented 3 years ago

@groenmarsmannetje, the buienradar integration (camera and sensor) do not publish rain precipitation. Those only show a map and a forecast so a sensor is needed for the precipitation

aex351 commented 3 years ago

You might want to take a look at the 'Neerslag App'. This combines the Sensors + Neerslag Card in to one Home Assistant package. https://github.com/aex351/home-assistant-neerslag-app