From 41a0bd4589badfef9e2f9b1ea5cdab7fc98ae399 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leonard=20G=C3=B6hrs?= Date: Fri, 24 May 2024 13:21:50 +0200 Subject: [PATCH 1/4] dut_power: make the ThreadPriorityValue conversion fallible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Leonard Göhrs --- src/dut_power.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/dut_power.rs b/src/dut_power.rs index a95623fe..b499441f 100644 --- a/src/dut_power.rs +++ b/src/dut_power.rs @@ -50,9 +50,12 @@ mod prio { use thread_priority::*; pub fn realtime_priority() -> Result<()> { + let prio = ThreadPriorityValue::try_from(10) + .map_err(|e| anyhow!("Failed to choose realtime priority level 10: {e:?}"))?; + set_thread_priority_and_policy( thread_native_id(), - ThreadPriority::Crossplatform(ThreadPriorityValue::try_from(10).unwrap()), + ThreadPriority::Crossplatform(prio), ThreadSchedulePolicy::Realtime(RealtimeThreadSchedulePolicy::Fifo), ) .map_err(|e| anyhow!("Failed to set up realtime priority {e:?}")) From 1e5c0502075c83703f51460c41c982d3ab8783b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leonard=20G=C3=B6hrs?= Date: Fri, 24 May 2024 13:23:43 +0200 Subject: [PATCH 2/4] dut_power: make turn_off_with_reason() fallible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The operating system call to set the GPIO output state may fail. Make sure that this results in an orderly teardown of the tacd instead of a panic. This also necessitates a change to the test and demo_mode gpio stubs, so they can be `?`-operatored into anyhow Results (simply by making them anyhow results). Signed-off-by: Leonard Göhrs --- src/digital_io/gpio/demo_mode.rs | 2 +- src/digital_io/gpio/test.rs | 2 +- src/dut_power.rs | 18 ++++++++++-------- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/digital_io/gpio/demo_mode.rs b/src/digital_io/gpio/demo_mode.rs index b9889ebb..dc54c374 100644 --- a/src/digital_io/gpio/demo_mode.rs +++ b/src/digital_io/gpio/demo_mode.rs @@ -27,7 +27,7 @@ pub struct LineHandle { } impl LineHandle { - pub fn set_value(&self, val: u8) -> Result<(), ()> { + pub fn set_value(&self, val: u8) -> Result<()> { // This does not actually set up any IIO things. // It is just a hack to let adc/iio/demo_mode.rs // communicate with this function so that toggling an output diff --git a/src/digital_io/gpio/test.rs b/src/digital_io/gpio/test.rs index db5b1478..8b25b59c 100644 --- a/src/digital_io/gpio/test.rs +++ b/src/digital_io/gpio/test.rs @@ -32,7 +32,7 @@ pub struct LineHandle { } impl LineHandle { - pub fn set_value(&self, val: u8) -> Result<(), ()> { + pub fn set_value(&self, val: u8) -> Result<()> { println!("GPIO simulation set {} to {}", self.name, val); self.val.store(val, Ordering::Relaxed); Ok(()) diff --git a/src/dut_power.rs b/src/dut_power.rs index b499441f..342a206c 100644 --- a/src/dut_power.rs +++ b/src/dut_power.rs @@ -263,10 +263,12 @@ fn turn_off_with_reason( pwr_line: &LineHandle, discharge_line: &LineHandle, fail_state: &AtomicU8, -) { - pwr_line.set_value(1 - PWR_LINE_ASSERTED).unwrap(); - discharge_line.set_value(DISCHARGE_LINE_ASSERTED).unwrap(); +) -> Result<()> { + pwr_line.set_value(1 - PWR_LINE_ASSERTED)?; + discharge_line.set_value(DISCHARGE_LINE_ASSERTED)?; fail_state.store(reason as u8, Ordering::Relaxed); + + Ok(()) } /// Labgrid has a fixed assumption of how a REST based power port should work. @@ -409,7 +411,7 @@ impl DutPwrThread { &pwr_line, &discharge_line, &state, - ); + )?; } else { // We have a fresh ADC value. Signal "everything is well" // to the watchdog task. @@ -466,7 +468,7 @@ impl DutPwrThread { &pwr_line, &discharge_line, &state, - ); + )?; continue; } @@ -477,7 +479,7 @@ impl DutPwrThread { &pwr_line, &discharge_line, &state, - ); + )?; continue; } @@ -488,7 +490,7 @@ impl DutPwrThread { &pwr_line, &discharge_line, &state, - ); + )?; continue; } @@ -521,7 +523,7 @@ impl DutPwrThread { } // Make sure to enter fail safe mode before leaving the thread - turn_off_with_reason(OutputState::Off, &pwr_line, &discharge_line, &state); + turn_off_with_reason(OutputState::Off, &pwr_line, &discharge_line, &state)?; Ok(()) })?; From 01252562326c2a6865e748c42537c7089c5c04f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leonard=20G=C3=B6hrs?= Date: Fri, 24 May 2024 13:28:53 +0200 Subject: [PATCH 3/4] dut_power: simplify the error handling inside the thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we can now return early from `wtb.spawn_thread`-spawned threads, because they handle error propagation out of the thread context for us we no longer have to use queues to send back error results manually. Use that to simplify error handling in the thread setup and the steady state. Signed-off-by: Leonard Göhrs --- src/dut_power.rs | 46 +++++++++++++++++++--------------------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/src/dut_power.rs b/src/dut_power.rs index 342a206c..c37d9834 100644 --- a/src/dut_power.rs +++ b/src/dut_power.rs @@ -338,7 +338,7 @@ impl DutPwrThread { // as well. // Use a queue to notify the calling thread if the priority setup // succeeded. - let (thread_res_tx, mut thread_res_rx) = bounded(1); + let (thread_tx, thread_rx) = bounded(1); // Spawn a high priority thread that handles the power status // in a realtimey fashion. @@ -353,24 +353,20 @@ impl DutPwrThread { let mut volt_filter = MedianFilter::<4>::new(); let mut curr_filter = MedianFilter::<4>::new(); - let (tick_weak, request, state) = match realtime_priority() { - Ok(_) => { - let tick = Arc::new(AtomicU32::new(0)); - let tick_weak = Arc::downgrade(&tick); + realtime_priority()?; - let request = Arc::new(AtomicU8::new(OutputRequest::Idle as u8)); - let state = Arc::new(AtomicU8::new(OutputState::Off as u8)); + let (tick_weak, request, state) = { + let tick = Arc::new(AtomicU32::new(0)); + let tick_weak = Arc::downgrade(&tick); - thread_res_tx - .try_send(Ok((tick, request.clone(), state.clone()))) - .unwrap(); + let request = Arc::new(AtomicU8::new(OutputRequest::Idle as u8)); + let state = Arc::new(AtomicU8::new(OutputState::Off as u8)); - (tick_weak, request, state) - } - Err(e) => { - thread_res_tx.try_send(Err(e)).unwrap(); - panic!() - } + thread_tx + .try_send((tick, request.clone(), state.clone())) + .expect("Queue that should be empty wasn't"); + + (tick_weak, request, state) }; // The grace period contains the number of loop iterations until @@ -501,22 +497,18 @@ impl DutPwrThread { match req { OutputRequest::Idle => {} OutputRequest::On => { - discharge_line - .set_value(1 - DISCHARGE_LINE_ASSERTED) - .unwrap(); - pwr_line.set_value(PWR_LINE_ASSERTED).unwrap(); + discharge_line.set_value(1 - DISCHARGE_LINE_ASSERTED)?; + pwr_line.set_value(PWR_LINE_ASSERTED)?; state.store(OutputState::On as u8, Ordering::Relaxed); } OutputRequest::Off => { - discharge_line.set_value(DISCHARGE_LINE_ASSERTED).unwrap(); - pwr_line.set_value(1 - PWR_LINE_ASSERTED).unwrap(); + discharge_line.set_value(DISCHARGE_LINE_ASSERTED)?; + pwr_line.set_value(1 - PWR_LINE_ASSERTED)?; state.store(OutputState::Off as u8, Ordering::Relaxed); } OutputRequest::OffFloating => { - discharge_line - .set_value(1 - DISCHARGE_LINE_ASSERTED) - .unwrap(); - pwr_line.set_value(1 - PWR_LINE_ASSERTED).unwrap(); + discharge_line.set_value(1 - DISCHARGE_LINE_ASSERTED)?; + pwr_line.set_value(1 - PWR_LINE_ASSERTED)?; state.store(OutputState::OffFloating as u8, Ordering::Relaxed); } } @@ -528,7 +520,7 @@ impl DutPwrThread { Ok(()) })?; - let (tick, request, state) = thread_res_rx.next().await.unwrap()?; + let (tick, request, state) = thread_rx.recv().await?; // The request and state topic use the same external path, this way one // can e.g. publish "On" to the topic and be sure that the output is From 79f241b8c7751a6d8080cf431b49c8b6f91d22db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leonard=20G=C3=B6hrs?= Date: Mon, 2 Oct 2023 16:11:33 +0200 Subject: [PATCH 4/4] adc: hardware: improve/simplify error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that errors from threads are properly propagated we can simplify the error handling and do not have to transfer `Result`s across queues if something goes wrong during thread setup. Signed-off-by: Leonard Göhrs --- src/adc/iio/hardware.rs | 57 +++++++++++------------------------------ 1 file changed, 15 insertions(+), 42 deletions(-) diff --git a/src/adc/iio/hardware.rs b/src/adc/iio/hardware.rs index d9033706..fcbfcbde 100644 --- a/src/adc/iio/hardware.rs +++ b/src/adc/iio/hardware.rs @@ -22,7 +22,7 @@ use std::path::Path; use std::sync::atomic::{AtomicU16, AtomicU64, Ordering}; use std::time::{Duration, Instant}; -use anyhow::{anyhow, Context, Error, Result}; +use anyhow::{anyhow, Context, Result}; use async_std::channel::bounded; use async_std::sync::Arc; @@ -275,40 +275,27 @@ impl IioThread { // setup was sucessful. // This is why we create Self inside the thread and send it back // to the calling thread via a queue. - let (thread_res_tx, thread_res_rx) = bounded(1); + let (thread_tx, thread_rx) = bounded(1); // Spawn a high priority thread that updates the atomic values in `thread`. wtb.spawn_thread(thread_name, move || { - let adc_setup_res = Self::adc_setup( + let (channels, mut buf) = Self::adc_setup( adc_name, trigger_name, sample_rate, channel_descs, buffer_len, - ); - let (thread, channels, mut buf) = match adc_setup_res { - Ok((channels, buf)) => { - let thread = Arc::new(Self { - ref_instant: Instant::now(), - timestamp: AtomicU64::new(TIMESTAMP_ERROR), - values: channels.iter().map(|_| AtomicU16::new(0)).collect(), - channel_descs, - }); - - (thread, channels, buf) - } - Err(e) => { - // Can not fail in practice as the queue is known to be empty - // at this point. - thread_res_tx - .try_send(Err(e)) - .expect("Failed to signal ADC setup error due to full queue"); - return Ok(()); - } - }; + )?; + + let thread = Arc::new(Self { + ref_instant: Instant::now(), + timestamp: AtomicU64::new(TIMESTAMP_ERROR), + values: channels.iter().map(|_| AtomicU16::new(0)).collect(), + channel_descs, + }); let thread_weak = Arc::downgrade(&thread); - let mut signal_ready = Some((thread, thread_res_tx)); + let mut signal_ready = Some((thread, thread_tx)); // Stop running as soon as the last reference to this Arc // is dropped (e.g. the weak reference can no longer be upgraded). @@ -318,18 +305,7 @@ impl IioThread { error!("Failed to refill {} ADC buffer: {}", adc_name, e); - // If the ADC has not yet produced any values we still have the - // queue at hand that signals readiness to the main thread. - // This gives us a chance to return an Err from new(). - // If the queue was already used just print an error instead. - if let Some((_, tx)) = signal_ready.take() { - // Can not fail in practice as the queue is only .take()n - // once and thus known to be empty. - tx.try_send(Err(Error::new(e))) - .expect("Failed to signal ADC setup error due to full queue"); - } - - break; + Err(e)?; } let values = channels.iter().map(|ch| { @@ -356,17 +332,14 @@ impl IioThread { if let Some((content, tx)) = signal_ready.take() { // Can not fail in practice as the queue is only .take()n // once and thus known to be empty. - tx.try_send(Ok(content)) - .expect("Failed to signal ADC setup completion due to full queue"); + tx.try_send(content)?; } } Ok(()) })?; - let thread = thread_res_rx.recv().await??; - - Ok(thread) + Ok(thread_rx.recv().await?) } pub async fn new_stm32(