-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Ambient Weather support #297
base: master
Are you sure you want to change the base?
Conversation
Any idea of when this update might be available? I'm excited to give it a try and stop using Weather Underground for this! |
It looks like different apis implementations handle this differently. OWM and WeeWX are using the timezone of the weather location. WU and Weatherflow are using the device timezone. I could make arguments for both variants :) . What do u prefer? |
It appears that My current PR wasn't setting the timezone at all, just using UTC. I'll update it to use the weather station timezone. |
Is there a way for me to test this new code? I'm currently still running on v3.3.3 because 3.3.5 broke the wind speed calculation from Weather Underground and hasn't been fixed yet (open issue). I'd prefer to have this direct integration with Ambient Weather. |
Now something is broken with my station and Weather Underground. I'd really like to have this new update to have native Ambient Weather support. @dhull @naofireblade Do you expect to have this code live soon? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments.
|
||
this.apiKey = apiKey; | ||
this.appKey = appKey; | ||
this.locationId = locationId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look like it is being used. Maybe change your constructor to not send this parameter.
'WindSpeed', | ||
'WindSpeedMax' | ||
], | ||
this.units = 's'; // Will report in SI units. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'units' isn't used in your code. I would suggest removing this line.
// HomeKit expects SI units. | ||
|
||
const report = { | ||
ObservationStation: json.macAddress, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why use the macAddress of the station here? According to your comments above, you have the json.info.location has the location of the station.
WindDirection: converter.getWindDirection(data.winddir), | ||
WindSpeed: mphToMps(data.windspeedmph), | ||
WindSpeedMax:mphToMps(data.windgustmph), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to this document: https://github.com/ambient-weather/api-docs/wiki/Device-Data-Specs
It looks like there is a RainEvent, which I suspect indicates if it is currently raining. You may want to add support for it, ie RainBool: data. eventrainin > 0 ? true:false,
As well as BatteryLevel
and LightningStrikes
.
const inchHgToHpa = (inch_hg) => inch_hg * 33.863886666667; | ||
const fahrenheitToCelsius = (f) => (f - 32) * 5 / 9; | ||
const inchToMm = (inch) => inch * 25.4; | ||
const mphToMps = (mph) => mph * 0.4470389; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to eventually move these into the converter.js file.
It also has a function in there to generate the WetBulb temperature based on temperature and humidity.
@@ -84,6 +85,10 @@ function WeatherPlusPlatform(_log, _config) | |||
this.stations.push(new tempest(config.key, config.locationId, config.conditionDetail, this.log, HomebridgeAPI.user.persistPath())); | |||
this.interval = 1; // Tempest broadcasts new data every minute, forecasts are limited to once per hour | |||
break; | |||
case "ambientweather": | |||
this.log.info("Adding station with weather service AmbientWeather named '" + config.nameNow + "'"); | |||
this.stations.push(new ambientweather(config.key, config.appKey, config.locationId, config.conditionDetail, this.log, HomebridgeAPI.user.persistPath())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe remove locationId
from constructor. If you do, you should also hide it in the config.schema.json file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
This is an update of my previous PR (#54) to add Ambient Weather support to homebridge-weather-plus. Sorry that it has taken me so long to update the PR for the changes to homebridge-weather-plus, but at the time I was forced to continue to use an older version of Homebridge by a different plugin and then I moved and didn't use Homebridge at all for several years.
I have a question about whether my code is properly setting the station's
ObservationTime
value. As far as I can this this characteristic is the observation's time inHH:MM:SS
format in the time zone that Homebridge is running in. Is that correct?