search

naofireblade / homebridge-weather-plus

A comprehensive weather plugin for homebridge.
MIT License
311 stars 61 forks source link

Fix tile labels, add sunrise/sunset and report sensor failure #285

Closed dacarson closed 3 months ago

dacarson commented 4 months ago

This fork has three fixes on it:

279 Report failing sensors. Shows as a general failure in app, also logs failure

247 Make use of ConfiguredName for compatibility values, to show values in the Home app where a native sensor isn't available

283 Report Sunset/Sunrise in forecast and current conditions when using Openweather API 2.5, and report Sunset/Sunrise for current conditions when using Openweather 3.0 API

Tested by running in my setup for a week, and 3.0 API was tested as documented here: https://github.com/jkreileder/homebridge-weather-plus/commit/e320cb91fd1499ea6773ce5d90d01edd63235696#r138496941

naofireblade commented 4 months ago

Amazing! Thank you for your continuous improvement of the plugin. I will try to check and merge both new PRs on Monday.

naofireblade commented 4 months ago

Sorry I had a busy week. Is this PR ready to merge or are u working on it again?

dacarson commented 4 months ago

It's ready to go. I have just been adding more fixes to it as I go along. Additional fixes on this PR are:

240 Sunrise/Sunset with OpenWeatherMap

122 Config UI not saving/restoring threshold values.

282 Additional logging to diagnose what might be going here

287 Light Level history in Eve App

45 Remove last parts of Yahoo! weather as it isn't supported

246 Additional debugging information in WeatherUnderground

112 Add Rain Gauge/Sensor to WeatherUnderground

I have been living on these changes with a Tempest configuration, a WeatherUnderground configuration and a OpenWeatherMap configuration.

wimleers commented 4 months ago

247 Make use of ConfiguredName for compatibility values, to show values in the Home app where a native sensor isn't available

Can you please move this out of this PR? This is a high-risk change, that only helps a subset of users. I have seen no naming issues.

dacarson commented 4 months ago

I can remove the changes for #247 out the PR. Though I do struggle to see that it only affects a subset of users and that it is a high risk issue. Can you try enabling setting 'Compatibility' to Apple Home (plus Eve)?

When I did that, I saw that compatibility mode simply didn't work. As described here when sensor names are updated, they are not updated in the app. There is a whole series of reports of "compatibility" mode not working.

With respect to high risk, I also beg to differ. Here is a link to the PR that has the change. The code that is impacted is only hit when you have Apple Home (plus Eve) compatibility mode set.

It seems valuable to have Apple Home (plus Eve) compatibility mode working.

wimleers commented 4 months ago

Aha, interesting!

Upon a second reading of the code, I think you're right that the risk is limited.

