-
Notifications
You must be signed in to change notification settings - Fork 12
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
[RSDK-9643] ota download timeout #373
[RSDK-9643] ota download timeout #373
Conversation
file_len | ||
); | ||
} | ||
Ok(None) => break, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any issues with an incomplete download should still be caught by the nwritten != content_len
check after this scope
micro-rdk/src/common/ota.rs
Outdated
.map_err(FrameError::Network) | ||
.or(async { | ||
async_io::Timer::after(Duration::from_secs(FRAME_TIMEOUT_SECS)).await; | ||
Err(FrameError::Timeout(FRAME_TIMEOUT_SECS as usize)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the goal is to timeout if a single frame is unresponsive for more than 30seconds, unblocking the caller (ConfigMonitor) so it can try again completely in 10 seconds (ConfigMonitor's default period)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with optional suggestion
micro-rdk/src/common/ota.rs
Outdated
@@ -120,6 +123,15 @@ pub(crate) enum ConfigError { | |||
Other(String), | |||
} | |||
|
|||
/// used to differentiate between no-frame and a timeout | |||
#[derive(Error, Debug)] | |||
pub(crate) enum FrameError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DownloadError
instead of FrameError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.