pimoroni / enviro

MIT License
101 stars 79 forks source link

Add sea level pressure calculation #159

Open sjefferson99 opened 1 year ago

sjefferson99 commented 1 year ago

This addresses #158

Added two new parameters in config.py to adjust reported pressure to calculated sea level pressure and if enabled, what height the weather station is above sea level.

I believe I have correctly followed the config file backwards compatibility approach.

The code is only applied to the weather board as I only have that available to test, but the calculation is performed in the helpers.py file and I can add the conditional code to report the adjusted value to other boards if others would like to test in this PR.

I have confirmed that defaults are correctly applied if config.py is not correctly populated and that the calculated values match the source website calculator and synoptic chart expectations. There are log info messages to assist here, which appear when the sea level option is enabled.

I would like to add the option and height configuration to the provisioning code, but would need some design guidance on whether this is added to the nickname page (which makes the page title inaccurate) or adding a new intermediate page either with a x.5 title number or refactoring all pages to introduce a new number midway.

All feedback welcome.

sjefferson99 commented 1 year ago

Added new key for sea level pressure, so it's always clear whether the pressure is observed or adjusted should the data need to be one of them specifically downstream such as a weather service submission.

Gadgetoid commented 2 months ago

This might be a little out of scope for Enviro, and something better dealt with downstream- although if the service requires compensated values (to match expectations or other data sources?) I'd guess we'd have to compensate here.

Either way, I'd suspect if you need this you'll be able to edit the config.py, so I wouldn't complicate the provisioning code. It's already a pain having a full-screen page on my mobile device with no easy access to - for example - copy and paste an API key from Adafruit IO.

This needs rebasing if you have the time/inclination, please!

sjefferson99 commented 2 months ago

@Gadgetoid Happy with your steer on target audience and UI complexity, so by all means let's leave it out of provisioning. Providing there are clear docs (I am hoping past me did a reasonable job and will check), then modifying config.py should be fine.

I only use Weather Underground as a destination as it's easiest for me and they require sea level pressure. Am hoping you get as far as my wunderground PR for main inclusion, as my testing at the moment on current release doesn't have a destination I can use so is just storing locally, which isn't stressing the wireless enough to be confident.

I will rebase ASAP this week.

Gadgetoid commented 2 months ago

Am hoping you get as far as my wunderground PR for main inclusion

I hadn't seen it, but if you rebase it should kick the CI and produce a (hopefully) working build against the latest firmware and we can go from there.

I'm trying to pick the right balance of feature-creep vs useful-features, but since Chris and Jon are both distracted I can probably get away with cramming everything into a YOLO 1.0.0 release before they notice 😆

It might actually be prudent to merge this PR and your Wunderground one, since they are closely related. That will make the CI produce builds that we can use to test both together and maybe work toward an interim release with these features.

sjefferson99 commented 2 months ago

@Gadgetoid I'll take a look at my PRs and see which are related to wunderground, will add some comments and tag you, you can then work out a release strategy, happy to merge stuff up if it makes more sense. I am also in favour of YOLO when I am not product owner.

Will rebase all my PRs where possible.

sjefferson99 commented 2 months ago

@Gadgetoid Looks like past me had us covered, #165 already has the dependent sea level and rain per day/hour branches merged in. I will rebase the wunderground branch and we can probably just take that one forward. I will also rebase the other two branches for completeness, but hopefully they can be dropped in favour of the combined branch.

sjefferson99 commented 2 months ago

@Gadgetoid rebased.