naofireblade / homebridge-weather-plus

A comprehensive weather plugin for homebridge.
MIT License
315 stars 63 forks source link

Support Tempest Weatherflow weather station #262

Closed dacarson closed 1 year ago

dacarson commented 1 year ago

Building upon this amazing weather plugin for Homebridge, these changes extend it to support the physical Tempest Weatherflow weather station. It also adds a couple more characteristics that are published by the wether station.

jlg89 commented 1 year ago

I installed your version, and as long as I only set it for Apple Home it seems to be OK. If I set it to Apple Home (plus Eve), I get this error:

[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] Loaded homebridge-weather-plus v3.2.11 child bridge successfully
[6/7/2023, 11:28:32 AM] Loaded 0 cached accessories from cachedAccessories.0E6AB4639D94.
[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] Adding station with weather service SmartWeatherAPI named 'Ravenwood Weather Station'
[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] server listening 0.0.0.0:50222
TypeError: Cannot read properties of undefined (reading 'UUID')
    at TemperatureSensor.Service.addCharacteristic (/usr/local/lib/node_modules/homebridge/node_modules/hap-nodejs/src/lib/Service.ts:584:44)
    at /usr/local/lib/node_modules/homebridge-weather-plus/accessories/currentConditions.js:91:36
    at Array.forEach (<anonymous>)
    at new CurrentConditionsWeatherAccessory (/usr/local/lib/node_modules/homebridge-weather-plus/accessories/currentConditions.js:66:61)
    at /usr/local/lib/node_modules/homebridge-weather-plus/index.js:97:30
    at Array.forEach (<anonymous>)
    at new WeatherPlusPlatform (/usr/local/lib/node_modules/homebridge-weather-plus/index.js:64:22)
    at ChildBridgeFork.startBridge (/usr/local/lib/node_modules/homebridge/src/childBridgeFork.ts:150:42)
[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] Child bridge process ended
[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] Process Ended. Code: 1, Signal: null

Also, the light level reading is always zero.

dacarson commented 1 year ago

Thanks!! I'll file a bug report in my repository and fix it there. Then propose a new merge here.

jlg89 commented 1 year ago

I get six occupancy sensors in HomeKit. "Rain detected" makes sense, the rest (Air Pressure, Total Precipitation, UV Index, Wind Direction, Wind Speed) seem like they should be some other kind of value-reporting device, but maybe HomeKit doesn't support what we need there?

wimleers commented 1 year ago

Thanks for this! πŸ‘ This is how it should be done: no cloud involved, only local communication πŸ‘

Will test in the coming days.

EDIT: I seem to be having a hard time getting this to work:

cd ~/Desktop
git clone git@github.com:dacarson/homebridge-weather-plus.git
cd homebridge-weather-plus
npm install
npm link

☝️ I can configure it just fine in the Homebridge UI, but then it doesn't load.

I think I found it though: your modifications do require('weather-formulas'), but it's not defined as a dependency πŸ˜… So of course it crashes during startup, and of course the WeatherPlus platform is then "not available".

EDIT2: a simple addition of "weather-formulas": "^1" to the list of dependencies in package.json followed by npm install results in a perfectly working setup without the need for a cloud connection πŸ‘ 🀩 My config:

{
    "units": "si",
    "interval": 1,
    "stations": [
        {
            "service": "smartweather",
            "language": "en",
            "compatibility": "eve",
            "conditionCategory": "detailed",
            "now": true,
            "extraHumidity": false
        }
    ],
    "name": "Tempest Weatherflow",
    "platform": "WeatherPlus",
    "_bridge": {
        "username": "1E:C6:9E:E5:98:06",
        "port": 32686
    }
}

@jlg89 Which app are you using? The official Home app does not support custom characteristics. You need Eve (free), Home+ (paid), Controller (paid) or another app, that supports the full range of HomeKit capabilities.

wimleers commented 1 year ago

image

image

image

I really hope this gets merged! I'll review the PR later for maintainability, to hopefully increase the odds this will get merged!

dacarson commented 1 year ago

I installed your version, and as long as I only set it for Apple Home it seems to be OK. If I set it to Apple Home (plus Eve), I get this error:

[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] Loaded homebridge-weather-plus v3.2.11 child bridge successfully
[6/7/2023, 11:28:32 AM] Loaded 0 cached accessories from cachedAccessories.0E6AB4639D94.
[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] Adding station with weather service SmartWeatherAPI named 'Ravenwood Weather Station'
[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] server listening 0.0.0.0:50222
TypeError: Cannot read properties of undefined (reading 'UUID')
    at TemperatureSensor.Service.addCharacteristic (/usr/local/lib/node_modules/homebridge/node_modules/hap-nodejs/src/lib/Service.ts:584:44)
    at /usr/local/lib/node_modules/homebridge-weather-plus/accessories/currentConditions.js:91:36
    at Array.forEach (<anonymous>)
    at new CurrentConditionsWeatherAccessory (/usr/local/lib/node_modules/homebridge-weather-plus/accessories/currentConditions.js:66:61)
    at /usr/local/lib/node_modules/homebridge-weather-plus/index.js:97:30
    at Array.forEach (<anonymous>)
    at new WeatherPlusPlatform (/usr/local/lib/node_modules/homebridge-weather-plus/index.js:64:22)
    at ChildBridgeFork.startBridge (/usr/local/lib/node_modules/homebridge/src/childBridgeFork.ts:150:42)
[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] Child bridge process ended
[6/7/2023, 11:28:32 AM] [homebridge-weather-plus] Process Ended. Code: 1, Signal: null

Also, the light level reading is always zero.

I found the issue with this, the problem was that I had "LightLevel" listed in the compatibility list. However, LightLevel is a native supported type and I implemented it that way. I will post a new update. I have a change on my branch to fix this.

dacarson commented 1 year ago

I have corrected the two issues report here, and will submit a new merge request. @wimleers Thank you for your feedback and I agree you should not need to go to the cloud @jlg89 Thank you for testing and your feedback too.

wimleers commented 1 year ago

@dacarson I found one more bug: if one restarts Homebridge, it'll forget the last values. So rain gets reset to no, total rain gets reset to 0.0 mm even if just a minute prior it was saying 2.0 mm, maximum wind speed gets reset to 0 m/s, et cetera.

So what appears to be missing right now is the use of node-persist, such as demonstrated by e.g. https://github.com/nfarina/homebridge-dummy. Adding that simple additional code would make this Homebridge plugin feel much more reliable!

wimleers commented 1 year ago

Also: why do you keep closing PRs? Just update this one? πŸ˜…

EDIT: especially because @naofireblade has already reacted positively above: he gave a πŸ‘ to my comment with the screenshots and in which I said I'd review this for maintainability 😊 So … can you please reopen this PR and just keep pushing commits? That'd be much easier for everyone to keep track of the current status!

dacarson commented 1 year ago

Also: why do you keep closing PRs? Just update this one? πŸ˜…

I'm new to GitHub, not sure of the right way to push a fork.

dacarson commented 1 year ago

So what appears to be missing right now is the use of node-persist, such as demonstrated by e.g. https://github.com/nfarina/homebridge-dummy. Adding that simple additional code would make this Homebridge plugin feel much more reliable!

I wanted to do something like this, but wasn't sure of the right approach. I'll take a look and try to get the changes done today.

wimleers commented 1 year ago

I'm new to GitHub, not sure of the right way to push a fork.

No worries! I'm happy to point you in the right direction 😊

It looks like you started pushing new commits to https://github.com/dacarson/homebridge-weather-plus/commits/Support-Tempest-Weathflow-weather-station.

But … if you instead push them to https://github.com/dacarson/homebridge-weather-plus/commits/master, they will automatically appear here!

You should be able to do that using these commands on your locally cloned fork:

git checkout master
git cherry-pick 418cb8e
git cherry-pick 0c2580c
git cherry-pick b7d4651
git push

That way you'll bring over those 3 commits you made in a new branch to the master branch, which the top of this page shows is the source for this pull request! 😊

Hope that helps!

And major kudos for figuring out the hard part: how to elegantly bring in Weatherflow's local data! πŸ‘ πŸ₯³ 🀩

dacarson commented 1 year ago

@dacarson I found one more bug: if one restarts Homebridge, it'll forget the last values. So rain gets reset to no, total rain gets reset to 0.0 mm even if just a minute prior it was saying 2.0 mm, maximum wind speed gets reset to 0 m/s, et cetera.

