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

Sunset shows 00:00 with certain longitude values. #3601

Closed
1 task done
zowper opened this issue Dec 20, 2023 · 13 comments · Fixed by #3612
Closed
1 task done

Sunset shows 00:00 with certain longitude values. #3601

zowper opened this issue Dec 20, 2023 · 13 comments · Fixed by #3612
Assignees
Labels
bug fixed in source This issue is unsolved in the latest release but fixed in master

Comments

@zowper
Copy link

zowper commented Dec 20, 2023

What happened?

A few days before December 7th (and including December 7th), if the longitude was set to 11.74, the sunset time displayed "00:00" and the sunset scheduling didn't work properly. If I changed the longitude to 111 flat, it worked properly.

A few days ago (around December 18th) 111 stopped working as well, but 110 now works. It appears to be a calculation error that changes over time.

To Reproduce Bug

As of last night, the bug can be recreated by setting the latitude to 40.40 and the longitude to 111. The exact numbers that reproduce the error will likely change over time. After setting that latitude and longitude and clicking "Save", you should see the sunset time display as "00:00".

Expected Behavior

It should calculate an actual sunset time for any valid latitude and longitude (which should be around 17:00 in this case for locations in Utah, USA).

Install Method

Binary from WLED.me

What version of WLED?

0.14.0

Which microcontroller/board are you seeing the problem on?

ESP32

Relevant log/trace output

No response

Anything else?

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@zowper zowper added the bug label Dec 20, 2023
@blazoncek
Copy link
Collaborator

Duplicate of #3400 and possibly others.
This may be math rounding error. /cc @softhack007

@softhack007
Copy link
Collaborator

softhack007 commented Dec 21, 2023

@blazoncek I found this in wikipedia, is it the same calculation as in WLED? The wikipedia formula also seems to work above the arctic circle.

https://en.m.wikipedia.org/wiki/Sunrise_equation

@blazoncek
Copy link
Collaborator

I have no idea where I borrowed that algorithm from (perhaps some NOAA resource). Not from Wikipedia though.

@softhack007
Copy link
Collaborator

softhack007 commented Dec 21, 2023

Could become complicated ...
I think the main numerical instability comes from using "tan". tan(angle) shoots towards infinity for angle values close to 90deg and 270deg. So a small inaccuracy is enough to "tip the boat over" and the math starts to produce wrong numbers. I don't think we can cure it by using double instead of float.

https://github.com/Aircoookie/WLED/blob/10faaaf531e5f6eb2c00e31cba0f2ecd18ef1fd3/wled00/ntp.cpp#L468-L469

The wikipedia formula avoids tan() and only uses sin() and cos(). So it might be a good alternative.

However some deeper thinking and lots of testing might be needed if we want to improve the calculations in WLED. Looks like something for 0_15, but I don't see a quick fix in 0_14_1.

@softhack007 softhack007 added this to the 0.15.0-alpha candidate milestone Dec 21, 2023
@softhack007
Copy link
Collaborator

softhack007 commented Dec 21, 2023

Update: A quick fix could be to avoid angles where tan() becomes unstable. Something like

if (abs(L-90.0f) < 1.0f)   L = (L<90.0f) ? 89.0f : 91.0f;
if (abs(L-270.0f) < 1.0f) L = (L<270.0f) ? 269.0f : 271.0f;

Maybe the angle "L" should first go through L2= fmodf(L, 360.0f); to stay within the unit circle of 0...360 degrees.

In worst case, this would make the final result wrong by 4-8 minutes.

@softhack007
Copy link
Collaborator

softhack007 commented Dec 21, 2023

@blazoncek maybe we could try my "quick fix" in 0_14_1, activated by a special checkbox like "alternative sunset formula (experimental)". So in normal case the unmodified "proven in use" calculation is used, but we could instruct people with problems to try and set the checkmark that activates my workaround. What do you think?

@blazoncek
Copy link
Collaborator

Agreed to work on it but not for 0.14.1. Perhaps 0.14.2 next month.

The use of sin_t/cos_t and other _t functions was introduced by Aircoookie to reduce RAM requirements for ESP8266, so switching to libc functions may not be a viable solution there as the available RAM keeps decreasing.
Avoiding tan errors by either using a different algorithm or fixing angle may be a viable option though. Angle fixing may still produce incorrect results so perhaps limiting tan output might be better. It would sure need a lot of testing in any case.

Even though these types of errors are annoying they are transient in nature (will go away in a day or two) and are limited to certain combinations of LAT & LON. So I would deem them low priority.

@softhack007
Copy link
Collaborator

softhack007 commented Dec 22, 2023