(Note that I cannot easily switch to a different mode, because that'd break all of my existing automations that depend on the current services+characteristics 😅)

I'll get this up and running on my local instance to help vet its safety 👍

dacarson commented 4 months ago

You can add a separate weather station. I have one of each setup. Though the config / setup UI file needs to be fixed to make it work with multiple stations. I fixed the config file here. The problem was all subsequent weather stations adopted the settings of the first. The index numbering was always 0 rather than the current weather station. There was also a typo that broke the ability to save thresholds (thresholds are used in compatibility mode).

Edit: The UI config file is loaded and executed when you hit the UI Configuration button for the plugin. So you can just replace that file without restarting, and it will work.

wimleers commented 4 months ago

5165f22 crashes upon boot:

[2/23/2024, 9:28:09 PM] [homebridge-weather-plus] Loaded homebridge-weather-plus v3.3.3 child bridge successfully
[2/23/2024, 9:28:09 PM] Loaded 0 cached accessories from cachedAccessories.0EC69EE59806.
[2/23/2024, 9:28:09 PM] [homebridge-weather-plus] Adding station with weather service TempestAPI named 'Now'
[2/23/2024, 9:28:09 PM] [homebridge-weather-plus] Restoring last readings
[2/23/2024, 9:28:09 PM] Initializing platform accessory 'Now'...
[2/23/2024, 9:28:09 PM] [homebridge-weather-plus] server listening 0.0.0.0:50222
[2/23/2024, 9:28:09 PM] Homebridge v1.7.0 (HAP v0.11.1) (homebridge-weather-plus) is running on port 32685.

/Users/admin/Desktop/homebridge-weather-plus/apis/weatherflow.js:588
                                callback(parseError, weather);
    ^
TypeError: Cannot read properties of undefined (reading 'time')
    at /Users/admin/Desktop/homebridge-weather-plus/apis/weatherflow.js:216:74
    at Request._callback (/Users/admin/Desktop/homebridge-weather-plus/apis/weatherflow.js:588:5)
    at Request.self.callback (/Users/admin/Desktop/homebridge-weather-plus/node_modules/request/request.js:185:22)
    at Request.emit (node:events:514:28)
    at Request.<anonymous> (/Users/admin/Desktop/homebridge-weather-plus/node_modules/request/request.js:1154:10)
    at Request.emit (node:events:514:28)
    at IncomingMessage.<anonymous> (/Users/admin/Desktop/homebridge-weather-plus/node_modules/request/request.js:1076:12)
    at Object.onceWrapper (node:events:628:28)
    at IncomingMessage.emit (node:events:526:35)
    at endReadableNT (node:internal/streams/readable:1589:12)
[2/23/2024, 9:28:10 PM] [homebridge-weather-plus] Child bridge process ended
wimleers commented 4 months ago

Note that this was happening with ZERO config changes.

Ah, after configuring stationId and key (confusingly named for the Weatherflow "Personal Use Token" 😅, personalUseToken would have been far clearer), that crash no longer occurs. Great. But that does mean that everybody who upgrades will run into this plugin crashing. It'd be much nicer if it didn't fail hard, but instead did a this.log.warn() and simply would not make that functionality work. Especially if you don't care about the forecasts (I didn't even enable them!).

dacarson commented 4 months ago

Agreed. Let me fix that.

wimleers commented 4 months ago

I'm seeing:

☝️ Where is that "code 1" coming from? Can we customize that? (Clearly Eve surfaces this best! 👏)

And getting this in the logs:

[2/23/2024, 9:41:37 PM] [homebridge-weather-plus] Sensor fail: Lightning failed Lightning noise Lightning disturber Pressure failed Temperature failed Relative Humidity failed Wind failed Precipitation failed Light/UV failed Power booster shore power

☝️ I think formatting could be better. Not multiple lines. Just some punctuation would do wonders.

dacarson commented 4 months ago

Unfortunately, there is only 'code 1' status fault recognized by HomeKit: https://developer.apple.com/documentation/homekit/hmcharacteristicvaluestatusfault

Edit: NoFault == 0 and generalFault == 1

Eve might use other numbers to give a more descriptive result, but I don't see anyone documenting those anywhere.

dacarson commented 4 months ago

https://github.com/naofireblade/homebridge-weather-plus/commit/5165f22a55baf24dc1e7cc6c0b3b254bee0f00e1 crashes upon boot:

Let me put in some code to make sure that it is resilient to missing or bad apiKey and/or stationId

wimleers commented 4 months ago

Thanks for everything you're doing — you're truly pushing this functionality to the next level! 🤩

I just enabled forecasts. For today + tomorrow. This is what those look like: today tomorrow

Observations:

dacarson commented 4 months ago
  • Observation time is listed for both but seems meaningless?

The way the code is structured, I need to return the same fields for each forecast day. So I can't list it for one day and not another.

Or is that the time it was based upon?

It is the time field provided in the BetterForecastCurrentConditions. I believe it is the time of the forecast. This is the same as how OpenWeatherMap populates that field. I think it is useful, it represent a time of when the forecast was made.

Or maybe it's only fetched once per hour?

I do only fetch it once an hour as (a) it is rate limited API call and (b) I can't see it changing more frequently than that. This is hard coded because the refresh interval is set to one minute to refresh the UDP data, and I don't want to call the forecast API each minute.

  • Why In 1 day and not just Tomorrow? 🤨

That decision was made before my time. Maybe it is for easier localization to other languages?

  • Why in the "Today" view does it say Rain probability: 20%, Condition: Rain possible (these make sense) and then Rain: Yes? I guess that's just because any non-zero chance would cause it to say Yes?

Correct. I followed the behavior in OpenWeatherMap, which is if the weather condition is (possible) Rain, Drizzle or Storm, then it sets the Rain to true. Similarly if the weather condition is (possible) Snow or Hail, then it sets the Snow to true. One difference is, I include Hail (Sleet) into the Rain checks and not the Snow checks. IMO that is better.

BTW The icon next to Condition in the Eve app does change depending on the forecast.

wimleers commented 4 months ago

Unfortunately, there is only 'code 1' status fault recognized by HomeKit: https://developer.apple.com/documentation/homekit/hmcharacteristicvaluestatusfault

Edit: NoFault == 0 and generalFault == 1

Eve might use other numbers to give a more descriptive result, but I don't see anyone documenting those anywhere.

If https://github.com/thoukydides/homebridge-homeconnect/issues/6#issuecomment-570773152 is accurate, you should be able to generate different error codes! 😊

wimleers commented 4 months ago 
[2/23/2024, 9:41:37 PM] [homebridge-weather-plus] Sensor fail: Lightning failed Lightning noise Lightning disturber Pressure failed Temperature failed Relative Humidity failed Wind failed Precipitation failed Light/UV failed Power booster shore power

☝️ I think formatting could be better. Not multiple lines. Just some punctuation would do wonders.

Also, this is now appearing once per minute. This means >90% of my Homebridge log is this exact same line. Can we reduce the reporting so it only reports once per hour? (You can keep a trivial counter and do something like let shouldWarnAboutFailures = observationsSinceLastFailure % 60 === 0; to achieve that.) The message could then be updated to say Will report again in one hour or after a restart of this Homebridge plugin.

wimleers commented 4 months ago

The way the code is structured, I need to return the same fields for each forecast day. So I can't list it for one day and not another.

👍

It is the time field provided in the BetterForecastCurrentConditions. I believe it is the time of the forecast. This is the same as how OpenWeatherMap populates that field. I think it is useful, it represent a time of when the forecast was made.

I see! I wish it was called Forecast time or Forecast last updated time or something like that then. But … per the previous point, that's not possible! So, yeah, it's kind of "observation time of this forecast", which is close enough 😄👍

I do only fetch it once an hour as (a) it is rate limited API call and (b) I can't see it changing more frequently than that. This is hard coded because the refresh interval is set to one minute to refresh the UDP data, and I don't want to call the forecast API each minute.

Totally reasonable! 👍

That decision was made before my time. Maybe it is for easier localization to other languages?

I see. 👍

BTW The icon next to Condition in the Eve app does change depending on the forecast.

🤩 The Eve app just keeps surprising — I'm impressed/surprised they have icons for so incredibly many non-official (i.e. not defined by the Apple HomeKit spec) characteristics! 😮

Thank you as always for your kind responses, your tireless work on this plugin. It is deeply appreciated. If you ever make it to Belgium, let me know. I'll get you the best beers 😄 (alcoholic or non-alcoholic)!

dacarson commented 4 months ago

Unfortunately, there is only 'code 1' status fault recognized by HomeKit: https://developer.apple.com/documentation/homekit/hmcharacteristicvaluestatusfault Edit: NoFault == 0 and generalFault == 1 Eve might use other numbers to give a more descriptive result, but I don't see anyone documenting those anywhere.

If thoukydides/homebridge-homeconnect#6 (comment) is accurate, you should be able to generate different error codes! 😊

You are correct. What I could do is just return the sensor_status number directly if there is an error. You would still need to know that that is what the number is, but it might be better.

dacarson commented 4 months ago

Summary of changes needed:

  1. If possible, calculate calculated fields even if there is a sensor error
  2. If there is a sensor error, return the sensor_status number directly as the StatusFault
  3. Don't continuously log the sensor failure, log it every hour.
  4. Make the code robust so that if apiKey and/or stationId values are missing or bad, don't crash.

Edit: 5. Format the sensor log entry better, add commas between failures Edit (2): 6. Make device_status more robust, and validate serial_number against observation before applying info from it.

