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

Random timezone change due to abstractapi account deletion? #36

Open
FergyA opened this issue Jan 21, 2023 · 9 comments
Open

Random timezone change due to abstractapi account deletion? #36

FergyA opened this issue Jan 21, 2023 · 9 comments
Labels
enhancement New feature or request

Comments

@FergyA
Copy link

FergyA commented Jan 21, 2023

First of all just wanted to say I love the project! From the moment I saw that the EleksTube IPS ran on an ESP32 I knew I wanted to try to add features like NTP and night dimming, but I wasn't confident I could with my knowledge level and time. I was super excited to see that yall beat me to it and made it so easy to follow.

I flashed my clock with your firmware back in June of '22 and it's been more or less flawless since, but recently I'm having trouble with the clock seemingly changing time zones randomly. (As in the minutes are still accurate, but the hour is incorrect). If I unplug the clock and plug it back in it seems to correct the issue, but only for a day or so. I decided to check on my abstractapi account to see if it was still registering geolocation API requests correctly only to find that my account no longer exists, and the API key I was using gives the error below.

{"error":{"message":"Invalid API key provided.","code":"unauthorized","details":null}}

I suspect this may be the cause of the desync, however I'm not sure why unplugging the clock and plugging it back in temporarily corrects the problem if that's the case. I would think if this error were being interpreted as an incorrect time zone that would also be true when the clock is freshly booted as well.

With that, I'm trying to find a way to fix this long term, so a) do you have any ideas why my abstractapi account may have been deleted (and with no notification or anything) b) can you think of why the firmware is interpreting a geolocation error the way that it is (and could anything changed in the last 6 months correct this?), and c) is there a way that the firmware can be made resilient to this? Perhaps having some default hardcoded timezone option as backup in case the geolocation fails? That way I don't need to reflash my clock with a new API key every 6 months if my abstractapi account continues to be an issue. Alternatively perhaps a way to change the geolocation API key without a reflash would be useful (say through a configuration webpage or MQTT).

TIA

@FergyA FergyA changed the title Random NTP desync due to abstractapi account deletion? Random timezone change due to abstractapi account deletion? Jan 21, 2023
@aly-fly
Copy link
Collaborator

aly-fly commented Jan 21, 2023

Just recently I got an email from AbstractApi, they were checking if I am still alive, because I haven't signed into the accout for a long time. Clicked the link in the email and everything works fine, my subscription was extended. Probably you missed that email. Or payment plan with them would also resolve this :) Yeah code could be updated to expect also this and keep same time zone as before. Btw, you can also disable AbstractApi and use timezone setting in the clock's menu.

@SmittyHalibut
Copy link
Owner

Honestly, I'd probably just use an EEPROM field to store the received time zone, and default to that if one isn't obtainable anymore for whatever reason.

Having said that, I'm not at a place where I can implement this, so consider this an unsolicited suggestion from some rando on the Internet, and take it with a grain of salt. ;-)

@FergyA
Copy link
Author

FergyA commented Jan 21, 2023

Ah hah, I totally missed that that's an option (skipping geoloc entirely). Perhaps take this issue as a suggestion to make that clearer in the readme then. I followed the directions to the letter and didn't enable MQTT since it was strictly noted as optional, but thought geolocation was a requirement. (Although I see now in the latest code that the geoloc API key is commented out by default, so perhaps that would have made me investigate).

