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 rain per hour and day results #169

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

Conversation

sjefferson99
Copy link
Contributor

To address #168 adding readings for rain per hour and rain today.

rain_per_hour: sum of rain logged in the last 60 minutes
rain_today: rain logged since midnight

Removed the line to delete the rain file after taking a reading.
Added two IFs to count timestamps for the last hour and for today
Only write back values to the file from today as older values no longer needed.

I created a new function log_rain() to reduce the duplication in startup() and check_trigger() as I thought I would update that block. But as we will check much more regularly than daily, there was no need to remove older readings here, but I kept the function anyway.

I have tested by grounding the rain pin and manipulating the rain file to make entries appear 2 hours ago and yesterday and works as expected. Will run the code in the wild to try and catch some real rain over time.

@sjefferson99
Copy link
Contributor Author

Added logic to capture data between readings that falls before midnight when the reading is after midnight. In theory this should really be backdated to 23:59:59 the previous day, but the complexity of writing that out to the readings file from the rainfall function didn't seem worth a slight inaccuracy in capture time to ensure that all rain was caught.

@sjefferson99
Copy link
Contributor Author

Weather underground highlighted that rain today needs to be captured in local time to make sense. As Micropython does not have the pytz module I have added a simple manual offset in config.py to account for other time zones and DST. I may add code to calculate BST with a config flag to reduce the manual effort in Britain without addressing all the other DST approaches.

The new code adjusts the contents of rain.txt to reflect local midnight, but the timestamp for uploads is still UTC (Z).

@ZodiusInfuser ZodiusInfuser added the enhancement New feature or request label Jul 10, 2023
@sjefferson99
Copy link
Contributor Author

Added simple function to lookup UK BST until 2030 and supporting config option with defaults.

@gbravery
Copy link

gbravery commented Aug 29, 2023

Feel I need to leave a comment - I've been running this code for a while (not added the BST aspect yet - so thanks for that!!).
I've had a funny error - i think it may have happened a couple of times.

The timestamp leapt to 2040 - "2040-01-01T00:00:05Z"
This results in an error:
timestamp_day("2040-01-01T00:00:05Z",1)
Traceback (most recent call last):
File "", line 1, in
File "", line 5, in timestamp_day
OverflowError: overflow converting long int to machine word

I've done a kludge for now:
def timestamp_day(dt, offset_hours):
time = timestamp(dt)
time = time + (offset_hours * 3600)
if time < 2147483648:
dt = utime.localtime(time)
day = int(dt[2])
else:
day = 0
return day

Obviously, the real issue is "why did my timestamp become 2040...?"

@sjefferson99
Copy link
Contributor Author

@gbravery glad to hear the code is working (mostly) well for you.

I've had a very quick google and I would guess the error is a result of exceeding a 32 bit limitation of some time library functions that basically stops works working after 2038. I assume the micropython libraries are written this way and will be updated some time before 2038, which stop this problem occurring. As such I don't believe there is anything to adjust in this PR, but do let me know if you think I should be using a more capable existing micropython library function to prevent this now should one exist, I only looked into this briefly.

I would recommend your kludge slices out the 2040 and replaces with 2023 rather than returning 0 to ensure your rain readings aren't totally confused on the assumption we can fix the timestamp issue in the next 4 months (or leave a one line to update if not).

Sounds like there does need to be an issue open for how your timestamp is being set to 2040 as you say above. I'll keep an eye out if you raise that with log files to see if I can spot anything.

@MrDrem
Copy link

MrDrem commented Aug 29, 2023

This set of commits should not be merged into the main branch.

BST should not be used for reporting times, times should be kept constant (so UTC is ideal as now, but otherwise GMT, or local equivalent).
As per issue #70 Rain is currently correctly reported as per the Met office reporting requirements.

Time adjustments and data concatenation should done on the reporting side, not on the data gathering side.

@sjefferson99
Copy link
Contributor Author

MrDrem - (I can't seem to find you in the @ list) Either you have misunderstood the intent of the PR or I have made an error in delivering that intent, please do let me know if the below is not what you are seeing in the code or I have missed something:

The existing Rainfall reading should be untouched (the new code calculates the delta from a larger log file instead of clearing previous readings to allow the extra calculations to be made) and show the mm since the last reading in UTC and I wouldn't want to adjust anything away from the MET office reporting requirements, as you have suggested.

The additions are for rain per hour and rain per day, which I have added in the enviro firmware instead of leaving the user to calculate from the data recorder as the weather underground destination in #165 requires these values to be submitted. So these are extra readings leaving the original untouched. If the extra readings are problematic, I could disable these unless the weather underground destination is selected and merge this PR into the Wunderground PR.

BST calculations are introduced only to ascertain the local midnight for the rain/day calculation, as I found my rain readings had a gap on weather underground when we hit summer. It is not expected that the BST calculation would be used to adjust the timestamp for reading submission, even weather underground expects these in UTC, but does adjust the local midnight far rain/day.

Let me know if any of the above aren't actually happening correctly in code or if further work is needed to properly support weather underground without impacting existing MET compliant logging either here or in #165.

@sjefferson99 sjefferson99 force-pushed the rain-per-hour-and-day branch from 3420d20 to b56bf6f Compare April 25, 2024 18:18
@sjefferson99
Copy link
Contributor Author

@Gadgetoid Rebased.

@Gadgetoid
Copy link
Member

Appreciated, thank you!

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.

5 participants