Skip to content
This repository was archived by the owner on Jul 22, 2022. It is now read-only.

Replace sdk/agentime.c with NTPClient back version #18

Closed
wants to merge 3 commits into from

Conversation

sandeepmistry
Copy link
Contributor

Implementation of #17.

The simplesample_http example runs great on WiFi101 and my Adafruit Huzzah. This gives us more portability with the library for boards in the future.

@obsoleted please view and try out.

@sandeepmistry sandeepmistry mentioned this pull request Apr 21, 2016
@obsoleted
Copy link
Contributor

I'd like to hold off on this for now.

I want to avoid replacing parts of the upstream sdk when possible.
For esp this results in NTP/time being configured within the library but not really usable outside the library. So if another library was calling If the NTPClient library used the internal esp implementation here that might help things but i'm not sure it is worth it for that library.

If we do go this route we probably would also want to expose the ntp configuration options to the caller (e.g. ntp servers).

But overall i'd like to investigate/think about this more before changing the library api surface and the contract between library and upstream sdk.

@sandeepmistry
Copy link
Contributor Author

Ok, fair enough let's hold off.

Originally I had thought, the agenttime layer could be customized by the platform as there was a separate file to handle mbed.

We've started to thinking about a TimeProvider interface, which NTPClient could implement. Longer term, this would be a cleaner fit, as the library could accept an sslClient and timeProvider.

@tameraw
Copy link
Contributor

tameraw commented Apr 12, 2017

Closing as it's been agreed to hold off. A backlog item has been created on our side to look into adding NTP into the platform_init.

@tameraw tameraw closed this Apr 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants