M-Labs/artiq-zynq
10
1
Fork 19
Code Issues 12 Pull Requests Releases Wiki Activity

idle kernel changes: run on startup, flashing new kernel after startup #276

Merged
sb10q merged 2 commits from srenblad/artiq-zynq:idle_kernel_rework_simple into master 2024-08-05 19:28:10 +08:00
Conversation 5 Commits 2 Files Changed 1 -5
1 changed files with 0 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 456935c9aa - Show all commits

5
src/runtime/src/comms.rs
View File

@ -729,11 +729,6 @@ async fn load_and_run_idle_kernel(
info!("Loading idle kernel");
let res = handle_flash_kernel(buffer, control, up_destinations, aux_mutex, routing_table, timer).await;
srenblad marked this conversation as resolved Outdated
sb10q commented 2024-07-31 17:29:04 +08:00
Outdated
Review
Copy Link

formatting

formatting
match res {
#[cfg(has_drtio)]
Err(Error::DestinationDown) => {
let mut countdown = timer.countdown();
delay(&mut countdown, Milliseconds(500)).await;
}
Err(_) => warn!("error loading idle kernel"),
_ => (),
sb10q commented 2024-07-31 17:29:35 +08:00
Outdated
Review
Copy Link

Why does this need to be aware of DRTIO?

DRTIO link status is to be handled manually by the user in idle kernels

Why does this need to be aware of DRTIO? DRTIO link status is to be handled manually by the user in idle kernels
srenblad commented 2024-07-31 17:54:47 +08:00
Outdated
Review
Copy Link

Added in #279. Inspecting handle_flash_kernel reveals this Error indeed does not occur for idle kernels.

@mwojcik safe to remove?

Added in #279. Inspecting `handle_flash_kernel` reveals this Error indeed does not occur for idle kernels. @mwojcik safe to remove?
mwojcik commented 2024-07-31 17:57:25 +08:00
Outdated
Review
Copy Link

It was there to ensure that DRTIO was fully set up before a startup kernel would run. It could take a considerable while for all required destinations to start. With an idle kernel that can run on startup as well, I would recommend keeping it.

It was there to ensure that DRTIO was fully set up before a startup kernel would run. It could take a considerable while for all required destinations to start. With an idle kernel that can run on startup as well, I would recommend keeping it.
sb10q commented 2024-07-31 17:58:12 +08:00
Outdated
Review
Copy Link

This is for the user to handle. That's why we have an API for it. The behavior hardcoded here of waiting for 1 second is messy and not applicable to all use cases.

This is for the user to handle. That's why we have an API for it. The behavior hardcoded here of waiting for 1 second is messy and not applicable to all use cases.
👍 1
mwojcik commented 2024-07-31 18:03:22 +08:00
Outdated
Review
Copy Link

Since all subkernels are distributed before the kernel starts (to reduce load and latency when the kernels are actually running), that doesn't give the user an opportunity to act accordingly. However, since idle kernels will be retried and restarted until they can finally run, this check could be removed here.

Since all subkernels are distributed before the kernel starts (to reduce load and latency when the kernels are actually running), that doesn't give the user an opportunity to act accordingly. However, since idle kernels will be retried and restarted until they can finally run, this check could be removed here.
sb10q commented 2024-08-01 07:08:41 +08:00
Outdated
Review
Copy Link

Idle subkernels and startup subkernels need to be flashed into their respective satellites. I think I mentioned this already.

Idle subkernels and startup subkernels need to be flashed into their respective satellites. I think I mentioned this already.
mwojcik commented 2024-08-01 14:01:53 +08:00
Outdated
Review
Copy Link

Oh then it would make perfect sense, but that's feasible only after artiq#2498 is implemented. So to get this PR merged before that, let's remove that part.

Oh then it would make perfect sense, but that's feasible only after [artiq#2498](https://github.com/m-labs/artiq/issues/2498) is implemented. So to get this PR merged before that, let's remove that part.
}