wimleers commented 4 months ago

With those additions/changes, the Tempest Weatherflow support would be more robust than ever before! 😄 I guess that's the silver lining in all three of us being affected by three different failure modes? 🙈 🤞

dacarson commented 4 months ago
  1. If possible, calculate calculated fields even if there is a sensor error

I checked the source code for the weather formulas I am using and it will produce NAN/failures if the numbers are not within range. I modified the code to ensure values are within range (20c96ea)

dacarson commented 4 months ago
  1. If there is a sensor error, return the sensor_status number directly as the StatusFault

Tried this and I get an error message from HomeBridge. 

[2/23/2024, 4:46:26 PM] [WeatherPlus] Sensor fail: Lightning disturber 
[2/23/2024, 4:46:48 PM] [homebridge-weather-plus] This plugin generated a warning from the characteristic 'Status Fault': characteristic was supplied illegal value: number 4 exceeded maximum of 1. See https://homebridge.io/w/JtMGR for more info.

With the exception of that one mention in comments, I could not find any Homebridge extension that has code that returns a value other than 0 or 1. I can override it and force the number through, but I am reluctant to do this as I don’t know the behavior in all apps

dacarson commented 4 months ago

Also, thank you very much for the testing and the feedback @wimleers and @jmissig. It does make the support better.

dacarson commented 4 months ago

3. Don't continuously log the sensor failure, log it every hour.

Changed the logging to log initial failure and then every hour so that it doesn't spam the logs with failure entries (https://github.com/naofireblade/homebridge-weather-plus/pull/285/commits/b99b173f7ce7e9abe9ca7b1d41ad9e329dd59086). Logging is reset when the sensor status is set back to zero. However, as there can be AIR and SKY units used, and they both separately report sensor status, I need to keep track of which unit is failing so I can reset logging for the correct unit. Elsewhere in the code I overload the Sky unit with the Tempest unit as it is unlikely to have both, so I do the same in the error reporting.

In this change, I also removed the variables that were tracking Temperature and Humidity sensor failures as those variables are no longer needed now that I am bounds checking the values.

dacarson commented 4 months ago
  1. Make the code robust so that if apiKey and/or stationId values are missing or bad, don't crash.

There are two changes needed here (https://github.com/naofireblade/homebridge-weather-plus/pull/285/commits/db03a9ecfaf8a8b57d13a77e6e8538bd80431a80). First change is to not attempt to fetch the forecast if the apiKey and/or stationId is not set. I use the lastForecastUpdate variable to indicate if we have valid parameters at all. If they are not set, don't define the variable, and as such never attempt to fetch a forecast. This is a valid configuration so I don't log this state. The second change is to handle invalid or incorrect apiKey and/or stationId values. I now check the URL response and fail if the response code is anything other than 200 (OK). This error will get logged. As the forecast is only fetched every hour, this error will only appear once an hour and won't spam the logs.

wimleers commented 4 months ago

https://github.com/naofireblade/homebridge-weather-plus/commit/20c96ea5b808b412ef94e488e5bb664ebeec645d specified 0 as the minimum temperature. That’s true in Kelvin (“absolute zero”), but not in Celsius, where negative degrees are freezing temperatures.

wimleers commented 4 months ago

It seems that ever since installing this branch (since https://github.com/naofireblade/homebridge-weather-plus/pull/285#issuecomment-1962003233), my temperature history, light level and air pressure history are no longer updating? 🤔

image image

wimleers commented 4 months ago

Update: works fine now, with no missing history 🤔 Seems like a glitch in the Eve app? I did restart it before reporting 🤷

dacarson commented 4 months ago

20c96ea specified 0 as the minimum temperature. That’s true in Kelvin (“absolute zero”), but not in Celsius, where negative degrees are freezing temperatures.

I realized this too, yesterday. I changed it in my internal code to -100 minimum. It'll be fixed in the next PR.

wimleers commented 4 months ago

Update from Weatherflow support:

Thanks! So I did forward this to our data engineers and it seems like the sensor flags didn't clear up properly when the Power Booster was installed. They are looking to see if they could fix this issue in a new firmware.

🎉

dacarson commented 4 months ago
  1. Format the sensor log entry better, add commas between failures

Added commas between each failing sensor in error string used in the log.

20c96ea specified 0 as the minimum temperature. That’s true in Kelvin (“absolute zero”), but not in Celsius, where negative degrees are freezing temperatures.

I realized this too, yesterday. I changed it in my internal code to -100 minimum. It'll be fixed in the next PR.

It is fixed here (https://github.com/naofireblade/homebridge-weather-plus/pull/285/commits/c2909f3a47bc269e35d3aba2fd09b36f4fa84522)

dacarson commented 4 months ago
  1. Make device_status more robust, and validate serial_number against observation before applying info from it.

I actually took care of this in (https://github.com/naofireblade/homebridge-weather-plus/commit/b99b173f7ce7e9abe9ca7b1d41ad9e329dd59086) where I separated the error handling of AIR vs SKY.

While testing with separate SKY/AIR units, I found that the battery calculations are wrong:

[2/26/2024, 11:00:52 AM] [homebridge-weather-plus] This plugin generated a warning from the characteristic 'Battery Level': characteristic was supplied illegal value: number -17 exceeded minimum of 0. See https://homebridge.io/w/JtMGR for more info.
[2/26/2024, 11:01:52 AM] [homebridge-weather-plus] This plugin generated a warning from the characteristic 'Battery Level': characteristic was supplied illegal value: number -16 exceeded minimum of 0. See https://homebridge.io/w/JtMGR for more info.
[2/26/2024, 11:02:52 AM] [homebridge-weather-plus] This plugin generated a warning from the characteristic 'Battery Level': characteristic was supplied illegal value: number -16 exceeded minimum of 0. See https://homebridge.io/w/JtMGR for more info.

So I need to add one more change to this fork.

  1. Fix SKY/AIR battery calculations so that they must be within 0 - 100 % and as there can only be one battery level, report the lowest value. As the user cares about low battery.
dacarson commented 4 months ago
  1. Fix SKY/AIR battery calculations so that they must be within 0 - 100 % and as there can only be one battery level, report the lowest value. As the user cares about low battery.

Fix (https://github.com/naofireblade/homebridge-weather-plus/pull/285/commits/6f2e152fbdfc656c2dd1d35d4e0a2dde9df0fc69). Remove partial duplicate handling of battery level from device_status message, and put all battery handling in the observation handling code. This was already the case for Tempest, just not the case for AIR/SKY sensors. As there can only be one battery level reported, but we may have AIR & SKY units, pick the lowest value and report it. Also switch the battery calculations for AIR/SKY to be the same as Tempest as the ranges and values seem close enough in observation.

dacarson commented 4 months ago

@wimleers all feedback has been applied 😀 Please try out this version and hit me up with questions and/or feedback.

wimleers commented 4 months ago

Deployed and testing 😊 Will report back in ~24 hours!

wimleers commented 4 months ago

Good news from Weatherflow support:

We wouldn't require you to perform a manual installation for a beta firmware. Once we fixed the issue, we should update the firmware for your Tempest device and the sensor flags should disappear. I apologize for the inconvenience!

wimleers commented 4 months ago

Deployed and testing 😊 Will report back in ~24 hours!

All is well!

Only one odd thing: the Observation time is seemingly not updating — it's still saying 00:00:15 while it's 19:25. That suggests the forecast is not being updated?

dacarson commented 4 months ago

Only one odd thing: the Observation time is seemingly not updating — it's still saying 00:00:15 while it's 19:25. That suggests the forecast is not being updated?

If everything else is updating, the I wonder if it is a glitch in the Eve app. There is no reason for it to not update.

wimleers commented 4 months ago

Only one odd thing: the Observation time is seemingly not updating — it's still saying 00:00:15 while it's 19:25. That suggests the forecast is not being updated?

If everything else is updating, the I wonder if it is a glitch in the Eve app. There is no reason for it to not update.

Worse, it doesn’t update at all anymore! I bet the recent refactoring broke it. Or the config key was renamed and I missed the log entry. Restarting.

wimleers commented 4 months ago

Nothing:

545577181065706 high at 231.106451200303
[2/28/2024, 4:13:47 PM] [homebridge-weather-plus] Restarting child bridge...
[2/28/2024, 4:13:47 PM] Got SIGTERM, shutting down child bridge process...
[2/28/2024, 4:13:52 PM] [homebridge-weather-plus] Child bridge process ended
[2/28/2024, 4:13:52 PM] [homebridge-weather-plus] Process Ended. Code: 143, Signal: null
[2/28/2024, 4:13:59 PM] [homebridge-weather-plus] Restarting Process...
[2/28/2024, 4:13:59 PM] [homebridge-weather-plus] Launched child bridge with PID 14905
[2/28/2024, 4:14:00 PM] Registering platform 'homebridge-weather-plus.WeatherPlus'
[2/28/2024, 4:14:00 PM] [homebridge-weather-plus] Loaded homebridge-weather-plus v3.3.3 child bridge successfully
[2/28/2024, 4:14:00 PM] Loaded 0 cached accessories from cachedAccessories.0EC69EE59806.
[2/28/2024, 4:14:00 PM] [homebridge-weather-plus] Adding station with weather service TempestAPI named 'Now'
[2/28/2024, 4:14:00 PM] [homebridge-weather-plus] Restoring last readings
[2/28/2024, 4:14:00 PM] Initializing platform accessory 'Now'...
[2/28/2024, 4:14:00 PM] Initializing platform accessory 'Today'...
[2/28/2024, 4:14:00 PM] Initializing platform accessory 'In 1 Day'...
[2/28/2024, 4:14:00 PM] [homebridge-weather-plus] server listening 0.0.0.0:50222
[2/28/2024, 4:14:00 PM] Homebridge v1.7.0 (HAP v0.11.1) (homebridge-weather-plus) is running on port 32685.
[2/28/2024, 4:14:01 PM] [homebridge-weather-plus] This plugin generated a warning from the characteristic 'Air Pressure': characteristic was supplied illegal value: number 0 exceeded minimum of 700. See https://homebridge.io/w/JtMGR for more info.
dacarson commented 4 months ago

If you continually see this line appearing in your logs:

[2/28/2024, 4:14:01 PM] [homebridge-weather-plus] This plugin generated a warning from the characteristic 'Air Pressure': characteristic was supplied illegal value: number 0 exceeded minimum of 700. See https://homebridge.io/w/JtMGR for more info.

It is because the plugin has not received any data from the weather station. (The error occurs as I initialize the Air Pressure value to zero, and is only changed when it receives some data from the weather station). I do expect it could appear once on startup because no data has been received yet, but the plugin has requested initial values.

To verify that the plugin is receiving data, you can change line 149:

this.log.debug(`Server got: ${message.type}`);

to this

this.log.debug(`Server got: ${msg.toString()}`);

FWIW, with separate AIR/SKY sensors, everything is working fine for me.

wimleers commented 4 months ago

I figured out why forecasts aren't updating:


/Users/admin/Desktop/homebridge-weather-plus/apis/weatherflow.js:637
                    callback(parseError, weather);
     ^
TypeError: Cannot read properties of undefined (reading 'time')
    at /Users/admin/Desktop/homebridge-weather-plus/apis/weatherflow.js:222:74
    at Request._callback (/Users/admin/Desktop/homebridge-weather-plus/apis/weatherflow.js:637:6)
    at Request.self.callback (/Users/admin/Desktop/homebridge-weather-plus/node_modules/request/request.js:185:22)
    at Request.emit (node:events:514:28)
    at Request.<anonymous> (/Users/admin/Desktop/homebridge-weather-plus/node_modules/request/request.js:1154:10)
    at Request.emit (node:events:514:28)
    at IncomingMessage.<anonymous> (/Users/admin/Desktop/homebridge-weather-plus/node_modules/request/request.js:1076:12)
    at Object.onceWrapper (node:events:628:28)
    at IncomingMessage.emit (node:events:526:35)
    at endReadableNT (node:internal/streams/readable:1589:12)
[2/28/2024, 10:00:02 PM] [homebridge-weather-plus] Child bridge process ended
[2/28/2024, 10:00:02 PM] [homebridge-weather-plus] Process Ended. Code: 1, Signal: null
[2/28/2024, 10:00:09 PM] [homebridge-weather-plus] Restarting Process...
[2/28/2024, 10:00:10 PM] [homebridge-weather-plus] Launched child bridge with PID 75453
wimleers commented 4 months ago

To verify that the plugin is receiving data, you can change line 149:

Data is being received. The Eve history view for Air Pressure works fine. 👍

But I'm seeing occasional drops it seems:

image
dacarson commented 4 months ago

I figured out why forecasts aren't updating:

Can you share your JSON config?

This error: TypeError: Cannot read properties of undefined (reading 'time') happens when it is trying to load the forecast, but the returned JSON is not a forecast. I thought I had fixed that by checking the http status code before trying to read the forecast (https://github.com/naofireblade/homebridge-weather-plus/commit/db03a9ecfaf8a8b57d13a77e6e8538bd80431a80), as well as not requesting the forecast if the apiKey and/or stationID was not provided.

Obviously, it isn't robust enough. To understand why it is failing your JSON would help.

wimleers commented 4 months ago

I figured out why forecasts aren't updating:

…

I suspect https://github.com/dacarson/homebridge-weather-plus/commit/db03a9ecfaf8a8b57d13a77e6e8538bd80431a80 caused this regression? (I find node.js backtraces notoriously difficult to read, due to its infamous "callback hell".)

Related: it might be worth doing a this.debug.info(…) whenever the forecast is updated — that'd result in 24 log entries per day, that seems reasonable? I'd suggest Successfully fetched a new forecast … for a 200 response and a this.log.error(…) with Failed to fetch new forecast. Got response with status code 500 containing: <raw response body> for any 4xx or 5xx response? A non-200 response should also cause a Status Fault for a forecast accessory?

dacarson commented 4 months ago

But I'm seeing occasional drops it seems:

My initial thought is that the value is reset, but I never actually reset a value. If a new value has not be received, then I just send the previous value. The only time the value is 'reset' is when the plugin is restarted and it has not received any data from the weather station yet. Thinking about this further, what I could do in this case is return an error when the data is requested. When a weather station returns an error, it doesn't update the values and it doesn't update the history.

wimleers commented 4 months ago

Can you share your JSON config?

Sure! Can you e-mail me at mail@ my name dot com? (The personal access token is for … personal access so I don't want to violate any Weatherflow ToS.)

but the returned JSON is not a forecast

Is it not just a non-JSON response? The logging I suggested in the comment I just posted sure would make that trivial to see, and would make for much more useful bug reports 😊

wimleers commented 4 months ago

The only time the value is 'reset' is when the plugin is restarted and it has not received any data from the weather station yet.

It's totally possible that this edge case occurs whenever A) there is an intentional Homebridge (child bridge) restart, B) it just happens to be within that 59 second time window where no data has been received yet, C) a crash like the one we're discussing in the surrounding comments is causing such restarts! 😄 🤓

dacarson commented 4 months ago

Can you share your JSON config?

Sure! Can you e-mail me at mail@ my name dot com? (The personal access token is for … personal access so I don't want to violate any Weatherflow ToS.)

Thanks.

but the returned JSON is not a forecast

Is it not just a non-JSON response? The logging I suggested in the comment I just posted sure would make that trivial to see, and would make for much more useful bug reports 😊

I get what you are saying for the logging but it should already be doing this. In the function getForecastData(callback) (line 612) I have if (response.statusCode == 200) { the response is ok, so parse the JSON and return it. } else { (line 638) return that an error occurred and the body that it received. Then in update(forecastDays, callback) it handles the return from getForecastData(callback) on line 221. If no error occurred, then process the result (line 222). If an error did occur, then log it:

this.log.error("Error retrieving weather Forecast");
this.log.error(result);

So, if anything but 200 response code is returned, I should just print this and move on.

In your case, you must have got a 200 response code back, but it is missing the time field in the JSON, which I don't get. BTW It is the time field that I use to set the Observation Time for the forecast.