Skip to content
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 sea level pressure calculation #159

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sjefferson99
Copy link
Contributor

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
Copy link
Contributor Author

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.

@ZodiusInfuser ZodiusInfuser added the enhancement New feature or request label Jul 10, 2023
@Gadgetoid
Copy link
Member

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!

@Gadgetoid Gadgetoid added this to the 1.0.0 milestone Apr 24, 2024
@sjefferson99
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@Gadgetoid rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants