starkillerOG / reolink_aio

Reolink NVR/camera API PyPI package
MIT License
65 stars 13 forks source link

Add timezone handling support to API #24

Closed xannor closed 1 year ago

xannor commented 1 year ago

This pull adds three methods to the API and one class to typings.

get_timezone() will return a timezone parsed from the GetTime Dst rules, if dst is enabled, otherwise it will create just a regular timezone off of the Time/offset property. The Dst rules provide the start and end range for when to apply dst for a given year. allowing for correct time adjustment between utc/other timezones. If there is no time data loaded in the host, this will return None

get_time() will return the local systems current time, adjusted and assigned to the timezone from get_timezone. If there is no time data loaded in the host, this will return None

async_get_time() is just a wrapper that will execute GetTime on the host, and throw if it cannot retrieve time, and then returns the time as get_time() but uses the response from the GetTime command instead of adjusting system time. This will ensure an accurate time, if it is needed, or what it is more usefull for is ensure GetTime is called if get_time returns None.

The Reolink_timezone class is an implementation of datetime.tzinfo, implementing the required methods and caching the rule calculations. I tried to optimize it to cache as much as possible so it can be as performant as possible, and so code using it does not have to be aware of or care about the caching. It is effectively an immutable class that is shared with all instances, so if multiple device api's are loaded and all have the same DST rules, only one timezone object should be created for those rules.

What this allows for, is for the local times of a device to be translated to utc, then back to the localtime of the system, or in the case of HomeAssistant, the localtime of the browser visiting. Without the Dst rules, the time would be an hour off when the visitor is in DST and the VODs being looked at are from a Standard Time range.

Hopefully this is a better explanation than the last one, let me know if there are any changes or clarifications you need.

xannor commented 1 year ago

I just saw you comment on the prior pull about not using get_ for non-IO calls. I have no problem with making that change I will make that change and see if you have any others before I push an update.

starkillerOG commented 1 year ago

@xannor Thank you for your contribution and this Pull Request, I think it looks good now. Also thanks for the more detailed explanation in this PR description. Sorry for taking so long, had some catching up to do after my vacation.

If you fix the mypy errors, this is ready to be merged.

xannor commented 1 year ago

I am rejecting the change to async_get_time, its purpose is to "always" fetch the time again, otherwise time() should be used. It uses the value of _time_settings as the current time of the device, which would be stale after a few seconds.

starkillerOG commented 1 year ago

@xannor alright, that is fine. Please fix the mypy errors (see tests), once those are fixed this can be merged.

xannor commented 1 year ago

added mypy fixes, it interesting that some of the things mypy complains about, pyright does not so there are some issues I never thought of. I also ditched the new overload, though it was "creative" and "technically" follows specs, having the new return a different type than the class is something mypy is in hot debate over so I just returned it to a factory method.

starkillerOG commented 1 year ago

@xannor thanks for your work on this, it is now merged. Let me know if you require a new version to be published to pypi. But I think you will need more followup PRs right?

xannor commented 1 year ago

For now I am working against the repo so I dont need a new version pushed yet (I'm just manually changing the reolink manifest from = to >= and using pypi install -e to use the dev environment) though now I can cleanup the changes I have relating to the VOD file stuff and start prepping that for PR.