[RSDK-9226] - fix time of day not setting when starting offline #352
Conversation
| @@ -520,7 +520,7 @@ where | |||
| Ok(data) => data, | |||
| Err(DataSyncError::NoCurrentTime) => { | |||
| log::error!("Could not calculate data timestamps, returning without flushing store"); | |||
| return Ok(()); | |||
| continue; | |||
There was a problem hiding this comment.
Is this right? I think the idea of returning out of run here is that if you didn't have enough info to calculate timestamps now, why would you move on to the next collector, which will just fail the same way? Instead, bail out of run and hope that the next time run is queued up by invoke that the situation has improved. As written with continue, won't this just result in the repeated logging of Could not calculate data timestamps, returning without flushing store, once for each configured collector, and then returning anyway?
| let tz = chrono_tz::Tz::UTC; | ||
| std::env::set_var("TZ", tz.name()); |
There was a problem hiding this comment.
- We probably should only be doing the env var set once, at startup. Also not clear to me that it is safe for us to be calling it at all, as an MT program:
In multi-threaded programs on other operating systems, the only safe option is to not use set_var or remove_var at all.
- Should we also be calling
tzset, per https://docs.espressif.com/projects/esp-idf/en/v5.0.1/esp32/api-reference/system/system_time.html#timezones
There was a problem hiding this comment.
Scrolling down on that page, it looks like tzset is only necessary for older versions of Newlib? It was working before without it, so I'd be a little surprised that we suddenly need it
There was a problem hiding this comment.
I didn't see anything further about tzset w.r.t. newlib on that page. Could you quote it?
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fine to defer this into a new ticket, but should we consider calling adjtime if we have a good local datetime but it differs from what app is telling us?
| let local_dt = Local::now().fixed_offset(); | ||
| // Viam does not pre-exist the year 2020, so if the year is before that | ||
| // at the very least the current time is wrong and needs to be corrected | ||
| if local_dt.year() < 2020 { |
There was a problem hiding this comment.
This value is now referenced in two places that need to remain in sync, should we ever decide to change it. Let's make a constant for it.
| let tv_sec = current_dt.timestamp() as i32; | ||
| let tv_usec = current_dt.timestamp_subsec_micros() as i32; | ||
| let current_timeval = timeval { tv_sec, tv_usec }; | ||
| let res = unsafe { |
There was a problem hiding this comment.
You don't really need res right?
if unsafe {...} != 0 { log::error!... ought to do
There was a problem hiding this comment.
or the esp! macro to convert it to a result
| use chrono::Datelike; | ||
| use chrono::Local; | ||
| use chrono::{format::ParseError, DateTime, FixedOffset}; |
There was a problem hiding this comment.
add these to existing import
| let tv_sec = current_dt.timestamp() as i32; | ||
| let tv_usec = current_dt.timestamp_subsec_micros() as i32; | ||
| let current_timeval = timeval { tv_sec, tv_usec }; | ||
| let res = unsafe { |
There was a problem hiding this comment.
or the esp! macro to convert it to a result
No description provided.