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

Fix time validity check to account for maximum possible timezone offset #507

Closed
wants to merge 2 commits into from

Conversation

kytpbs
Copy link
Contributor

@kytpbs kytpbs commented Aug 23, 2024

Fixes #506

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.39%. Comparing base (9166399) to head (14eff91).
Report is 13 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #507   +/-   ##
=======================================
  Coverage   95.39%   95.39%           
=======================================
  Files          33       33           
  Lines        1520     1520           
=======================================
  Hits         1450     1450           
  Misses         70       70           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kytpbs
Copy link
Contributor Author

kytpbs commented Aug 30, 2024

@pennam could you review the PR? Would be very appreciated.

@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Sep 7, 2024
@per1234 per1234 requested a review from pennam September 7, 2024 14:08
Copy link
Collaborator

@pennam pennam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kytpbs,
thanks for your contribution and sorry for the delay I was OOF the past two weeks. I've got only one style change request. I suggested you the changes in order to match the comment style of the rest of the file.

Thanks

src/utility/time/TimeService.cpp Outdated Show resolved Hide resolved
Co-authored-by: Mattia Pennasilico <[email protected]>
@pennam
Copy link
Collaborator

pennam commented Sep 9, 2024

Thanks, tomorrow morning i will give this PR the last spin and then i will merge it.

Copy link

github-actions bot commented Sep 9, 2024

Memory usage change @ 14eff91

Board flash % RAM for global variables %
arduino:esp32:nano_nora 🔺 +8 - +12 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_edge:edge_control 🔺 0 - +64 0.0 - +0.01 0 - 0 0.0 - 0.0
arduino:mbed_giga:giga 🔺 0 - +64 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nanorp2040connect 🔺 0 - +12 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nicla:nicla_vision 🔺 0 - +64 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_opta:opta 🔺 0 - +64 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A
arduino:renesas_portenta:portenta_c33 🔺 0 - +24 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:renesas_uno:unor4wifi 🔺 +16 - +16 +0.01 - +0.01 0 - 0 0.0 - 0.0
arduino:samd:mkr1000 🔺 0 - +16 0.0 - +0.01 0 - 0 0.0 - 0.0
arduino:samd:mkrgsm1400 🔺 0 - +16 0.0 - +0.01 0 - 0 0.0 - 0.0
arduino:samd:mkrnb1500 🔺 0 - +16 0.0 - +0.01 0 - 0 0.0 - 0.0
arduino:samd:mkrwan1300 🔺 0 - +16 0.0 - +0.01 0 - 0 0.0 - 0.0
arduino:samd:mkrwifi1010 🔺 0 - +16 0.0 - +0.01 0 - 0 0.0 - 0.0
arduino:samd:nano_33_iot 🔺 0 - +16 0.0 - +0.01 0 - 0 0.0 - 0.0
esp32:esp32:esp32 🔺 +24 - +32 0.0 - 0.0 0 - 0 0.0 - 0.0
esp8266:esp8266:huzzah 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table
Board examples/ArduinoIoTCloud-Advanced
flash
% examples/ArduinoIoTCloud-Advanced
RAM for global variables
% examples/ArduinoIoTCloud-Basic
flash
% examples/ArduinoIoTCloud-Basic
RAM for global variables
% examples/ArduinoIoTCloud-Callbacks
flash
% examples/ArduinoIoTCloud-Callbacks
RAM for global variables
% examples/ArduinoIoTCloud-Schedule
flash
% examples/ArduinoIoTCloud-Schedule
RAM for global variables
% examples/utility/ArduinoIoTCloud_Travis_CI
flash
% examples/utility/ArduinoIoTCloud_Travis_CI
RAM for global variables
% examples/ArduinoIoTCloud-DeferredOTA
flash
% examples/ArduinoIoTCloud-DeferredOTA
RAM for global variables
% examples/utility/Provisioning
flash
% examples/utility/Provisioning
RAM for global variables
% examples/utility/SelfProvisioning
flash
% examples/utility/SelfProvisioning
RAM for global variables
%
arduino:esp32:nano_nora 12 0.0 0 0.0 12 0.0 0 0.0 12 0.0 0 0.0 12 0.0 0 0.0 8 0.0 0 0.0 12 0.0 0 0.0
arduino:mbed_edge:edge_control 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 64 0.01 0 0.0 0 0.0 0 0.0
arduino:mbed_giga:giga 64 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 64 0.0 0 0.0 0 0.0 0 0.0
arduino:mbed_nano:nanorp2040connect 12 0.0 0 0.0 12 0.0 0 0.0 12 0.0 0 0.0 12 0.0 0 0.0 12 0.0 0 0.0 12 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
arduino:mbed_nicla:nicla_vision 0 0.0 0 0.0 64 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 64 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
arduino:mbed_opta:opta 0 0.0 0 0.0 64 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 64 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
arduino:renesas_portenta:portenta_c33 24 0.0 0 0.0 24 0.0 0 0.0 24 0.0 0 0.0 24 0.0 0 0.0 16 0.0 0 0.0 0 0.0 0 0.0
arduino:renesas_uno:unor4wifi 16 0.01 0 0.0 16 0.01 0 0.0 16 0.01 0 0.0 16 0.01 0 0.0 16 0.01 0 0.0
arduino:samd:mkr1000 8 0.0 0 0.0 16 0.01 0 0.0 8 0.0 0 0.0 8 0.0 0 0.0 16 0.01 0 0.0 0 0.0 0 0.0
arduino:samd:mkrgsm1400 16 0.01 0 0.0 8 0.0 0 0.0 16 0.01 0 0.0 8 0.0 0 0.0 16 0.01 0 0.0 0 0.0 0 0.0
arduino:samd:mkrnb1500 16 0.01 0 0.0 8 0.0 0 0.0 16 0.01 0 0.0 8 0.0 0 0.0 16 0.01 0 0.0 0 0.0 0 0.0
arduino:samd:mkrwan1300 16 0.01 0 0.0 16 0.01 0 0.0 0 0.0 0 0.0 8 0.0 0 0.0 8 0.0 0 0.0
arduino:samd:mkrwifi1010 8 0.0 0 0.0 8 0.0 0 0.0 16 0.01 0 0.0 8 0.0 0 0.0 16 0.01 0 0.0 8 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
arduino:samd:nano_33_iot 8 0.0 0 0.0 8 0.0 0 0.0 16 0.01 0 0.0 8 0.0 0 0.0 16 0.01 0 0.0 8 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
esp32:esp32:esp32 24 0.0 0 0.0 24 0.0 0 0.0 24 0.0 0 0.0 24 0.0 0 0.0 32 0.0 0 0.0 24 0.0 0 0.0
esp8266:esp8266:huzzah 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
Click for full report CSV
Board,examples/ArduinoIoTCloud-Advanced<br>flash,%,examples/ArduinoIoTCloud-Advanced<br>RAM for global variables,%,examples/ArduinoIoTCloud-Basic<br>flash,%,examples/ArduinoIoTCloud-Basic<br>RAM for global variables,%,examples/ArduinoIoTCloud-Callbacks<br>flash,%,examples/ArduinoIoTCloud-Callbacks<br>RAM for global variables,%,examples/ArduinoIoTCloud-Schedule<br>flash,%,examples/ArduinoIoTCloud-Schedule<br>RAM for global variables,%,examples/utility/ArduinoIoTCloud_Travis_CI<br>flash,%,examples/utility/ArduinoIoTCloud_Travis_CI<br>RAM for global variables,%,examples/ArduinoIoTCloud-DeferredOTA<br>flash,%,examples/ArduinoIoTCloud-DeferredOTA<br>RAM for global variables,%,examples/utility/Provisioning<br>flash,%,examples/utility/Provisioning<br>RAM for global variables,%,examples/utility/SelfProvisioning<br>flash,%,examples/utility/SelfProvisioning<br>RAM for global variables,%
arduino:esp32:nano_nora,12,0.0,0,0.0,12,0.0,0,0.0,12,0.0,0,0.0,12,0.0,0,0.0,8,0.0,0,0.0,12,0.0,0,0.0
arduino:mbed_edge:edge_control,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,64,0.01,0,0.0,0,0.0,0,0.0,,,,
arduino:mbed_giga:giga,64,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,64,0.0,0,0.0,0,0.0,0,0.0
arduino:mbed_nano:nanorp2040connect,12,0.0,0,0.0,12,0.0,0,0.0,12,0.0,0,0.0,12,0.0,0,0.0,12,0.0,0,0.0,12,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0
arduino:mbed_nicla:nicla_vision,0,0.0,0,0.0,64,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,64,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,,,,
arduino:mbed_opta:opta,0,0.0,0,0.0,64,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,64,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,,,,
arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,,,,
arduino:renesas_portenta:portenta_c33,24,0.0,0,0.0,24,0.0,0,0.0,24,0.0,0,0.0,24,0.0,0,0.0,16,0.0,0,0.0,,,,,0,0.0,0,0.0,,,,
arduino:renesas_uno:unor4wifi,16,0.01,0,0.0,16,0.01,0,0.0,16,0.01,0,0.0,16,0.01,0,0.0,16,0.01,0,0.0,,,,,,,,,,,,
arduino:samd:mkr1000,8,0.0,0,0.0,16,0.01,0,0.0,8,0.0,0,0.0,8,0.0,0,0.0,16,0.01,0,0.0,,,,,0,0.0,0,0.0,,,,
arduino:samd:mkrgsm1400,16,0.01,0,0.0,8,0.0,0,0.0,16,0.01,0,0.0,8,0.0,0,0.0,16,0.01,0,0.0,,,,,0,0.0,0,0.0,,,,
arduino:samd:mkrnb1500,16,0.01,0,0.0,8,0.0,0,0.0,16,0.01,0,0.0,8,0.0,0,0.0,16,0.01,0,0.0,,,,,0,0.0,0,0.0,,,,
arduino:samd:mkrwan1300,16,0.01,0,0.0,16,0.01,0,0.0,0,0.0,0,0.0,8,0.0,0,0.0,8,0.0,0,0.0,,,,,,,,,,,,
arduino:samd:mkrwifi1010,8,0.0,0,0.0,8,0.0,0,0.0,16,0.01,0,0.0,8,0.0,0,0.0,16,0.01,0,0.0,8,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0
arduino:samd:nano_33_iot,8,0.0,0,0.0,8,0.0,0,0.0,16,0.01,0,0.0,8,0.0,0,0.0,16,0.01,0,0.0,8,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0
esp32:esp32:esp32,24,0.0,0,0.0,24,0.0,0,0.0,24,0.0,0,0.0,24,0.0,0,0.0,32,0.0,0,0.0,24,0.0,0,0.0,,,,,,,,
esp8266:esp8266:huzzah,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,,,,,,,,,,,,