As for missing the renew email I suspected it may have been something like that... I shovel services like this onto an email from a certain 3 letter acronym email service that's mostly popular with old people, and they're notorious for just flat out not delivering emails like this... (not even to the spam folder). I guess if I want to use it long term with geolocation (which let's be real I really don't... since it's probably going to stay in this timezone forever) I should use a proper email service to register for the API. (Or as you say, pay the subscription).

Anyway thank you for the help and quick responses. It always astounds me how useful the issues sections of various github projects are!

I'll leave this open in case you want to use this as impetus to refine the readme, but otherwise consider this resolved on my end!

@SmittyHalibut
Copy link
Owner

I think your comment about it losing its mind when the API call doesn't work is a valid one. This ticket should be to make that code more robust. AND to update the documentation to make it more clear. :-)

@FergyA
Copy link
Author

FergyA commented Jan 21, 2023

It looks like the current code actually won't compile with the MQTT_CLIENT and GEOLOCATION_API_KEY lines commented out as they're defaulted in USER_DEFINES.h. MQTT stays disabled since that's reliant on the MQTT_ENABLED flag, but it doesn't look like there's an equivalent for geoloc. (There was in previous versions, but I don't think it actually did anything and it no longer appears to be there as of the migration of all the settings to USER_DEFINES). I can leave the api key blank, but I'm guessing that means it's still hitting the service and could put me right back where I started.

I'm going to go ahead and register an abstractapi on my proper email account and solve this that way, but take this as a suggestion to properly allow disabling of geolocation.

@aly-fly
Copy link
Collaborator

aly-fly commented Jan 23, 2023

Just a comment on usage of Timezone information... It is true that there is a very slim chance of clock moving into different time zone. However Daylight Saving Time (DST) changes happen twice every year and on different days around the world. So there are 3 options:

  • Let user deal with it and change manually (not optimal, this is how it was in the first version).
  • Implement information / API that learns on which day DST changes are in current location. Info would be obtained on startup only (or input by user - clumsy).
  • Get time offset every night, as it is implemented now.

I thought of all of this and decided to implement 3rd option. I agree that current error handling is not good, hope to have a free time slot any time soon to fix it :)

@FergyA
Copy link
Author

FergyA commented Nov 18, 2023

Bit of a necro, but I was maintaining my AbstractAPI account (to try and avoid the issue that caused this thread) and discovered they've changed their terms.

Apparently their free account no longer refreshes monthly, and is now limited to 1000 api pulls period over the lifetime of the account. Admittedly 1000 pulls is ~2.75 years of usage at 1 pull a day, but that means you need to make a new account and reflash the clock firmware every 2.75ish years (since this is the only way to change the API key) to keep it working. A paid account isn't really an option either at $100/yr or $49/month.

Seems this might be motivation to implement a means of disabling geolocation if this is ever revisited. Yes it means losing easy auto DST, but maybe this could be implemented another way. A toggleable DST option in the menus that just offsets the time by 1hr would probably be simplest.

@aly-fly
Copy link
Collaborator

aly-fly commented Nov 20, 2023

Manual time zone & DST is already implemented. Scroll through the clock's menu.

Better API calls error handling must be implemented... I haven't run into this yet, so didn't take time to look into this...

Yeah I also think about what to do with my "api points". Maybe they can be topped up by making subscription once and cancelling next month? Somebody has to test this.

@FergyA
Copy link
Author

FergyA commented Nov 20, 2023

Ah hah, I totally overlooked the DST offset. Thank you! I think there's still going to be an issue using it when the abstract account runs out of points though if the geolocation still can't truly be disabled and the error handling is still buggy. (Meaning the original bug I saw with the clock randomly changing time zones probably will be triggered again).

Even if the error handling is fixed it still seems wasteful to hit an API daily that you know you can't use. I remember digging into the code when I started having issues and the geoloc disable variable in the config file wasn't actually read by anything. I tried adding a simple logic loop around the geoloc code, but ran into a chain of resulting build errrors that I chased a few layers deep, but it was getting messy so I gave up. (I'm a mechanical engineer... coding isn't one of my talents unfortunately).

Anyway, just wanted to point out that looking at the geoloc disable code is probably lower hanging fruit (or at least easier to debug) and would also solve this problem. Know I totally don't have the right to make demands, so just wanted to reiterate my thanks for your work on this project.

@aly-fly aly-fly added the enhancement New feature or request label Jan 18, 2024
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

No branches or pull requests

3 participants