feat: sntp #14
feat: sntp #14
Conversation
| } | ||
|
|
||
| impl EspSntp { | ||
| pub fn new(conf: SntpConf) -> Result<Self> { |
There was a problem hiding this comment.
Nit: perhaps take the conf by ref?
| } | ||
| } | ||
|
|
||
| unsafe fn init(conf: SntpConf) -> Result<Self> { |
| } | ||
| } | ||
|
|
||
| unsafe fn init(conf: SntpConf) -> Result<Self> { |
There was a problem hiding this comment.
You probably want to reduce the unsafe not to cover the whole function?
I had a few like those that I reduced in the meantime.
| @@ -0,0 +1,191 @@ | |||
| use core::{time::Duration}; | |||
|
|
|||
| use anyhow::*; | |||
There was a problem hiding this comment.
I'm eradicating anyhow from the esp-idf-svc APIs, as it kinda does not belong in a library. Only remaining usage (I think) is in httpd, but that module is anyway going away in favor of http::server You should be using EspError. And yes, there's a complication with the LwIP errors that we need to handle and I'll comment in a separate review, in a separate pull request.
There was a problem hiding this comment.
I probably just copied it from one of the things you did, sntp was the first thing I worked on
I'll replace it with EspError if possible
| waiter.start(); | ||
|
|
||
| let c_tz = CString::new("TZ")?; | ||
| let c_tz_val = CString::new("UTC")?; |
There was a problem hiding this comment.
Do we really want to hard-code the timezone as UTC? I might want to do EST? Also, what do timezones have to do with sntp (which - I hope - is just working with epoch millis (with or without leap seconds, but definitely not concerned with the local timezone))?
There was a problem hiding this comment.
No, probably don't want to hard-code this, I went off examples from espressif, I'll check again if it's necessary
|
Since I did not know when (or even if) you would be resuming work on this PR, I've taken it and - with a few changes - merged it: d20b9bd The changes in details:
Thanks for your effort! |
Why would you not want a way to wait for it? |
Will get the time from a NTP server
After you can use the usual time APIs and get the real-world time (i.e.
std::time::SystemTime::now())Depends on #13