@pennam
Copy link
Collaborator

pennam commented Sep 10, 2024

Hi @kytpbs I've made some tests and found a glitch with this changes:

EPOCH_AT_COMPILE_TIME is now considered a valid time and thus it will be stored inside the RTC instead of skipping it and retry to get a valid time.

I'm working to fix this regression and will propose you a couple of commits to be integrated in this PR.

@arduino-libraries arduino-libraries deleted a comment from github-actions bot Sep 10, 2024
@pennam
Copy link
Collaborator

pennam commented Sep 11, 2024

Hi @kytpbs I've prepared this two commits if you want to cherry-pick them and doublecheck your issue is fixed.

pennam@419b39b
pennam@012b73e

@kytpbs
Copy link
Contributor Author

kytpbs commented Sep 11, 2024

at these lines

#ifdef HAS_TCP
    utc = getRemoteTime();
#endif
#ifdef HAS_LORA
    /* Just keep incrementing stored RTC value*/
    /* Just keep incrementing stored RTC value starting from EPOCH_AT_COMPILE_TIME */
    utc = getRTC();
#endif

shouldn't we first check if we have RTC and use that and return or vice versa, since if HAS_TCP and HAS_LORA are both defined (for example ESP32), we will first get the remote time, just to overwrite it to getRTC?

I know this is not part of the commits but it peaked my interest looking at the diffs

@pennam
Copy link
Collaborator

pennam commented Sep 11, 2024

Hi @kytpbs thanks for checking! None of the supported boards has both HAS_TCP and HAS_LORA defined. The sync method is intended as a way to keep in sync the RTC with some other more reliable time source:

If user has defined a custom sync function, this method will be used otherwise if baord HAS_TCP we try to get time from NTP or connection handler. Afaik there is no (simple) way to get a timestamp from LoRa network so we rely on the RTC value that at boot time would be configured to EPOCH_AT_COMPILE_TIME.

If in the future we will have a board with both TCP and LoRa probably it would be better to rewrite the function like this to avoid overwriting the value:

#ifdef HAS_TCP
    utc = getRemoteTime();
#else
    /* Just keep incrementing stored RTC value starting from EPOCH_AT_COMPILE_TIME */
    utc = getRTC();
#endif

@kytpbs
Copy link
Contributor Author

kytpbs commented Sep 11, 2024

Thanks for the explanation

I will double-check if the issue is fixed tonight if I can since it only occurs after 12 AM

what should I do with your commits/branch

  • cherry-pick add them to this branch
  • merge that branch into this one
  • do nothing since you will just merge your branch which also has my commits instead?

@kytpbs
Copy link
Contributor Author

kytpbs commented Sep 12, 2024

Just tested my branch and things worked correctly

@pennam
Copy link
Collaborator

pennam commented Sep 12, 2024

@kytpbs Thanks for the test! Please cherry-pick the commits on this branch so we can better keep track of this PR history and related issue.

@pennam
Copy link
Collaborator

pennam commented Sep 16, 2024

Superseded by #509

@pennam pennam closed this Sep 16, 2024
@per1234 per1234 added the conclusion: duplicate Has already been submitted label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: duplicate Has already been submitted topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"ArduinoIoTCloudTCP::handle_SyncTime could not get valid time" caused by timezone diffrence
4 participants