I've looked at the math again - the critical calculation involving tan only depends on longitude and date. Latitude is used later. I agree it should be a transient "glitch", but it also means that almost every user might have "glitch day" once in a year.

Actually another workaround is possible

  • create a wrapper function around getSunriseUTC()
  • the wrapper calls getSunriseUTC().
  • if result > 0, return result.
  • if result == 0, then return getSunriseUTC() but for the previous day.

This way we avoid touching the math, but still we are able to return a useful approximation if case of error. "No sunrise" should actually not happen unless users are behind the polar circles, which is excluded in the settings page already.

@blazoncek what's your opinion on this two step approach:

  1. I make a PR for 0.14.1, implementing my "use yesterday" idea. It can be done locally (with a few lines of code) in ntp.cpp, and will only have a minor performance impact on "glitch day".
  2. We look into the "wikipedia" formula for 0.15, going for a proper solution that also works below the polar circle latitude.

@softhack007 softhack007 self-assigned this Dec 22, 2023
@blazoncek
Copy link
Collaborator

That would be good. Perhaps just do it without wrapper? Calling recursively?

@blazoncek blazoncek reopened this Dec 22, 2023
softhack007 added a commit that referenced this issue Dec 22, 2023
if case of invalid or impossible sunset/sunrise results, retry with the previous day. max 3 days back, to prevent infinite loops and far-away results.
@softhack007 softhack007 added the fixed in source This issue is unsolved in the latest release but fixed in master label Dec 23, 2023
@zowper
Copy link
Author

zowper commented Dec 23, 2023

Thanks so much everyone for your help! 😁🙏

For what it's worth, it seems like it didn't come on for at least three nights in a row (before I got around to troubleshooting it).

If you implement a recursive function that goes back until it hits a valid day, that could work!

Alternatively, you could simply store a valid sunset time. Then, only allow it to be overwritten if the newly calculated sunset time is non-zero. 😀👍

@zowper
Copy link
Author

zowper commented Dec 23, 2023

Also, storing a valid sunset time (and overwriting it with newly calculated times) seems less likey to crash the system if for some reason it cannot calculate a valid sunset time (i.e. bad input coordinates etc.).

softhack007 added a commit that referenced this issue Dec 23, 2023
workaround for invalid sunrise/sunset times (#3601)
@softhack007
Copy link
Collaborator

softhack007 commented Dec 23, 2023

Thanks so much everyone for your help! 😁🙏

For what it's worth, it seems like it didn't come on for at least three nights in a row (before I got around to troubleshooting it).

Thanks :-)

The problem we found is "transient", meaning it will only last for one or two days, but could repeat one year later.

There are two possible "triggers":

  1. Location is very close to the polar circle, and its a few days before Dec 24th or Jun 21st. In this case WLED is right, there is no sunrise because its arctic day/night.
  2. your local sunrise/sunset time on a specific day is exactly the same as your timezone "UTC offset". For example when you are in UTC-8, and sunrise is 8:00am, you would get 00:00 --> fixed with workaround for invalid sunrise/sunset times (#3601) #3612
  3. for certain angles - 90deg and 270deg - between sun direction and an observers longitude on earth, there is a "math hick-up" and the calculation fails. fixed in workaround for invalid sunrise/sunset times (#3601) #3612 by going back 1,2,or 3 days.

we don't store a previous days value, because the user might enter a new location, or she/he could experience "arctic day/night". Also we don''t search recursively for more than 3 times, because there is a danger of infinite loops and going back too many days will produce meaningless results.

But our fix should improve stability a lot.

@softhack007 softhack007 linked a pull request Dec 23, 2023 that will close this issue
@softhack007
Copy link
Collaborator

softhack007 commented Dec 23, 2023

bad input coordinates

For this scenario, WLED historically follows a "you asked for it, you got it" approach 😉 - also know as "WYGIWYAF what you got is what you asked for"

I understand you want the feature to be as reliable as possible, so we try to reduce errors but we won't try to read user's minds. Makes life easier for us developers 😅

zanhecht pushed a commit to zanhecht/WLED that referenced this issue Dec 27, 2023
if case of invalid or impossible sunset/sunrise results, retry with the previous day. max 3 days back, to prevent infinite loops and far-away results.
JPZV pushed a commit to JPZV/WLED that referenced this issue Jan 18, 2024
if case of invalid or impossible sunset/sunrise results, retry with the previous day. max 3 days back, to prevent infinite loops and far-away results.
DedeHai pushed a commit to DedeHai/WLED that referenced this issue Jan 27, 2024
if case of invalid or impossible sunset/sunrise results, retry with the previous day. max 3 days back, to prevent infinite loops and far-away results.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fixed in source This issue is unsolved in the latest release but fixed in master
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants