briis / smartweather

WeatherFlow Smart Weather Component for Home Assistant
MIT License
108 stars 11 forks source link

Changed Forecast Weather source to Tempest API #36

Closed max-rousseau closed 4 years ago

max-rousseau commented 4 years ago

Quick updates to feed the data from the Tempest SmartWeather API, probably some clean-up needed. I tested this with my own weather station & HassOS 4.11. Note that I tested this with a public station. If your Tempest station is not public, either make it so or you would need to figure what is your API key to access it.

Only need the following for the weather entity to work in configuration.yaml, the key is standard DEV key:

weather:
  - platform: smartweather

smartweather:
  station_id: <your station id here>
  api_key: 20c70eae-e62f-4d3b-b3a4-8586e90f3ac8

All the data is pulled from SmartWeatherCurrentData.update() and the weather.py module pulls it from hass.data[DATA_SMARTWEATHER].

I suspect there is still some issues with unit types & conversions.

briis commented 4 years ago

Thanks Max, I have a full weekend (Daughter is getting Married), and I want to look in more detail before I Merge this. Initial thoughts are: Home Assistant is not too happy with IO being performed directly in the Integration, so I might take the IO part of your code and move it to the IO module (https://github.com/briis/pysmartweatherio). I will revert back early next week. Thanks

max-rousseau commented 4 years ago

Awesome, enjoy the weekend and indeed to feel free to commit any changes here. I agree with you on the IO and if I recall correctly I have a comment on the data object to the effect of "move me to pysmartweatherio" :) If I get a few more hours I may also add a little more of error/exceptions handling, as-is its pretty bare.

Congrats to the happy couple!

max-rousseau commented 4 years ago

Added a new commit as I have observed over the weekend that the API sometimes returns forecast data for days that are past. New code checks the day_start_local time and discards them if they are < than datetime.now()

briis commented 4 years ago

Very good - I have worked on this all day, and have taken your Forecast code and implemented this in to the basic IO module. At the same time I have re-written the whole IO module and converted it to use Async calls, so basically it is a brand new module.

With that done, I then also re-wrote the whole SmartWeather component, and converted it to a real Integration, so that it is now configured from the UI, and you don't need (you can't) add it from Yaml. Give me a day more, and I have something for you to test.

One thing that is still a little problem, is mapping to Home Assistant conditions, so that we see the correct icons. Have you found anywhere a list of the different conditions or condition icons that can be returned from this? If I had that, the mapping is easy done.

So for now I will not merge this, but I am copying some of the code.

briis commented 4 years ago

Hi again, I finished earlier than I thought, and I have something for you to try out. In order to get this to work you will first have to:

Then copy the files from this directory to your computer.

If you are happy, I will close this request, without merging, and merge the V2.0 branch instead.

max-rousseau commented 4 years ago

I have no problems with you lifting the code and putting it in its better home, sounds like a bunch of other great improvements made their way in at the same time! Let me get the v2 a test run here and will report back.

max-rousseau commented 4 years ago

Saw a few things that I'm trying to track down...

smartweather.get_station_name()

This seems to get name up to a space character, in my case the station name has a space in it so it truncates prematurely

My weather card shows up the current temperature in Celsius but with a *F next to it. Some kind of unit conversion issue. I recall it was a bit finicky which is why I ended up putting it as this in my code:

self.observations['temperature_unit'] = TEMP_CELSIUS

I think the station ignores any unit passed and always returns Celsius.

max-rousseau commented 4 years ago

Otherwise, the config flow is awesome!

max-rousseau commented 4 years ago

Ah, the sensors didn't pick up either. I saw this in the log:

2020-08-11 16:22:09 ERROR (MainThread) [homeassistant.components.sensor] Error while setting up smartweather platform for sensor
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 179, in _async_setup_platform
    await asyncio.wait_for(asyncio.shield(task), SLOW_SETUP_MAX_WAIT)
  File "/usr/local/lib/python3.8/asyncio/tasks.py", line 483, in wait_for
    return fut.result()
  File "/config/custom_components/smartweather/sensor.py", line 136, in async_setup_entry
    if not entry.data[CONF_ADD_SENSORS]:
KeyError: 'add_sensors'
briis commented 4 years ago

Thanks for testing so far, and glad you like the Config Flow. I don't have a Tempest unit as these are only shipped to the US at present, so I need a test station. Could you give me your station id, so that I can see what you see? I did find a Tempest station id, and when testing with this, I have found that the current station data is not returned, so I need to figure out what the difference is. There is most likely something from your code I did not copy, but hard to compare without the raw data.

With regards to the Celcius/Fahrenheit on the Weather Card. You are right that the weather entity is only accepting Celcius as the input value, and then does all the Temperature conversions by itself. And that is also what I send to it. On my standard Lovelace Weather Card I get the right F values when selecting Imperial as Unit System, so that is also why I would like to test with your station id.

EDIT: Found an error in the IO module, so I believe the C/F problem will be solved on the next release.

briis commented 4 years ago

Hi I have made some changes. Could re-download the files, and overwrite the existing ones, and then restart HA, and let me know what errors you get. Thanks

max-rousseau commented 4 years ago

Great, I will test the new version and let you know. For data, I have put example JSON data as comments in my commit so that may help a bit. Otherwise, I've tested with a few public tempest stations from: https://tempestwx.com/map/

max-rousseau commented 4 years ago

Still getting this error for both sensor.py and binary_sensor.py

     if not entry.data[CONF_ADD_SENSORS]:
KeyError: 'add_sensors'

The comment refers to a configuration item but I dont recall having seen this in the config flow?

Other error I saw is this one, which I suspect is one of those async challenges:

  File "/config/custom_components/smartweather/sensor.py", line 196, in state
    return round(value, 1)
TypeError: type NoneType doesn't define __round__ method

I fixed both locally pretty easily with a simple try statement.

briis commented 4 years ago

I simply don't understand why you are getting the first error, I have not been able to reproduce that. The reason this line is there, is that if you remove the Check in the Box on the Config page, the sensors will not be installed, only the Weather Entity, but it should be stored every time no matter if you click it or not. Could you try to remove SmartWeather from the Integration and add it again, to see if the error persist?

The rounding error needs to be handled, but I need to figure out what data is wrong. I don't get this error on my SKY & AIR unit, so there most be a value that I expect returns a numbers that doesn't in your case. Could you share your Station ID with me, so that I can test and find the field that is not returned on a Tempest station?

max-rousseau commented 4 years ago

I don't recall having seen that checkbox so that might be why, perhaps issue with config_flow.py?

Try testing with station id 9973

max-rousseau commented 4 years ago

I'm also thinking it might be useful to keep the smartweather prefix on the sensor naming scheme, not entirely sure of how HASS works but I'm suspecting it would be somewhat conflict-prone if we just have sensor.humidity, etc..

briis commented 4 years ago

I made a few more tweaks, but I cannot in anyway reproduce the add_sensors error you are seeing. My best suggestion is to completely remove everything SmartWeather releated, including the directory in custom_components, restart HA, and the add it again, to make sure that there are no old cached files around. Once you add the integration, the Config Box should look like this: image

Where I have highlighted the Check Box we are talking about. Per default this is checked, and you don't have to change that.

I also took your suggestion, and all sensors are now pre-fixed with smartweather_

max-rousseau commented 4 years ago

Oh good news on the prefix! For the key error, I wouldn't sweat trying to reproduce. I'm a fan of Python EAFP style, I would just put the following code in and assume if something went weird, we put sensors in.

    try:
        if not entry.data[CONF_ADD_SENSORS]:
            return
    except KeyError:
       _LOGGER.warning(f"Warning: no add_sensors in integration config, assuming true.")
max-rousseau commented 4 years ago

I've identified a new issue as well with the temperature, on the weather card it sometimes shows it as 20.599999999999998 °F, this may be related to the rounding issue that I've messed with. I'll double check that. I also recall seeing somewhere there was a "precision level" configuration.

briis commented 4 years ago

The precision level is defined in the generic weather entity, and should as such be inherited. I will implement some rounding in the IO module that should solve this.

I WILL NOW CLOSE THIS PULL REQUEST without merging, and then merge the V2.0 with the master branch instead. We can then continue the discussion and test in the Issue #35 you already created.

max-rousseau commented 4 years ago

Yes, works for me. I was going to say this is an awkward place to discuss 2.0 troubleshooting :)