jim-easterbrook / pywws

Python software for USB Wireless WeatherStations
https://pywws.readthedocs.io/
GNU General Public License v2.0
204 stars 62 forks source link

More complete default for template_txt for service.mqtt #83

Closed michlv closed 5 years ago

jim-easterbrook commented 5 years ago

Won't this break existing users' setups by renaming things, e.g. wind_dir is renamed wind_dir_degrees?

michlv commented 5 years ago

Won't this break existing users' setups by renaming things, e.g. wind_dir is renamed wind_dir_degrees?

Hi Jim, not it will not. This is only used as default in weather.ini file if template_txt is not specified. Even then, when pywws stops and template_txt was not specified, pywws will write new version of weather.ini defining the template_txt with the current version. Practically this means that nobody has weather.ini without template_txt. So it really only helps new people.

I think that this new version is much better default. I tried to connect pywws to Domoticz and it took me a while to come up with this default, which provides enough info. If it was like this right from start, it would have taken me much less time to integrate.

jim-easterbrook commented 5 years ago

Thanks, I hadn't really thought it through properly. I'll merge in a day or two if none of the MQTT gurus (@GeekyTim @sgtwilko @matt2005 @mortenf @rk295) has any objection.

sgtwilko commented 5 years ago

My only comment would be that this change should be documented.

If I were setting up a new weather station with the intent of using an existing MQTT setup it wouldn't be obvious why two setups on the same version behave differently.

jim-easterbrook commented 5 years ago

Could the original names be included as well? I notice that illuminance and UV values are in the new default, so most users (i.e. the ones without solar sensors) will be forced to edit their template in future.

michlv commented 5 years ago

With regards of setting up new weather station. I would think that this highly depends if the user already used pywws or not. I would think that if that is existing user, one would copy over weather.ini from the other installation and amend it, in which case there is no issue, because weather.ini will have the original template_txt. These new defaults will only affect new users, who are setting this up the first time by using documentation.

michlv commented 5 years ago

With regards of the uv and illuminance, I thought that if one uses the below format:

illuminance \'"illuminance_lux" : "%.1f",\'

the value only gets output if the weather station supports it. Otherwise nothing (at least https://pywws.readthedocs.io/en/latest/api/pywws.template.html#module-pywws.template for the #key suggest it.). So I think the MQTT message will not have anything for illuminance or uv, if the weather station model does not support those.

I have not actually tested it with pretending that my weather station does not support these values. I'll give it a try.

michlv commented 5 years ago

And documenting the change, I have put a line into the change log. Is that what you had in mind sgtwilko?

jim-easterbrook commented 5 years ago

Stations without solar sensors will most likely raise a KeyError exception if you try to use solar data. This is why other service modules, such as underground, add solar data to the template in the __init__ method. Missing data values, for example when the station briefly loses contact with its outside sensors, are given the value None and can be omitted or represented by '-' or whatever in a template as required.

michlv commented 5 years ago

Thanks Jim, I think about this and come back hopefully with workable solution :-)

sgtwilko commented 5 years ago

@michlv I was thinking it should also be in the official documents on readthedocs.io

Just something in the MQTT section mentioning the default template has breaking changes from version X.

jim-easterbrook commented 5 years ago

There's a standard in the Sphinx markup for this. I'd have to look it up but it results in a section starting with something like "New in version vNN.N:".

GeekyTim commented 5 years ago

Thanks, I hadn't really thought it through properly. I'll merge in a day or two if none of the MQTT gurus (@GeekyTim @sgtwilko @matt2005 @mortenf @rk295) has any objection.

No objection for me! Currently working on my weather clock v2 where I'm taking MQTT dat from pywws and displaying on a 64x64 LED matrix. These updates are always welcome!

michlv commented 5 years ago

Hopefully I have addressed all comments now.