Montecri / GNSSTimeServer

WiFi-enabled GNSS (GPS, BeiDou, GLONASS, Galileo) fed NTP/RDATE server based on ESP8266/ESP32 and Arduino
MIT License
136 stars 21 forks source link

Use "timelib.h" #7

Closed cheise closed 1 year ago

cheise commented 1 year ago

https://github.com/Montecri/GPSTimeServer/blob/5a0cee3bb3972d495a00e4791f701c804b7f2d9c/src/main.cpp#L653

Why don't you let "timelib.h" calculate it for you?

timestamp = now() + seventyYears;

so you don't need all the code for the time calculation.

Montecri commented 1 year ago

Sounds good. Will work on this today. Thank you!

Montecri commented 1 year ago

Hi @mmarkin !

Believe you replaced:

timestamp = numberOfSecondsSince1900Epoch(year(t), month(t), day(t), hour(t), minute(t), second(t));

with:

timestamp = CalculateNTP(year(t), month(t), day(t), hour(t), minute(t), second(t));

On Dual Display tree.

@cheise above is proposing what appears to be a more concise and elegant solution.

I've updated the main tree with the suggestion, tested and appear to work ok.

Would like to implement that on the Dual Display tree?

Thank you!

mmarkin commented 1 year ago

Hi Cristiano

Thanks for the suggestions. Yes, I should have realized timelib already has the timestamp. There was no need to calculate it again either by using ziggy's function or the longer version of it that I wrote. All that has to be done is to convert timelib's timestamp from UNIX to NTP by adding 70 years.

Also there was no need for the > 3000 age check when syncing with GPS.

I made the changes to my server and it's still working properly. I also updated the Dual OLED post on GitHub.

Cheers, Mitch Markin

On Mon, Feb 6, 2023 at 1:05 PM Cristiano Monteiro @.***> wrote:

Hi @mmarkin https://github.com/mmarkin !

Believe you replaced:

timestamp = numberOfSecondsSince1900Epoch(year(t), month(t), day(t), hour(t), minute(t), second(t));

with:

timestamp = CalculateNTP(year(t), month(t), day(t), hour(t), minute(t), second(t));

On Dual Display tree.

@cheise https://github.com/cheise above is proposing what appears to be a more concise and elegant solution.

I've updated the main tree with the suggestion, tested and appear to work ok.

Would like to implement that on the Dual Display tree?

Thank you!

— Reply to this email directly, view it on GitHub https://github.com/Montecri/GPSTimeServer/issues/7#issuecomment-1419754201, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVRWOKZXONDV2ETW7HONNTWWFRQTANCNFSM6AAAAAAUSSLAAM . You are receiving this because you were mentioned.Message ID: @.***>

cheise commented 1 year ago

In Germany we say: "You can't see the tree for the trees" or "why make it complicated when it's easy" I hope the translation is correct. Best Regards

mmarkin commented 1 year ago

In Canada the saying is "you can't see the forest for the trees." I didn't write most of the code for this project but I did go through it trying to make it more efficient. I fixed the bug in the timestamp calculations. But even though I am very familiar with timelib and have used it numerous times for clock projects, it never occurred to me that timelib already has the timestamp.

Sometimes you get so focused on one solution the obvious easier solution gets overlooked.

Mitch Markin

On Mon, Feb 6, 2023 at 5:07 PM cheise @.***> wrote:

In Germany we say: "You can't see the tree for the trees" or "why make it complicated when it's easy" I hope the translation is correct. Best Regards

— Reply to this email directly, view it on GitHub https://github.com/Montecri/GPSTimeServer/issues/7#issuecomment-1420022459, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVRWOK356IHOY7MLBOLS6LWWGN6PANCNFSM6AAAAAAUSSLAAM . You are receiving this because you were mentioned.Message ID: @.***>

Montecri commented 1 year ago

Hi guys!

I remember I added >3000 to debug a specific situation (for which, of course, I don't recall) and forgot to remove it.

Went over the code so many times to make it work with my hardware and specific needs that, at some point, it kind of becomes fuzzy. Unless it's a code-breaking bug you don't see the opportunities anymore. But I was certain that people online would find these and help fix them.

Like my ugly display code, fixed by @mmarkin, and these fixed by @cheise

Thank you all!

Montecri commented 1 year ago

Fixed as of today.

cheise commented 1 year ago

I ported the code to RP2040 Raspberry Pico, the non wifi variant. That's when I noticed the problems. I have built a lightweight network based GPS NTP device, not WiFi based. Your code helped me a lot to understand many things. Great work from you :)

It looks like that:

NTP-Server

Montecri commented 1 year ago

Very nice!

Have you posted your port on Github or something?

If you want we can created a RP2040 branch for it, like I did for Mitch Dual Display and PIR version, so he can maintain it.

Regards,

cheise commented 1 year ago

Under https://github.com/cheise/PICO_GPS_TimeServer_PLUS you can see my newest public code. Now it has support for the newer TinyGPSPlus libary ;) This Version is for the i2c Display what you use, too. Functionaly the same, without the "reset / WiFi" Button, i use the "run" pin on RPI Pico for resetting the board with a Button.