So what appears to be missing right now is the use of node-persist, such as demonstrated by e.g. https://github.com/nfarina/homebridge-dummy. Adding that simple additional code would make this Homebridge plugin feel much more reliable!

I agree with what you are asking, it is something that has bothered me too. This will be interesting to implement because I would need to track how much time has passed. The plugin or homebridge may have stopped only briefly (aka restart) or it could have been down for a day. If it was only brief, then I could rebuild my arrays that track the amount of rain per hour (so that I can report the rain in last 24hrs) but if an hour or a day has passed then I need to recalculate. I need to wait for the first broadcast packet from Tempest to get initial values (except accumulated values). It broadcasts the weather data every minute over UDP on the home network. This is why all values are empty/null/0 for up to a minute. I could go to internet and get the last values the weather station reported, but I would prefer not to do that. This may take a little more time for me to think how to best handle this.

wimleers commented 1 year ago

I need to wait for the first broadcast packet from Tempest to get initial values (except accumulated values). It broadcasts the weather data every minute over UDP on the home network. This is why all values are empty/null/0 for up to a minute.

I get that!

But … isn't there a much simpler solution? Whenever you update the current values for each characteristic, persist them. That means the Observation time will be persisted too. Which means that after restarting Homebridge, it'll just show the data of the last observation time.

This will be interesting to implement because I would need to track how much time has passed. The plugin or homebridge may have stopped only briefly (aka restart) or it could have been down for a day. If it was only brief, then I could rebuild my arrays that track the amount of rain per hour (so that I can report the rain in last 24hrs) but if an hour or a day has passed then I need to recalculate.

I had to do something similar in one of my Homebridge plugins. I only need to track the "today" data (not past 24 hours) though, and the information I receive there is always cumulative, so it's simpler than this case. Still, I think it can be a good starting point for you, because it shows how you could account for missed seconds (in the case of the Tempest Weatherflow: number of missed observations I think?):

(If you want to be fancy, you could also report % completeness of the cumulative data. But IMHO just the above would be good enough.)

EDIT: are you sure you need to track all data for the running 24 hours (i.e. past 86400 seconds)? Because even Weatherflow itself only reports "today" and "yesterday" for rain!

dacarson commented 1 year ago

My Weatherflow implementation conforms to the architecture of this plugin, which was written with the idea of fetching (pulling) weather information from internet on a periodic basis. It does not need to persist data, the data is always available. The Tempest weather data is broadcast (pushed) on it's own periodic basis, and is not always available. For this plugin, it is only the Tempest weather source that needs to persist information.

The way I see persistance implemented in the examples you provided is through Characteristic updates - aka when the data is pushed to HomeKit. Characteristic updates are handled in general fashion for all weather sources in the updateWeather function. (FWIW, I think the way this was done was very elegant). It doesn't make sense to implement persistence there as it would do it for every weather data source.

I could look doing my own persistence implementation, in the smartweather.js file, in the SmartWeatherAPI update() function I could store the JSON object that.currentReport to disk, as that is what is provided to the updateWeather function. When my code is initialized, aka whenSmartWeatherAPI constructor() is called, just load the JSON object from disk. Total Rain value will be correct if I take this approach but Rain last hour won't be. See below.

EDIT: are you sure you need to track all data for the running 24 hours (i.e. past 86400 seconds)? Because even Weatherflow itself only reports "today" and "yesterday" for rain!

The weather station broadcast rain accumulation for the last minute. It doesn't keep track of rain accumulation over the last hour. I need to keep track of the last 60 minutes of rainfall because the value for "Rain last hour" could be requested at 10:28am, therefore the last hour is 9:28am - 10:28am. The Eve value isn't 'Rain current hour' it is 'Rain last hour'. Maybe it is ok to zero this one out....

Note one other drawback of the pull architecture when applied to this weather station is that I can't update the Rain/Lightning/Wind status immediately. The weather station broadcasts those statuses immediately, but it won't get pushed to HomeKit till when the updateWeather timer fires: setTimeout(this.updateWeather.bind(this), (this.interval) * 60 * 1000);

I think in the long term I should write my own homebridge plugin from scratch that operates via a push model. Everytime data is received from the weather station, push it immediately to HomeKit. With my own plugin, I could also properly support weather station sensor failures (which has happened to me on different units), device serial number(s) and firmware value.

wimleers commented 1 year ago

It does not need to persist data, the data is always available. … For this plugin, it is only the Tempest weather source that needs to persist information.

Makes sense!

I could look doing my own persistence implementation, in the smartweather.js file

+1, that's equivalent to adding client-side caching to avoid hitting the API unnecessarily πŸ‘ Also similar to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#stale-while-revalidate

Total Rain value will be correct if I take this approach but Rain last hour won't be.

Right, to get it perfectly accurate will be tricky. But I think if we persist not just the current report but the data behind the current report, it'd be totally possible. getHourlyAccumulatedRain() would need to be refined: rainAccumulationMinute currently endlessly rotates between 0, 1, …, 59, 0, 1, … (in other words: %59). What if instead of relying on that, you'd rely on const reportedMinute = moment.unix(message.obs[0][0]).format('mm')? (You already have that.currentReport.ObservationTime = moment.unix(message.obs[0][0]).format('HH:mm:ss');.) Whenever you receive such a reportedMinute, you'd compute const expectedMinute = (reportedMinute + 1) % 59;. Whenever a new message arrives and expectedMinute !== reportedMinute, that must mean we've skipped a minute. That means we'd want to set all minutes starting with expectedMinute and ending at (reportedMinute - 1) % 59 to zero, because we have no choice but to assume no rain fell.

The weather station broadcasts those statuses immediately, but it won't get pushed to HomeKit till when the updateWeather timer fires

First: that's not a big deal I think, because updates every few minutes are already plenty? πŸ˜„

Second: I'm sure that we can work with @naofireblade to explicitly support push data sources such as this one, to allow a different update mechanism in that case! πŸ€“

I think in the long term I should write my own homebridge plugin from scratch that operates via a push model. Everytime data is received from the weather station, push it immediately to HomeKit.

Fair. But … we'd lose out on the many benefits that this centralized plugin has: it made it a LOT simpler to write this! Contrast what you did here and how feature-rich it is with https://github.com/chasenicholl/homebridge-weatherflow-tempest β€” that uses only occupancy sensors, which results in barely information being exposed!

With my own plugin, I could also properly support weather station sensor failures (which has happened to me on different units), device serial number(s) and firmware value.

I'm convinced that for this too, we could work with @naofireblade. 😊 (Although this for sure seems less important?)

dacarson commented 1 year ago

I have already submitted a change to master that persists the data in smartweather.js

I have also been thinking about how to store the rain over the last hour. I think there might be a way to use node-persist TTL for the data. If I persist it, and tag the data as only being valid for an hour. And fix the array storage so that it does actually line up with the minutes. (Which reminds me that I should tag the other data indicating that it is only valid for 24hrs - the reason for 24hrs is because the observation time only shows time, not date).

RE: Push vs Pull data model. Currently Index.js 'knows' about the weather API in use and pulls data from it. The weather API does not know about Index.js, so it can't push to it.

I didn't know about https://github.com/chasenicholl/homebridge-weatherflow-tempest, I'll take a look at what they did. However, I do agree that it was much easier to implement into this plugin.

wimleers commented 1 year ago

I have already submitted a change to master that persists the data in smartweather.js

Yep, and I'm running it as of a few minutes ago! πŸ˜„

I have also been thinking about how to store the rain over the last hour. I think there might be a way to use node-persist TTL for the data. If I persist it, and tag the data as only being valid for an hour.

Ohh, I like that too! πŸ‘ πŸ‘

And fix the array storage so that it does actually line up with the minutes.

Yep, that's what I basically proposed above 😊

dacarson commented 1 year ago

I didn't know about https://github.com/chasenicholl/homebridge-weatherflow-tempest, I'll take a look at what they did.

Interesting approach for the weatherflow - that plugin goes to the internet for all data. I do see that Tempest now offer a forecast API. Maybe in a future version, I could support the forecast data. https://weatherflow.github.io/Tempest/api/swagger/#!/forecast/getBetterForecast

wimleers commented 1 year ago

I didn't know about https://github.com/chasenicholl/homebridge-weatherflow-tempest, I'll take a look at what they did.

Interesting approach for the weatherflow - that plugin goes to the internet for all data. I do see that Tempest now offer a forecast API. Maybe in a future version, I could support the forecast data. https://weatherflow.github.io/Tempest/api/swagger/#!/forecast/getBetterForecast

HAH! I literally just was working on a review suggesting precisely that! πŸ˜„ πŸ‘

dacarson commented 1 year ago

Reviewed all code review changes, and corrected the ones that I have no questions about.

dacarson commented 1 year ago

Lastly, added support for the saving and restoring hourly rainfall :-)

dacarson commented 1 year ago

Incorporated feedback

dacarson commented 1 year ago

FWIW, I wasn't able to get setItemSync(name, array[]) and array = getItemSync(name) to work. getItemSync wasn't loading the array. I could hack it, cause I could see how setItemSync stored arrays and load the array back in manually but I can't be guaranteed that the way setItemSync would always store arrays that way. So I left it the way I had it.

wimleers commented 1 year ago

I think the primary remaining thing is … updating the README, to proudly add a 4th column to the table? πŸ€“ πŸ˜„

Thank you so much for your epic effort here! πŸ˜ŠπŸ™

dacarson commented 1 year ago

Updated the readme and correct my original statement about forecasts - it is available just not implemented yet.

dacarson commented 1 year ago

Thank you for all the feedback, it is appreciated.

wimleers commented 1 year ago

@naofireblade Thoughts on merging this? 😊 πŸ™

naofireblade commented 1 year ago

Hi, thanks for the reminder. I will check and merge it on this weekend.

dacarson commented 1 year ago

Hi, wondering when we might be able to merge this into trunk. Let me know if there is anything I can do to help.

naofireblade commented 1 year ago

Sorry, done! I will create the npm release tomorrow.

wimleers commented 1 year ago

Fantastic! 🀩

Can’t wait for this to ship in a release! 😊

naofireblade commented 1 year ago

Done. Thank you very much for the the perfect preparation.

jmissig commented 1 year ago

Everyone involved: thanks so much!

wimleers commented 1 year ago

Yay, thanks so much, @naofireblade for your responsiveness and your admirable maintainership, and thanks especially to @dacarson for building this awesome integration in the first place!

To my delight and surprise, it was easier to get this side of my weather station going than to properly install it πŸ™ˆ πŸ˜‚ Just did the npm unlinking dance and I'm now on a proper release again instead of a git clone πŸ₯³

@jmissig Glad you like it β€” especially meaningful to see the "thanks" coming from somebody who works at Apple 😊

wimleers commented 1 year ago

@dacarson I'm occasionally seeing

[9/17/2023, 3:39:41 AM] [homebridge-weather-plus] JSON Parse Exception: {"serial_number":"ST-00109058","type":"evt_precip","hub_sn":"HB-00123987","evt":[1694914780]} ReferenceError: getConditionCategory is not defined

At that time, I most definitely was not using the Eve app or any other app πŸ˜… Have you seen this too?

dacarson commented 1 year ago

I haven't seen it but I'll take a look to see what could cause it.

dacarson commented 1 year ago

Found the issue. Do you want to file a separate bug for the fix?

The issue is, in the function parseMessage(message) there is this code:

...
        if (message.type == 'evt_precip') {
            that.currentReport.ObservationStation = message.serial_number;
            that.currentReport.ObservationTime = moment.unix(message.evt[0]).format('HH:mm:ss');
            that.currentReport.ConditionCategory = getConditionCategory(1, this.conditionDetail); // It has started to rain
            that.currentReport.RainBool = true;
        }
...

This handles the Tempest perception detected object that is sent out. The bug is this line: that.currentReport.ConditionCategory = getConditionCategory(1, this.conditionDetail); // It has started to rain should be: that.currentReport.ConditionCategory = this.getConditionCategory(1, this.conditionDetail); // It has started to rain

I hadn't seen it as it doesn't rain much here in San Francisco. I didn't test the code enough when it is raining.

wimleers commented 1 year ago

Looks like it already landed in https://github.com/naofireblade/homebridge-weather-plus/commit/3db4013b2e2e8f28e806d081f1bba7a1b596bd8d! 🀯