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

Backported getLocalTime from ESP32 Arduino #8407

Closed
wants to merge 1 commit into from
Closed

Backported getLocalTime from ESP32 Arduino #8407

wants to merge 1 commit into from

Conversation

h1aji
Copy link

@h1aji h1aji commented Dec 14, 2021

New PR. Removed previosly added modifications.
This time just importing new getLocalTime from ESP32 Arduino core

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 14, 2021

It seems that you have to rename or remove this one (which is slightly different).

@h1aji
Copy link
Author

h1aji commented Dec 15, 2021

the only part that appears different is - uint32_t count = ms / 10;

I assume we can modify

and make it
getLocalTime(&tmstruct, 500);
then delete getLocalTime from that sketch

@earlephilhower what do you think?

@h1aji
Copy link
Author

h1aji commented Dec 16, 2021

@d-a-v does this look ok now?

return true;
}

while (count--)
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this loop, maximum timeout is 10 times the value of ms, which is why Earle divided it by 10.
Original and probably later updated / fixed esp32 code is there with a more accurate loop.
Do you think you can improve your proposal ?

Copy link
Author

Choose a reason for hiding this comment

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

Okay then, do you have an idea how LittleFS_Timestamp.ino sketch should be modified?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is OK to use the esp32 implementation globally and remove littlefs test's one.

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 16, 2021

About the CI error, time.cpp is not included in host tests because it holds standard functions already available on host's system.
I suggest moving the function into a new file (timehelper.cpp?) and adding this file in the host build Makefile.

@h1aji
Copy link
Author

h1aji commented Dec 16, 2021

It looks like this PR causing lots of troubles. Shall I close it?

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 16, 2021

Shall I close it?

No :) if you want to do it.

(please tell if you only need the feature without adding it yourself)
(please comment if you don't agree with the review)

@h1aji
Copy link
Author

h1aji commented Dec 16, 2021

I dont mind if you add it yourself, otherwise I am happy with this commit, I wasnt sure how to change that sketch file, that why i though asking you

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 16, 2021

otherwise I am happy with this commit

I used updated esp32 code in #8413, moved the function into another file so it can be used for the esp8266, emulation on host, and host tests, and removed the old local copy from the littleFS test.

You can get inspiration from it to fix the CI builds if you like.

@h1aji
Copy link
Author

h1aji commented Dec 17, 2021

@d-a-v nice. tthank you.
I will close this PR

@h1aji h1aji closed this Dec 17, 2021
d-a-v added a commit to d-a-v/Arduino that referenced this pull request Jan 3, 2022
d-a-v added a commit that referenced this pull request Jan 4, 2022
* import getLocalTime() from esp32/Arduino
follows #8407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants