-
Notifications
You must be signed in to change notification settings - Fork 109
fix(ota-hil): set recovery mode before capture logs #903
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
base: main
Are you sure you want to change the base?
Changes from 20 commits
d2fea56
195d75e
d02e2b5
9040984
9ba8e91
8866016
b7f0532
a950a82
f85882a
912df00
68cb5cd
1543d27
b75b812
8dda1fb
3725ede
7b12495
2466287
7940035
f92ac94
9a55d7c
7973a70
a8d615b
9fe311c
8a42a30
368513b
b50d887
e27f1cc
19fb770
7e24ebf
7a7c7c5
82f5f84
c17337e
3687bf4
33497fc
0522779
484fa1b
d150f51
0ef98f2
8781765
8b471ea
1b07e90
e83bfa5
1f65059
449a924
0c15f91
ceae6e6
4f8b4b2
5cc74ab
83a80fd
c8df636
32b2f1f
16746e9
0299eda
d972cb2
b96d50f
6c1ae63
8be233d
d7b211a
fe8d64b
a16deaa
2651eef
c377b95
8e02331
55f290b
cb86863
62f59a0
3a9c42e
3cb60a9
22e027c
7a1b966
fe82002
1df5421
6f2a845
73b249a
022ae7b
d2ff64c
269826c
3568b0b
857ec45
7662565
b113e94
bec8b5d
b2b4038
2c89aa4
65dacd1
0e6ac47
3b35464
aef3ffe
2a7ff45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,9 @@ pub async fn is_recovery_mode_detected() -> Result<bool> { | |
| /// If `device` is `None`, will get the first available device. | ||
| #[tracing::instrument] | ||
| pub async fn reboot(recovery: bool, device: Option<&FtdiId>) -> Result<()> { | ||
| const DEFAULT_HOLDING_DELAY: u64 = 10; | ||
| const INBETWEEN_DELAY: u64 = 4; | ||
|
|
||
| fn make_ftdi(device: Option<FtdiId>) -> Result<FtdiGpio> { | ||
| let builder = FtdiGpio::builder(); | ||
| let builder = match &device { | ||
|
|
@@ -37,7 +40,12 @@ pub async fn reboot(recovery: bool, device: Option<&FtdiId>) -> Result<()> { | |
|
|
||
| info!("Turning off"); | ||
| let device_clone = device.cloned(); | ||
| let ftdi = tokio::task::spawn_blocking(|| -> Result<_, color_eyre::Report> { | ||
| let recovery_state = if recovery { | ||
| OutputState::Low | ||
| } else { | ||
| OutputState::High | ||
| }; | ||
| let ftdi = tokio::task::spawn_blocking(move || -> Result<_, color_eyre::Report> { | ||
| for d in FtdiGpio::list_devices().wrap_err("failed to list ftdi devices")? { | ||
| debug!( | ||
| "ftdi device: desc:{}, serial:{}, vid:{}, pid:{}", | ||
|
|
@@ -46,18 +54,18 @@ pub async fn reboot(recovery: bool, device: Option<&FtdiId>) -> Result<()> { | |
| } | ||
| let mut ftdi = make_ftdi(device_clone)?; | ||
| ftdi.set_pin(BUTTON_PIN, OutputState::Low)?; | ||
| ftdi.set_pin(RECOVERY_PIN, OutputState::High)?; | ||
| ftdi.set_pin(RECOVERY_PIN, recovery_state)?; | ||
| Ok(ftdi) | ||
| }) | ||
| .await | ||
| .wrap_err("task panicked")??; | ||
| tokio::time::sleep(Duration::from_secs(10)).await; | ||
| tokio::time::sleep(Duration::from_secs(DEFAULT_HOLDING_DELAY)).await; | ||
|
|
||
| info!("Resetting FTDI"); | ||
| tokio::task::spawn_blocking(move || ftdi.destroy()) | ||
| .await | ||
| .wrap_err("task panicked")??; | ||
| tokio::time::sleep(Duration::from_secs(4)).await; | ||
| tokio::time::sleep(Duration::from_secs(INBETWEEN_DELAY)).await; | ||
|
|
||
| info!("Turning on"); | ||
| let device_clone = device.cloned(); | ||
|
|
@@ -74,7 +82,7 @@ pub async fn reboot(recovery: bool, device: Option<&FtdiId>) -> Result<()> { | |
| }) | ||
| .await | ||
| .wrap_err("task panicked")??; | ||
| tokio::time::sleep(Duration::from_secs(4)).await; | ||
| tokio::time::sleep(Duration::from_secs(10)).await; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are not using the const here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is only blocknig comment - everything else is great job! |
||
|
|
||
| tokio::task::spawn_blocking(move || ftdi.destroy()) | ||
| .await | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,11 @@ pub struct Ota { | |
| /// Serial port ID for boot log capture (alternative to --serial-path) | ||
| #[arg(long, group = "serial")] | ||
| serial_id: Option<String>, | ||
|
|
||
| /// Skip NTP time synchronization check before the first reboot (after wipe_overlays). | ||
| /// Time sync will still be checked after reboot and before starting the update. | ||
| #[arg(long, default_value = "false")] | ||
| skip_time_sync_before_reboot: bool, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, clap::ValueEnum)] | ||
|
|
@@ -94,6 +99,13 @@ impl Ota { | |
| let _start_time = Instant::now(); | ||
| info!("Starting OTA update to version: {}", self.target_version); | ||
|
|
||
| if let Some(log_dir) = self.log_file.parent() { | ||
| tokio::fs::create_dir_all(log_dir).await.wrap_err_with(|| { | ||
| format!("Failed to create log directory: {}", log_dir.display()) | ||
| })?; | ||
| info!("Log directory created/verified: {}", log_dir.display()); | ||
| } | ||
|
|
||
| let session = self.connect_ssh().await.inspect_err(|e| { | ||
| println!("OTA_RESULT=FAILED"); | ||
| println!("OTA_ERROR=SSH_CONNECTION_FAILED: {e}"); | ||
|
|
@@ -105,7 +117,19 @@ impl Ota { | |
| system::wipe_overlays(&session).await.inspect_err(|e| { | ||
| error!("Failed to wipe overlays: {}", e); | ||
| })?; | ||
| info!("Overlays wiped successfully, rebooting device"); | ||
| info!("Overlays wiped successfully"); | ||
|
|
||
| if !self.skip_time_sync_before_reboot { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why we need this I wonder ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, basically the problem was with the worldcoin-key-retrival service. With pearl, teh harware clock is not working so if it NTP server is not synced then worldcoin-attest is failing and also key-retrieval. This should be fixed with new key-retrieval fix, that is why i ketp it as a flag, so we can test older commits, that don't have key-retrival fixed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am also not getting it sorry.
That is not true. The clock will synchronize and https requests is going to succeed. And the delay is not big for that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By default this is not correct. Because reboot is manually triggered faster than the NTP server syncrhonizes the clock.... |
||
| info!("Waiting for NTP time synchronization before reboot"); | ||
| system::wait_for_time_sync(&session) | ||
| .await | ||
| .inspect_err(|e| { | ||
| error!("Failed to sync time before reboot: {}", e); | ||
| })?; | ||
| info!("NTP time synchronized, rebooting device"); | ||
| } else { | ||
| info!("Skipping NTP time synchronization before reboot (--skip-time-sync-before-reboot flag set)"); | ||
| } | ||
|
|
||
| system::reboot_orb(&session).await?; | ||
| info!("Reboot command sent to Orb device"); | ||
|
|
||
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.
move to the top of the file or maybe have as separate arg to cli so you don't tune it in sources every time?
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.
I will fix!