From f7f1017fedb4293fb435ddd7d8b30bad6ec248ad Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 11 Nov 2023 19:05:46 +0100 Subject: [PATCH 01/33] continue CFDP handlers --- satrs-core/Cargo.toml | 6 +- satrs-core/src/cfdp/dest.rs | 209 +++++++++++++++++++++++++----------- satrs-core/src/cfdp/mod.rs | 96 +++++++++++++++-- 3 files changed, 235 insertions(+), 76 deletions(-) diff --git a/satrs-core/Cargo.toml b/satrs-core/Cargo.toml index 6fa8535..65ee5e7 100644 --- a/satrs-core/Cargo.toml +++ b/satrs-core/Cargo.toml @@ -73,11 +73,11 @@ features = ["all"] optional = true [dependencies.spacepackets] -version = "0.7.0-beta.2" +# version = "0.7.0-beta.2" default-features = false -# git = "https://egit.irs.uni-stuttgart.de/rust/spacepackets.git" +git = "https://egit.irs.uni-stuttgart.de/rust/spacepackets.git" # rev = "79d26e1a6" -# branch = "" +branch = "writable_pdu_packet" [dependencies.cobs] git = "https://github.com/robamu/cobs.rs.git" diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index b66365a..f54d22c 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -9,8 +9,10 @@ use crate::cfdp::user::TransactionFinishedParams; use super::{ user::{CfdpUser, MetadataReceivedParams}, - PacketInfo, PacketTarget, State, TransactionId, TransactionStep, CRC_32, + CheckTimerCreator, PacketInfo, PacketTarget, RemoteEntityConfigProvider, State, TransactionId, + TransactionStep, CRC_32, }; +use alloc::boxed::Box; use smallvec::SmallVec; use spacepackets::{ cfdp::{ @@ -19,10 +21,10 @@ use spacepackets::{ file_data::FileDataPdu, finished::{DeliveryCode, FileStatus, FinishedPdu}, metadata::{MetadataGenericParams, MetadataPdu}, - CommonPduConfig, FileDirectiveType, PduError, PduHeader, + CommonPduConfig, FileDirectiveType, PduError, PduHeader, WritablePduPacket, }, tlv::{msg_to_user::MsgToUserTlv, EntityIdTlv, TlvType}, - ConditionCode, PduType, TransmissionMode, + ConditionCode, PduType, }, util::UnsignedByteField, }; @@ -34,6 +36,8 @@ pub struct DestinationHandler { state: State, tparams: TransactionParams, packets_to_send_ctx: PacketsToSendContext, + remote_cfg_table: Box, + check_timer_creator: Box, } #[derive(Debug, Default)] @@ -152,25 +156,38 @@ pub enum DestError { } impl DestinationHandler { - pub fn new(id: impl Into) -> Self { + pub fn new( + entity_id: impl Into, + remote_cfg_table: Box, + check_timer_creator: Box, + ) -> Self { Self { - id: id.into(), + id: entity_id.into(), step: TransactionStep::Idle, state: State::Idle, tparams: Default::default(), packets_to_send_ctx: Default::default(), + remote_cfg_table, + check_timer_creator, } } - pub fn state_machine(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { + pub fn state_machine( + &mut self, + cfdp_user: &mut impl CfdpUser, + packet_to_insert: Option<&PacketInfo>, + ) -> Result<(), DestError> { + if let Some(packet) = packet_to_insert { + self.insert_packet(packet)?; + } match self.state { State::Idle => todo!(), - State::BusyClass1Nacked => self.fsm_nacked(cfdp_user), - State::BusyClass2Acked => todo!("acknowledged mode not implemented yet"), + State::Busy => self.fsm_busy(cfdp_user), + State::Suspended => todo!(), } } - pub fn insert_packet(&mut self, packet_info: &PacketInfo) -> Result<(), DestError> { + fn insert_packet(&mut self, packet_info: &PacketInfo) -> Result<(), DestError> { if packet_info.target() != PacketTarget::DestEntity { // Unwrap is okay here, a PacketInfo for a file data PDU should always have the // destination as the target. @@ -297,11 +314,7 @@ impl DestinationHandler { } } } - if self.tparams.pdu_conf.trans_mode == TransmissionMode::Unacknowledged { - self.state = State::BusyClass1Nacked; - } else { - self.state = State::BusyClass2Acked; - } + self.state = State::Busy; self.step = TransactionStep::TransactionStart; Ok(()) } @@ -338,7 +351,7 @@ impl DestinationHandler { // TODO: Check progress, and implement transfer completion timer as specified in the // standard. This timer protects against out of order arrival of packets. if self.tparams.tstate.progress != self.tparams.file_size() {} - if self.state == State::BusyClass1Nacked { + if self.state == State::Busy { self.step = TransactionStep::TransferCompletion; } else { self.step = TransactionStep::SendingAckPdu; @@ -367,7 +380,7 @@ impl DestinationHandler { Ok(false) } - fn fsm_nacked(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { + fn fsm_busy(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { if self.step == TransactionStep::TransactionStart { self.transaction_start(cfdp_user)?; } @@ -496,7 +509,10 @@ impl DestinationHandler { #[cfg(test)] mod tests { - use core::sync::atomic::{AtomicU8, Ordering}; + use core::{ + cell::Cell, + sync::atomic::{AtomicU8, Ordering}, + }; #[allow(unused_imports)] use std::println; use std::{env::temp_dir, fs}; @@ -504,10 +520,14 @@ mod tests { use alloc::{format, string::String}; use rand::Rng; use spacepackets::{ - cfdp::{lv::Lv, ChecksumType}, + cfdp::{lv::Lv, pdu::WritablePduPacket, ChecksumType, TransmissionMode}, util::{UbfU16, UnsignedByteFieldU16}, }; + use crate::cfdp::{ + CheckTimer, CheckTimerCreator, RemoteEntityConfig, StdRemoteEntityConfigProvider, + }; + use super::*; const LOCAL_ID: UnsignedByteFieldU16 = UnsignedByteFieldU16::new(1); @@ -608,6 +628,63 @@ mod tests { } } + struct TestCheckTimer { + counter: Cell, + expiry_count: u32, + } + + impl CheckTimer for TestCheckTimer { + fn has_expired(&self) -> bool { + let current_counter = self.counter.get(); + if self.expiry_count == current_counter { + return true; + } + self.counter.set(current_counter + 1); + false + } + } + + impl TestCheckTimer { + pub fn new(expiry_after_x_calls: u32) -> Self { + Self { + counter: Cell::new(0), + expiry_count: expiry_after_x_calls, + } + } + } + + struct TestCheckTimerCreator { + expiry_counter_for_source_entity: u32, + expiry_counter_for_dest_entity: u32, + } + + impl TestCheckTimerCreator { + pub fn new( + expiry_counter_for_source_entity: u32, + expiry_counter_for_dest_entity: u32, + ) -> Self { + Self { + expiry_counter_for_source_entity, + expiry_counter_for_dest_entity, + } + } + } + + impl CheckTimerCreator for TestCheckTimerCreator { + fn get_check_timer_provider( + &self, + _local_id: &UnsignedByteField, + _remote_id: &UnsignedByteField, + entity_type: crate::cfdp::EntityType, + ) -> Box { + if entity_type == crate::cfdp::EntityType::Sending { + Box::new(TestCheckTimer::new(self.expiry_counter_for_source_entity)) + } else { + Box::new(TestCheckTimer::new(self.expiry_counter_for_dest_entity)) + } + } + } + fn init_check(handler: &DestinationHandler) { assert_eq!(handler.state(), State::Idle); assert_eq!(handler.step(), TransactionStep::Idle); @@ -626,9 +703,31 @@ mod tests { (src_path, file_path) } + fn basic_remote_cfg_table() -> StdRemoteEntityConfigProvider { + let mut table = StdRemoteEntityConfigProvider::default(); + let remote_entity_cfg = RemoteEntityConfig::new_with_default_values( + UnsignedByteFieldU16::new(1).into(), + 1024, + 1024, + true, + true, + TransmissionMode::Unacknowledged, + ChecksumType::Crc32, + ); + table.add_config(&remote_entity_cfg); + table + } + + fn default_dest_handler() -> DestinationHandler { + DestinationHandler::new( + REMOTE_ID, + Box::new(basic_remote_cfg_table()), + Box::new(TestCheckTimerCreator::new(2, 2)), + ) + } #[test] fn test_basic() { - let dest_handler = DestinationHandler::new(REMOTE_ID); + let dest_handler = default_dest_handler(); init_check(&dest_handler); } @@ -655,37 +754,20 @@ mod tests { ) } - fn insert_metadata_pdu( - metadata_pdu: &MetadataPdu, - buf: &mut [u8], - dest_handler: &mut DestinationHandler, - ) { - let written_len = metadata_pdu + fn create_packet_info<'a>( + pdu: &'a impl WritablePduPacket, + buf: &'a mut [u8], + ) -> PacketInfo<'a> { + let written_len = pdu .write_to_bytes(buf) .expect("writing metadata PDU failed"); - let packet_info = - PacketInfo::new(&buf[..written_len]).expect("generating packet info failed"); - let insert_result = dest_handler.insert_packet(&packet_info); - if let Err(e) = insert_result { - panic!("insert result error: {e}"); - } + PacketInfo::new(&buf[..written_len]).expect("generating packet info failed") } - - fn insert_eof_pdu( - file_data: &[u8], - pdu_header: &PduHeader, - buf: &mut [u8], - dest_handler: &mut DestinationHandler, - ) { + fn create_no_error_eof(file_data: &[u8], pdu_header: &PduHeader, buf: &mut [u8]) -> EofPdu { let mut digest = CRC_32.digest(); digest.update(file_data); let crc32 = digest.finalize(); - let eof_pdu = EofPdu::new_no_error(*pdu_header, crc32, file_data.len() as u64); - let result = eof_pdu.write_to_bytes(buf); - assert!(result.is_ok()); - let packet_info = PacketInfo::new(&buf).expect("generating packet info failed"); - let result = dest_handler.insert_packet(&packet_info); - assert!(result.is_ok()); + EofPdu::new_no_error(*pdu_header, crc32, file_data.len() as u64) } #[test] @@ -700,23 +782,24 @@ mod tests { expected_file_size: 0, }; // We treat the destination handler like it is a remote entity. - let mut dest_handler = DestinationHandler::new(REMOTE_ID); + let mut dest_handler = default_dest_handler(); init_check(&dest_handler); let seq_num = UbfU16::new(0); let pdu_header = create_pdu_header(seq_num); let metadata_pdu = create_metadata_pdu(&pdu_header, src_name.as_path(), dest_name.as_path(), 0); - insert_metadata_pdu(&metadata_pdu, &mut buf, &mut dest_handler); - let result = dest_handler.state_machine(&mut test_user); + let packet_info = create_packet_info(&metadata_pdu, &mut buf); + let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); if let Err(e) = result { panic!("dest handler fsm error: {e}"); } assert_ne!(dest_handler.state(), State::Idle); assert_eq!(dest_handler.step(), TransactionStep::ReceivingFileDataPdus); - insert_eof_pdu(&[], &pdu_header, &mut buf, &mut dest_handler); - let result = dest_handler.state_machine(&mut test_user); + let eof_pdu = create_no_error_eof(&[], &pdu_header, &mut buf); + let packet_info = create_packet_info(&eof_pdu, &mut buf); + let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); assert_eq!(dest_handler.state(), State::Idle); assert_eq!(dest_handler.step(), TransactionStep::Idle); @@ -740,7 +823,7 @@ mod tests { expected_file_size: file_data.len(), }; // We treat the destination handler like it is a remote entity. - let mut dest_handler = DestinationHandler::new(REMOTE_ID); + let mut dest_handler = default_dest_handler(); init_check(&dest_handler); let seq_num = UbfU16::new(0); @@ -751,8 +834,8 @@ mod tests { dest_name.as_path(), file_data.len() as u64, ); - insert_metadata_pdu(&metadata_pdu, &mut buf, &mut dest_handler); - let result = dest_handler.state_machine(&mut test_user); + let packet_info = create_packet_info(&metadata_pdu, &mut buf); + let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); if let Err(e) = result { panic!("dest handler fsm error: {e}"); } @@ -769,11 +852,12 @@ mod tests { if let Err(e) = result { panic!("destination handler packet insertion error: {e}"); } - let result = dest_handler.state_machine(&mut test_user); + let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); - insert_eof_pdu(file_data, &pdu_header, &mut buf, &mut dest_handler); - let result = dest_handler.state_machine(&mut test_user); + let eof_pdu = create_no_error_eof(&file_data, &pdu_header, &mut buf); + let packet_info = create_packet_info(&eof_pdu, &mut buf); + let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); assert_eq!(dest_handler.state(), State::Idle); assert_eq!(dest_handler.step(), TransactionStep::Idle); @@ -800,7 +884,7 @@ mod tests { }; // We treat the destination handler like it is a remote entity. - let mut dest_handler = DestinationHandler::new(REMOTE_ID); + let mut dest_handler = default_dest_handler(); init_check(&dest_handler); let seq_num = UbfU16::new(0); @@ -811,8 +895,8 @@ mod tests { dest_name.as_path(), random_data.len() as u64, ); - insert_metadata_pdu(&metadata_pdu, &mut buf, &mut dest_handler); - let result = dest_handler.state_machine(&mut test_user); + let packet_info = create_packet_info(&metadata_pdu, &mut buf); + let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); if let Err(e) = result { panic!("dest handler fsm error: {e}"); } @@ -835,7 +919,7 @@ mod tests { if let Err(e) = result { panic!("destination handler packet insertion error: {e}"); } - let result = dest_handler.state_machine(&mut test_user); + let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); // Second file data PDU @@ -853,11 +937,12 @@ mod tests { if let Err(e) = result { panic!("destination handler packet insertion error: {e}"); } - let result = dest_handler.state_machine(&mut test_user); + let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); - insert_eof_pdu(&random_data, &pdu_header, &mut buf, &mut dest_handler); - let result = dest_handler.state_machine(&mut test_user); + let eof_pdu = create_no_error_eof(&random_data, &pdu_header, &mut buf); + let packet_info = create_packet_info(&eof_pdu, &mut buf); + let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); assert_eq!(dest_handler.state(), State::Idle); assert_eq!(dest_handler.step(), TransactionStep::Idle); diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index dc6e87a..58edbff 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -1,4 +1,7 @@ +use core::hash::Hash; + use crc::{Crc, CRC_32_CKSUM}; +use hashbrown::HashMap; use spacepackets::{ cfdp::{ pdu::{FileDirectiveType, PduError, PduHeader}, @@ -35,7 +38,7 @@ pub enum EntityType { /// For the receiving entity, this timer determines the expiry period for incrementing a check /// counter after an EOF PDU is received for an incomplete file transfer. This allows out-of-order /// reception of file data PDUs and EOF PDUs. Also see 4.6.3.3 of the CFDP standard. -pub trait CheckTimerProvider { +pub trait CheckTimer { fn has_expired(&self) -> bool; } @@ -50,10 +53,11 @@ pub trait CheckTimerProvider { #[cfg(feature = "alloc")] pub trait CheckTimerCreator { fn get_check_timer_provider( + &self, local_id: &UnsignedByteField, remote_id: &UnsignedByteField, entity_type: EntityType, - ) -> Box; + ) -> Box; } /// Simple implementation of the [CheckTimerProvider] trait assuming a standard runtime. @@ -75,7 +79,7 @@ impl StdCheckTimer { } #[cfg(feature = "std")] -impl CheckTimerProvider for StdCheckTimer { +impl CheckTimer for StdCheckTimer { fn has_expired(&self) -> bool { let elapsed_time = self.start_time.elapsed(); if elapsed_time.as_secs() > self.expiry_time_seconds { @@ -85,22 +89,78 @@ impl CheckTimerProvider for StdCheckTimer { } } -#[derive(Debug)] +#[derive(Debug, Copy, Clone)] pub struct RemoteEntityConfig { pub entity_id: UnsignedByteField, pub max_file_segment_len: usize, - pub closure_requeted_by_default: bool, + pub max_packet_len: usize, + pub closure_requested_by_default: bool, pub crc_on_transmission_by_default: bool, pub default_transmission_mode: TransmissionMode, pub default_crc_type: ChecksumType, pub check_limit: u32, } -pub trait RemoteEntityConfigProvider { - fn get_remote_config(&self, remote_id: &UnsignedByteField) -> Option<&RemoteEntityConfig>; +impl RemoteEntityConfig { + pub fn new_with_default_values( + entity_id: UnsignedByteField, + max_file_segment_len: usize, + max_packet_len: usize, + closure_requested_by_default: bool, + crc_on_transmission_by_default: bool, + default_transmission_mode: TransmissionMode, + default_crc_type: ChecksumType, + ) -> Self { + Self { + entity_id, + max_file_segment_len, + max_packet_len, + closure_requested_by_default, + crc_on_transmission_by_default, + default_transmission_mode, + default_crc_type, + check_limit: 2, + } + } } -#[derive(Debug, PartialEq, Eq, Copy, Clone)] +pub trait RemoteEntityConfigProvider { + /// Retrieve the remote entity configuration for the given remote ID. + fn get_remote_config(&self, remote_id: u64) -> Option<&RemoteEntityConfig>; + fn get_remote_config_mut(&mut self, remote_id: u64) -> Option<&mut RemoteEntityConfig>; + /// Add a new remote configuration. Return [True] if the configuration was + /// inserted successfully, and [False] if a configuration already exists. + fn add_config(&mut self, cfg: &RemoteEntityConfig) -> bool; + /// Remote a configuration. Returns [True] if the configuration was removed successfully, + /// and [False] if no configuration exists for the given remote ID. + fn remove_config(&mut self, remote_id: u64) -> bool; +} + +#[cfg(feature = "std")] +#[derive(Default)] +pub struct StdRemoteEntityConfigProvider { + remote_cfg_table: HashMap, +} + +#[cfg(feature = "std")] +impl RemoteEntityConfigProvider for StdRemoteEntityConfigProvider { + fn get_remote_config(&self, remote_id: u64) -> Option<&RemoteEntityConfig> { + self.remote_cfg_table.get(&remote_id) + } + fn get_remote_config_mut(&mut self, remote_id: u64) -> Option<&mut RemoteEntityConfig> { + self.remote_cfg_table.get_mut(&remote_id) + } + fn add_config(&mut self, cfg: &RemoteEntityConfig) -> bool { + self.remote_cfg_table + .insert(cfg.entity_id.value(), *cfg) + .is_some() + } + fn remove_config(&mut self, remote_id: u64) -> bool { + self.remote_cfg_table.remove(&remote_id).is_some() + } +} + +#[derive(Debug, Eq, Copy, Clone)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct TransactionId { source_id: UnsignedByteField, @@ -121,6 +181,20 @@ impl TransactionId { } } +impl Hash for TransactionId { + fn hash(&self, state: &mut H) { + self.source_id.value().hash(state); + self.seq_num.value().hash(state); + } +} + +impl PartialEq for TransactionId { + fn eq(&self, other: &Self) -> bool { + self.source_id.value() == other.source_id.value() + && self.seq_num.value() == other.seq_num.value() + } +} + #[derive(Debug, Copy, Clone, PartialEq, Eq)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub enum TransactionStep { @@ -136,8 +210,8 @@ pub enum TransactionStep { #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub enum State { Idle = 0, - BusyClass1Nacked = 2, - BusyClass2Acked = 3, + Busy = 1, + Suspended = 2, } pub const CRC_32: Crc = Crc::::new(&CRC_32_CKSUM); @@ -249,7 +323,7 @@ mod tests { eof::EofPdu, file_data::FileDataPdu, metadata::{MetadataGenericParams, MetadataPdu}, - CommonPduConfig, FileDirectiveType, PduHeader, + CommonPduConfig, FileDirectiveType, PduHeader, WritablePduPacket, }, PduType, }; -- 2.43.0 From 274ae654cd5eaa7710d1bd6e6a6e614d494c520a Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 24 Nov 2023 15:02:22 +0100 Subject: [PATCH 02/33] add new filesystem abstractions --- satrs-core/Cargo.toml | 4 +- satrs-core/src/cfdp/dest.rs | 6 +- satrs-core/src/cfdp/filestore.rs | 257 +++++++++++++++++++++++++++++++ satrs-core/src/cfdp/mod.rs | 1 + 4 files changed, 263 insertions(+), 5 deletions(-) create mode 100644 satrs-core/src/cfdp/filestore.rs diff --git a/satrs-core/Cargo.toml b/satrs-core/Cargo.toml index 65ee5e7..2b6d78c 100644 --- a/satrs-core/Cargo.toml +++ b/satrs-core/Cargo.toml @@ -76,8 +76,7 @@ optional = true # version = "0.7.0-beta.2" default-features = false git = "https://egit.irs.uni-stuttgart.de/rust/spacepackets.git" -# rev = "79d26e1a6" -branch = "writable_pdu_packet" +rev = "9e74266b768df80fd4118b04c4cc997aba8c85b9" [dependencies.cobs] git = "https://github.com/robamu/cobs.rs.git" @@ -91,6 +90,7 @@ zerocopy = "0.7" once_cell = "1.13" serde_json = "1" rand = "0.8" +tempfile = "3" [dev-dependencies.postcard] version = "1" diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index f54d22c..5cbc745 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -771,7 +771,7 @@ mod tests { } #[test] - fn test_empty_file_transfer() { + fn test_empty_file_transfer_not_acked() { let (src_name, dest_name) = init_full_filenames(); assert!(!Path::exists(&dest_name)); let mut buf: [u8; 512] = [0; 512]; @@ -810,7 +810,7 @@ mod tests { } #[test] - fn test_small_file_transfer() { + fn test_small_file_transfer_not_acked() { let (src_name, dest_name) = init_full_filenames(); assert!(!Path::exists(&dest_name)); let file_data_str = "Hello World!"; @@ -869,7 +869,7 @@ mod tests { } #[test] - fn test_segmented_file_transfer() { + fn test_segmented_file_transfer_not_acked() { let (src_name, dest_name) = init_full_filenames(); assert!(!Path::exists(&dest_name)); let mut rng = rand::thread_rng(); diff --git a/satrs-core/src/cfdp/filestore.rs b/satrs-core/src/cfdp/filestore.rs new file mode 100644 index 0000000..5b03594 --- /dev/null +++ b/satrs-core/src/cfdp/filestore.rs @@ -0,0 +1,257 @@ +use alloc::string::{String, ToString}; +use crc::{Crc, CRC_32_CKSUM}; +use spacepackets::cfdp::ChecksumType; +use spacepackets::ByteConversionError; +#[cfg(feature = "std")] +pub use stdmod::*; + +pub const CRC_32: Crc = Crc::::new(&CRC_32_CKSUM); + +pub enum FilestoreError { + FileDoesNotExist, + FileAlreadyExists, + DirDoesNotExist, + Permission, + IsNotFile, + IsNotDirectory, + ByteConversion(ByteConversionError), + Io { + raw_errno: Option, + string: String, + }, + ChecksumTypeNotImplemented(ChecksumType), +} + +impl From for FilestoreError { + fn from(value: ByteConversionError) -> Self { + Self::ByteConversion(value) + } +} + +#[cfg(feature = "std")] +impl From for FilestoreError { + fn from(value: std::io::Error) -> Self { + Self::Io { + raw_errno: value.raw_os_error(), + string: value.to_string(), + } + } +} + +pub trait VirtualFilestore { + fn create_file(&self, file_path: &str) -> Result<(), FilestoreError>; + + fn remove_file(&self, file_path: &str) -> Result<(), FilestoreError>; + + /// Truncating a file means deleting all its data so the resulting file is empty. + /// This can be more efficient than removing and re-creating a file. + fn truncate_file(&self, file_path: &str) -> Result<(), FilestoreError>; + + fn remove_dir(&self, file_path: &str, all: bool) -> Result<(), FilestoreError>; + + fn read_data( + &self, + file_path: &str, + offset: u64, + read_len: u64, + buf: &mut [u8], + ) -> Result<(), FilestoreError>; + + fn write_data(&self, file: &str, offset: u64, buf: &[u8]) -> Result<(), FilestoreError>; + + fn filename_from_full_path<'a>(&self, path: &'a str) -> Option<&'a str>; + + fn is_file(&self, path: &str) -> bool; + + fn is_dir(&self, path: &str) -> bool { + !self.is_file(path) + } + + fn exists(&self, path: &str) -> bool; + + /// This special function is the CFDP specific abstraction to verify the checksum of a file. + /// This allows to keep OS specific details like reading the whole file in the most efficient + /// manner inside the file system abstraction. + fn checksum_verify( + &self, + file_path: &str, + checksum_type: ChecksumType, + expected_checksum: u32, + verification_buf: &mut [u8], + ) -> Result; +} + +#[cfg(feature = "std")] +pub mod stdmod { + use super::*; + use std::{ + fs::{self, File, OpenOptions}, + io::{BufReader, Read, Seek, SeekFrom, Write}, + path::Path, + }; + + pub struct NativeFilestore {} + + impl VirtualFilestore for NativeFilestore { + fn create_file(&self, file_path: &str) -> Result<(), FilestoreError> { + if self.exists(file_path) { + return Err(FilestoreError::FileAlreadyExists); + } + File::create(file_path)?; + Ok(()) + } + + fn remove_file(&self, file_path: &str) -> Result<(), FilestoreError> { + if !self.exists(file_path) { + return Ok(()); + } + if !self.is_file(file_path) { + return Err(FilestoreError::IsNotFile); + } + fs::remove_file(file_path)?; + Ok(()) + } + + fn truncate_file(&self, file_path: &str) -> Result<(), FilestoreError> { + if !self.exists(file_path) { + return Ok(()); + } + if !self.is_file(file_path) { + return Err(FilestoreError::IsNotFile); + } + OpenOptions::new() + .write(true) + .truncate(true) + .open(file_path)?; + Ok(()) + } + + fn remove_dir(&self, dir_path: &str, all: bool) -> Result<(), FilestoreError> { + if !self.exists(dir_path) { + return Err(FilestoreError::DirDoesNotExist); + } + if !self.is_dir(dir_path) { + return Err(FilestoreError::IsNotDirectory); + } + if !all { + fs::remove_dir(dir_path)?; + return Ok(()); + } + fs::remove_dir_all(dir_path)?; + Ok(()) + } + + fn read_data( + &self, + file_name: &str, + offset: u64, + read_len: u64, + buf: &mut [u8], + ) -> Result<(), FilestoreError> { + if buf.len() < read_len as usize { + return Err(ByteConversionError::ToSliceTooSmall { + found: buf.len(), + expected: read_len as usize, + } + .into()); + } + if !self.exists(file_name) { + return Err(FilestoreError::FileDoesNotExist); + } + if !self.is_file(file_name) { + return Err(FilestoreError::IsNotFile); + } + let mut file = File::open(file_name)?; + file.seek(SeekFrom::Start(offset))?; + file.read_exact(&mut buf[0..read_len as usize])?; + Ok(()) + } + + fn write_data(&self, file: &str, offset: u64, buf: &[u8]) -> Result<(), FilestoreError> { + if !self.exists(file) { + return Err(FilestoreError::FileDoesNotExist); + } + if !self.is_file(file) { + return Err(FilestoreError::IsNotFile); + } + let mut file = File::open(file)?; + file.seek(SeekFrom::Start(offset))?; + file.write_all(buf)?; + Ok(()) + } + + fn filename_from_full_path<'a>(&self, path: &'a str) -> Option<&'a str> { + // Convert the path string to a Path + let path = Path::new(path); + + // Extract the file name using the file_name() method + path.file_name().and_then(|name| name.to_str()) + } + + fn is_file(&self, path: &str) -> bool { + let path = Path::new(path); + path.is_file() + } + + fn is_dir(&self, path: &str) -> bool { + let path = Path::new(path); + path.is_dir() + } + + fn exists(&self, path: &str) -> bool { + let path = Path::new(path); + if !path.exists() { + return false; + } + true + } + + fn checksum_verify( + &self, + file_path: &str, + checksum_type: ChecksumType, + expected_checksum: u32, + verification_buf: &mut [u8], + ) -> Result { + match checksum_type { + ChecksumType::Modular => { + todo!(); + } + ChecksumType::Crc32 => { + let mut digest = CRC_32.digest(); + let file_to_check = File::open(file_path)?; + let mut buf_reader = BufReader::new(file_to_check); + loop { + let bytes_read = buf_reader.read(verification_buf)?; + if bytes_read == 0 { + break; + } + digest.update(&verification_buf[0..bytes_read]); + } + if digest.finalize() == expected_checksum { + return Ok(true); + } + Ok(false) + } + ChecksumType::NullChecksum => Ok(true), + _ => Err(FilestoreError::ChecksumTypeNotImplemented(checksum_type)), + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + #[test] + fn test_basic_native_filestore() { + let tmpdir = tempdir().expect("creating tmpdir failed"); + let file_path = tmpdir.path().join("test.txt"); + let native_filestore = NativeFilestore {}; + let result = + native_filestore.create_file(file_path.to_str().expect("getting str for file failed")); + assert!(result.is_ok()); + } +} diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index 58edbff..f99712e 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -17,6 +17,7 @@ use serde::{Deserialize, Serialize}; #[cfg(feature = "std")] pub mod dest; +pub mod filestore; #[cfg(feature = "std")] pub mod source; pub mod user; -- 2.43.0 From da8858eae098fb5e217e998b6bab9f08c8b4b4da Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 24 Nov 2023 15:47:03 +0100 Subject: [PATCH 03/33] well this is annoying.. --- satrs-core/src/cfdp/filestore.rs | 120 +++++++++++++++++++++++++++++-- 1 file changed, 116 insertions(+), 4 deletions(-) diff --git a/satrs-core/src/cfdp/filestore.rs b/satrs-core/src/cfdp/filestore.rs index 5b03594..97bcba4 100644 --- a/satrs-core/src/cfdp/filestore.rs +++ b/satrs-core/src/cfdp/filestore.rs @@ -1,12 +1,16 @@ use alloc::string::{String, ToString}; +use core::fmt::Display; use crc::{Crc, CRC_32_CKSUM}; use spacepackets::cfdp::ChecksumType; use spacepackets::ByteConversionError; #[cfg(feature = "std")] +use std::error::Error; +#[cfg(feature = "std")] pub use stdmod::*; pub const CRC_32: Crc = Crc::::new(&CRC_32_CKSUM); +#[derive(Debug, Clone)] pub enum FilestoreError { FileDoesNotExist, FileAlreadyExists, @@ -28,6 +32,53 @@ impl From for FilestoreError { } } +impl Display for FilestoreError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + FilestoreError::FileDoesNotExist => { + write!(f, "file does not exist") + } + FilestoreError::FileAlreadyExists => { + write!(f, "file already exists") + } + FilestoreError::DirDoesNotExist => { + write!(f, "directory does not exist") + } + FilestoreError::Permission => { + write!(f, "permission error") + } + FilestoreError::IsNotFile => { + write!(f, "is not a file") + } + FilestoreError::IsNotDirectory => { + write!(f, "is not a directory") + } + FilestoreError::ByteConversion(e) => { + write!(f, "filestore error: {e}") + } + FilestoreError::Io { raw_errno, string } => { + write!( + f, + "filestore generic IO error with raw errno {:?}: {}", + raw_errno, string + ) + } + FilestoreError::ChecksumTypeNotImplemented(checksum_type) => { + write!(f, "checksum {:?} not implemented", checksum_type) + } + } + } +} + +impl Error for FilestoreError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + match self { + FilestoreError::ByteConversion(e) => Some(e), + _ => None, + } + } +} + #[cfg(feature = "std")] impl From for FilestoreError { fn from(value: std::io::Error) -> Self { @@ -174,7 +225,7 @@ pub mod stdmod { if !self.is_file(file) { return Err(FilestoreError::IsNotFile); } - let mut file = File::open(file)?; + let mut file = OpenOptions::new().write(true).open(file)?; file.seek(SeekFrom::Start(offset))?; file.write_all(buf)?; Ok(()) @@ -242,16 +293,77 @@ pub mod stdmod { #[cfg(test)] mod tests { + use std::{fs, path::Path, println}; + use super::*; use tempfile::tempdir; + const NATIVE_FS: NativeFilestore = NativeFilestore {}; + #[test] - fn test_basic_native_filestore() { + fn test_basic_native_filestore_create() { let tmpdir = tempdir().expect("creating tmpdir failed"); let file_path = tmpdir.path().join("test.txt"); - let native_filestore = NativeFilestore {}; let result = - native_filestore.create_file(file_path.to_str().expect("getting str for file failed")); + NATIVE_FS.create_file(file_path.to_str().expect("getting str for file failed")); assert!(result.is_ok()); + let path = Path::new(&file_path); + assert!(path.exists()); + assert!(NATIVE_FS.exists(file_path.to_str().unwrap())); + assert!(NATIVE_FS.is_file(file_path.to_str().unwrap())); + fs::remove_dir_all(tmpdir).expect("clearing tmpdir failed"); + } + + #[test] + fn test_basic_native_fs_exists() { + let tmpdir = tempdir().expect("creating tmpdir failed"); + let file_path = tmpdir.path().join("test.txt"); + assert!(!NATIVE_FS.exists(file_path.to_str().unwrap())); + NATIVE_FS + .create_file(file_path.to_str().expect("getting str for file failed")) + .unwrap(); + assert!(NATIVE_FS.exists(file_path.to_str().unwrap())); + assert!(NATIVE_FS.is_file(file_path.to_str().unwrap())); + fs::remove_dir_all(tmpdir).expect("clearing tmpdir failed"); + } + + #[test] + fn test_basic_native_fs_write() { + let tmpdir = tempdir().expect("creating tmpdir failed"); + let file_path = tmpdir.path().join("test.txt"); + assert!(!NATIVE_FS.exists(file_path.to_str().unwrap())); + NATIVE_FS + .create_file(file_path.to_str().expect("getting str for file failed")) + .unwrap(); + assert!(NATIVE_FS.exists(file_path.to_str().unwrap())); + assert!(NATIVE_FS.is_file(file_path.to_str().unwrap())); + println!("{}", file_path.to_str().unwrap()); + let write_data = "hello world\n"; + NATIVE_FS + .write_data(file_path.to_str().unwrap(), 0, write_data.as_bytes()) + .expect("writing to file failed"); + let read_back = fs::read_to_string(file_path).expect("reading back data failed"); + assert_eq!(read_back, write_data); + fs::remove_dir_all(tmpdir).expect("clearing tmpdir failed"); + } + + #[test] + fn test_basic_native_fs_read() { + let tmpdir = tempdir().expect("creating tmpdir failed"); + let file_path = tmpdir.path().join("test.txt"); + assert!(!NATIVE_FS.exists(file_path.to_str().unwrap())); + NATIVE_FS + .create_file(file_path.to_str().expect("getting str for file failed")) + .unwrap(); + assert!(NATIVE_FS.exists(file_path.to_str().unwrap())); + assert!(NATIVE_FS.is_file(file_path.to_str().unwrap())); + println!("{}", file_path.to_str().unwrap()); + let write_data = "hello world\n"; + NATIVE_FS + .write_data(file_path.to_str().unwrap(), 0, write_data.as_bytes()) + .expect("writing to file failed"); + let read_back = fs::read_to_string(file_path).expect("reading back data failed"); + assert_eq!(read_back, write_data); + fs::remove_dir_all(tmpdir).expect("clearing tmpdir failed"); } } -- 2.43.0 From f6d2cfa042d880240242cc4d4af473f9ae1ed024 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 6 Dec 2023 18:20:29 +0100 Subject: [PATCH 04/33] let's try the new spacepackets version --- satrs-core/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/satrs-core/Cargo.toml b/satrs-core/Cargo.toml index 2b6d78c..a3391fb 100644 --- a/satrs-core/Cargo.toml +++ b/satrs-core/Cargo.toml @@ -76,7 +76,7 @@ optional = true # version = "0.7.0-beta.2" default-features = false git = "https://egit.irs.uni-stuttgart.de/rust/spacepackets.git" -rev = "9e74266b768df80fd4118b04c4cc997aba8c85b9" +rev = "47a93354952834eea435f8ec6c561af4750a2068" [dependencies.cobs] git = "https://github.com/robamu/cobs.rs.git" -- 2.43.0 From 0b81198c0311f918475c79c3ad66b836c4119c54 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 6 Dec 2023 19:59:27 +0100 Subject: [PATCH 05/33] that will be a lot of API changes --- satrs-core/src/pus/verification.rs | 4 ++-- satrs-core/src/tmtc/tm_helper.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/satrs-core/src/pus/verification.rs b/satrs-core/src/pus/verification.rs index 47c2689..52606e8 100644 --- a/satrs-core/src/pus/verification.rs +++ b/satrs-core/src/pus/verification.rs @@ -87,7 +87,7 @@ use delegate::delegate; use serde::{Deserialize, Serialize}; use spacepackets::ecss::tc::IsPusTelecommand; use spacepackets::ecss::tm::{PusTmCreator, PusTmSecondaryHeader}; -use spacepackets::ecss::{EcssEnumeration, PusError, SerializablePusPacket}; +use spacepackets::ecss::{EcssEnumeration, PusError, WritablePusPacket}; use spacepackets::{CcsdsPacket, PacketId, PacketSequenceCtrl}; use spacepackets::{SpHeader, MAX_APID}; @@ -353,7 +353,7 @@ impl<'src_data, State, SuccessOrFailure> VerificationSendable<'src_data, State, } pub fn len_packed(&self) -> usize { - self.pus_tm.as_ref().unwrap().len_packed() + self.pus_tm.as_ref().unwrap().len_written() } pub fn pus_tm(&self) -> &PusTmCreator<'src_data> { diff --git a/satrs-core/src/tmtc/tm_helper.rs b/satrs-core/src/tmtc/tm_helper.rs index 06ba532..6dca7ee 100644 --- a/satrs-core/src/tmtc/tm_helper.rs +++ b/satrs-core/src/tmtc/tm_helper.rs @@ -11,7 +11,7 @@ pub mod std_mod { use crate::pool::{ShareablePoolProvider, SharedPool, StoreAddr}; use crate::pus::EcssTmtcError; use spacepackets::ecss::tm::PusTmCreator; - use spacepackets::ecss::SerializablePusPacket; + use spacepackets::ecss::WritablePusPacket; use std::sync::{Arc, RwLock}; #[derive(Clone)] @@ -32,7 +32,7 @@ pub mod std_mod { pub fn add_pus_tm(&self, pus_tm: &PusTmCreator) -> Result { let mut pg = self.pool.write().map_err(|_| EcssTmtcError::StoreLock)?; - let (addr, buf) = pg.free_element(pus_tm.len_packed())?; + let (addr, buf) = pg.free_element(pus_tm.len_written())?; pus_tm .write_to_bytes(buf) .expect("writing PUS TM to store failed"); @@ -59,7 +59,7 @@ impl PusTmWithCdsShortHelper { &'a mut self, service: u8, subservice: u8, - source_data: Option<&'a [u8]>, + source_data: &'a [u8], seq_count: u16, ) -> PusTmCreator { let time_stamp = TimeProvider::from_now_with_u16_days().unwrap(); @@ -71,7 +71,7 @@ impl PusTmWithCdsShortHelper { &'a mut self, service: u8, subservice: u8, - source_data: Option<&'a [u8]>, + source_data: &'a [u8], stamper: &TimeProvider, seq_count: u16, ) -> PusTmCreator { @@ -83,7 +83,7 @@ impl PusTmWithCdsShortHelper { &'a self, service: u8, subservice: u8, - source_data: Option<&'a [u8]>, + source_data: &'a [u8], seq_count: u16, ) -> PusTmCreator { let mut reply_header = SpHeader::tm_unseg(self.apid, seq_count, 0).unwrap(); -- 2.43.0 From 774d9b5961dafca2e6fc1a0d2550d7098db7307e Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 7 Dec 2023 13:29:26 +0100 Subject: [PATCH 06/33] bumped spacepackets --- satrs-core/src/cfdp/dest.rs | 23 +++++++++---------- satrs-core/src/cfdp/mod.rs | 4 ++-- satrs-core/src/encoding/ccsds.rs | 2 +- .../src/hal/std/tcp_spacepackets_server.rs | 2 +- satrs-core/src/hal/std/udp_server.rs | 4 ++-- satrs-core/src/pus/event.rs | 2 +- satrs-core/src/pus/scheduler.rs | 2 +- satrs-core/src/pus/test.rs | 4 ++-- satrs-core/src/pus/verification.rs | 2 +- satrs-core/src/tmtc/ccsds_distrib.rs | 21 +++++++++-------- satrs-core/src/tmtc/pus_distrib.rs | 2 +- satrs-core/tests/hk_helpers.rs | 10 ++++---- satrs-core/tests/tcp_servers.rs | 2 +- 13 files changed, 40 insertions(+), 40 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 5cbc745..0ecdf70 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -19,11 +19,11 @@ use spacepackets::{ pdu::{ eof::EofPdu, file_data::FileDataPdu, - finished::{DeliveryCode, FileStatus, FinishedPdu}, - metadata::{MetadataGenericParams, MetadataPdu}, - CommonPduConfig, FileDirectiveType, PduError, PduHeader, WritablePduPacket, + finished::{DeliveryCode, FileStatus, FinishedPduCreator}, + metadata::{MetadataGenericParams, MetadataPduReader}, + CommonPduConfig, FileDirectiveType, PduError, PduHeader, WritablePduPacket, CfdpPdu, }, - tlv::{msg_to_user::MsgToUserTlv, EntityIdTlv, TlvType}, + tlv::{msg_to_user::MsgToUserTlv, EntityIdTlv, TlvType, GenericTlv}, ConditionCode, PduType, }, util::UnsignedByteField, @@ -227,7 +227,7 @@ impl DestinationHandler { let finished_pdu = if self.tparams.tstate.condition_code == ConditionCode::NoError || self.tparams.tstate.condition_code == ConditionCode::UnsupportedChecksumType { - FinishedPdu::new_default( + FinishedPduCreator::new_default( pdu_header, self.tparams.tstate.delivery_code, self.tparams.tstate.file_status, @@ -235,7 +235,7 @@ impl DestinationHandler { } else { // TODO: Are there cases where this ID is actually the source entity ID? let entity_id = EntityIdTlv::new(self.id); - FinishedPdu::new_with_error( + FinishedPduCreator::new_with_error( pdu_header, self.tparams.tstate.condition_code, self.tparams.tstate.delivery_code, @@ -283,7 +283,7 @@ impl DestinationHandler { if self.state != State::Idle { return Err(DestError::RecvdMetadataButIsBusy); } - let metadata_pdu = MetadataPdu::from_bytes(raw_packet)?; + let metadata_pdu = MetadataPduReader::from_bytes(raw_packet)?; self.tparams.reset(); self.tparams.tstate.metadata_params = *metadata_pdu.metadata_params(); let src_name = metadata_pdu.src_file_name(); @@ -302,7 +302,7 @@ impl DestinationHandler { self.tparams.file_properties.dest_file_name_len = dest_name.len_value(); self.tparams.pdu_conf = *metadata_pdu.pdu_header().common_pdu_conf(); self.tparams.msgs_to_user_size = 0; - if metadata_pdu.options().is_some() { + if !metadata_pdu.options().is_empty() { for option_tlv in metadata_pdu.options_iter().unwrap() { if option_tlv.is_standard_tlv() && option_tlv.tlv_type().unwrap() == TlvType::MsgToUser @@ -520,7 +520,7 @@ mod tests { use alloc::{format, string::String}; use rand::Rng; use spacepackets::{ - cfdp::{lv::Lv, pdu::WritablePduPacket, ChecksumType, TransmissionMode}, + cfdp::{lv::Lv, pdu::{WritablePduPacket, metadata::MetadataPduCreator}, ChecksumType, TransmissionMode}, util::{UbfU16, UnsignedByteFieldU16}, }; @@ -743,14 +743,13 @@ mod tests { src_name: &'filename Path, dest_name: &'filename Path, file_size: u64, - ) -> MetadataPdu<'filename, 'filename, 'static> { + ) -> MetadataPduCreator<'filename, 'filename, 'static> { let metadata_params = MetadataGenericParams::new(false, ChecksumType::Crc32, file_size); - MetadataPdu::new( + MetadataPduCreator::new_no_opts( *pdu_header, metadata_params, Lv::new_from_str(src_name.as_os_str().to_str().unwrap()).unwrap(), Lv::new_from_str(dest_name.as_os_str().to_str().unwrap()).unwrap(), - None, ) } diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index f99712e..a40c1ee 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -323,7 +323,7 @@ mod tests { pdu::{ eof::EofPdu, file_data::FileDataPdu, - metadata::{MetadataGenericParams, MetadataPdu}, + metadata::{MetadataGenericParams, MetadataPduCreator}, CommonPduConfig, FileDirectiveType, PduHeader, WritablePduPacket, }, PduType, @@ -347,7 +347,7 @@ mod tests { let dest_file_name = "hello-dest.txt"; let src_lv = Lv::new_from_str(src_file_name).unwrap(); let dest_lv = Lv::new_from_str(dest_file_name).unwrap(); - let metadata_pdu = MetadataPdu::new(pdu_header, metadata_params, src_lv, dest_lv, None); + let metadata_pdu = MetadataPduCreator::new_no_opts(pdu_header, metadata_params, src_lv, dest_lv); metadata_pdu .write_to_bytes(&mut buf) .expect("writing metadata PDU failed"); diff --git a/satrs-core/src/encoding/ccsds.rs b/satrs-core/src/encoding/ccsds.rs index ecd4ff5..37694d7 100644 --- a/satrs-core/src/encoding/ccsds.rs +++ b/satrs-core/src/encoding/ccsds.rs @@ -113,7 +113,7 @@ pub fn parse_buffer_for_ccsds_space_packets( #[cfg(test)] mod tests { use spacepackets::{ - ecss::{tc::PusTcCreator, SerializablePusPacket}, + ecss::{tc::PusTcCreator, WritablePusPacket}, PacketId, SpHeader, }; diff --git a/satrs-core/src/hal/std/tcp_spacepackets_server.rs b/satrs-core/src/hal/std/tcp_spacepackets_server.rs index c124a47..6ade0d1 100644 --- a/satrs-core/src/hal/std/tcp_spacepackets_server.rs +++ b/satrs-core/src/hal/std/tcp_spacepackets_server.rs @@ -173,7 +173,7 @@ mod tests { use alloc::{boxed::Box, sync::Arc}; use hashbrown::HashSet; use spacepackets::{ - ecss::{tc::PusTcCreator, SerializablePusPacket}, + ecss::{tc::PusTcCreator, WritablePusPacket}, PacketId, SpHeader, }; diff --git a/satrs-core/src/hal/std/udp_server.rs b/satrs-core/src/hal/std/udp_server.rs index 28e0328..6f2cb5b 100644 --- a/satrs-core/src/hal/std/udp_server.rs +++ b/satrs-core/src/hal/std/udp_server.rs @@ -19,7 +19,7 @@ use std::vec::Vec; /// /// ``` /// use std::net::{IpAddr, Ipv4Addr, SocketAddr, UdpSocket}; -/// use spacepackets::ecss::SerializablePusPacket; +/// use spacepackets::ecss::WritablePusPacket; /// use satrs_core::hal::std::udp_server::UdpTcServer; /// use satrs_core::tmtc::{ReceivesTc, ReceivesTcCore}; /// use spacepackets::SpHeader; @@ -144,7 +144,7 @@ mod tests { use crate::hal::std::udp_server::{ReceiveResult, UdpTcServer}; use crate::tmtc::ReceivesTcCore; use spacepackets::ecss::tc::PusTcCreator; - use spacepackets::ecss::SerializablePusPacket; + use spacepackets::ecss::WritablePusPacket; use spacepackets::SpHeader; use std::boxed::Box; use std::collections::VecDeque; diff --git a/satrs-core/src/pus/event.rs b/satrs-core/src/pus/event.rs index 165493d..9424b24 100644 --- a/satrs-core/src/pus/event.rs +++ b/satrs-core/src/pus/event.rs @@ -147,7 +147,7 @@ impl EventReporterBase { Ok(PusTmCreator::new( &mut sp_header, sec_header, - Some(&buf[0..current_idx]), + &buf[0..current_idx], true, )) } diff --git a/satrs-core/src/pus/scheduler.rs b/satrs-core/src/pus/scheduler.rs index ce98095..820b1f4 100644 --- a/satrs-core/src/pus/scheduler.rs +++ b/satrs-core/src/pus/scheduler.rs @@ -626,7 +626,7 @@ mod tests { use crate::pool::{LocalPool, PoolCfg, PoolProvider, StoreAddr, StoreError}; use alloc::collections::btree_map::Range; use spacepackets::ecss::tc::{PusTcCreator, PusTcReader, PusTcSecondaryHeader}; - use spacepackets::ecss::SerializablePusPacket; + use spacepackets::ecss::WritablePusPacket; use spacepackets::time::{cds, TimeWriter, UnixTimestamp}; use spacepackets::SpHeader; use std::time::Duration; diff --git a/satrs-core/src/pus/test.rs b/satrs-core/src/pus/test.rs index fd5c05a..fbef28a 100644 --- a/satrs-core/src/pus/test.rs +++ b/satrs-core/src/pus/test.rs @@ -72,7 +72,7 @@ impl PusServiceHandler for PusService17TestHandler { // Sequence count will be handled centrally in TM funnel. let mut reply_header = SpHeader::tm_unseg(self.psb.tm_apid, 0, 0).unwrap(); let tc_header = PusTmSecondaryHeader::new_simple(17, 2, &time_stamp); - let ping_reply = PusTmCreator::new(&mut reply_header, tc_header, None, true); + let ping_reply = PusTmCreator::new(&mut reply_header, tc_header, &[], true); let result = self .psb .tm_sender @@ -118,7 +118,7 @@ mod tests { use crate::tmtc::tm_helper::SharedTmStore; use spacepackets::ecss::tc::{PusTcCreator, PusTcSecondaryHeader}; use spacepackets::ecss::tm::PusTmReader; - use spacepackets::ecss::{PusPacket, SerializablePusPacket}; + use spacepackets::ecss::{PusPacket, WritablePusPacket}; use spacepackets::{SequenceFlags, SpHeader}; use std::boxed::Box; use std::sync::{mpsc, RwLock}; diff --git a/satrs-core/src/pus/verification.rs b/satrs-core/src/pus/verification.rs index 52606e8..cca7566 100644 --- a/satrs-core/src/pus/verification.rs +++ b/satrs-core/src/pus/verification.rs @@ -877,7 +877,7 @@ impl VerificationReporterCore { PusTmCreator::new( sp_header, tm_sec_header, - Some(&src_data_buf[0..source_data_len]), + &src_data_buf[0..source_data_len], true, ) } diff --git a/satrs-core/src/tmtc/ccsds_distrib.rs b/satrs-core/src/tmtc/ccsds_distrib.rs index 58fb8c6..ea9bf30 100644 --- a/satrs-core/src/tmtc/ccsds_distrib.rs +++ b/satrs-core/src/tmtc/ccsds_distrib.rs @@ -21,7 +21,7 @@ //! use satrs_core::tmtc::ccsds_distrib::{CcsdsPacketHandler, CcsdsDistributor}; //! use satrs_core::tmtc::{ReceivesTc, ReceivesTcCore}; //! use spacepackets::{CcsdsPacket, SpHeader}; -//! use spacepackets::ecss::SerializablePusPacket; +//! use spacepackets::ecss::WritablePusPacket; //! use spacepackets::ecss::tc::{PusTc, PusTcCreator}; //! //! #[derive (Default)] @@ -226,7 +226,7 @@ pub(crate) mod tests { use super::*; use crate::tmtc::ccsds_distrib::{CcsdsDistributor, CcsdsPacketHandler}; use spacepackets::ecss::tc::PusTcCreator; - use spacepackets::ecss::SerializablePusPacket; + use spacepackets::ecss::WritablePusPacket; use spacepackets::CcsdsPacket; use std::collections::VecDeque; use std::sync::{Arc, Mutex}; @@ -244,9 +244,10 @@ pub(crate) mod tests { &buf[0..size] } + type SharedPacketQueue = Arc)>>>; pub struct BasicApidHandlerSharedQueue { - pub known_packet_queue: Arc)>>>, - pub unknown_packet_queue: Arc)>>>, + pub known_packet_queue: SharedPacketQueue, + pub unknown_packet_queue: SharedPacketQueue, } #[derive(Default)] @@ -268,11 +269,11 @@ pub(crate) mod tests { ) -> Result<(), Self::Error> { let mut vec = Vec::new(); vec.extend_from_slice(tc_raw); - Ok(self - .known_packet_queue + self.known_packet_queue .lock() .unwrap() - .push_back((sp_header.apid(), vec))) + .push_back((sp_header.apid(), vec)); + Ok(()) } fn handle_unknown_apid( @@ -282,11 +283,11 @@ pub(crate) mod tests { ) -> Result<(), Self::Error> { let mut vec = Vec::new(); vec.extend_from_slice(tc_raw); - Ok(self - .unknown_packet_queue + self.unknown_packet_queue .lock() .unwrap() - .push_back((sp_header.apid(), vec))) + .push_back((sp_header.apid(), vec)); + Ok(()) } } diff --git a/satrs-core/src/tmtc/pus_distrib.rs b/satrs-core/src/tmtc/pus_distrib.rs index a400484..e4eefb1 100644 --- a/satrs-core/src/tmtc/pus_distrib.rs +++ b/satrs-core/src/tmtc/pus_distrib.rs @@ -18,7 +18,7 @@ //! # Example //! //! ```rust -//! use spacepackets::ecss::SerializablePusPacket; +//! use spacepackets::ecss::WritablePusPacket; //! use satrs_core::tmtc::pus_distrib::{PusDistributor, PusServiceProvider}; //! use satrs_core::tmtc::{ReceivesTc, ReceivesTcCore}; //! use spacepackets::SpHeader; diff --git a/satrs-core/tests/hk_helpers.rs b/satrs-core/tests/hk_helpers.rs index 07264ef..9228365 100644 --- a/satrs-core/tests/hk_helpers.rs +++ b/satrs-core/tests/hk_helpers.rs @@ -1,7 +1,7 @@ #![allow(dead_code)] use core::mem::size_of; use serde::{Deserialize, Serialize}; -use spacepackets::ecss::{Ptc, RealPfc, UnsignedPfc}; +use spacepackets::ecss::{Ptc, PfcReal, PfcUnsigned}; use spacepackets::time::cds::TimeProvider; use spacepackets::time::{CcsdsTimeProvider, TimeWriter}; @@ -64,7 +64,7 @@ impl TestMgmHkWithIndividualValidity { curr_idx += 1; buf[curr_idx] = Ptc::Real as u8; curr_idx += 1; - buf[curr_idx] = RealPfc::Float as u8; + buf[curr_idx] = PfcReal::Float as u8; curr_idx += 1; buf[curr_idx..curr_idx + size_of::()].copy_from_slice(&self.temp.val.to_be_bytes()); curr_idx += size_of::(); @@ -75,7 +75,7 @@ impl TestMgmHkWithIndividualValidity { curr_idx += 1; buf[curr_idx] = Ptc::UnsignedInt as u8; curr_idx += 1; - buf[curr_idx] = UnsignedPfc::TwoBytes as u8; + buf[curr_idx] = PfcUnsigned::TwoBytes as u8; curr_idx += 1; buf[curr_idx] = 3; curr_idx += 1; @@ -100,7 +100,7 @@ impl TestMgmHkWithGroupValidity { curr_idx += 1; buf[curr_idx] = Ptc::Real as u8; curr_idx += 1; - buf[curr_idx] = RealPfc::Float as u8; + buf[curr_idx] = PfcReal::Float as u8; curr_idx += 1; buf[curr_idx..curr_idx + size_of::()].copy_from_slice(&self.temp.to_be_bytes()); curr_idx += size_of::(); @@ -109,7 +109,7 @@ impl TestMgmHkWithGroupValidity { curr_idx += 1; buf[curr_idx] = Ptc::UnsignedInt as u8; curr_idx += 1; - buf[curr_idx] = UnsignedPfc::TwoBytes as u8; + buf[curr_idx] = PfcUnsigned::TwoBytes as u8; curr_idx += 1; buf[curr_idx] = 3; for val in self.mgm_vals { diff --git a/satrs-core/tests/tcp_servers.rs b/satrs-core/tests/tcp_servers.rs index 251eead..e5297c3 100644 --- a/satrs-core/tests/tcp_servers.rs +++ b/satrs-core/tests/tcp_servers.rs @@ -28,7 +28,7 @@ use satrs_core::{ tmtc::{ReceivesTcCore, TmPacketSourceCore}, }; use spacepackets::{ - ecss::{tc::PusTcCreator, SerializablePusPacket}, + ecss::{tc::PusTcCreator, WritablePusPacket}, PacketId, SpHeader, }; use std::{boxed::Box, collections::VecDeque, sync::Arc, vec::Vec}; -- 2.43.0 From 51d3c9b6e8a7cbd7e8ff1691fd5f542fd4451185 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 7 Dec 2023 14:05:50 +0100 Subject: [PATCH 07/33] another spacepackets update --- satrs-core/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/satrs-core/Cargo.toml b/satrs-core/Cargo.toml index a3391fb..66772c0 100644 --- a/satrs-core/Cargo.toml +++ b/satrs-core/Cargo.toml @@ -76,7 +76,7 @@ optional = true # version = "0.7.0-beta.2" default-features = false git = "https://egit.irs.uni-stuttgart.de/rust/spacepackets.git" -rev = "47a93354952834eea435f8ec6c561af4750a2068" +rev = "297cfad22637d3b07a1b27abe56d9a607b5b82a7" [dependencies.cobs] git = "https://github.com/robamu/cobs.rs.git" -- 2.43.0 From c766ab2d710cddf4bdf357c96a66f0f979da1ed9 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 13 Dec 2023 15:15:22 +0100 Subject: [PATCH 08/33] come on, neotest.. --- satrs-core/src/cfdp/dest.rs | 109 ++++++++++++++++++++++++++++--- satrs-core/src/cfdp/filestore.rs | 1 + satrs-core/src/cfdp/mod.rs | 102 ++++++++++++++++++++++++++++- satrs-core/tests/hk_helpers.rs | 2 +- 4 files changed, 201 insertions(+), 13 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 0ecdf70..1487467 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -8,9 +8,10 @@ use std::{ use crate::cfdp::user::TransactionFinishedParams; use super::{ + filestore::{NativeFilestore, VirtualFilestore}, user::{CfdpUser, MetadataReceivedParams}, - CheckTimerCreator, PacketInfo, PacketTarget, RemoteEntityConfigProvider, State, TransactionId, - TransactionStep, CRC_32, + CheckTimerCreator, DefaultFaultHandler, PacketInfo, PacketTarget, RemoteEntityConfig, + RemoteEntityConfigProvider, State, TransactionId, TransactionStep, CRC_32, }; use alloc::boxed::Box; use smallvec::SmallVec; @@ -21,10 +22,10 @@ use spacepackets::{ file_data::FileDataPdu, finished::{DeliveryCode, FileStatus, FinishedPduCreator}, metadata::{MetadataGenericParams, MetadataPduReader}, - CommonPduConfig, FileDirectiveType, PduError, PduHeader, WritablePduPacket, CfdpPdu, + CfdpPdu, CommonPduConfig, FileDirectiveType, PduError, PduHeader, WritablePduPacket, }, - tlv::{msg_to_user::MsgToUserTlv, EntityIdTlv, TlvType, GenericTlv}, - ConditionCode, PduType, + tlv::{msg_to_user::MsgToUserTlv, EntityIdTlv, GenericTlv, TlvType}, + ConditionCode, FaultHandlerCode, PduType, TransmissionMode, }, util::UnsignedByteField, }; @@ -36,6 +37,8 @@ pub struct DestinationHandler { state: State, tparams: TransactionParams, packets_to_send_ctx: PacketsToSendContext, + vfs: Box, + default_fault_handler: DefaultFaultHandler, remote_cfg_table: Box, check_timer_creator: Box, } @@ -77,6 +80,7 @@ impl Default for TransferState { } } } + #[derive(Debug)] struct TransactionParams { tstate: TransferState, @@ -85,6 +89,13 @@ struct TransactionParams { cksum_buf: [u8; 1024], msgs_to_user_size: usize, msgs_to_user_buf: [u8; 1024], + remote_cfg: Option, +} + +impl TransactionParams { + fn transmission_mode(&self) -> TransmissionMode { + self.pdu_conf.trans_mode + } } impl Default for FileProperties { @@ -118,6 +129,7 @@ impl Default for TransactionParams { msgs_to_user_buf: [0; 1024], tstate: Default::default(), file_properties: Default::default(), + remote_cfg: None, } } } @@ -153,11 +165,15 @@ pub enum DestError { PathConversion(#[from] Utf8Error), #[error("error building dest path from source file name and dest folder")] PathConcatError, + #[error("no remote entity configuration found for {0:?}")] + NoRemoteCfgFound(UnsignedByteField), } impl DestinationHandler { pub fn new( entity_id: impl Into, + vfs: Box, + default_fault_handler: DefaultFaultHandler, remote_cfg_table: Box, check_timer_creator: Box, ) -> Self { @@ -167,6 +183,8 @@ impl DestinationHandler { state: State::Idle, tparams: Default::default(), packets_to_send_ctx: Default::default(), + vfs, + default_fault_handler, remote_cfg_table, check_timer_creator, } @@ -286,6 +304,15 @@ impl DestinationHandler { let metadata_pdu = MetadataPduReader::from_bytes(raw_packet)?; self.tparams.reset(); self.tparams.tstate.metadata_params = *metadata_pdu.metadata_params(); + let remote_cfg = self + .remote_cfg_table + .get_remote_config(metadata_pdu.dest_id().value()); + if remote_cfg.is_none() { + return Err(DestError::NoRemoteCfgFound(metadata_pdu.dest_id())); + } + self.tparams.remote_cfg = Some(*remote_cfg.unwrap()); + + // TODO: Support for metadata only PDUs. let src_name = metadata_pdu.src_file_name(); if src_name.is_empty() { return Err(DestError::EmptySrcFileField); @@ -332,17 +359,24 @@ impl DestinationHandler { Ok(()) } - #[allow(clippy::needless_if)] pub fn handle_eof_pdu(&mut self, raw_packet: &[u8]) -> Result<(), DestError> { if self.state == State::Idle || self.step != TransactionStep::ReceivingFileDataPdus { return Err(DestError::WrongStateForFileDataAndEof); } let eof_pdu = EofPdu::from_bytes(raw_packet)?; - let checksum = eof_pdu.file_checksum(); + if eof_pdu.condition_code() == ConditionCode::NoError { + self.handle_no_error_eof_pdu(&eof_pdu)?; + } else { + todo!("implement cancel request handling"); + } + Ok(()) + } + + fn handle_no_error_eof_pdu(&mut self, eof_pdu: &EofPdu) -> Result<(), DestError> { // For a standard disk based file system, which is assumed to be used for now, the file // will always be retained. This might change in the future. self.tparams.tstate.file_status = FileStatus::Retained; - if self.checksum_check(checksum)? { + if self.checksum_check(eof_pdu.file_checksum())? { self.tparams.tstate.condition_code = ConditionCode::NoError; self.tparams.tstate.delivery_code = DeliveryCode::Complete; } else { @@ -351,7 +385,7 @@ impl DestinationHandler { // TODO: Check progress, and implement transfer completion timer as specified in the // standard. This timer protects against out of order arrival of packets. if self.tparams.tstate.progress != self.tparams.file_size() {} - if self.state == State::Busy { + if self.tparams.transmission_mode() == TransmissionMode::Unacknowledged { self.step = TransactionStep::TransferCompletion; } else { self.step = TransactionStep::SendingAckPdu; @@ -364,6 +398,7 @@ impl DestinationHandler { } fn checksum_check(&mut self, expected_checksum: u32) -> Result { + // TODO: Implement this using the new virtual filestore abstraction. let mut digest = CRC_32.digest(); let file_to_check = File::open(&self.tparams.file_properties.dest_path_buf)?; let mut buf_reader = BufReader::new(file_to_check); @@ -492,6 +527,11 @@ impl DestinationHandler { Ok(()) } + fn declare_fault(&mut self, condition_code: ConditionCode) -> FaultHandlerCode { + todo!("implement this. requires cached fault handler abstraction"); + FaultHandlerCode::IgnoreError + } + fn reset(&mut self) { self.step = TransactionStep::Idle; self.state = State::Idle; @@ -520,12 +560,17 @@ mod tests { use alloc::{format, string::String}; use rand::Rng; use spacepackets::{ - cfdp::{lv::Lv, pdu::{WritablePduPacket, metadata::MetadataPduCreator}, ChecksumType, TransmissionMode}, + cfdp::{ + lv::Lv, + pdu::{metadata::MetadataPduCreator, WritablePduPacket}, + ChecksumType, TransmissionMode, + }, util::{UbfU16, UnsignedByteFieldU16}, }; use crate::cfdp::{ CheckTimer, CheckTimerCreator, RemoteEntityConfig, StdRemoteEntityConfigProvider, + UserFaultHandler, }; use super::*; @@ -553,6 +598,47 @@ mod tests { } } + #[derive(Default)] + struct TestFaultHandler { + notice_of_suspension_count: u32, + notice_of_cancellation_count: u32, + abandoned_count: u32, + ignored_count: u32, + } + + impl UserFaultHandler for TestFaultHandler { + fn notice_of_suspension_cb( + &mut self, + transaction_id: TransactionId, + cond: ConditionCode, + progress: u64, + ) { + self.notice_of_suspension_count += 1; + } + + fn notice_of_cancellation_cb( + &mut self, + transaction_id: TransactionId, + cond: ConditionCode, + progress: u64, + ) { + self.notice_of_cancellation_count += 1; + } + + fn abandoned_cb( + &mut self, + transaction_id: TransactionId, + cond: ConditionCode, + progress: u64, + ) { + self.abandoned_count += 1; + } + + fn ignore_cb(&mut self, transaction_id: TransactionId, cond: ConditionCode, progress: u64) { + self.ignored_count += 1; + } + } + impl CfdpUser for TestCfdpUser { fn transaction_indication(&mut self, id: &crate::cfdp::TransactionId) { self.generic_id_check(id); @@ -719,8 +805,11 @@ mod tests { } fn default_dest_handler() -> DestinationHandler { + let test_fault_handler = TestFaultHandler::default(); DestinationHandler::new( REMOTE_ID, + Box::new(NativeFilestore::default()), + DefaultFaultHandler::new(Box::new(test_fault_handler)), Box::new(basic_remote_cfg_table()), Box::new(TestCheckTimerCreator::new(2, 2)), ) diff --git a/satrs-core/src/cfdp/filestore.rs b/satrs-core/src/cfdp/filestore.rs index 97bcba4..72fdb4e 100644 --- a/satrs-core/src/cfdp/filestore.rs +++ b/satrs-core/src/cfdp/filestore.rs @@ -141,6 +141,7 @@ pub mod stdmod { path::Path, }; + #[derive(Default)] pub struct NativeFilestore {} impl VirtualFilestore for NativeFilestore { diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index a40c1ee..82c6f64 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -5,7 +5,7 @@ use hashbrown::HashMap; use spacepackets::{ cfdp::{ pdu::{FileDirectiveType, PduError, PduHeader}, - ChecksumType, PduType, TransmissionMode, + ChecksumType, ConditionCode, FaultHandlerCode, PduType, TransmissionMode, }, util::UnsignedByteField, }; @@ -161,6 +161,103 @@ impl RemoteEntityConfigProvider for StdRemoteEntityConfigProvider { } } +/// This trait introduces some callbacks which will be called when a particular CFDP fault +/// handler is called. This allows to implement some CFDP features like fault handler logging, +/// which would not be possible generically otherwise. +pub trait UserFaultHandler { + fn notice_of_suspension_cb( + &mut self, + transaction_id: TransactionId, + cond: ConditionCode, + progress: u64, + ); + + fn notice_of_cancellation_cb( + &mut self, + transaction_id: TransactionId, + cond: ConditionCode, + progress: u64, + ); + + fn abandoned_cb(&mut self, transaction_id: TransactionId, cond: ConditionCode, progress: u64); + + fn ignore_cb(&mut self, transaction_id: TransactionId, cond: ConditionCode, progress: u64); +} + +pub struct DefaultFaultHandler { + handler_array: [FaultHandlerCode; 10], + user_fault_handler: Box, +} + +impl DefaultFaultHandler { + fn condition_code_to_array_index(conditon_code: ConditionCode) -> Option { + Some(match conditon_code { + ConditionCode::PositiveAckLimitReached => 0, + ConditionCode::KeepAliveLimitReached => 1, + ConditionCode::InvalidTransmissionMode => 2, + ConditionCode::FilestoreRejection => 3, + ConditionCode::FileChecksumFailure => 4, + ConditionCode::FileSizeError => 5, + ConditionCode::NakLimitReached => 6, + ConditionCode::InactivityDetected => 7, + ConditionCode::CheckLimitReached => 8, + ConditionCode::UnsupportedChecksumType => 9, + _ => return None, + }) + } + + pub fn new(user_fault_handler: Box) -> Self { + let mut init_array = [FaultHandlerCode::NoticeOfCancellation; 10]; + init_array + [Self::condition_code_to_array_index(ConditionCode::FileChecksumFailure).unwrap()] = + FaultHandlerCode::IgnoreError; + init_array[Self::condition_code_to_array_index(ConditionCode::UnsupportedChecksumType) + .unwrap()] = FaultHandlerCode::IgnoreError; + Self { + handler_array: init_array, + user_fault_handler, + } + } + + fn report_fault( + &mut self, + transaction_id: TransactionId, + condition: ConditionCode, + progress: u64, + ) -> FaultHandlerCode { + let array_idx = Self::condition_code_to_array_index(condition); + if array_idx.is_none() { + return FaultHandlerCode::IgnoreError; + } + let fh_code = self.handler_array[array_idx.unwrap()]; + match fh_code { + FaultHandlerCode::NoticeOfCancellation => { + self.user_fault_handler.notice_of_cancellation_cb( + transaction_id, + condition, + progress, + ); + } + FaultHandlerCode::NoticeOfSuspension => { + self.user_fault_handler.notice_of_suspension_cb( + transaction_id, + condition, + progress, + ); + } + FaultHandlerCode::IgnoreError => { + self.user_fault_handler + .ignore_cb(transaction_id, condition, progress); + } + FaultHandlerCode::AbandonTransaction => { + self.user_fault_handler + .abandoned_cb(transaction_id, condition, progress); + } + } + fh_code + } +} + #[derive(Debug, Eq, Copy, Clone)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct TransactionId { @@ -347,7 +444,8 @@ mod tests { let dest_file_name = "hello-dest.txt"; let src_lv = Lv::new_from_str(src_file_name).unwrap(); let dest_lv = Lv::new_from_str(dest_file_name).unwrap(); - let metadata_pdu = MetadataPduCreator::new_no_opts(pdu_header, metadata_params, src_lv, dest_lv); + let metadata_pdu = + MetadataPduCreator::new_no_opts(pdu_header, metadata_params, src_lv, dest_lv); metadata_pdu .write_to_bytes(&mut buf) .expect("writing metadata PDU failed"); diff --git a/satrs-core/tests/hk_helpers.rs b/satrs-core/tests/hk_helpers.rs index 9228365..8791b1e 100644 --- a/satrs-core/tests/hk_helpers.rs +++ b/satrs-core/tests/hk_helpers.rs @@ -1,7 +1,7 @@ #![allow(dead_code)] use core::mem::size_of; use serde::{Deserialize, Serialize}; -use spacepackets::ecss::{Ptc, PfcReal, PfcUnsigned}; +use spacepackets::ecss::{PfcReal, PfcUnsigned, Ptc}; use spacepackets::time::cds::TimeProvider; use spacepackets::time::{CcsdsTimeProvider, TimeWriter}; -- 2.43.0 From 5a55993452b03676a67e8123d22c5d62bedeafd8 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 13 Dec 2023 15:35:21 +0100 Subject: [PATCH 09/33] fix some tests --- satrs-core/src/pus/scheduler.rs | 2 +- satrs-core/src/pus/test.rs | 2 +- satrs-core/src/pus/verification.rs | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/satrs-core/src/pus/scheduler.rs b/satrs-core/src/pus/scheduler.rs index 820b1f4..392eb87 100644 --- a/satrs-core/src/pus/scheduler.rs +++ b/satrs-core/src/pus/scheduler.rs @@ -857,7 +857,7 @@ mod tests { let mut sp_header = SpHeader::tc_unseg(apid_to_set, 105, 0).unwrap(); let mut sec_header = PusTcSecondaryHeader::new_simple(17, 1); sec_header.source_id = src_id_to_set; - let ping_tc = PusTcCreator::new(&mut sp_header, sec_header, None, true); + let ping_tc = PusTcCreator::new_no_app_data(&mut sp_header, sec_header, true); let req_id = RequestId::from_tc(&ping_tc); assert_eq!(req_id.source_id(), src_id_to_set); assert_eq!(req_id.apid(), apid_to_set); diff --git a/satrs-core/src/pus/test.rs b/satrs-core/src/pus/test.rs index fbef28a..c205178 100644 --- a/satrs-core/src/pus/test.rs +++ b/satrs-core/src/pus/test.rs @@ -154,7 +154,7 @@ mod tests { // Create a ping TC, verify acceptance. let mut sp_header = SpHeader::tc(TEST_APID, SequenceFlags::Unsegmented, 0, 0).unwrap(); let sec_header = PusTcSecondaryHeader::new_simple(17, 1); - let ping_tc = PusTcCreator::new(&mut sp_header, sec_header, None, true); + let ping_tc = PusTcCreator::new_no_app_data(&mut sp_header, sec_header, true); let token = verification_handler.add_tc(&ping_tc); let token = verification_handler .acceptance_success(token, None) diff --git a/satrs-core/src/pus/verification.rs b/satrs-core/src/pus/verification.rs index cca7566..35e4695 100644 --- a/satrs-core/src/pus/verification.rs +++ b/satrs-core/src/pus/verification.rs @@ -1438,6 +1438,7 @@ mod tests { fn base_tc_init(app_data: Option<&[u8]>) -> (PusTcCreator, RequestId) { let mut sph = SpHeader::tc_unseg(TEST_APID, 0x34, 0).unwrap(); let tc_header = PusTcSecondaryHeader::new_simple(17, 1); + let app_data = app_data.unwrap_or(&[]); let pus_tc = PusTcCreator::new(&mut sph, tc_header, app_data, true); let req_id = RequestId::new(&pus_tc); (pus_tc, req_id) @@ -2162,7 +2163,7 @@ mod tests { let mut sph = SpHeader::tc_unseg(TEST_APID, 0, 0).unwrap(); let tc_header = PusTcSecondaryHeader::new_simple(17, 1); - let pus_tc_0 = PusTcCreator::new(&mut sph, tc_header, None, true); + let pus_tc_0 = PusTcCreator::new_no_app_data(&mut sph, tc_header, true); let init_token = reporter.add_tc(&pus_tc_0); // Complete success sequence for a telecommand -- 2.43.0 From bd6e1637e491713e4e1aae63ecf0838cc9cc08a9 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 13 Dec 2023 15:49:26 +0100 Subject: [PATCH 10/33] the check finally works again --- satrs-core/Cargo.toml | 6 +++--- satrs-example/Cargo.toml | 4 ++-- satrs-example/src/bin/simpleclient.rs | 2 +- satrs-example/src/main.rs | 2 +- satrs-mib/Cargo.toml | 3 ++- satrs-mib/codegen/Cargo.toml | 3 ++- 6 files changed, 11 insertions(+), 9 deletions(-) diff --git a/satrs-core/Cargo.toml b/satrs-core/Cargo.toml index 66772c0..c46fc3b 100644 --- a/satrs-core/Cargo.toml +++ b/satrs-core/Cargo.toml @@ -73,10 +73,10 @@ features = ["all"] optional = true [dependencies.spacepackets] -# version = "0.7.0-beta.2" +version = "0.7.0-beta.3" default-features = false -git = "https://egit.irs.uni-stuttgart.de/rust/spacepackets.git" -rev = "297cfad22637d3b07a1b27abe56d9a607b5b82a7" +# git = "https://egit.irs.uni-stuttgart.de/rust/spacepackets.git" +# rev = "297cfad22637d3b07a1b27abe56d9a607b5b82a7" [dependencies.cobs] git = "https://github.com/robamu/cobs.rs.git" diff --git a/satrs-example/Cargo.toml b/satrs-example/Cargo.toml index 3d61254..1f96d1a 100644 --- a/satrs-example/Cargo.toml +++ b/satrs-example/Cargo.toml @@ -23,5 +23,5 @@ thiserror = "1" path = "../satrs-core" [dependencies.satrs-mib] -version = "0.1.0-alpha.1" -# path = "../satrs-mib" +# version = "0.1.0-alpha.1" +path = "../satrs-mib" diff --git a/satrs-example/src/bin/simpleclient.rs b/satrs-example/src/bin/simpleclient.rs index 6f6de79..4d0dab7 100644 --- a/satrs-example/src/bin/simpleclient.rs +++ b/satrs-example/src/bin/simpleclient.rs @@ -2,7 +2,7 @@ use satrs_core::pus::verification::RequestId; use satrs_core::spacepackets::ecss::tc::PusTcCreator; use satrs_core::spacepackets::ecss::tm::PusTmReader; use satrs_core::{ - spacepackets::ecss::{PusPacket, SerializablePusPacket}, + spacepackets::ecss::{PusPacket, WritablePusPacket}, spacepackets::SpHeader, }; use satrs_example::{OBSW_SERVER_ADDR, SERVER_PORT}; diff --git a/satrs-example/src/main.rs b/satrs-example/src/main.rs index c537ab2..2a692a3 100644 --- a/satrs-example/src/main.rs +++ b/satrs-example/src/main.rs @@ -466,7 +466,7 @@ fn main() { let pus_tm = PusTmCreator::new( &mut sp_header, sec_header, - Some(&buf), + &buf, true, ); let addr = aocs_tm_store diff --git a/satrs-mib/Cargo.toml b/satrs-mib/Cargo.toml index e901ebe..b64813c 100644 --- a/satrs-mib/Cargo.toml +++ b/satrs-mib/Cargo.toml @@ -23,7 +23,8 @@ version = "1" optional = true [dependencies.satrs-core] -version = "0.1.0-alpha.1" +path = "../satrs-core" +# version = "0.1.0-alpha.1" # git = "https://egit.irs.uni-stuttgart.de/rust/sat-rs.git" # branch = "main" # rev = "35e1f7a983f6535c5571186e361fe101d4306b89" diff --git a/satrs-mib/codegen/Cargo.toml b/satrs-mib/codegen/Cargo.toml index 9a3887c..1ad3810 100644 --- a/satrs-mib/codegen/Cargo.toml +++ b/satrs-mib/codegen/Cargo.toml @@ -20,7 +20,8 @@ quote = "1" proc-macro2 = "1" [dependencies.satrs-core] -version = "0.1.0-alpha.1" +path = "../../satrs-core" +# version = "0.1.0-alpha.1" # git = "https://egit.irs.uni-stuttgart.de/rust/sat-rs.git" # branch = "main" # rev = "35e1f7a983f6535c5571186e361fe101d4306b89" -- 2.43.0 From 1b1bef2958835a386c8b6bd23743b544dd9d273f Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 13 Dec 2023 18:55:26 +0100 Subject: [PATCH 11/33] make it compile again --- satrs-core/src/cfdp/dest.rs | 164 +++++++++++++++++++++++------------- satrs-core/src/cfdp/mod.rs | 50 ++++++++++- 2 files changed, 154 insertions(+), 60 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 1487467..5bbedd7 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -1,17 +1,17 @@ use core::str::{from_utf8, Utf8Error}; use std::{ fs::{metadata, File}, - io::{BufReader, Read, Seek, SeekFrom, Write}, + io::{Seek, SeekFrom, Write}, path::{Path, PathBuf}, }; use crate::cfdp::user::TransactionFinishedParams; use super::{ - filestore::{NativeFilestore, VirtualFilestore}, + filestore::{FilestoreError, VirtualFilestore}, user::{CfdpUser, MetadataReceivedParams}, - CheckTimerCreator, DefaultFaultHandler, PacketInfo, PacketTarget, RemoteEntityConfig, - RemoteEntityConfigProvider, State, TransactionId, TransactionStep, CRC_32, + CheckTimerCreator, LocalEntityConfig, PacketInfo, PacketTarget, RemoteEntityConfig, + RemoteEntityConfigProvider, State, TransactionId, TransactionStep, }; use alloc::boxed::Box; use smallvec::SmallVec; @@ -32,13 +32,12 @@ use spacepackets::{ use thiserror::Error; pub struct DestinationHandler { - id: UnsignedByteField, + local_cfg: LocalEntityConfig, step: TransactionStep, state: State, tparams: TransactionParams, packets_to_send_ctx: PacketsToSendContext, vfs: Box, - default_fault_handler: DefaultFaultHandler, remote_cfg_table: Box, check_timer_creator: Box, } @@ -61,7 +60,7 @@ struct FileProperties { #[derive(Debug)] struct TransferState { transaction_id: Option, - progress: usize, + progress: u64, condition_code: ConditionCode, delivery_code: DeliveryCode, file_status: FileStatus, @@ -111,8 +110,8 @@ impl Default for FileProperties { } impl TransactionParams { - fn file_size(&self) -> usize { - self.tstate.metadata_params.file_size as usize + fn file_size(&self) -> u64 { + self.tstate.metadata_params.file_size } fn metadata_params(&self) -> &MetadataGenericParams { @@ -171,20 +170,18 @@ pub enum DestError { impl DestinationHandler { pub fn new( - entity_id: impl Into, + local_cfg: LocalEntityConfig, vfs: Box, - default_fault_handler: DefaultFaultHandler, remote_cfg_table: Box, check_timer_creator: Box, ) -> Self { Self { - id: entity_id.into(), + local_cfg, step: TransactionStep::Idle, state: State::Idle, tparams: Default::default(), packets_to_send_ctx: Default::default(), vfs, - default_fault_handler, remote_cfg_table, check_timer_creator, } @@ -196,7 +193,7 @@ impl DestinationHandler { packet_to_insert: Option<&PacketInfo>, ) -> Result<(), DestError> { if let Some(packet) = packet_to_insert { - self.insert_packet(packet)?; + self.insert_packet(cfdp_user, packet)?; } match self.state { State::Idle => todo!(), @@ -205,7 +202,11 @@ impl DestinationHandler { } } - fn insert_packet(&mut self, packet_info: &PacketInfo) -> Result<(), DestError> { + fn insert_packet( + &mut self, + cfdp_user: &mut impl CfdpUser, + packet_info: &PacketInfo, + ) -> Result<(), DestError> { if packet_info.target() != PacketTarget::DestEntity { // Unwrap is okay here, a PacketInfo for a file data PDU should always have the // destination as the target. @@ -219,6 +220,7 @@ impl DestinationHandler { return Err(DestError::DirectiveExpected); } self.handle_file_directive( + cfdp_user, packet_info.pdu_directive.unwrap(), packet_info.raw_packet, ) @@ -252,7 +254,7 @@ impl DestinationHandler { ) } else { // TODO: Are there cases where this ID is actually the source entity ID? - let entity_id = EntityIdTlv::new(self.id); + let entity_id = EntityIdTlv::new(self.local_cfg.id); FinishedPduCreator::new_with_error( pdu_header, self.tparams.tstate.condition_code, @@ -276,11 +278,12 @@ impl DestinationHandler { pub fn handle_file_directive( &mut self, + cfdp_user: &mut impl CfdpUser, pdu_directive: FileDirectiveType, raw_packet: &[u8], ) -> Result<(), DestError> { match pdu_directive { - FileDirectiveType::EofPdu => self.handle_eof_pdu(raw_packet)?, + FileDirectiveType::EofPdu => self.handle_eof_pdu(cfdp_user, raw_packet)?, FileDirectiveType::FinishedPdu | FileDirectiveType::NakPdu | FileDirectiveType::KeepAlivePdu => { @@ -359,11 +362,19 @@ impl DestinationHandler { Ok(()) } - pub fn handle_eof_pdu(&mut self, raw_packet: &[u8]) -> Result<(), DestError> { + pub fn handle_eof_pdu( + &mut self, + cfdp_user: &mut impl CfdpUser, + raw_packet: &[u8], + ) -> Result<(), DestError> { if self.state == State::Idle || self.step != TransactionStep::ReceivingFileDataPdus { return Err(DestError::WrongStateForFileDataAndEof); } let eof_pdu = EofPdu::from_bytes(raw_packet)?; + if self.local_cfg.indication_cfg.eof_recv { + // Unwrap is okay here, application logic ensures that transaction ID is valid here. + cfdp_user.eof_recvd_indication(self.tparams.tstate.transaction_id.as_ref().unwrap()); + } if eof_pdu.condition_code() == ConditionCode::NoError { self.handle_no_error_eof_pdu(&eof_pdu)?; } else { @@ -372,49 +383,73 @@ impl DestinationHandler { Ok(()) } - fn handle_no_error_eof_pdu(&mut self, eof_pdu: &EofPdu) -> Result<(), DestError> { + fn handle_no_error_eof_pdu(&mut self, eof_pdu: &EofPdu) -> Result { + // CFDP 4.6.1.2.9: Declare file size error if progress exceeds file size + if self.tparams.tstate.progress > eof_pdu.file_size() + && self.declare_fault(ConditionCode::FileSizeError) != FaultHandlerCode::IgnoreError + { + return Ok(true); + } + + if self.tparams.transmission_mode() == TransmissionMode::Unacknowledged + && !self.checksum_verify(eof_pdu.file_checksum()) + { + self.start_check_limit_handling() + } + + // TODO: Continue here based on Python implementation. + // For a standard disk based file system, which is assumed to be used for now, the file // will always be retained. This might change in the future. + /* self.tparams.tstate.file_status = FileStatus::Retained; - if self.checksum_check(eof_pdu.file_checksum())? { + if self.checksum_verify(eof_pdu.file_checksum())? { self.tparams.tstate.condition_code = ConditionCode::NoError; self.tparams.tstate.delivery_code = DeliveryCode::Complete; } else { self.tparams.tstate.condition_code = ConditionCode::FileChecksumFailure; } + */ // TODO: Check progress, and implement transfer completion timer as specified in the // standard. This timer protects against out of order arrival of packets. - if self.tparams.tstate.progress != self.tparams.file_size() {} + // if self.tparams.tstate.progress != self.tparams.file_size() {} if self.tparams.transmission_mode() == TransmissionMode::Unacknowledged { self.step = TransactionStep::TransferCompletion; } else { self.step = TransactionStep::SendingAckPdu; } - Ok(()) + Ok(false) } + fn checksum_verify(&mut self, checksum: u32) -> bool { + match self.vfs.checksum_verify( + self.tparams.file_properties.dest_path_buf.to_str().unwrap(), + self.tparams.metadata_params().checksum_type, + checksum, + &mut self.tparams.cksum_buf, + ) { + Ok(checksum_success) => checksum_success, + Err(e) => match e { + FilestoreError::ChecksumTypeNotImplemented(checksum) => { + self.declare_fault(ConditionCode::UnsupportedChecksumType); + // For this case, the applicable algorithm shall the the null checksum, which + // is always succesful. + true + } + _ => { + self.declare_fault(ConditionCode::FilestoreRejection); + // Treat this equivalent to a failed checksum procedure. + false + } + }, + } + } + + fn start_check_limit_handling(&mut self) {} pub fn handle_prompt_pdu(&mut self, _raw_packet: &[u8]) -> Result<(), DestError> { todo!(); } - fn checksum_check(&mut self, expected_checksum: u32) -> Result { - // TODO: Implement this using the new virtual filestore abstraction. - let mut digest = CRC_32.digest(); - let file_to_check = File::open(&self.tparams.file_properties.dest_path_buf)?; - let mut buf_reader = BufReader::new(file_to_check); - loop { - let bytes_read = buf_reader.read(&mut self.tparams.cksum_buf)?; - if bytes_read == 0 { - break; - } - digest.update(&self.tparams.cksum_buf[0..bytes_read]); - } - if digest.finalize() == expected_checksum { - return Ok(true); - } - Ok(false) - } - fn fsm_busy(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { if self.step == TransactionStep::TransactionStart { self.transaction_start(cfdp_user)?; @@ -472,7 +507,7 @@ impl DestinationHandler { let metadata_recvd_params = MetadataReceivedParams { id, source_id, - file_size: self.tparams.tstate.metadata_params.file_size, + file_size: self.tparams.file_size(), src_file_name: src_name, dest_file_name: dest_name, msgs_to_user: &msgs_to_user[..num_msgs_to_user], @@ -528,10 +563,28 @@ impl DestinationHandler { } fn declare_fault(&mut self, condition_code: ConditionCode) -> FaultHandlerCode { - todo!("implement this. requires cached fault handler abstraction"); - FaultHandlerCode::IgnoreError + let fh_code = self + .local_cfg + .default_fault_handler + .get_fault_handler(condition_code); + match fh_code { + FaultHandlerCode::NoticeOfCancellation => { + self.notice_of_cancellation(); + } + FaultHandlerCode::NoticeOfSuspension => self.notice_of_suspension(), + FaultHandlerCode::IgnoreError => todo!(), + FaultHandlerCode::AbandonTransaction => todo!(), + } + self.local_cfg.default_fault_handler.report_fault( + self.tparams.tstate.transaction_id.unwrap(), + condition_code, + self.tparams.tstate.progress, + ) } + fn notice_of_cancellation(&mut self) {} + fn notice_of_suspension(&mut self) {} + fn reset(&mut self) { self.step = TransactionStep::Idle; self.state = State::Idle; @@ -569,8 +622,9 @@ mod tests { }; use crate::cfdp::{ - CheckTimer, CheckTimerCreator, RemoteEntityConfig, StdRemoteEntityConfigProvider, - UserFaultHandler, + filestore::NativeFilestore, CheckTimer, CheckTimerCreator, DefaultFaultHandler, + IndicationConfig, RemoteEntityConfig, StdRemoteEntityConfigProvider, UserFaultHandler, + CRC_32, }; use super::*; @@ -806,10 +860,14 @@ mod tests { fn default_dest_handler() -> DestinationHandler { let test_fault_handler = TestFaultHandler::default(); + let local_entity_cfg = LocalEntityConfig { + id: REMOTE_ID.into(), + indication_cfg: IndicationConfig::default(), + default_fault_handler: DefaultFaultHandler::new(Box::new(test_fault_handler)), + }; DestinationHandler::new( - REMOTE_ID, + local_entity_cfg, Box::new(NativeFilestore::default()), - DefaultFaultHandler::new(Box::new(test_fault_handler)), Box::new(basic_remote_cfg_table()), Box::new(TestCheckTimerCreator::new(2, 2)), ) @@ -936,10 +994,6 @@ mod tests { .write_to_bytes(&mut buf) .expect("writing file data PDU failed"); let packet_info = PacketInfo::new(&buf).expect("creating packet info failed"); - let result = dest_handler.insert_packet(&packet_info); - if let Err(e) = result { - panic!("destination handler packet insertion error: {e}"); - } let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); @@ -1003,10 +1057,6 @@ mod tests { .write_to_bytes(&mut buf) .expect("writing file data PDU failed"); let packet_info = PacketInfo::new(&buf).expect("creating packet info failed"); - let result = dest_handler.insert_packet(&packet_info); - if let Err(e) = result { - panic!("destination handler packet insertion error: {e}"); - } let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); @@ -1021,10 +1071,6 @@ mod tests { .write_to_bytes(&mut buf) .expect("writing file data PDU failed"); let packet_info = PacketInfo::new(&buf).expect("creating packet info failed"); - let result = dest_handler.insert_packet(&packet_info); - if let Err(e) = result { - panic!("destination handler packet insertion error: {e}"); - } let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index 82c6f64..436cdd4 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -206,6 +206,18 @@ impl DefaultFaultHandler { }) } + pub fn set_fault_handler( + &mut self, + condition_code: ConditionCode, + fault_handler: FaultHandlerCode, + ) { + let array_idx = Self::condition_code_to_array_index(condition_code); + if array_idx.is_none() { + return; + } + self.handler_array[array_idx.unwrap()] = fault_handler; + } + pub fn new(user_fault_handler: Box) -> Self { let mut init_array = [FaultHandlerCode::NoticeOfCancellation; 10]; init_array @@ -219,7 +231,15 @@ impl DefaultFaultHandler { } } - fn report_fault( + pub fn get_fault_handler(&self, condition_code: ConditionCode) -> FaultHandlerCode { + let array_idx = Self::condition_code_to_array_index(condition_code); + if array_idx.is_none() { + return FaultHandlerCode::IgnoreError; + } + self.handler_array[array_idx.unwrap()] + } + + pub fn report_fault( &mut self, transaction_id: TransactionId, condition: ConditionCode, @@ -258,6 +278,34 @@ impl DefaultFaultHandler { } } +pub struct IndicationConfig { + pub eof_sent: bool, + pub eof_recv: bool, + pub file_segment_recv: bool, + pub transaction_finished: bool, + pub suspended: bool, + pub resumed: bool, +} + +impl Default for IndicationConfig { + fn default() -> Self { + Self { + eof_sent: true, + eof_recv: true, + file_segment_recv: true, + transaction_finished: true, + suspended: true, + resumed: true, + } + } +} + +pub struct LocalEntityConfig { + pub id: UnsignedByteField, + pub indication_cfg: IndicationConfig, + pub default_fault_handler: DefaultFaultHandler, +} + #[derive(Debug, Eq, Copy, Clone)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct TransactionId { -- 2.43.0 From 0ecb7184166d03cea773748a3725b49932820521 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 14 Dec 2023 00:29:02 +0100 Subject: [PATCH 12/33] this works better apparently --- satrs-core/src/cfdp/dest.rs | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 5bbedd7..7236727 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -309,7 +309,7 @@ impl DestinationHandler { self.tparams.tstate.metadata_params = *metadata_pdu.metadata_params(); let remote_cfg = self .remote_cfg_table - .get_remote_config(metadata_pdu.dest_id().value()); + .get_remote_config(metadata_pdu.source_id().value()); if remote_cfg.is_none() { return Err(DestError::NoRemoteCfgFound(metadata_pdu.dest_id())); } @@ -632,11 +632,6 @@ mod tests { const LOCAL_ID: UnsignedByteFieldU16 = UnsignedByteFieldU16::new(1); const REMOTE_ID: UnsignedByteFieldU16 = UnsignedByteFieldU16::new(2); - const SRC_NAME: &str = "__cfdp__source-file"; - const DEST_NAME: &str = "__cfdp__dest-file"; - - static ATOMIC_COUNTER: AtomicU8 = AtomicU8::new(0); - #[derive(Default)] struct TestCfdpUser { next_expected_seq_num: u64, @@ -831,16 +826,13 @@ mod tests { } fn init_full_filenames() -> (PathBuf, PathBuf) { - let mut file_path = temp_dir(); - let mut src_path = file_path.clone(); - // Atomic counter used to allow concurrent tests. - let unique_counter = ATOMIC_COUNTER.fetch_add(1, Ordering::Relaxed); - // Create unique test filenames. - let src_name_unique = format!("{SRC_NAME}{}.txt", unique_counter); - let dest_name_unique = format!("{DEST_NAME}{}.txt", unique_counter); - src_path.push(src_name_unique); - file_path.push(dest_name_unique); - (src_path, file_path) + ( + tempfile::TempPath::from_path("/tmp/test.txt").to_path_buf(), + tempfile::NamedTempFile::new() + .unwrap() + .into_temp_path() + .to_path_buf(), + ) } fn basic_remote_cfg_table() -> StdRemoteEntityConfigProvider { @@ -867,7 +859,7 @@ mod tests { }; DestinationHandler::new( local_entity_cfg, - Box::new(NativeFilestore::default()), + Box::::default(), Box::new(basic_remote_cfg_table()), Box::new(TestCheckTimerCreator::new(2, 2)), ) -- 2.43.0 From fbd05a4a25feba0242580c2bc8ea8b592f1b6531 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 14 Dec 2023 12:10:22 +0100 Subject: [PATCH 13/33] add check limit handling --- satrs-core/src/cfdp/dest.rs | 259 +++++++++++++++++++++++------------- satrs-core/src/cfdp/mod.rs | 44 +++--- 2 files changed, 186 insertions(+), 117 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 7236727..0b564b2 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -1,17 +1,12 @@ -use core::str::{from_utf8, Utf8Error}; -use std::{ - fs::{metadata, File}, - io::{Seek, SeekFrom, Write}, - path::{Path, PathBuf}, -}; - use crate::cfdp::user::TransactionFinishedParams; +use core::str::{from_utf8, Utf8Error}; +use std::path::{Path, PathBuf}; use super::{ filestore::{FilestoreError, VirtualFilestore}, user::{CfdpUser, MetadataReceivedParams}, - CheckTimerCreator, LocalEntityConfig, PacketInfo, PacketTarget, RemoteEntityConfig, - RemoteEntityConfigProvider, State, TransactionId, TransactionStep, + CheckTimer, CheckTimerCreator, EntityType, LocalEntityConfig, PacketInfo, PacketTarget, + RemoteEntityConfig, RemoteEntityConfigProvider, State, TransactionId, TransactionStep, }; use alloc::boxed::Box; use smallvec::SmallVec; @@ -60,22 +55,28 @@ struct FileProperties { #[derive(Debug)] struct TransferState { transaction_id: Option, + metadata_params: MetadataGenericParams, progress: u64, condition_code: ConditionCode, delivery_code: DeliveryCode, file_status: FileStatus, - metadata_params: MetadataGenericParams, + checksum: u32, + current_check_count: u32, + current_check_timer: Option>, } impl Default for TransferState { fn default() -> Self { Self { transaction_id: None, + metadata_params: Default::default(), progress: Default::default(), condition_code: ConditionCode::NoError, delivery_code: DeliveryCode::Incomplete, file_status: FileStatus::Unreported, - metadata_params: Default::default(), + checksum: 0, + current_check_count: 0, + current_check_timer: None, } } } @@ -137,6 +138,7 @@ impl TransactionParams { fn reset(&mut self) { self.tstate.condition_code = ConditionCode::NoError; self.tstate.delivery_code = DeliveryCode::Incomplete; + self.tstate.file_status = FileStatus::Unreported; } } @@ -160,10 +162,12 @@ pub enum DestError { Pdu(#[from] PduError), #[error("io error {0}")] Io(#[from] std::io::Error), + #[error("file store error {0}")] + Filestore(#[from] FilestoreError), #[error("path conversion error {0}")] PathConversion(#[from] Utf8Error), #[error("error building dest path from source file name and dest folder")] - PathConcatError, + PathConcat, #[error("no remote entity configuration found for {0:?}")] NoRemoteCfgFound(UnsignedByteField), } @@ -241,25 +245,26 @@ impl DestinationHandler { return Ok(None); } let directive = self.packets_to_send_ctx.directive.unwrap(); + let tstate = self.tstate(); let written_size = match directive { FileDirectiveType::FinishedPdu => { let pdu_header = PduHeader::new_no_file_data(self.tparams.pdu_conf, 0); - let finished_pdu = if self.tparams.tstate.condition_code == ConditionCode::NoError - || self.tparams.tstate.condition_code == ConditionCode::UnsupportedChecksumType + let finished_pdu = if tstate.condition_code == ConditionCode::NoError + || tstate.condition_code == ConditionCode::UnsupportedChecksumType { FinishedPduCreator::new_default( pdu_header, - self.tparams.tstate.delivery_code, - self.tparams.tstate.file_status, + tstate.delivery_code, + tstate.file_status, ) } else { // TODO: Are there cases where this ID is actually the source entity ID? let entity_id = EntityIdTlv::new(self.local_cfg.id); FinishedPduCreator::new_with_error( pdu_header, - self.tparams.tstate.condition_code, - self.tparams.tstate.delivery_code, - self.tparams.tstate.file_status, + tstate.condition_code, + tstate.delivery_code, + tstate.file_status, entity_id, ) }; @@ -354,11 +359,14 @@ impl DestinationHandler { return Err(DestError::WrongStateForFileDataAndEof); } let fd_pdu = FileDataPdu::from_bytes(raw_packet)?; - let mut dest_file = File::options() - .write(true) - .open(&self.tparams.file_properties.dest_path_buf)?; - dest_file.seek(SeekFrom::Start(fd_pdu.offset()))?; - dest_file.write_all(fd_pdu.file_data())?; + if let Err(e) = self.vfs.write_data( + self.tparams.file_properties.dest_path_buf.to_str().unwrap(), + fd_pdu.offset(), + fd_pdu.file_data(), + ) { + self.declare_fault(ConditionCode::FilestoreRejection); + return Err(e.into()); + } Ok(()) } @@ -375,50 +383,57 @@ impl DestinationHandler { // Unwrap is okay here, application logic ensures that transaction ID is valid here. cfdp_user.eof_recvd_indication(self.tparams.tstate.transaction_id.as_ref().unwrap()); } - if eof_pdu.condition_code() == ConditionCode::NoError { - self.handle_no_error_eof_pdu(&eof_pdu)?; + let regular_transfer_finish = if eof_pdu.condition_code() == ConditionCode::NoError { + self.handle_no_error_eof_pdu(&eof_pdu)? } else { todo!("implement cancel request handling"); + }; + if regular_transfer_finish { + self.file_transfer_complete_transition(); } Ok(()) } + /// Returns whether the transfer can be completed regularly. fn handle_no_error_eof_pdu(&mut self, eof_pdu: &EofPdu) -> Result { // CFDP 4.6.1.2.9: Declare file size error if progress exceeds file size if self.tparams.tstate.progress > eof_pdu.file_size() && self.declare_fault(ConditionCode::FileSizeError) != FaultHandlerCode::IgnoreError { - return Ok(true); - } - - if self.tparams.transmission_mode() == TransmissionMode::Unacknowledged - && !self.checksum_verify(eof_pdu.file_checksum()) + return Ok(false); + } else if (self.tparams.tstate.progress < eof_pdu.file_size()) + && self.tparams.transmission_mode() == TransmissionMode::Acknowledged { - self.start_check_limit_handling() + // CFDP 4.6.4.3.1: The end offset of the last received file segment and the file + // size as stated in the EOF PDU is not the same, so we need to add that segment to + // the lost segments for the deferred lost segment detection procedure. + // TODO: Proper lost segment handling. + // self._params.acked_params.lost_seg_tracker.add_lost_segment( + // (self._params.fp.progress, self._params.fp.file_size_eof) + // ) } - // TODO: Continue here based on Python implementation. - - // For a standard disk based file system, which is assumed to be used for now, the file - // will always be retained. This might change in the future. - /* - self.tparams.tstate.file_status = FileStatus::Retained; - if self.checksum_verify(eof_pdu.file_checksum())? { - self.tparams.tstate.condition_code = ConditionCode::NoError; - self.tparams.tstate.delivery_code = DeliveryCode::Complete; - } else { - self.tparams.tstate.condition_code = ConditionCode::FileChecksumFailure; + self.tparams.tstate.checksum = eof_pdu.file_checksum(); + if self.tparams.transmission_mode() == TransmissionMode::Unacknowledged + && !self.checksum_verify(self.tparams.tstate.checksum) + { + if self.declare_fault(ConditionCode::FileChecksumFailure) + != FaultHandlerCode::IgnoreError + { + return Ok(false); + } + self.start_check_limit_handling(); } - */ - // TODO: Check progress, and implement transfer completion timer as specified in the - // standard. This timer protects against out of order arrival of packets. - // if self.tparams.tstate.progress != self.tparams.file_size() {} + Ok(true) + } + + fn file_transfer_complete_transition(&mut self) { if self.tparams.transmission_mode() == TransmissionMode::Unacknowledged { self.step = TransactionStep::TransferCompletion; } else { + // TODO: Prepare ACK PDU somehow. self.step = TransactionStep::SendingAckPdu; } - Ok(false) } fn checksum_verify(&mut self, checksum: u32) -> bool { @@ -430,7 +445,7 @@ impl DestinationHandler { ) { Ok(checksum_success) => checksum_success, Err(e) => match e { - FilestoreError::ChecksumTypeNotImplemented(checksum) => { + FilestoreError::ChecksumTypeNotImplemented(_) => { self.declare_fault(ConditionCode::UnsupportedChecksumType); // For this case, the applicable algorithm shall the the null checksum, which // is always succesful. @@ -445,7 +460,43 @@ impl DestinationHandler { } } - fn start_check_limit_handling(&mut self) {} + fn start_check_limit_handling(&mut self) { + self.step = TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling; + self.tparams.tstate.current_check_timer = + Some(self.check_timer_creator.get_check_timer_provider( + &self.local_cfg.id, + &self.tparams.remote_cfg.unwrap().entity_id, + EntityType::Receiving, + )); + self.tparams.tstate.current_check_count = 0; + } + + fn check_limit_handling(&mut self) { + if self.tparams.tstate.current_check_timer.is_none() { + return; + } + let check_timer = self.tparams.tstate.current_check_timer.as_ref().unwrap(); + if check_timer.has_expired() { + if self.checksum_verify(self.tparams.tstate.checksum) { + self.file_transfer_complete_transition(); + return; + } + if self.tparams.tstate.current_check_count + 1 + >= self.tparams.remote_cfg.unwrap().check_limit + { + self.declare_fault(ConditionCode::CheckLimitReached); + } else { + self.tparams.tstate.current_check_count += 1; + self.tparams + .tstate + .current_check_timer + .as_mut() + .unwrap() + .reset(); + } + } + } + pub fn handle_prompt_pdu(&mut self, _raw_packet: &[u8]) -> Result<(), DestError> { todo!(); } @@ -457,6 +508,9 @@ impl DestinationHandler { if self.step == TransactionStep::TransferCompletion { self.transfer_completion(cfdp_user)?; } + if self.step == TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling { + self.check_limit_handling(); + } if self.step == TransactionStep::SendingAckPdu { todo!("no support for acknowledged mode yet"); } @@ -515,39 +569,43 @@ impl DestinationHandler { self.tparams.tstate.transaction_id = Some(id); cfdp_user.metadata_recvd_indication(&metadata_recvd_params); - if dest_path.exists() { - let dest_metadata = metadata(dest_path)?; - if dest_metadata.is_dir() { - // Create new destination path by concatenating the last part of the source source - // name and the destination folder. For example, for a source file of /tmp/hello.txt - // and a destination name of /home/test, the resulting file name should be - // /home/test/hello.txt - let source_path = Path::new(from_utf8( - &self.tparams.file_properties.src_file_name - [..self.tparams.file_properties.src_file_name_len], - )?); - - let source_name = source_path.file_name(); - if source_name.is_none() { - return Err(DestError::PathConcatError); - } - let source_name = source_name.unwrap(); - self.tparams.file_properties.dest_path_buf.push(source_name); + // TODO: This is the only remaining function which uses std.. the easiest way would + // probably be to use a static pre-allocated dest path buffer to store any concatenated + // paths. + if dest_path.exists() && self.vfs.is_dir(dest_path.to_str().unwrap()) { + // Create new destination path by concatenating the last part of the source source + // name and the destination folder. For example, for a source file of /tmp/hello.txt + // and a destination name of /home/test, the resulting file name should be + // /home/test/hello.txt + let source_path = Path::new(from_utf8( + &self.tparams.file_properties.src_file_name + [..self.tparams.file_properties.src_file_name_len], + )?); + let source_name = source_path.file_name(); + if source_name.is_none() { + return Err(DestError::PathConcat); } + let source_name = source_name.unwrap(); + self.tparams.file_properties.dest_path_buf.push(source_name); } - // This function does exactly what we require: Create a new file if it does not exist yet - // and trucate an existing one. - File::create(&self.tparams.file_properties.dest_path_buf)?; + let dest_path_str = self.tparams.file_properties.dest_path_buf.to_str().unwrap(); + if self.vfs.exists(dest_path_str) { + self.vfs.truncate_file(dest_path_str)?; + } else { + self.vfs.create_file(dest_path_str)?; + } + self.tparams.tstate.file_status = FileStatus::Retained; self.step = TransactionStep::ReceivingFileDataPdus; Ok(()) } fn transfer_completion(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { + let tstate = self.tstate(); let transaction_finished_params = TransactionFinishedParams { - id: self.tparams.tstate.transaction_id.unwrap(), - condition_code: self.tparams.tstate.condition_code, - delivery_code: self.tparams.tstate.delivery_code, - file_status: self.tparams.tstate.file_status, + id: tstate.transaction_id.unwrap(), + condition_code: tstate.condition_code, + delivery_code: tstate.delivery_code, + file_status: tstate.file_status, }; cfdp_user.transaction_finished_indication(&transaction_finished_params); // This function should never be called with metadata parameters not set @@ -575,10 +633,11 @@ impl DestinationHandler { FaultHandlerCode::IgnoreError => todo!(), FaultHandlerCode::AbandonTransaction => todo!(), } + let tstate = self.tstate(); self.local_cfg.default_fault_handler.report_fault( - self.tparams.tstate.transaction_id.unwrap(), + tstate.transaction_id.unwrap(), condition_code, - self.tparams.tstate.progress, + tstate.progress, ) } @@ -598,19 +657,20 @@ impl DestinationHandler { self.step = TransactionStep::SendingFinishedPdu; Ok(()) } + + fn tstate(&self) -> &TransferState { + &self.tparams.tstate + } } #[cfg(test)] mod tests { - use core::{ - cell::Cell, - sync::atomic::{AtomicU8, Ordering}, - }; + use core::cell::Cell; + use std::fs; #[allow(unused_imports)] use std::println; - use std::{env::temp_dir, fs}; - use alloc::{format, string::String}; + use alloc::{collections::VecDeque, string::String}; use rand::Rng; use spacepackets::{ cfdp::{ @@ -649,10 +709,10 @@ mod tests { #[derive(Default)] struct TestFaultHandler { - notice_of_suspension_count: u32, - notice_of_cancellation_count: u32, - abandoned_count: u32, - ignored_count: u32, + notice_of_suspension_queue: VecDeque<(TransactionId, ConditionCode, u64)>, + notice_of_cancellation_queue: VecDeque<(TransactionId, ConditionCode, u64)>, + abandoned_queue: VecDeque<(TransactionId, ConditionCode, u64)>, + ignored_queue: VecDeque<(TransactionId, ConditionCode, u64)>, } impl UserFaultHandler for TestFaultHandler { @@ -662,7 +722,8 @@ mod tests { cond: ConditionCode, progress: u64, ) { - self.notice_of_suspension_count += 1; + self.notice_of_suspension_queue + .push_back((transaction_id, cond, progress)) } fn notice_of_cancellation_cb( @@ -671,7 +732,8 @@ mod tests { cond: ConditionCode, progress: u64, ) { - self.notice_of_cancellation_count += 1; + self.notice_of_cancellation_queue + .push_back((transaction_id, cond, progress)) } fn abandoned_cb( @@ -680,11 +742,13 @@ mod tests { cond: ConditionCode, progress: u64, ) { - self.abandoned_count += 1; + self.abandoned_queue + .push_back((transaction_id, cond, progress)) } fn ignore_cb(&mut self, transaction_id: TransactionId, cond: ConditionCode, progress: u64) { - self.ignored_count += 1; + self.ignored_queue + .push_back((transaction_id, cond, progress)) } } @@ -763,6 +827,7 @@ mod tests { } } + #[derive(Debug)] struct TestCheckTimer { counter: Cell, expiry_count: u32, @@ -777,6 +842,9 @@ mod tests { self.counter.set(current_counter + 1); false } + fn reset(&mut self) { + self.counter.set(0); + } } impl TestCheckTimer { @@ -901,7 +969,8 @@ mod tests { .expect("writing metadata PDU failed"); PacketInfo::new(&buf[..written_len]).expect("generating packet info failed") } - fn create_no_error_eof(file_data: &[u8], pdu_header: &PduHeader, buf: &mut [u8]) -> EofPdu { + + fn create_no_error_eof(file_data: &[u8], pdu_header: &PduHeader) -> EofPdu { let mut digest = CRC_32.digest(); digest.update(file_data); let crc32 = digest.finalize(); @@ -935,7 +1004,7 @@ mod tests { assert_ne!(dest_handler.state(), State::Idle); assert_eq!(dest_handler.step(), TransactionStep::ReceivingFileDataPdus); - let eof_pdu = create_no_error_eof(&[], &pdu_header, &mut buf); + let eof_pdu = create_no_error_eof(&[], &pdu_header); let packet_info = create_packet_info(&eof_pdu, &mut buf); let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); @@ -989,7 +1058,7 @@ mod tests { let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); - let eof_pdu = create_no_error_eof(&file_data, &pdu_header, &mut buf); + let eof_pdu = create_no_error_eof(file_data, &pdu_header); let packet_info = create_packet_info(&eof_pdu, &mut buf); let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); @@ -1066,7 +1135,7 @@ mod tests { let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); - let eof_pdu = create_no_error_eof(&random_data, &pdu_header, &mut buf); + let eof_pdu = create_no_error_eof(&random_data, &pdu_header); let packet_info = create_packet_info(&eof_pdu, &mut buf); let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); assert!(result.is_ok()); diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index 436cdd4..7422798 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -1,4 +1,4 @@ -use core::hash::Hash; +use core::{cell::RefCell, fmt::Debug, hash::Hash}; use crc::{Crc, CRC_32_CKSUM}; use hashbrown::HashMap; @@ -39,8 +39,9 @@ pub enum EntityType { /// For the receiving entity, this timer determines the expiry period for incrementing a check /// counter after an EOF PDU is received for an incomplete file transfer. This allows out-of-order /// reception of file data PDUs and EOF PDUs. Also see 4.6.3.3 of the CFDP standard. -pub trait CheckTimer { +pub trait CheckTimer: Debug { fn has_expired(&self) -> bool; + fn reset(&mut self); } /// A generic trait which allows CFDP entities to create check timers which are required to @@ -64,6 +65,7 @@ pub trait CheckTimerCreator { /// Simple implementation of the [CheckTimerProvider] trait assuming a standard runtime. /// It also assumes that a second accuracy of the check timer period is sufficient. #[cfg(feature = "std")] +#[derive(Debug)] pub struct StdCheckTimer { expiry_time_seconds: u64, start_time: std::time::Instant, @@ -88,6 +90,10 @@ impl CheckTimer for StdCheckTimer { } false } + + fn reset(&mut self) { + self.start_time = std::time::Instant::now(); + } } #[derive(Debug, Copy, Clone)] @@ -186,7 +192,9 @@ pub trait UserFaultHandler { pub struct DefaultFaultHandler { handler_array: [FaultHandlerCode; 10], - user_fault_handler: Box, + // Could also change the user fault handler trait to have non mutable methods, but that limits + // flexbility on the user side.. + user_fault_handler: RefCell>, } impl DefaultFaultHandler { @@ -227,7 +235,7 @@ impl DefaultFaultHandler { .unwrap()] = FaultHandlerCode::IgnoreError; Self { handler_array: init_array, - user_fault_handler, + user_fault_handler: RefCell::new(user_fault_handler), } } @@ -240,7 +248,7 @@ impl DefaultFaultHandler { } pub fn report_fault( - &mut self, + &self, transaction_id: TransactionId, condition: ConditionCode, progress: u64, @@ -250,28 +258,19 @@ impl DefaultFaultHandler { return FaultHandlerCode::IgnoreError; } let fh_code = self.handler_array[array_idx.unwrap()]; + let mut handler_mut = self.user_fault_handler.borrow_mut(); match fh_code { FaultHandlerCode::NoticeOfCancellation => { - self.user_fault_handler.notice_of_cancellation_cb( - transaction_id, - condition, - progress, - ); + handler_mut.notice_of_cancellation_cb(transaction_id, condition, progress); } FaultHandlerCode::NoticeOfSuspension => { - self.user_fault_handler.notice_of_suspension_cb( - transaction_id, - condition, - progress, - ); + handler_mut.notice_of_suspension_cb(transaction_id, condition, progress); } FaultHandlerCode::IgnoreError => { - self.user_fault_handler - .ignore_cb(transaction_id, condition, progress); + handler_mut.ignore_cb(transaction_id, condition, progress); } FaultHandlerCode::AbandonTransaction => { - self.user_fault_handler - .abandoned_cb(transaction_id, condition, progress); + handler_mut.abandoned_cb(transaction_id, condition, progress); } } fh_code @@ -347,9 +346,10 @@ pub enum TransactionStep { Idle = 0, TransactionStart = 1, ReceivingFileDataPdus = 2, - SendingAckPdu = 3, - TransferCompletion = 4, - SendingFinishedPdu = 5, + ReceivingFileDataPdusWithCheckLimitHandling = 3, + SendingAckPdu = 4, + TransferCompletion = 5, + SendingFinishedPdu = 6, } #[derive(Debug, Copy, Clone, PartialEq, Eq)] -- 2.43.0 From 7615729af9c7d55c44c932704b03d19683ec78f4 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 14 Dec 2023 12:17:16 +0100 Subject: [PATCH 14/33] fix tests --- satrs-core/src/pus/verification.rs | 2 +- satrs-core/tests/pus_verification.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/satrs-core/src/pus/verification.rs b/satrs-core/src/pus/verification.rs index 35e4695..ff38b89 100644 --- a/satrs-core/src/pus/verification.rs +++ b/satrs-core/src/pus/verification.rs @@ -39,7 +39,7 @@ //! //! let mut sph = SpHeader::tc_unseg(TEST_APID, 0, 0).unwrap(); //! let tc_header = PusTcSecondaryHeader::new_simple(17, 1); -//! let pus_tc_0 = PusTcCreator::new(&mut sph, tc_header, None, true); +//! let pus_tc_0 = PusTcCreator::new_no_app_data(&mut sph, tc_header, true); //! let init_token = reporter.add_tc(&pus_tc_0); //! //! // Complete success sequence for a telecommand diff --git a/satrs-core/tests/pus_verification.rs b/satrs-core/tests/pus_verification.rs index 097eba8..17abd37 100644 --- a/satrs-core/tests/pus_verification.rs +++ b/satrs-core/tests/pus_verification.rs @@ -9,7 +9,7 @@ pub mod crossbeam_test { use satrs_core::tmtc::tm_helper::SharedTmStore; use spacepackets::ecss::tc::{PusTcCreator, PusTcReader, PusTcSecondaryHeader}; use spacepackets::ecss::tm::PusTmReader; - use spacepackets::ecss::{EcssEnumU16, EcssEnumU8, PusPacket, SerializablePusPacket}; + use spacepackets::ecss::{EcssEnumU16, EcssEnumU8, PusPacket, WritablePusPacket}; use spacepackets::SpHeader; use std::sync::{Arc, RwLock}; use std::thread; @@ -54,16 +54,16 @@ pub mod crossbeam_test { let mut tc_guard = shared_tc_pool_0.write().unwrap(); let mut sph = SpHeader::tc_unseg(TEST_APID, 0, 0).unwrap(); let tc_header = PusTcSecondaryHeader::new_simple(17, 1); - let pus_tc_0 = PusTcCreator::new(&mut sph, tc_header, None, true); + let pus_tc_0 = PusTcCreator::new_no_app_data(&mut sph, tc_header, true); req_id_0 = RequestId::new(&pus_tc_0); - let (addr, mut buf) = tc_guard.free_element(pus_tc_0.len_packed()).unwrap(); + let (addr, mut buf) = tc_guard.free_element(pus_tc_0.len_written()).unwrap(); pus_tc_0.write_to_bytes(&mut buf).unwrap(); tx_tc_0.send(addr).unwrap(); let mut sph = SpHeader::tc_unseg(TEST_APID, 1, 0).unwrap(); let tc_header = PusTcSecondaryHeader::new_simple(5, 1); - let pus_tc_1 = PusTcCreator::new(&mut sph, tc_header, None, true); + let pus_tc_1 = PusTcCreator::new_no_app_data(&mut sph, tc_header, true); req_id_1 = RequestId::new(&pus_tc_1); - let (addr, mut buf) = tc_guard.free_element(pus_tc_0.len_packed()).unwrap(); + let (addr, mut buf) = tc_guard.free_element(pus_tc_0.len_written()).unwrap(); pus_tc_1.write_to_bytes(&mut buf).unwrap(); tx_tc_1.send(addr).unwrap(); } -- 2.43.0 From 4e81fd2e169500b2c6dbcf126f91d61ba2c30802 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 14 Dec 2023 12:27:37 +0100 Subject: [PATCH 15/33] some improvements --- satrs-core/tests/pus_verification.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/satrs-core/tests/pus_verification.rs b/satrs-core/tests/pus_verification.rs index 17abd37..8c8a0af 100644 --- a/satrs-core/tests/pus_verification.rs +++ b/satrs-core/tests/pus_verification.rs @@ -56,15 +56,15 @@ pub mod crossbeam_test { let tc_header = PusTcSecondaryHeader::new_simple(17, 1); let pus_tc_0 = PusTcCreator::new_no_app_data(&mut sph, tc_header, true); req_id_0 = RequestId::new(&pus_tc_0); - let (addr, mut buf) = tc_guard.free_element(pus_tc_0.len_written()).unwrap(); - pus_tc_0.write_to_bytes(&mut buf).unwrap(); + let (addr, buf) = tc_guard.free_element(pus_tc_0.len_written()).unwrap(); + pus_tc_0.write_to_bytes(buf).unwrap(); tx_tc_0.send(addr).unwrap(); let mut sph = SpHeader::tc_unseg(TEST_APID, 1, 0).unwrap(); let tc_header = PusTcSecondaryHeader::new_simple(5, 1); let pus_tc_1 = PusTcCreator::new_no_app_data(&mut sph, tc_header, true); req_id_1 = RequestId::new(&pus_tc_1); - let (addr, mut buf) = tc_guard.free_element(pus_tc_0.len_written()).unwrap(); - pus_tc_1.write_to_bytes(&mut buf).unwrap(); + let (addr, buf) = tc_guard.free_element(pus_tc_0.len_written()).unwrap(); + pus_tc_1.write_to_bytes(buf).unwrap(); tx_tc_1.send(addr).unwrap(); } let verif_sender_0 = thread::spawn(move || { @@ -81,16 +81,14 @@ pub mod crossbeam_test { tc_buf[0..tc_len].copy_from_slice(buf); } let (_tc, _) = PusTcReader::new(&tc_buf[0..tc_len]).unwrap(); - let accepted_token; let token = reporter_with_sender_0.add_tc_with_req_id(req_id_0); - accepted_token = reporter_with_sender_0 + let accepted_token = reporter_with_sender_0 .acceptance_success(token, Some(&FIXED_STAMP)) .expect("Acceptance success failed"); // Do some start handling here - let started_token; - started_token = reporter_with_sender_0 + let started_token = reporter_with_sender_0 .start_success(accepted_token, Some(&FIXED_STAMP)) .expect("Start success failed"); // Do some step handling here @@ -158,8 +156,7 @@ pub mod crossbeam_test { RequestId::from_bytes(&pus_tm.source_data()[0..RequestId::SIZE_AS_BYTES]) .expect("reading request ID from PUS TM source data failed"); if !verif_map.contains_key(&req_id) { - let mut content = Vec::new(); - content.push(pus_tm.subservice()); + let content =vec![pus_tm.subservice()]; verif_map.insert(req_id, content); } else { let content = verif_map.get_mut(&req_id).unwrap(); -- 2.43.0 From 37c2f72cbcc4512d855a7ae31eff456600f3fcea Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 14 Dec 2023 14:22:52 +0100 Subject: [PATCH 16/33] cargo fmt --- satrs-core/tests/pus_verification.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/satrs-core/tests/pus_verification.rs b/satrs-core/tests/pus_verification.rs index 8c8a0af..9c9fc63 100644 --- a/satrs-core/tests/pus_verification.rs +++ b/satrs-core/tests/pus_verification.rs @@ -156,7 +156,7 @@ pub mod crossbeam_test { RequestId::from_bytes(&pus_tm.source_data()[0..RequestId::SIZE_AS_BYTES]) .expect("reading request ID from PUS TM source data failed"); if !verif_map.contains_key(&req_id) { - let content =vec![pus_tm.subservice()]; + let content = vec![pus_tm.subservice()]; verif_map.insert(req_id, content); } else { let content = verif_map.get_mut(&req_id).unwrap(); -- 2.43.0 From 27c5e4d14eadc6763af5f4bbf57a8336a2e30965 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 15 Dec 2023 13:49:24 +0100 Subject: [PATCH 17/33] cleaned up tests --- satrs-core/src/cfdp/dest.rs | 346 ++++++++++++++++++------------------ 1 file changed, 175 insertions(+), 171 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 0b564b2..9a55350 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -621,6 +621,9 @@ impl DestinationHandler { } fn declare_fault(&mut self, condition_code: ConditionCode) -> FaultHandlerCode { + // Cache those, because they might be reset when abandoning the transaction. + let transaction_id = self.tstate().transaction_id.unwrap(); + let progress = self.tstate().progress; let fh_code = self .local_cfg .default_fault_handler @@ -630,19 +633,19 @@ impl DestinationHandler { self.notice_of_cancellation(); } FaultHandlerCode::NoticeOfSuspension => self.notice_of_suspension(), - FaultHandlerCode::IgnoreError => todo!(), - FaultHandlerCode::AbandonTransaction => todo!(), + FaultHandlerCode::IgnoreError => (), + FaultHandlerCode::AbandonTransaction => self.abandon_transaction(), } - let tstate = self.tstate(); - self.local_cfg.default_fault_handler.report_fault( - tstate.transaction_id.unwrap(), - condition_code, - tstate.progress, - ) + self.local_cfg + .default_fault_handler + .report_fault(transaction_id, condition_code, progress) } fn notice_of_cancellation(&mut self) {} fn notice_of_suspension(&mut self) {} + fn abandon_transaction(&mut self) { + self.reset(); + } fn reset(&mut self) { self.step = TransactionStep::Idle; @@ -670,7 +673,7 @@ mod tests { #[allow(unused_imports)] use std::println; - use alloc::{collections::VecDeque, string::String}; + use alloc::{collections::VecDeque, string::String, vec::Vec}; use rand::Rng; use spacepackets::{ cfdp::{ @@ -697,7 +700,7 @@ mod tests { next_expected_seq_num: u64, expected_full_src_name: String, expected_full_dest_name: String, - expected_file_size: usize, + expected_file_size: u64, } impl TestCfdpUser { @@ -783,7 +786,7 @@ mod tests { ); assert_eq!(md_recvd_params.msgs_to_user.len(), 0); assert_eq!(md_recvd_params.source_id, LOCAL_ID.into()); - assert_eq!(md_recvd_params.file_size as usize, self.expected_file_size); + assert_eq!(md_recvd_params.file_size, self.expected_file_size); } fn file_segment_recvd_indication( @@ -888,6 +891,110 @@ mod tests { } } + struct TestClass { + handler: DestinationHandler, + src_path: PathBuf, + dest_path: PathBuf, + check_dest_file: bool, + check_handler_idle_at_drop: bool, + expected_file_size: u64, + pdu_header: PduHeader, + expected_full_data: Vec, + buf: [u8; 512], + } + + impl TestClass { + fn new() -> Self { + let dest_handler = default_dest_handler(); + let (src_path, dest_path) = init_full_filenames(); + assert!(!Path::exists(&dest_path)); + let handler = Self { + handler: dest_handler, + src_path, + dest_path, + check_dest_file: true, + check_handler_idle_at_drop: true, + expected_file_size: 0, + pdu_header: create_pdu_header(UbfU16::new(0)), + expected_full_data: Vec::new(), + buf: [0; 512], + }; + handler.state_check(State::Idle, TransactionStep::Idle); + handler + } + + fn test_user_from_cached_paths(&self, expected_file_size: u64) -> TestCfdpUser { + TestCfdpUser { + next_expected_seq_num: 0, + expected_full_src_name: self.src_path.to_string_lossy().into(), + expected_full_dest_name: self.dest_path.to_string_lossy().into(), + expected_file_size, + } + } + + fn generic_transfer_init( + &mut self, + user: &mut impl CfdpUser, + file_size: u64, + ) -> Result<(), DestError> { + self.expected_file_size = file_size; + let metadata_pdu = create_metadata_pdu( + &self.pdu_header, + self.src_path.as_path(), + self.dest_path.as_path(), + file_size, + ); + let packet_info = create_packet_info(&metadata_pdu, &mut self.buf); + self.handler.state_machine(user, Some(&packet_info)) + } + + fn generic_file_data_insert( + &mut self, + user: &mut impl CfdpUser, + offset: u64, + file_data_chunk: &[u8], + ) -> Result<(), DestError> { + let filedata_pdu = + FileDataPdu::new_no_seg_metadata(self.pdu_header, offset, file_data_chunk); + filedata_pdu + .write_to_bytes(&mut self.buf) + .expect("writing file data PDU failed"); + let packet_info = PacketInfo::new(&self.buf).expect("creating packet info failed"); + self.handler.state_machine(user, Some(&packet_info)) + } + + fn generic_eof_no_error( + &mut self, + user: &mut impl CfdpUser, + expected_full_data: Vec, + ) -> Result<(), DestError> { + self.expected_full_data = expected_full_data; + let eof_pdu = create_no_error_eof(&self.expected_full_data, &self.pdu_header); + let packet_info = create_packet_info(&eof_pdu, &mut self.buf); + self.handler.state_machine(user, Some(&packet_info)) + } + + fn state_check(&self, state: State, step: TransactionStep) { + assert_eq!(self.handler.state(), state); + assert_eq!(self.handler.step(), step); + } + } + + impl Drop for TestClass { + fn drop(&mut self) { + if self.check_handler_idle_at_drop { + self.state_check(State::Idle, TransactionStep::Idle); + } + if self.check_dest_file { + assert!(Path::exists(&self.dest_path)); + let read_content = fs::read(&self.dest_path).expect("reading back string failed"); + assert_eq!(read_content.len() as u64, self.expected_file_size); + assert_eq!(read_content, self.expected_full_data); + assert!(fs::remove_file(self.dest_path.as_path()).is_ok()); + } + } + } + fn init_check(handler: &DestinationHandler) { assert_eq!(handler.state(), State::Idle); assert_eq!(handler.step(), TransactionStep::Idle); @@ -932,11 +1039,6 @@ mod tests { Box::new(TestCheckTimerCreator::new(2, 2)), ) } - #[test] - fn test_basic() { - let dest_handler = default_dest_handler(); - init_check(&dest_handler); - } fn create_pdu_header(seq_num: impl Into) -> PduHeader { let mut pdu_conf = @@ -971,181 +1073,83 @@ mod tests { } fn create_no_error_eof(file_data: &[u8], pdu_header: &PduHeader) -> EofPdu { - let mut digest = CRC_32.digest(); - digest.update(file_data); - let crc32 = digest.finalize(); + let crc32 = if !file_data.is_empty() { + let mut digest = CRC_32.digest(); + digest.update(file_data); + digest.finalize() + } else { + 0 + }; EofPdu::new_no_error(*pdu_header, crc32, file_data.len() as u64) } + #[test] + fn test_basic() { + default_dest_handler(); + } + #[test] fn test_empty_file_transfer_not_acked() { - let (src_name, dest_name) = init_full_filenames(); - assert!(!Path::exists(&dest_name)); - let mut buf: [u8; 512] = [0; 512]; - let mut test_user = TestCfdpUser { - next_expected_seq_num: 0, - expected_full_src_name: src_name.to_string_lossy().into(), - expected_full_dest_name: dest_name.to_string_lossy().into(), - expected_file_size: 0, - }; - // We treat the destination handler like it is a remote entity. - let mut dest_handler = default_dest_handler(); - init_check(&dest_handler); - - let seq_num = UbfU16::new(0); - let pdu_header = create_pdu_header(seq_num); - let metadata_pdu = - create_metadata_pdu(&pdu_header, src_name.as_path(), dest_name.as_path(), 0); - let packet_info = create_packet_info(&metadata_pdu, &mut buf); - let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); - if let Err(e) = result { - panic!("dest handler fsm error: {e}"); - } - assert_ne!(dest_handler.state(), State::Idle); - assert_eq!(dest_handler.step(), TransactionStep::ReceivingFileDataPdus); - - let eof_pdu = create_no_error_eof(&[], &pdu_header); - let packet_info = create_packet_info(&eof_pdu, &mut buf); - let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); - assert!(result.is_ok()); - assert_eq!(dest_handler.state(), State::Idle); - assert_eq!(dest_handler.step(), TransactionStep::Idle); - assert!(Path::exists(&dest_name)); - let read_content = fs::read(&dest_name).expect("reading back string failed"); - assert_eq!(read_content.len(), 0); - assert!(fs::remove_file(dest_name).is_ok()); + let mut test_obj = TestClass::new(); + let mut test_user = test_obj.test_user_from_cached_paths(0); + test_obj + .generic_transfer_init(&mut test_user, 0) + .expect("transfer init failed"); + test_obj.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + test_obj + .generic_eof_no_error(&mut test_user, Vec::new()) + .expect("EOF no error insertion failed"); } #[test] fn test_small_file_transfer_not_acked() { - let (src_name, dest_name) = init_full_filenames(); - assert!(!Path::exists(&dest_name)); let file_data_str = "Hello World!"; let file_data = file_data_str.as_bytes(); - let mut buf: [u8; 512] = [0; 512]; - let mut test_user = TestCfdpUser { - next_expected_seq_num: 0, - expected_full_src_name: src_name.to_string_lossy().into(), - expected_full_dest_name: dest_name.to_string_lossy().into(), - expected_file_size: file_data.len(), - }; - // We treat the destination handler like it is a remote entity. - let mut dest_handler = default_dest_handler(); - init_check(&dest_handler); + let file_size = file_data.len() as u64; - let seq_num = UbfU16::new(0); - let pdu_header = create_pdu_header(seq_num); - let metadata_pdu = create_metadata_pdu( - &pdu_header, - src_name.as_path(), - dest_name.as_path(), - file_data.len() as u64, - ); - let packet_info = create_packet_info(&metadata_pdu, &mut buf); - let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); - if let Err(e) = result { - panic!("dest handler fsm error: {e}"); - } - assert_ne!(dest_handler.state(), State::Idle); - assert_eq!(dest_handler.step(), TransactionStep::ReceivingFileDataPdus); - - let offset = 0; - let filedata_pdu = FileDataPdu::new_no_seg_metadata(pdu_header, offset, file_data); - filedata_pdu - .write_to_bytes(&mut buf) - .expect("writing file data PDU failed"); - let packet_info = PacketInfo::new(&buf).expect("creating packet info failed"); - let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); - assert!(result.is_ok()); - - let eof_pdu = create_no_error_eof(file_data, &pdu_header); - let packet_info = create_packet_info(&eof_pdu, &mut buf); - let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); - assert!(result.is_ok()); - assert_eq!(dest_handler.state(), State::Idle); - assert_eq!(dest_handler.step(), TransactionStep::Idle); - - assert!(Path::exists(&dest_name)); - let read_content = fs::read_to_string(&dest_name).expect("reading back string failed"); - assert_eq!(read_content, file_data_str); - assert!(fs::remove_file(dest_name).is_ok()); + let mut test_obj = TestClass::new(); + let mut test_user = test_obj.test_user_from_cached_paths(file_size); + test_obj + .generic_transfer_init(&mut test_user, file_size) + .expect("transfer init failed"); + test_obj.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + test_obj + .generic_file_data_insert(&mut test_user, 0, file_data) + .expect("file data insertion failed"); + test_obj + .generic_eof_no_error(&mut test_user, file_data.to_vec()) + .expect("EOF no error insertion failed"); } #[test] fn test_segmented_file_transfer_not_acked() { - let (src_name, dest_name) = init_full_filenames(); - assert!(!Path::exists(&dest_name)); let mut rng = rand::thread_rng(); let mut random_data = [0u8; 512]; rng.fill(&mut random_data); - let mut buf: [u8; 512] = [0; 512]; - let mut test_user = TestCfdpUser { - next_expected_seq_num: 0, - expected_full_src_name: src_name.to_string_lossy().into(), - expected_full_dest_name: dest_name.to_string_lossy().into(), - expected_file_size: random_data.len(), - }; - - // We treat the destination handler like it is a remote entity. - let mut dest_handler = default_dest_handler(); - init_check(&dest_handler); - - let seq_num = UbfU16::new(0); - let pdu_header = create_pdu_header(seq_num); - let metadata_pdu = create_metadata_pdu( - &pdu_header, - src_name.as_path(), - dest_name.as_path(), - random_data.len() as u64, - ); - let packet_info = create_packet_info(&metadata_pdu, &mut buf); - let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); - if let Err(e) = result { - panic!("dest handler fsm error: {e}"); - } - assert_ne!(dest_handler.state(), State::Idle); - assert_eq!(dest_handler.step(), TransactionStep::ReceivingFileDataPdus); - - // First file data PDU - let mut offset: usize = 0; + let file_size = random_data.len() as u64; let segment_len = 256; - let filedata_pdu = FileDataPdu::new_no_seg_metadata( - pdu_header, - offset as u64, - &random_data[0..segment_len], - ); - filedata_pdu - .write_to_bytes(&mut buf) - .expect("writing file data PDU failed"); - let packet_info = PacketInfo::new(&buf).expect("creating packet info failed"); - let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); - assert!(result.is_ok()); - // Second file data PDU - offset += segment_len; - let filedata_pdu = FileDataPdu::new_no_seg_metadata( - pdu_header, - offset as u64, - &random_data[segment_len..], - ); - filedata_pdu - .write_to_bytes(&mut buf) - .expect("writing file data PDU failed"); - let packet_info = PacketInfo::new(&buf).expect("creating packet info failed"); - let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); - assert!(result.is_ok()); - - let eof_pdu = create_no_error_eof(&random_data, &pdu_header); - let packet_info = create_packet_info(&eof_pdu, &mut buf); - let result = dest_handler.state_machine(&mut test_user, Some(&packet_info)); - assert!(result.is_ok()); - assert_eq!(dest_handler.state(), State::Idle); - assert_eq!(dest_handler.step(), TransactionStep::Idle); - - // Clean up - assert!(Path::exists(&dest_name)); - let read_content = fs::read(&dest_name).expect("reading back string failed"); - assert_eq!(read_content, random_data); - assert!(fs::remove_file(dest_name).is_ok()); + let mut test_obj = TestClass::new(); + let mut test_user = test_obj.test_user_from_cached_paths(file_size); + test_obj + .generic_transfer_init(&mut test_user, file_size) + .expect("transfer init failed"); + test_obj.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + test_obj + .generic_file_data_insert(&mut test_user, 0, &random_data[0..segment_len]) + .expect("file data insertion failed"); + test_obj + .generic_file_data_insert( + &mut test_user, + segment_len as u64, + &random_data[segment_len..], + ) + .expect("file data insertion failed"); + test_obj + .generic_eof_no_error(&mut test_user, random_data.to_vec()) + .expect("EOF no error insertion failed"); } + + #[test] + fn test_check_limit_handling() {} } -- 2.43.0 From 9605dbb13a9f6dd5524fcca2adf49eef24fb142e Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 15 Dec 2023 15:30:41 +0100 Subject: [PATCH 18/33] mocking the check timer is tricky --- satrs-core/src/cfdp/dest.rs | 97 ++++++++++++++++++++++--------------- 1 file changed, 59 insertions(+), 38 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 9a55350..029d3cc 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -423,6 +423,7 @@ impl DestinationHandler { return Ok(false); } self.start_check_limit_handling(); + return Ok(false); } Ok(true) } @@ -668,12 +669,12 @@ impl DestinationHandler { #[cfg(test)] mod tests { - use core::cell::Cell; + use core::{cell::Cell, sync::atomic::AtomicBool}; use std::fs; #[allow(unused_imports)] use std::println; - use alloc::{collections::VecDeque, string::String, vec::Vec}; + use alloc::{collections::VecDeque, string::String, sync::Arc, vec::Vec}; use rand::Rng; use spacepackets::{ cfdp::{ @@ -833,17 +834,12 @@ mod tests { #[derive(Debug)] struct TestCheckTimer { counter: Cell, - expiry_count: u32, + expired: Arc, } impl CheckTimer for TestCheckTimer { fn has_expired(&self) -> bool { - let current_counter = self.counter.get(); - if self.expiry_count == current_counter { - return true; - } - self.counter.set(current_counter + 1); - false + self.expired.load(core::sync::atomic::Ordering::Relaxed) } fn reset(&mut self) { self.counter.set(0); @@ -851,28 +847,26 @@ mod tests { } impl TestCheckTimer { - pub fn new(expiry_after_x_calls: u32) -> Self { + pub fn new(expired_flag: Arc) -> Self { Self { counter: Cell::new(0), - expiry_count: expiry_after_x_calls, + expired: expired_flag, } } + + fn set_expired(&mut self) { + self.expired + .store(true, core::sync::atomic::Ordering::Relaxed); + } } struct TestCheckTimerCreator { - expiry_counter_for_source_entity: u32, - expiry_counter_for_dest_entity: u32, + expired_flag: Arc, } impl TestCheckTimerCreator { - pub fn new( - expiry_counter_for_source_entity: u32, - expiry_counter_for_dest_entity: u32, - ) -> Self { - Self { - expiry_counter_for_source_entity, - expiry_counter_for_dest_entity, - } + pub fn new(expired_flag: Arc) -> Self { + Self { expired_flag } } } @@ -881,17 +875,14 @@ mod tests { &self, _local_id: &UnsignedByteField, _remote_id: &UnsignedByteField, - entity_type: crate::cfdp::EntityType, + _entity_type: crate::cfdp::EntityType, ) -> Box { - if entity_type == crate::cfdp::EntityType::Sending { - Box::new(TestCheckTimer::new(self.expiry_counter_for_source_entity)) - } else { - Box::new(TestCheckTimer::new(self.expiry_counter_for_dest_entity)) - } + Box::new(TestCheckTimer::new(self.expired_flag.clone())) } } struct TestClass { + check_timer_expired: Arc, handler: DestinationHandler, src_path: PathBuf, dest_path: PathBuf, @@ -905,10 +896,12 @@ mod tests { impl TestClass { fn new() -> Self { - let dest_handler = default_dest_handler(); + let check_timer_expired = Arc::new(AtomicBool::new(false)); + let dest_handler = default_dest_handler(check_timer_expired.clone()); let (src_path, dest_path) = init_full_filenames(); assert!(!Path::exists(&dest_path)); let handler = Self { + check_timer_expired, handler: dest_handler, src_path, dest_path, @@ -923,6 +916,11 @@ mod tests { handler } + fn set_check_timer_expired(&mut self) { + self.check_timer_expired + .store(true, core::sync::atomic::Ordering::Relaxed); + } + fn test_user_from_cached_paths(&self, expected_file_size: u64) -> TestCfdpUser { TestCfdpUser { next_expected_seq_num: 0, @@ -995,11 +993,6 @@ mod tests { } } - fn init_check(handler: &DestinationHandler) { - assert_eq!(handler.state(), State::Idle); - assert_eq!(handler.step(), TransactionStep::Idle); - } - fn init_full_filenames() -> (PathBuf, PathBuf) { ( tempfile::TempPath::from_path("/tmp/test.txt").to_path_buf(), @@ -1025,7 +1018,7 @@ mod tests { table } - fn default_dest_handler() -> DestinationHandler { + fn default_dest_handler(check_timer_expired: Arc) -> DestinationHandler { let test_fault_handler = TestFaultHandler::default(); let local_entity_cfg = LocalEntityConfig { id: REMOTE_ID.into(), @@ -1036,7 +1029,7 @@ mod tests { local_entity_cfg, Box::::default(), Box::new(basic_remote_cfg_table()), - Box::new(TestCheckTimerCreator::new(2, 2)), + Box::new(TestCheckTimerCreator::new(check_timer_expired)), ) } @@ -1053,7 +1046,12 @@ mod tests { dest_name: &'filename Path, file_size: u64, ) -> MetadataPduCreator<'filename, 'filename, 'static> { - let metadata_params = MetadataGenericParams::new(false, ChecksumType::Crc32, file_size); + let checksum_type = if file_size == 0 { + ChecksumType::NullChecksum + } else { + ChecksumType::Crc32 + }; + let metadata_params = MetadataGenericParams::new(false, checksum_type, file_size); MetadataPduCreator::new_no_opts( *pdu_header, metadata_params, @@ -1085,7 +1083,7 @@ mod tests { #[test] fn test_basic() { - default_dest_handler(); + default_dest_handler(Arc::default()); } #[test] @@ -1151,5 +1149,28 @@ mod tests { } #[test] - fn test_check_limit_handling() {} + fn test_check_limit_handling() { + let mut rng = rand::thread_rng(); + let mut random_data = [0u8; 512]; + rng.fill(&mut random_data); + let file_size = random_data.len() as u64; + let segment_len = 256; + + let mut test_obj = TestClass::new(); + let mut test_user = test_obj.test_user_from_cached_paths(file_size); + test_obj + .generic_transfer_init(&mut test_user, file_size) + .expect("transfer init failed"); + test_obj.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + test_obj + .generic_file_data_insert(&mut test_user, 0, &random_data[0..segment_len]) + .expect("file data insertion failed"); + test_obj + .generic_eof_no_error(&mut test_user, random_data.to_vec()) + .expect("EOF no error insertion failed"); + test_obj.state_check( + State::Busy, + TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling, + ); + } } -- 2.43.0 From 2f07fdfe83375d28fed273791fcdae14c852b38f Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 15 Dec 2023 16:08:39 +0100 Subject: [PATCH 19/33] first test for check limit handling --- satrs-core/src/cfdp/dest.rs | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 029d3cc..61fdb75 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -355,7 +355,10 @@ impl DestinationHandler { } pub fn handle_file_data(&mut self, raw_packet: &[u8]) -> Result<(), DestError> { - if self.state == State::Idle || self.step != TransactionStep::ReceivingFileDataPdus { + if self.state == State::Idle + || (self.step != TransactionStep::ReceivingFileDataPdus + && self.step != TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling) + { return Err(DestError::WrongStateForFileDataAndEof); } let fd_pdu = FileDataPdu::from_bytes(raw_packet)?; @@ -506,12 +509,12 @@ impl DestinationHandler { if self.step == TransactionStep::TransactionStart { self.transaction_start(cfdp_user)?; } - if self.step == TransactionStep::TransferCompletion { - self.transfer_completion(cfdp_user)?; - } if self.step == TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling { self.check_limit_handling(); } + if self.step == TransactionStep::TransferCompletion { + self.transfer_completion(cfdp_user)?; + } if self.step == TransactionStep::SendingAckPdu { todo!("no support for acknowledged mode yet"); } @@ -853,11 +856,6 @@ mod tests { expired: expired_flag, } } - - fn set_expired(&mut self) { - self.expired - .store(true, core::sync::atomic::Ordering::Relaxed); - } } struct TestCheckTimerCreator { @@ -1164,7 +1162,7 @@ mod tests { test_obj.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); test_obj .generic_file_data_insert(&mut test_user, 0, &random_data[0..segment_len]) - .expect("file data insertion failed"); + .expect("file data insertion 0 failed"); test_obj .generic_eof_no_error(&mut test_user, random_data.to_vec()) .expect("EOF no error insertion failed"); @@ -1172,5 +1170,17 @@ mod tests { State::Busy, TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling, ); + test_obj + .generic_file_data_insert( + &mut test_user, + segment_len as u64, + &random_data[segment_len..], + ) + .expect("file data insertion 1 failed"); + test_obj.set_check_timer_expired(); + test_obj + .handler + .state_machine(&mut test_user, None) + .expect("fsm failure"); } } -- 2.43.0 From 28a8b18329bc111ee85900387780f66153fa4670 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 15 Dec 2023 16:10:46 +0100 Subject: [PATCH 20/33] the file store still required alloc --- satrs-core/src/cfdp/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index 7422798..110b588 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -17,6 +17,7 @@ use serde::{Deserialize, Serialize}; #[cfg(feature = "std")] pub mod dest; +#[cfg(feature = "alloc")] pub mod filestore; #[cfg(feature = "std")] pub mod source; -- 2.43.0 From 7e8be538e0a2bdffcc65ab4b3ec2422bc2facc08 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 15 Dec 2023 17:18:28 +0100 Subject: [PATCH 21/33] improve CFDP tests --- satrs-core/src/cfdp/dest.rs | 267 +++++++++++++++++++++++------------- satrs-core/src/cfdp/user.rs | 39 +++++- 2 files changed, 208 insertions(+), 98 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 61fdb75..b10afab 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -4,7 +4,7 @@ use std::path::{Path, PathBuf}; use super::{ filestore::{FilestoreError, VirtualFilestore}, - user::{CfdpUser, MetadataReceivedParams}, + user::{CfdpUser, FileSegmentRecvdParams, MetadataReceivedParams}, CheckTimer, CheckTimerCreator, EntityType, LocalEntityConfig, PacketInfo, PacketTarget, RemoteEntityConfig, RemoteEntityConfigProvider, State, TransactionId, TransactionStep, }; @@ -229,7 +229,7 @@ impl DestinationHandler { packet_info.raw_packet, ) } - PduType::FileData => self.handle_file_data(packet_info.raw_packet), + PduType::FileData => self.handle_file_data(cfdp_user, packet_info.raw_packet), } } @@ -354,7 +354,11 @@ impl DestinationHandler { Ok(()) } - pub fn handle_file_data(&mut self, raw_packet: &[u8]) -> Result<(), DestError> { + pub fn handle_file_data( + &mut self, + user: &mut impl CfdpUser, + raw_packet: &[u8], + ) -> Result<(), DestError> { if self.state == State::Idle || (self.step != TransactionStep::ReceivingFileDataPdus && self.step != TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling) @@ -362,6 +366,14 @@ impl DestinationHandler { return Err(DestError::WrongStateForFileDataAndEof); } let fd_pdu = FileDataPdu::from_bytes(raw_packet)?; + if self.local_cfg.indication_cfg.file_segment_recv { + user.file_segment_recvd_indication(&FileSegmentRecvdParams { + id: self.tstate().transaction_id.unwrap(), + offset: fd_pdu.offset(), + length: fd_pdu.file_data().len(), + segment_metadata: fd_pdu.segment_metadata(), + }); + } if let Err(e) = self.vfs.write_data( self.tparams.file_properties.dest_path_buf.to_str().unwrap(), fd_pdu.offset(), @@ -689,9 +701,9 @@ mod tests { }; use crate::cfdp::{ - filestore::NativeFilestore, CheckTimer, CheckTimerCreator, DefaultFaultHandler, - IndicationConfig, RemoteEntityConfig, StdRemoteEntityConfigProvider, UserFaultHandler, - CRC_32, + filestore::NativeFilestore, user::OwnedMetadataRecvdParams, CheckTimer, CheckTimerCreator, + DefaultFaultHandler, IndicationConfig, RemoteEntityConfig, StdRemoteEntityConfigProvider, + UserFaultHandler, CRC_32, }; use super::*; @@ -699,21 +711,137 @@ mod tests { const LOCAL_ID: UnsignedByteFieldU16 = UnsignedByteFieldU16::new(1); const REMOTE_ID: UnsignedByteFieldU16 = UnsignedByteFieldU16::new(2); + pub struct FileSegmentRecvdParamsNoSegMetadata { + pub id: TransactionId, + pub offset: u64, + pub length: usize, + } + #[derive(Default)] struct TestCfdpUser { next_expected_seq_num: u64, expected_full_src_name: String, expected_full_dest_name: String, expected_file_size: u64, + transaction_indication_call_count: u32, + eof_recvd_call_count: u32, + finished_indic_queue: VecDeque, + metadata_recv_queue: VecDeque, + file_seg_recvd_queue: VecDeque, } impl TestCfdpUser { + fn new( + next_expected_seq_num: u64, + expected_full_src_name: String, + expected_full_dest_name: String, + expected_file_size: u64, + ) -> Self { + Self { + next_expected_seq_num, + expected_full_src_name, + expected_full_dest_name, + expected_file_size, + transaction_indication_call_count: 0, + eof_recvd_call_count: 0, + finished_indic_queue: VecDeque::new(), + metadata_recv_queue: VecDeque::new(), + file_seg_recvd_queue: VecDeque::new(), + } + } + fn generic_id_check(&self, id: &crate::cfdp::TransactionId) { assert_eq!(id.source_id, LOCAL_ID.into()); assert_eq!(id.seq_num().value(), self.next_expected_seq_num); } } + impl CfdpUser for TestCfdpUser { + fn transaction_indication(&mut self, id: &crate::cfdp::TransactionId) { + self.generic_id_check(id); + self.transaction_indication_call_count += 1; + } + + fn eof_sent_indication(&mut self, id: &crate::cfdp::TransactionId) { + self.generic_id_check(id); + } + + fn transaction_finished_indication( + &mut self, + finished_params: &crate::cfdp::user::TransactionFinishedParams, + ) { + self.generic_id_check(&finished_params.id); + self.finished_indic_queue.push_back(*finished_params); + } + + fn metadata_recvd_indication( + &mut self, + md_recvd_params: &crate::cfdp::user::MetadataReceivedParams, + ) { + self.generic_id_check(&md_recvd_params.id); + assert_eq!( + String::from(md_recvd_params.src_file_name), + self.expected_full_src_name + ); + assert_eq!( + String::from(md_recvd_params.dest_file_name), + self.expected_full_dest_name + ); + assert_eq!(md_recvd_params.msgs_to_user.len(), 0); + assert_eq!(md_recvd_params.source_id, LOCAL_ID.into()); + assert_eq!(md_recvd_params.file_size, self.expected_file_size); + self.metadata_recv_queue.push_back(md_recvd_params.into()); + } + + fn file_segment_recvd_indication( + &mut self, + segment_recvd_params: &crate::cfdp::user::FileSegmentRecvdParams, + ) { + self.generic_id_check(&segment_recvd_params.id); + self.file_seg_recvd_queue + .push_back(FileSegmentRecvdParamsNoSegMetadata { + id: segment_recvd_params.id, + offset: segment_recvd_params.offset, + length: segment_recvd_params.length, + }) + } + + fn report_indication(&mut self, _id: &crate::cfdp::TransactionId) {} + + fn suspended_indication( + &mut self, + _id: &crate::cfdp::TransactionId, + _condition_code: ConditionCode, + ) { + panic!("unexpected suspended indication"); + } + + fn resumed_indication(&mut self, _id: &crate::cfdp::TransactionId, _progresss: u64) {} + + fn fault_indication( + &mut self, + _id: &crate::cfdp::TransactionId, + _condition_code: ConditionCode, + _progress: u64, + ) { + panic!("unexpected fault indication"); + } + + fn abandoned_indication( + &mut self, + _id: &crate::cfdp::TransactionId, + _condition_code: ConditionCode, + _progress: u64, + ) { + panic!("unexpected abandoned indication"); + } + + fn eof_recvd_indication(&mut self, id: &crate::cfdp::TransactionId) { + self.generic_id_check(id); + self.eof_recvd_call_count += 1; + } + } + #[derive(Default)] struct TestFaultHandler { notice_of_suspension_queue: VecDeque<(TransactionId, ConditionCode, u64)>, @@ -759,81 +887,6 @@ mod tests { } } - impl CfdpUser for TestCfdpUser { - fn transaction_indication(&mut self, id: &crate::cfdp::TransactionId) { - self.generic_id_check(id); - } - - fn eof_sent_indication(&mut self, id: &crate::cfdp::TransactionId) { - self.generic_id_check(id); - } - - fn transaction_finished_indication( - &mut self, - finished_params: &crate::cfdp::user::TransactionFinishedParams, - ) { - self.generic_id_check(&finished_params.id); - } - - fn metadata_recvd_indication( - &mut self, - md_recvd_params: &crate::cfdp::user::MetadataReceivedParams, - ) { - self.generic_id_check(&md_recvd_params.id); - assert_eq!( - String::from(md_recvd_params.src_file_name), - self.expected_full_src_name - ); - assert_eq!( - String::from(md_recvd_params.dest_file_name), - self.expected_full_dest_name - ); - assert_eq!(md_recvd_params.msgs_to_user.len(), 0); - assert_eq!(md_recvd_params.source_id, LOCAL_ID.into()); - assert_eq!(md_recvd_params.file_size, self.expected_file_size); - } - - fn file_segment_recvd_indication( - &mut self, - _segment_recvd_params: &crate::cfdp::user::FileSegmentRecvdParams, - ) { - } - - fn report_indication(&mut self, _id: &crate::cfdp::TransactionId) {} - - fn suspended_indication( - &mut self, - _id: &crate::cfdp::TransactionId, - _condition_code: ConditionCode, - ) { - panic!("unexpected suspended indication"); - } - - fn resumed_indication(&mut self, _id: &crate::cfdp::TransactionId, _progresss: u64) {} - - fn fault_indication( - &mut self, - _id: &crate::cfdp::TransactionId, - _condition_code: ConditionCode, - _progress: u64, - ) { - panic!("unexpected fault indication"); - } - - fn abandoned_indication( - &mut self, - _id: &crate::cfdp::TransactionId, - _condition_code: ConditionCode, - _progress: u64, - ) { - panic!("unexpected abandoned indication"); - } - - fn eof_recvd_indication(&mut self, id: &crate::cfdp::TransactionId) { - self.generic_id_check(id); - } - } - #[derive(Debug)] struct TestCheckTimer { counter: Cell, @@ -903,8 +956,8 @@ mod tests { handler: dest_handler, src_path, dest_path, - check_dest_file: true, - check_handler_idle_at_drop: true, + check_dest_file: false, + check_handler_idle_at_drop: false, expected_file_size: 0, pdu_header: create_pdu_header(UbfU16::new(0)), expected_full_data: Vec::new(), @@ -914,23 +967,31 @@ mod tests { handler } + fn indication_cfg_mut(&mut self) -> &mut IndicationConfig { + &mut self.handler.local_cfg.indication_cfg + } + + fn indication_cfg(&mut self) -> &IndicationConfig { + &self.handler.local_cfg.indication_cfg + } + fn set_check_timer_expired(&mut self) { self.check_timer_expired .store(true, core::sync::atomic::Ordering::Relaxed); } fn test_user_from_cached_paths(&self, expected_file_size: u64) -> TestCfdpUser { - TestCfdpUser { - next_expected_seq_num: 0, - expected_full_src_name: self.src_path.to_string_lossy().into(), - expected_full_dest_name: self.dest_path.to_string_lossy().into(), + TestCfdpUser::new( + 0, + self.src_path.to_string_lossy().into(), + self.dest_path.to_string_lossy().into(), expected_file_size, - } + ) } fn generic_transfer_init( &mut self, - user: &mut impl CfdpUser, + user: &mut TestCfdpUser, file_size: u64, ) -> Result<(), DestError> { self.expected_file_size = file_size; @@ -941,12 +1002,14 @@ mod tests { file_size, ); let packet_info = create_packet_info(&metadata_pdu, &mut self.buf); - self.handler.state_machine(user, Some(&packet_info)) + let result = self.handler.state_machine(user, Some(&packet_info)); + assert_eq!(user.metadata_recv_queue.len(), 1); + result } fn generic_file_data_insert( &mut self, - user: &mut impl CfdpUser, + user: &mut TestCfdpUser, offset: u64, file_data_chunk: &[u8], ) -> Result<(), DestError> { @@ -956,18 +1019,33 @@ mod tests { .write_to_bytes(&mut self.buf) .expect("writing file data PDU failed"); let packet_info = PacketInfo::new(&self.buf).expect("creating packet info failed"); - self.handler.state_machine(user, Some(&packet_info)) + let result = self.handler.state_machine(user, Some(&packet_info)); + if self.indication_cfg().file_segment_recv { + assert!(!user.file_seg_recvd_queue.is_empty()); + assert_eq!(user.file_seg_recvd_queue.back().unwrap().offset, offset); + assert_eq!( + user.file_seg_recvd_queue.back().unwrap().length, + file_data_chunk.len() + ); + } + result } fn generic_eof_no_error( &mut self, - user: &mut impl CfdpUser, + user: &mut TestCfdpUser, expected_full_data: Vec, ) -> Result<(), DestError> { self.expected_full_data = expected_full_data; let eof_pdu = create_no_error_eof(&self.expected_full_data, &self.pdu_header); let packet_info = create_packet_info(&eof_pdu, &mut self.buf); - self.handler.state_machine(user, Some(&packet_info)) + self.check_handler_idle_at_drop = true; + self.check_dest_file = true; + let result = self.handler.state_machine(user, Some(&packet_info)); + if self.indication_cfg().eof_recv { + assert_eq!(user.eof_recvd_call_count, 1); + } + result } fn state_check(&self, state: State, step: TransactionStep) { @@ -1159,6 +1237,7 @@ mod tests { test_obj .generic_transfer_init(&mut test_user, file_size) .expect("transfer init failed"); + test_obj.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); test_obj .generic_file_data_insert(&mut test_user, 0, &random_data[0..segment_len]) diff --git a/satrs-core/src/cfdp/user.rs b/satrs-core/src/cfdp/user.rs index 9047bbd..65ce909 100644 --- a/satrs-core/src/cfdp/user.rs +++ b/satrs-core/src/cfdp/user.rs @@ -1,10 +1,10 @@ use spacepackets::{ cfdp::{ pdu::{ - file_data::RecordContinuationState, + file_data::SegmentMetadata, finished::{DeliveryCode, FileStatus}, }, - tlv::msg_to_user::MsgToUserTlv, + tlv::{msg_to_user::MsgToUserTlv, WritableTlv}, ConditionCode, }, util::UnsignedByteField, @@ -30,13 +30,44 @@ pub struct MetadataReceivedParams<'src_file, 'dest_file, 'msgs_to_user> { pub msgs_to_user: &'msgs_to_user [MsgToUserTlv<'msgs_to_user>], } +#[cfg(feature = "alloc")] +#[derive(Debug)] +pub struct OwnedMetadataRecvdParams { + pub id: TransactionId, + pub source_id: UnsignedByteField, + pub file_size: u64, + pub src_file_name: alloc::string::String, + pub dest_file_name: alloc::string::String, + pub msgs_to_user: alloc::vec::Vec>, +} + +#[cfg(feature = "alloc")] +impl From> for OwnedMetadataRecvdParams { + fn from(value: MetadataReceivedParams) -> Self { + Self::from(&value) + } +} + +#[cfg(feature = "alloc")] +impl From<&MetadataReceivedParams<'_, '_, '_>> for OwnedMetadataRecvdParams { + fn from(value: &MetadataReceivedParams) -> Self { + Self { + id: value.id, + source_id: value.source_id, + file_size: value.file_size, + src_file_name: value.src_file_name.into(), + dest_file_name: value.dest_file_name.into(), + msgs_to_user: value.msgs_to_user.iter().map(|tlv| tlv.to_vec()).collect(), + } + } +} + #[derive(Debug)] pub struct FileSegmentRecvdParams<'seg_meta> { pub id: TransactionId, pub offset: u64, pub length: usize, - pub rec_cont_state: Option, - pub segment_metadata: Option<&'seg_meta [u8]>, + pub segment_metadata: Option<&'seg_meta SegmentMetadata<'seg_meta>>, } pub trait CfdpUser { -- 2.43.0 From 7f301a0771b5bee8385f0518054a0d642dc91a0a Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 15 Dec 2023 17:21:23 +0100 Subject: [PATCH 22/33] only allow CFDP for alloc for now --- satrs-core/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/satrs-core/src/lib.rs b/satrs-core/src/lib.rs index 22699e0..061f728 100644 --- a/satrs-core/src/lib.rs +++ b/satrs-core/src/lib.rs @@ -20,6 +20,8 @@ extern crate downcast_rs; #[cfg(any(feature = "std", test))] extern crate std; +#[cfg(feature = "alloc")] +#[cfg_attr(doc_cfg, doc(cfg(feature = "alloc")))] pub mod cfdp; pub mod encoding; pub mod error; -- 2.43.0 From 620ffbb131a8e1e3358376122abb5ee08568c821 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 18 Dec 2023 15:28:15 +0100 Subject: [PATCH 23/33] add test for reached check limit --- satrs-core/src/cfdp/dest.rs | 175 +++++++++++++++++++++++++++--------- satrs-core/src/cfdp/mod.rs | 2 + 2 files changed, 136 insertions(+), 41 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index b10afab..a6fa9e5 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -52,6 +52,12 @@ struct FileProperties { dest_path_buf: PathBuf, } +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +enum CompletionDisposition { + Completed = 0, + Cancelled = 1, +} + #[derive(Debug)] struct TransferState { transaction_id: Option, @@ -60,6 +66,7 @@ struct TransferState { condition_code: ConditionCode, delivery_code: DeliveryCode, file_status: FileStatus, + completion_disposition: CompletionDisposition, checksum: u32, current_check_count: u32, current_check_timer: Option>, @@ -74,6 +81,7 @@ impl Default for TransferState { condition_code: ConditionCode::NoError, delivery_code: DeliveryCode::Incomplete, file_status: FileStatus::Unreported, + completion_disposition: CompletionDisposition::Completed, checksum: 0, current_check_count: 0, current_check_timer: None, @@ -206,31 +214,13 @@ impl DestinationHandler { } } - fn insert_packet( - &mut self, - cfdp_user: &mut impl CfdpUser, - packet_info: &PacketInfo, - ) -> Result<(), DestError> { - if packet_info.target() != PacketTarget::DestEntity { - // Unwrap is okay here, a PacketInfo for a file data PDU should always have the - // destination as the target. - return Err(DestError::CantProcessPacketType( - packet_info.pdu_directive().unwrap(), - )); - } - match packet_info.pdu_type { - PduType::FileDirective => { - if packet_info.pdu_directive.is_none() { - return Err(DestError::DirectiveExpected); - } - self.handle_file_directive( - cfdp_user, - packet_info.pdu_directive.unwrap(), - packet_info.raw_packet, - ) - } - PduType::FileData => self.handle_file_data(cfdp_user, packet_info.raw_packet), + /// Returns [None] if the state machine is IDLE, and the transmission mode of the current + /// request otherwise. + pub fn current_transmission_mode(&self) -> Option { + if self.state == State::Idle { + return None; } + Some(self.tparams.transmission_mode()) } pub fn packet_to_send_ready(&self) -> bool { @@ -281,7 +271,34 @@ impl DestinationHandler { Ok(Some((directive, written_size))) } - pub fn handle_file_directive( + fn insert_packet( + &mut self, + cfdp_user: &mut impl CfdpUser, + packet_info: &PacketInfo, + ) -> Result<(), DestError> { + if packet_info.target() != PacketTarget::DestEntity { + // Unwrap is okay here, a PacketInfo for a file data PDU should always have the + // destination as the target. + return Err(DestError::CantProcessPacketType( + packet_info.pdu_directive().unwrap(), + )); + } + match packet_info.pdu_type { + PduType::FileDirective => { + if packet_info.pdu_directive.is_none() { + return Err(DestError::DirectiveExpected); + } + self.handle_file_directive( + cfdp_user, + packet_info.pdu_directive.unwrap(), + packet_info.raw_packet, + ) + } + PduType::FileData => self.handle_file_data(cfdp_user, packet_info.raw_packet), + } + } + + fn handle_file_directive( &mut self, cfdp_user: &mut impl CfdpUser, pdu_directive: FileDirectiveType, @@ -305,7 +322,7 @@ impl DestinationHandler { Ok(()) } - pub fn handle_metadata_pdu(&mut self, raw_packet: &[u8]) -> Result<(), DestError> { + fn handle_metadata_pdu(&mut self, raw_packet: &[u8]) -> Result<(), DestError> { if self.state != State::Idle { return Err(DestError::RecvdMetadataButIsBusy); } @@ -354,7 +371,7 @@ impl DestinationHandler { Ok(()) } - pub fn handle_file_data( + fn handle_file_data( &mut self, user: &mut impl CfdpUser, raw_packet: &[u8], @@ -385,7 +402,7 @@ impl DestinationHandler { Ok(()) } - pub fn handle_eof_pdu( + fn handle_eof_pdu( &mut self, cfdp_user: &mut impl CfdpUser, raw_packet: &[u8], @@ -616,6 +633,33 @@ impl DestinationHandler { } fn transfer_completion(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { + self.notice_of_completion(cfdp_user)?; + if self.tparams.transmission_mode() == TransmissionMode::Acknowledged + || self.tparams.metadata_params().closure_requested + { + self.prepare_finished_pdu()?; + self.step = TransactionStep::SendingFinishedPdu; + } else { + self.reset(); + } + Ok(()) + } + + fn notice_of_completion(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { + if self.tstate().completion_disposition == CompletionDisposition::Completed { + // TODO: Execute any filestore requests + } else if self + .tparams + .remote_cfg + .as_ref() + .unwrap() + .disposition_on_cancellation + && self.tstate().delivery_code == DeliveryCode::Incomplete + { + self.vfs + .remove_file(self.tparams.file_properties.dest_path_buf.to_str().unwrap())?; + self.tstate_mut().file_status = FileStatus::DiscardDeliberately; + } let tstate = self.tstate(); let transaction_finished_params = TransactionFinishedParams { id: tstate.transaction_id.unwrap(), @@ -624,15 +668,6 @@ impl DestinationHandler { file_status: tstate.file_status, }; cfdp_user.transaction_finished_indication(&transaction_finished_params); - // This function should never be called with metadata parameters not set - if self.tparams.metadata_params().closure_requested { - self.prepare_finished_pdu()?; - self.step = TransactionStep::SendingFinishedPdu; - } else { - self.reset(); - self.state = State::Idle; - self.step = TransactionStep::Idle; - } Ok(()) } @@ -646,7 +681,7 @@ impl DestinationHandler { .get_fault_handler(condition_code); match fh_code { FaultHandlerCode::NoticeOfCancellation => { - self.notice_of_cancellation(); + self.notice_of_cancellation(condition_code); } FaultHandlerCode::NoticeOfSuspension => self.notice_of_suspension(), FaultHandlerCode::IgnoreError => (), @@ -657,7 +692,12 @@ impl DestinationHandler { .report_fault(transaction_id, condition_code, progress) } - fn notice_of_cancellation(&mut self) {} + fn notice_of_cancellation(&mut self, condition_code: ConditionCode) { + self.step = TransactionStep::TransferCompletion; + self.tstate_mut().condition_code = condition_code; + self.tstate_mut().completion_disposition = CompletionDisposition::Cancelled; + } + fn notice_of_suspension(&mut self) {} fn abandon_transaction(&mut self) { self.reset(); @@ -680,6 +720,10 @@ impl DestinationHandler { fn tstate(&self) -> &TransferState { &self.tparams.tstate } + + fn tstate_mut(&mut self) -> &mut TransferState { + &mut self.tparams.tstate + } } #[cfg(test)] @@ -967,6 +1011,7 @@ mod tests { handler } + #[allow(dead_code)] fn indication_cfg_mut(&mut self) -> &mut IndicationConfig { &mut self.handler.local_cfg.indication_cfg } @@ -1004,6 +1049,10 @@ mod tests { let packet_info = create_packet_info(&metadata_pdu, &mut self.buf); let result = self.handler.state_machine(user, Some(&packet_info)); assert_eq!(user.metadata_recv_queue.len(), 1); + assert_eq!( + self.handler.current_transmission_mode().unwrap(), + TransmissionMode::Unacknowledged + ); result } @@ -1159,7 +1208,8 @@ mod tests { #[test] fn test_basic() { - default_dest_handler(Arc::default()); + let dest_handler = default_dest_handler(Arc::default()); + assert!(dest_handler.current_transmission_mode().is_none()); } #[test] @@ -1225,7 +1275,7 @@ mod tests { } #[test] - fn test_check_limit_handling() { + fn test_check_limit_handling_transfer_success() { let mut rng = rand::thread_rng(); let mut random_data = [0u8; 512]; rng.fill(&mut random_data); @@ -1262,4 +1312,47 @@ mod tests { .state_machine(&mut test_user, None) .expect("fsm failure"); } + + #[test] + fn test_check_limit_handling_limit_reached() { + let mut rng = rand::thread_rng(); + let mut random_data = [0u8; 512]; + rng.fill(&mut random_data); + let file_size = random_data.len() as u64; + let segment_len = 256; + + let mut test_obj = TestClass::new(); + let mut test_user = test_obj.test_user_from_cached_paths(file_size); + test_obj + .generic_transfer_init(&mut test_user, file_size) + .expect("transfer init failed"); + + test_obj.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + test_obj + .generic_file_data_insert(&mut test_user, 0, &random_data[0..segment_len]) + .expect("file data insertion 0 failed"); + test_obj + .generic_eof_no_error(&mut test_user, random_data.to_vec()) + .expect("EOF no error insertion failed"); + test_obj.state_check( + State::Busy, + TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling, + ); + test_obj.set_check_timer_expired(); + test_obj + .handler + .state_machine(&mut test_user, None) + .expect("fsm error"); + test_obj.state_check( + State::Busy, + TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling, + ); + test_obj.set_check_timer_expired(); + test_obj + .handler + .state_machine(&mut test_user, None) + .expect("fsm error"); + test_obj.state_check(State::Idle, TransactionStep::Idle); + test_obj.check_dest_file = false; + } } diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index 110b588..5916235 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -107,6 +107,7 @@ pub struct RemoteEntityConfig { pub default_transmission_mode: TransmissionMode, pub default_crc_type: ChecksumType, pub check_limit: u32, + pub disposition_on_cancellation: bool, } impl RemoteEntityConfig { @@ -128,6 +129,7 @@ impl RemoteEntityConfig { default_transmission_mode, default_crc_type, check_limit: 2, + disposition_on_cancellation: false, } } } -- 2.43.0 From 42cb3f7e6b23b869fc842f783dc93d3dd002d9ef Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 18 Dec 2023 16:16:28 +0100 Subject: [PATCH 24/33] verify fault handling correctness --- satrs-core/src/cfdp/dest.rs | 149 ++++++++++++++++++++++++++++-------- 1 file changed, 119 insertions(+), 30 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index a6fa9e5..28ba401 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -216,13 +216,17 @@ impl DestinationHandler { /// Returns [None] if the state machine is IDLE, and the transmission mode of the current /// request otherwise. - pub fn current_transmission_mode(&self) -> Option { + pub fn transmission_mode(&self) -> Option { if self.state == State::Idle { return None; } Some(self.tparams.transmission_mode()) } + pub fn transaction_id(&self) -> Option { + self.tstate().transaction_id + } + pub fn packet_to_send_ready(&self) -> bool { self.packets_to_send_ctx.packet_available } @@ -399,6 +403,7 @@ impl DestinationHandler { self.declare_fault(ConditionCode::FilestoreRejection); return Err(e.into()); } + self.tstate_mut().progress += fd_pdu.file_data().len() as u64; Ok(()) } @@ -698,7 +703,9 @@ impl DestinationHandler { self.tstate_mut().completion_disposition = CompletionDisposition::Cancelled; } - fn notice_of_suspension(&mut self) {} + fn notice_of_suspension(&mut self) { + // TODO: Implement suspension handling. + } fn abandon_transaction(&mut self) { self.reset(); } @@ -729,9 +736,9 @@ impl DestinationHandler { #[cfg(test)] mod tests { use core::{cell::Cell, sync::atomic::AtomicBool}; - use std::fs; #[allow(unused_imports)] use std::println; + use std::{fs, sync::Mutex}; use alloc::{collections::VecDeque, string::String, sync::Arc, vec::Vec}; use rand::Rng; @@ -886,12 +893,12 @@ mod tests { } } - #[derive(Default)] + #[derive(Default, Clone)] struct TestFaultHandler { - notice_of_suspension_queue: VecDeque<(TransactionId, ConditionCode, u64)>, - notice_of_cancellation_queue: VecDeque<(TransactionId, ConditionCode, u64)>, - abandoned_queue: VecDeque<(TransactionId, ConditionCode, u64)>, - ignored_queue: VecDeque<(TransactionId, ConditionCode, u64)>, + notice_of_suspension_queue: Arc>>, + notice_of_cancellation_queue: Arc>>, + abandoned_queue: Arc>>, + ignored_queue: Arc>>, } impl UserFaultHandler for TestFaultHandler { @@ -901,8 +908,11 @@ mod tests { cond: ConditionCode, progress: u64, ) { - self.notice_of_suspension_queue - .push_back((transaction_id, cond, progress)) + self.notice_of_suspension_queue.lock().unwrap().push_back(( + transaction_id, + cond, + progress, + )) } fn notice_of_cancellation_cb( @@ -912,6 +922,8 @@ mod tests { progress: u64, ) { self.notice_of_cancellation_queue + .lock() + .unwrap() .push_back((transaction_id, cond, progress)) } @@ -922,15 +934,40 @@ mod tests { progress: u64, ) { self.abandoned_queue + .lock() + .unwrap() .push_back((transaction_id, cond, progress)) } fn ignore_cb(&mut self, transaction_id: TransactionId, cond: ConditionCode, progress: u64) { self.ignored_queue + .lock() + .unwrap() .push_back((transaction_id, cond, progress)) } } + impl TestFaultHandler { + fn suspension_queue_empty(&self) -> bool { + self.notice_of_suspension_queue.lock().unwrap().is_empty() + } + fn cancellation_queue_empty(&self) -> bool { + self.notice_of_cancellation_queue.lock().unwrap().is_empty() + } + fn ignored_queue_empty(&self) -> bool { + self.ignored_queue.lock().unwrap().is_empty() + } + fn abandoned_queue_empty(&self) -> bool { + self.abandoned_queue.lock().unwrap().is_empty() + } + fn all_queues_empty(&self) -> bool { + self.suspension_queue_empty() + && self.cancellation_queue_empty() + && self.ignored_queue_empty() + && self.abandoned_queue_empty() + } + } + #[derive(Debug)] struct TestCheckTimer { counter: Cell, @@ -976,7 +1013,7 @@ mod tests { } } - struct TestClass { + struct DestHandlerTester { check_timer_expired: Arc, handler: DestinationHandler, src_path: PathBuf, @@ -989,10 +1026,10 @@ mod tests { buf: [u8; 512], } - impl TestClass { - fn new() -> Self { + impl DestHandlerTester { + fn new(fault_handler: TestFaultHandler) -> Self { let check_timer_expired = Arc::new(AtomicBool::new(false)); - let dest_handler = default_dest_handler(check_timer_expired.clone()); + let dest_handler = default_dest_handler(fault_handler, check_timer_expired.clone()); let (src_path, dest_path) = init_full_filenames(); assert!(!Path::exists(&dest_path)); let handler = Self { @@ -1011,6 +1048,10 @@ mod tests { handler } + fn dest_path(&self) -> &PathBuf { + &self.dest_path + } + #[allow(dead_code)] fn indication_cfg_mut(&mut self) -> &mut IndicationConfig { &mut self.handler.local_cfg.indication_cfg @@ -1038,7 +1079,7 @@ mod tests { &mut self, user: &mut TestCfdpUser, file_size: u64, - ) -> Result<(), DestError> { + ) -> Result { self.expected_file_size = file_size; let metadata_pdu = create_metadata_pdu( &self.pdu_header, @@ -1047,13 +1088,13 @@ mod tests { file_size, ); let packet_info = create_packet_info(&metadata_pdu, &mut self.buf); - let result = self.handler.state_machine(user, Some(&packet_info)); + self.handler.state_machine(user, Some(&packet_info))?; assert_eq!(user.metadata_recv_queue.len(), 1); assert_eq!( - self.handler.current_transmission_mode().unwrap(), + self.handler.transmission_mode().unwrap(), TransmissionMode::Unacknowledged ); - result + Ok(self.handler.transaction_id().unwrap()) } fn generic_file_data_insert( @@ -1103,7 +1144,7 @@ mod tests { } } - impl Drop for TestClass { + impl Drop for DestHandlerTester { fn drop(&mut self) { if self.check_handler_idle_at_drop { self.state_check(State::Idle, TransactionStep::Idle); @@ -1143,8 +1184,10 @@ mod tests { table } - fn default_dest_handler(check_timer_expired: Arc) -> DestinationHandler { - let test_fault_handler = TestFaultHandler::default(); + fn default_dest_handler( + test_fault_handler: TestFaultHandler, + check_timer_expired: Arc, + ) -> DestinationHandler { let local_entity_cfg = LocalEntityConfig { id: REMOTE_ID.into(), indication_cfg: IndicationConfig::default(), @@ -1208,13 +1251,16 @@ mod tests { #[test] fn test_basic() { - let dest_handler = default_dest_handler(Arc::default()); - assert!(dest_handler.current_transmission_mode().is_none()); + let fault_handler = TestFaultHandler::default(); + let dest_handler = default_dest_handler(fault_handler.clone(), Arc::default()); + assert!(dest_handler.transmission_mode().is_none()); + assert!(fault_handler.all_queues_empty()); } #[test] fn test_empty_file_transfer_not_acked() { - let mut test_obj = TestClass::new(); + let fault_handler = TestFaultHandler::default(); + let mut test_obj = DestHandlerTester::new(fault_handler.clone()); let mut test_user = test_obj.test_user_from_cached_paths(0); test_obj .generic_transfer_init(&mut test_user, 0) @@ -1223,6 +1269,7 @@ mod tests { test_obj .generic_eof_no_error(&mut test_user, Vec::new()) .expect("EOF no error insertion failed"); + assert!(fault_handler.all_queues_empty()); } #[test] @@ -1230,8 +1277,9 @@ mod tests { let file_data_str = "Hello World!"; let file_data = file_data_str.as_bytes(); let file_size = file_data.len() as u64; + let fault_handler = TestFaultHandler::default(); - let mut test_obj = TestClass::new(); + let mut test_obj = DestHandlerTester::new(fault_handler.clone()); let mut test_user = test_obj.test_user_from_cached_paths(file_size); test_obj .generic_transfer_init(&mut test_user, file_size) @@ -1243,6 +1291,7 @@ mod tests { test_obj .generic_eof_no_error(&mut test_user, file_data.to_vec()) .expect("EOF no error insertion failed"); + assert!(fault_handler.all_queues_empty()); } #[test] @@ -1252,8 +1301,9 @@ mod tests { rng.fill(&mut random_data); let file_size = random_data.len() as u64; let segment_len = 256; + let fault_handler = TestFaultHandler::default(); - let mut test_obj = TestClass::new(); + let mut test_obj = DestHandlerTester::new(fault_handler.clone()); let mut test_user = test_obj.test_user_from_cached_paths(file_size); test_obj .generic_transfer_init(&mut test_user, file_size) @@ -1272,6 +1322,7 @@ mod tests { test_obj .generic_eof_no_error(&mut test_user, random_data.to_vec()) .expect("EOF no error insertion failed"); + assert!(fault_handler.all_queues_empty()); } #[test] @@ -1281,10 +1332,11 @@ mod tests { rng.fill(&mut random_data); let file_size = random_data.len() as u64; let segment_len = 256; + let fault_handler = TestFaultHandler::default(); - let mut test_obj = TestClass::new(); + let mut test_obj = DestHandlerTester::new(fault_handler.clone()); let mut test_user = test_obj.test_user_from_cached_paths(file_size); - test_obj + let transaction_id = test_obj .generic_transfer_init(&mut test_user, file_size) .expect("transfer init failed"); @@ -1311,6 +1363,13 @@ mod tests { .handler .state_machine(&mut test_user, None) .expect("fsm failure"); + + let ignored_queue = fault_handler.ignored_queue.lock().unwrap(); + assert_eq!(ignored_queue.len(), 1); + let cancelled = *ignored_queue.front().unwrap(); + assert_eq!(cancelled.0, transaction_id); + assert_eq!(cancelled.1, ConditionCode::FileChecksumFailure); + assert_eq!(cancelled.2, segment_len as u64); } #[test] @@ -1321,9 +1380,10 @@ mod tests { let file_size = random_data.len() as u64; let segment_len = 256; - let mut test_obj = TestClass::new(); + let fault_handler = TestFaultHandler::default(); + let mut test_obj = DestHandlerTester::new(fault_handler.clone()); let mut test_user = test_obj.test_user_from_cached_paths(file_size); - test_obj + let transaction_id = test_obj .generic_transfer_init(&mut test_user, file_size) .expect("transfer init failed"); @@ -1353,6 +1413,35 @@ mod tests { .state_machine(&mut test_user, None) .expect("fsm error"); test_obj.state_check(State::Idle, TransactionStep::Idle); + + assert!(fault_handler + .notice_of_suspension_queue + .lock() + .unwrap() + .is_empty()); + + let ignored_queue = fault_handler.ignored_queue.lock().unwrap(); + assert_eq!(ignored_queue.len(), 1); + let cancelled = *ignored_queue.front().unwrap(); + assert_eq!(cancelled.0, transaction_id); + assert_eq!(cancelled.1, ConditionCode::FileChecksumFailure); + assert_eq!(cancelled.2, segment_len as u64); + + let cancelled_queue = fault_handler.notice_of_cancellation_queue.lock().unwrap(); + assert_eq!(cancelled_queue.len(), 1); + let cancelled = *cancelled_queue.front().unwrap(); + assert_eq!(cancelled.0, transaction_id); + assert_eq!(cancelled.1, ConditionCode::CheckLimitReached); + assert_eq!(cancelled.2, segment_len as u64); + + drop(cancelled_queue); + + // Check that the broken file exists. test_obj.check_dest_file = false; + assert!(Path::exists(test_obj.dest_path())); + let read_content = fs::read(test_obj.dest_path()).expect("reading back string failed"); + assert_eq!(read_content.len(), segment_len); + assert_eq!(read_content, &random_data[0..segment_len]); + assert!(fs::remove_file(test_obj.dest_path().as_path()).is_ok()); } } -- 2.43.0 From 71ce43eca6b49ab6c2af0eac9f02dc9d9cb6caf7 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 18 Dec 2023 16:26:29 +0100 Subject: [PATCH 25/33] added some docs --- satrs-core/src/cfdp/dest.rs | 40 ++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 28ba401..8e5fd5f 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -26,17 +26,6 @@ use spacepackets::{ }; use thiserror::Error; -pub struct DestinationHandler { - local_cfg: LocalEntityConfig, - step: TransactionStep, - state: State, - tparams: TransactionParams, - packets_to_send_ctx: PacketsToSendContext, - vfs: Box, - remote_cfg_table: Box, - check_timer_creator: Box, -} - #[derive(Debug, Default)] struct PacketsToSendContext { packet_available: bool, @@ -180,6 +169,33 @@ pub enum DestError { NoRemoteCfgFound(UnsignedByteField), } +/// This is the primary CFDP destination handler. It models the CFDP destination entity, which is +/// primarily responsible for receiving files sent from another CFDP entity. It performs the +/// reception side of File Copy Operations. + +/// The following core functions are the primary interface for interacting with the destination +/// handler: + +/// 1. [DestinationHandler::state_machine]: Can be used to insert packets into the destination +/// handler. Please note that the destination handler can also only process Metadata, EOF and +/// Prompt PDUs in addition to ACK PDUs where the acknowledged PDU is the Finished PDU. +/// 2. [DestinationHandler::get_next_packet]: Retrieve next packet to be sent back to the remote +/// CFDP source entity ID. + +/// A new file transfer (Metadata PDU reception) is only be accepted if the handler is in the +/// IDLE state. Furthermore, packet insertion is not allowed until all packets to send were +/// retrieved after a state machine call. +pub struct DestinationHandler { + local_cfg: LocalEntityConfig, + step: TransactionStep, + state: State, + tparams: TransactionParams, + packets_to_send_ctx: PacketsToSendContext, + vfs: Box, + remote_cfg_table: Box, + check_timer_creator: Box, +} + impl DestinationHandler { pub fn new( local_cfg: LocalEntityConfig, @@ -231,7 +247,7 @@ impl DestinationHandler { self.packets_to_send_ctx.packet_available } - pub fn get_next_packet_to_send( + pub fn get_next_packet( &self, buf: &mut [u8], ) -> Result, DestError> { -- 2.43.0 From 303a9ab58153df7c7b8fc4de867e4aa0a534946f Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 18 Dec 2023 17:08:23 +0100 Subject: [PATCH 26/33] more docs --- satrs-core/src/cfdp/dest.rs | 24 ++++++-- satrs-core/src/cfdp/mod.rs | 110 ++++++++++++++++++++++++++++++++---- 2 files changed, 120 insertions(+), 14 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 8e5fd5f..bb13ec5 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -155,6 +155,8 @@ pub enum DestError { EmptySrcFileField, #[error("empty dest file field")] EmptyDestFileField, + #[error("packets to be sent are still left")] + PacketToSendLeft, #[error("pdu error {0}")] Pdu(#[from] PduError), #[error("io error {0}")] @@ -176,10 +178,12 @@ pub enum DestError { /// The following core functions are the primary interface for interacting with the destination /// handler: -/// 1. [DestinationHandler::state_machine]: Can be used to insert packets into the destination -/// handler. Please note that the destination handler can also only process Metadata, EOF and -/// Prompt PDUs in addition to ACK PDUs where the acknowledged PDU is the Finished PDU. -/// 2. [DestinationHandler::get_next_packet]: Retrieve next packet to be sent back to the remote +/// 1. [DestinationHandler::state_machine] - Can be used to insert packets into the destination +/// handler and/or advance the state machine. Advancing the state machine might generate new +/// packets to be sent to the remote entity. Please note that the destination handler can also +/// only process Metadata, EOF and Prompt PDUs in addition to ACK PDUs where the acknowledged +/// PDU is the Finished PDU. +/// 2. [DestinationHandler::get_next_packet] - Retrieve next packet to be sent back to the remote /// CFDP source entity ID. /// A new file transfer (Metadata PDU reception) is only be accepted if the handler is in the @@ -215,11 +219,23 @@ impl DestinationHandler { } } + /// This is the core function to drive the destination handler. It is also used to insert + /// packets into the destination handler. + /// + /// Please note that this function will fail if there are still packets which need to be + /// retrieved with [Self::get_next_packet]. After each state machine call, the user has to + /// retrieve all packets before calling the state machine again. The state machine should + /// either be called if a packet with the appropriate destination ID is received, or + /// periodically in IDLE periods to perform all CFDP related tasks, for example checking for + /// timeouts or missed file segments. pub fn state_machine( &mut self, cfdp_user: &mut impl CfdpUser, packet_to_insert: Option<&PacketInfo>, ) -> Result<(), DestError> { + if self.packet_to_send_ready() { + return Err(DestError::PacketToSendLeft); + } if let Some(packet) = packet_to_insert { self.insert_packet(cfdp_user, packet)?; } diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index 5916235..0df270e 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -1,3 +1,5 @@ +//! This module contains the implementation of the CFDP high level classes as specified in the +//! CCSDS 727.0-B-5. use core::{cell::RefCell, fmt::Debug, hash::Hash}; use crc::{Crc, CRC_32_CKSUM}; @@ -47,7 +49,7 @@ pub trait CheckTimer: Debug { /// A generic trait which allows CFDP entities to create check timers which are required to /// implement special procedures in unacknowledged transmission mode, as specified in 4.6.3.2 -/// and 4.6.3.3. The [CheckTimerProvider] provides more information about the purpose of the +/// and 4.6.3.3. The [CheckTimer] documentation provides more information about the purpose of the /// check timer. /// /// This trait also allows the creation of different check timers depending on @@ -63,7 +65,7 @@ pub trait CheckTimerCreator { ) -> Box; } -/// Simple implementation of the [CheckTimerProvider] trait assuming a standard runtime. +/// Simple implementation of the [CheckTimerCreator] trait assuming a standard runtime. /// It also assumes that a second accuracy of the check timer period is sufficient. #[cfg(feature = "std")] #[derive(Debug)] @@ -97,17 +99,76 @@ impl CheckTimer for StdCheckTimer { } } +/// This structure models the remote entity configuration information as specified in chapter 8.3 +/// of the CFDP standard. + +/// Some of the fields which were not considered necessary for the Rust implementation +/// were omitted. Some other fields which are not contained inside the standard but are considered +/// necessary for the Rust implementation are included. +/// +/// ## Notes on Positive Acknowledgment Procedures +/// +/// The `positive_ack_timer_interval_seconds` and `positive_ack_timer_expiration_limit` will +/// be used for positive acknowledgement procedures as specified in CFDP chapter 4.7. The sending +/// entity will start the timer for any PDUs where an acknowledgment is required (e.g. EOF PDU). +/// Once the expected ACK response has not been received for that interval, as counter will be +/// incremented and the timer will be reset. Once the counter exceeds the +/// `positive_ack_timer_expiration_limit`, a Positive ACK Limit Reached fault will be declared. +/// +/// ## Notes on Deferred Lost Segment Procedures +/// +/// This procedure will be active if an EOF (No Error) PDU is received in acknowledged mode. After +/// issuing the NAK sequence which has the whole file scope, a timer will be started. The timer is +/// reset when missing segments or missing metadata is received. The timer will be deactivated if +/// all missing data is received. If the timer expires, a new NAK sequence will be issued and a +/// counter will be incremented, which can lead to a NAK Limit Reached fault being declared. +/// +/// ## Fields +/// +/// * `entity_id` - The ID of the remote entity. +/// * `max_packet_len` - This determines of all PDUs generated for that remote entity in addition +/// to the `max_file_segment_len` attribute which also determines the size of file data PDUs. +/// * `max_file_segment_len` The maximum file segment length which determines the maximum size +/// of file data PDUs in addition to the `max_packet_len` attribute. If this field is set +/// to None, the maximum file segment length will be derived from the maximum packet length. +/// If this has some value which is smaller than the segment value derived from +/// `max_packet_len`, this value will be picked. +/// * `closure_requested_by_default` - If the closure requested field is not supplied as part of +/// the Put Request, it will be determined from this field in the remote configuration. +/// * `crc_on_transmission_by_default` - If the CRC option is not supplied as part of the Put +/// Request, it will be determined from this field in the remote configuration. +/// * `default_transmission_mode` - If the transmission mode is not supplied as part of the +/// Put Request, it will be determined from this field in the remote configuration. +/// * `disposition_on_cancellation` - Determines whether an incomplete received file is discard on +/// transaction cancellation. Defaults to False. +/// * `default_crc_type` - Default checksum type used to calculate for all file transmissions to +/// this remote entity. +/// * `check_limit` - This timer determines the expiry period for incrementing a check counter +/// after an EOF PDU is received for an incomplete file transfer. This allows out-of-order +/// reception of file data PDUs and EOF PDUs. Also see 4.6.3.3 of the CFDP standard. Defaults to +/// 2, so the check limit timer may expire twice. +/// * `positive_ack_timer_interval_seconds`- See the notes on the Positive Acknowledgment +/// Procedures inside the class documentation. Expected as floating point seconds. Defaults to +/// 10 seconds. +/// * `positive_ack_timer_expiration_limit` - See the notes on the Positive Acknowledgment +/// Procedures inside the class documentation. Defaults to 2, so the timer may expire twice. +/// * `immediate_nak_mode` - Specifies whether a NAK sequence should be issued immediately when a +/// file data gap or lost metadata is detected in the acknowledged mode. Defaults to True. +/// * `nak_timer_interval_seconds` - See the notes on the Deferred Lost Segment Procedure inside +/// the class documentation. Expected as floating point seconds. Defaults to 10 seconds. +/// * `nak_timer_expiration_limit` - See the notes on the Deferred Lost Segment Procedure inside +/// the class documentation. Defaults to 2, so the timer may expire two times. #[derive(Debug, Copy, Clone)] pub struct RemoteEntityConfig { pub entity_id: UnsignedByteField, - pub max_file_segment_len: usize, pub max_packet_len: usize, + pub max_file_segment_len: usize, pub closure_requested_by_default: bool, pub crc_on_transmission_by_default: bool, pub default_transmission_mode: TransmissionMode, + pub disposition_on_cancellation: bool, pub default_crc_type: ChecksumType, pub check_limit: u32, - pub disposition_on_cancellation: bool, } impl RemoteEntityConfig { @@ -138,11 +199,11 @@ pub trait RemoteEntityConfigProvider { /// Retrieve the remote entity configuration for the given remote ID. fn get_remote_config(&self, remote_id: u64) -> Option<&RemoteEntityConfig>; fn get_remote_config_mut(&mut self, remote_id: u64) -> Option<&mut RemoteEntityConfig>; - /// Add a new remote configuration. Return [True] if the configuration was - /// inserted successfully, and [False] if a configuration already exists. + /// Add a new remote configuration. Return [true] if the configuration was + /// inserted successfully, and [false] if a configuration already exists. fn add_config(&mut self, cfg: &RemoteEntityConfig) -> bool; - /// Remote a configuration. Returns [True] if the configuration was removed successfully, - /// and [False] if no configuration exists for the given remote ID. + /// Remote a configuration. Returns [true] if the configuration was removed successfully, + /// and [false] if no configuration exists for the given remote ID. fn remove_config(&mut self, remote_id: u64) -> bool; } @@ -171,8 +232,15 @@ impl RemoteEntityConfigProvider for StdRemoteEntityConfigProvider { } /// This trait introduces some callbacks which will be called when a particular CFDP fault -/// handler is called. This allows to implement some CFDP features like fault handler logging, -/// which would not be possible generically otherwise. +/// handler is called. +/// +/// It is passed into the CFDP handlers as part of the [DefaultFaultHandler] and the local entity +/// configuration and provides a way to specify custom user error handlers. This allows to +/// implement some CFDP features like fault handler logging, which would not be possible +/// generically otherwise. +/// +/// For each error reported by the [DefaultFaultHandler], the appropriate fault handler callback +/// will be called depending on the [FaultHandlerCode]. pub trait UserFaultHandler { fn notice_of_suspension_cb( &mut self, @@ -193,6 +261,26 @@ pub trait UserFaultHandler { fn ignore_cb(&mut self, transaction_id: TransactionId, cond: ConditionCode, progress: u64); } +/// This structure is used to implement the fault handling as specified in chapter 4.8 of the CFDP +/// standard. +/// +/// It does so by mapping each applicable [spacepackets::cfdp::ConditionCode] to a fault handler +/// which is denoted by the four [spacepackets::cfdp::FaultHandlerCode]s. This code is used +/// to select the error handling inside the CFDP handler itself in addition to dispatching to a +/// user-provided callback function provided by the [UserFaultHandler]. +/// +/// Some note on the provided default settings: +/// +/// - Checksum failures will be ignored by default. This is because for unacknowledged transfers, +/// cancelling the transfer immediately would interfere with the check limit mechanism specified +/// in chapter 4.6.3.3. +/// - Unsupported checksum types will also be ignored by default. Even if the checksum type is +/// not supported the file transfer might still have worked properly. +/// +/// For all other faults, the default fault handling operation will be to cancel the transaction. +/// These defaults can be overriden by using the [Self::set_fault_handler] method. +/// Please note that in any case, fault handler overrides can be specified by the sending CFDP +/// entity. pub struct DefaultFaultHandler { handler_array: [FaultHandlerCode; 10], // Could also change the user fault handler trait to have non mutable methods, but that limits @@ -308,6 +396,8 @@ pub struct LocalEntityConfig { pub default_fault_handler: DefaultFaultHandler, } +/// The CFDP transaction ID of a CFDP transaction consists of the source entity ID and the sequence +/// number of that transfer which is also determined by the CFDP source entity. #[derive(Debug, Eq, Copy, Clone)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct TransactionId { -- 2.43.0 From 4cf96ce0d50d4907e5583f6167fe9288a98b164f Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 22 Dec 2023 14:08:25 +0100 Subject: [PATCH 27/33] extend remote cfg fields --- satrs-core/src/cfdp/mod.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index 0df270e..cf18fe7 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -166,9 +166,14 @@ pub struct RemoteEntityConfig { pub closure_requested_by_default: bool, pub crc_on_transmission_by_default: bool, pub default_transmission_mode: TransmissionMode, - pub disposition_on_cancellation: bool, pub default_crc_type: ChecksumType, + pub positive_ack_timer_interval_seconds: f32, + pub positive_ack_timer_expiration_limit: u32, pub check_limit: u32, + pub disposition_on_cancellation: bool, + pub immediate_nak_mode: bool, + pub nak_timer_interval_seconds: f32, + pub nak_timer_expiration_limit: u32, } impl RemoteEntityConfig { @@ -190,7 +195,12 @@ impl RemoteEntityConfig { default_transmission_mode, default_crc_type, check_limit: 2, + positive_ack_timer_interval_seconds: 10.0, + positive_ack_timer_expiration_limit: 2, disposition_on_cancellation: false, + immediate_nak_mode: true, + nak_timer_interval_seconds: 10.0, + nak_timer_expiration_limit: 2, } } } -- 2.43.0 From 7776847364bea2a745069752cf2e00e8d01c05da Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 22 Dec 2023 19:24:48 +0100 Subject: [PATCH 28/33] rework timer and packet send handling --- satrs-core/src/cfdp/dest.rs | 202 ++++++++++++++++++++---------------- satrs-core/src/cfdp/mod.rs | 45 ++++++-- 2 files changed, 144 insertions(+), 103 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index bb13ec5..627a3cd 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -6,7 +6,8 @@ use super::{ filestore::{FilestoreError, VirtualFilestore}, user::{CfdpUser, FileSegmentRecvdParams, MetadataReceivedParams}, CheckTimer, CheckTimerCreator, EntityType, LocalEntityConfig, PacketInfo, PacketTarget, - RemoteEntityConfig, RemoteEntityConfigProvider, State, TransactionId, TransactionStep, + RemoteEntityConfig, RemoteEntityConfigProvider, State, TimerContext, TransactionId, + TransactionStep, }; use alloc::boxed::Box; use smallvec::SmallVec; @@ -26,12 +27,6 @@ use spacepackets::{ }; use thiserror::Error; -#[derive(Debug, Default)] -struct PacketsToSendContext { - packet_available: bool, - directive: Option, -} - #[derive(Debug)] struct FileProperties { src_file_name: [u8; u8::MAX as usize], @@ -171,6 +166,15 @@ pub enum DestError { NoRemoteCfgFound(UnsignedByteField), } +pub trait CfdpPacketSender: Send { + fn send_pdu( + &mut self, + pdu_type: PduType, + file_directive_type: Option, + raw_pdu: &[u8], + ) -> Result<(), PduError>; +} + /// This is the primary CFDP destination handler. It models the CFDP destination entity, which is /// primarily responsible for receiving files sent from another CFDP entity. It performs the /// reception side of File Copy Operations. @@ -194,7 +198,8 @@ pub struct DestinationHandler { step: TransactionStep, state: State, tparams: TransactionParams, - packets_to_send_ctx: PacketsToSendContext, + packet_buf: alloc::vec::Vec, + packet_sender: Box, vfs: Box, remote_cfg_table: Box, check_timer_creator: Box, @@ -203,6 +208,8 @@ pub struct DestinationHandler { impl DestinationHandler { pub fn new( local_cfg: LocalEntityConfig, + max_packet_len: usize, + packet_sender: Box, vfs: Box, remote_cfg_table: Box, check_timer_creator: Box, @@ -212,7 +219,8 @@ impl DestinationHandler { step: TransactionStep::Idle, state: State::Idle, tparams: Default::default(), - packets_to_send_ctx: Default::default(), + packet_buf: alloc::vec![0; max_packet_len], + packet_sender, vfs, remote_cfg_table, check_timer_creator, @@ -232,10 +240,7 @@ impl DestinationHandler { &mut self, cfdp_user: &mut impl CfdpUser, packet_to_insert: Option<&PacketInfo>, - ) -> Result<(), DestError> { - if self.packet_to_send_ready() { - return Err(DestError::PacketToSendLeft); - } + ) -> Result { if let Some(packet) = packet_to_insert { self.insert_packet(cfdp_user, packet)?; } @@ -259,54 +264,6 @@ impl DestinationHandler { self.tstate().transaction_id } - pub fn packet_to_send_ready(&self) -> bool { - self.packets_to_send_ctx.packet_available - } - - pub fn get_next_packet( - &self, - buf: &mut [u8], - ) -> Result, DestError> { - if !self.packet_to_send_ready() { - return Ok(None); - } - let directive = self.packets_to_send_ctx.directive.unwrap(); - let tstate = self.tstate(); - let written_size = match directive { - FileDirectiveType::FinishedPdu => { - let pdu_header = PduHeader::new_no_file_data(self.tparams.pdu_conf, 0); - let finished_pdu = if tstate.condition_code == ConditionCode::NoError - || tstate.condition_code == ConditionCode::UnsupportedChecksumType - { - FinishedPduCreator::new_default( - pdu_header, - tstate.delivery_code, - tstate.file_status, - ) - } else { - // TODO: Are there cases where this ID is actually the source entity ID? - let entity_id = EntityIdTlv::new(self.local_cfg.id); - FinishedPduCreator::new_with_error( - pdu_header, - tstate.condition_code, - tstate.delivery_code, - tstate.file_status, - entity_id, - ) - }; - finished_pdu.write_to_bytes(buf)? - } - FileDirectiveType::AckPdu => todo!(), - FileDirectiveType::NakPdu => todo!(), - FileDirectiveType::KeepAlivePdu => todo!(), - _ => { - // This should never happen and is considered an internal impl error - panic!("invalid file directive {directive:?} for dest handler send packet"); - } - }; - Ok(Some((directive, written_size))) - } - fn insert_packet( &mut self, cfdp_user: &mut impl CfdpUser, @@ -532,12 +489,14 @@ impl DestinationHandler { fn start_check_limit_handling(&mut self) { self.step = TransactionStep::ReceivingFileDataPdusWithCheckLimitHandling; - self.tparams.tstate.current_check_timer = - Some(self.check_timer_creator.get_check_timer_provider( - &self.local_cfg.id, - &self.tparams.remote_cfg.unwrap().entity_id, - EntityType::Receiving, - )); + self.tparams.tstate.current_check_timer = Some( + self.check_timer_creator + .get_check_timer_provider(TimerContext::CheckLimit { + local_id: self.local_cfg.id, + remote_id: self.tparams.remote_cfg.unwrap().entity_id, + entity_type: EntityType::Receiving, + }), + ); self.tparams.tstate.current_check_count = 0; } @@ -571,7 +530,8 @@ impl DestinationHandler { todo!(); } - fn fsm_busy(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { + fn fsm_busy(&mut self, cfdp_user: &mut impl CfdpUser) -> Result { + let mut sent_packets = 0; if self.step == TransactionStep::TransactionStart { self.transaction_start(cfdp_user)?; } @@ -579,7 +539,7 @@ impl DestinationHandler { self.check_limit_handling(); } if self.step == TransactionStep::TransferCompletion { - self.transfer_completion(cfdp_user)?; + sent_packets += self.transfer_completion(cfdp_user)?; } if self.step == TransactionStep::SendingAckPdu { todo!("no support for acknowledged mode yet"); @@ -587,7 +547,7 @@ impl DestinationHandler { if self.step == TransactionStep::SendingFinishedPdu { self.reset(); } - Ok(()) + Ok(sent_packets) } /// Get the step, which denotes the exact step of a pending CFDP transaction when applicable. @@ -669,17 +629,18 @@ impl DestinationHandler { Ok(()) } - fn transfer_completion(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { + fn transfer_completion(&mut self, cfdp_user: &mut impl CfdpUser) -> Result { + let mut sent_packets = 0; self.notice_of_completion(cfdp_user)?; if self.tparams.transmission_mode() == TransmissionMode::Acknowledged || self.tparams.metadata_params().closure_requested { - self.prepare_finished_pdu()?; + sent_packets += self.send_finished_pdu()?; self.step = TransactionStep::SendingFinishedPdu; } else { self.reset(); } - Ok(()) + Ok(sent_packets) } fn notice_of_completion(&mut self, cfdp_user: &mut impl CfdpUser) -> Result<(), DestError> { @@ -745,15 +706,36 @@ impl DestinationHandler { fn reset(&mut self) { self.step = TransactionStep::Idle; self.state = State::Idle; - self.packets_to_send_ctx.packet_available = false; + // self.packets_to_send_ctx.packet_available = false; self.tparams.reset(); } - fn prepare_finished_pdu(&mut self) -> Result<(), DestError> { - self.packets_to_send_ctx.packet_available = true; - self.packets_to_send_ctx.directive = Some(FileDirectiveType::FinishedPdu); - self.step = TransactionStep::SendingFinishedPdu; - Ok(()) + fn send_finished_pdu(&mut self) -> Result { + let tstate = self.tstate(); + + let pdu_header = PduHeader::new_no_file_data(self.tparams.pdu_conf, 0); + let finished_pdu = if tstate.condition_code == ConditionCode::NoError + || tstate.condition_code == ConditionCode::UnsupportedChecksumType + { + FinishedPduCreator::new_default(pdu_header, tstate.delivery_code, tstate.file_status) + } else { + // TODO: Are there cases where this ID is actually the source entity ID? + let entity_id = EntityIdTlv::new(self.local_cfg.id); + FinishedPduCreator::new_with_error( + pdu_header, + tstate.condition_code, + tstate.delivery_code, + tstate.file_status, + entity_id, + ) + }; + finished_pdu.write_to_bytes(&mut self.packet_buf)?; + self.packet_sender.send_pdu( + finished_pdu.pdu_type(), + finished_pdu.file_directive_type(), + &self.packet_buf[0..finished_pdu.len_written()], + )?; + Ok(1) } fn tstate(&self) -> &TransferState { @@ -800,6 +782,27 @@ mod tests { pub length: usize, } + type SharedPduPacketQueue = Arc, Vec)>>>; + #[derive(Default, Clone)] + struct TestCfdpSender { + packet_queue: SharedPduPacketQueue, + } + + impl CfdpPacketSender for TestCfdpSender { + fn send_pdu( + &mut self, + pdu_type: PduType, + file_directive_type: Option, + raw_pdu: &[u8], + ) -> Result<(), PduError> { + self.packet_queue.lock().unwrap().push_back(( + pdu_type, + file_directive_type, + raw_pdu.to_vec(), + )); + Ok(()) + } + } #[derive(Default)] struct TestCfdpUser { next_expected_seq_num: u64, @@ -1025,28 +1028,33 @@ mod tests { } struct TestCheckTimerCreator { - expired_flag: Arc, + check_limit_expired_flag: Arc, } impl TestCheckTimerCreator { pub fn new(expired_flag: Arc) -> Self { - Self { expired_flag } + Self { + check_limit_expired_flag: expired_flag, + } } } impl CheckTimerCreator for TestCheckTimerCreator { - fn get_check_timer_provider( - &self, - _local_id: &UnsignedByteField, - _remote_id: &UnsignedByteField, - _entity_type: crate::cfdp::EntityType, - ) -> Box { - Box::new(TestCheckTimer::new(self.expired_flag.clone())) + fn get_check_timer_provider(&self, timer_context: TimerContext) -> Box { + match timer_context { + TimerContext::CheckLimit { .. } => { + Box::new(TestCheckTimer::new(self.check_limit_expired_flag.clone())) + } + _ => { + panic!("invalid check timer creator, can only be used for check limit handling") + } + } } } struct DestHandlerTester { check_timer_expired: Arc, + test_sender: TestCfdpSender, handler: DestinationHandler, src_path: PathBuf, dest_path: PathBuf, @@ -1061,11 +1069,17 @@ mod tests { impl DestHandlerTester { fn new(fault_handler: TestFaultHandler) -> Self { let check_timer_expired = Arc::new(AtomicBool::new(false)); - let dest_handler = default_dest_handler(fault_handler, check_timer_expired.clone()); + let test_sender = TestCfdpSender::default(); + let dest_handler = default_dest_handler( + fault_handler, + test_sender.clone(), + check_timer_expired.clone(), + ); let (src_path, dest_path) = init_full_filenames(); assert!(!Path::exists(&dest_path)); let handler = Self { check_timer_expired, + test_sender, handler: dest_handler, src_path, dest_path, @@ -1134,7 +1148,7 @@ mod tests { user: &mut TestCfdpUser, offset: u64, file_data_chunk: &[u8], - ) -> Result<(), DestError> { + ) -> Result { let filedata_pdu = FileDataPdu::new_no_seg_metadata(self.pdu_header, offset, file_data_chunk); filedata_pdu @@ -1157,7 +1171,7 @@ mod tests { &mut self, user: &mut TestCfdpUser, expected_full_data: Vec, - ) -> Result<(), DestError> { + ) -> Result { self.expected_full_data = expected_full_data; let eof_pdu = create_no_error_eof(&self.expected_full_data, &self.pdu_header); let packet_info = create_packet_info(&eof_pdu, &mut self.buf); @@ -1218,6 +1232,7 @@ mod tests { fn default_dest_handler( test_fault_handler: TestFaultHandler, + test_packet_sender: TestCfdpSender, check_timer_expired: Arc, ) -> DestinationHandler { let local_entity_cfg = LocalEntityConfig { @@ -1227,6 +1242,8 @@ mod tests { }; DestinationHandler::new( local_entity_cfg, + 2048, + Box::new(test_packet_sender), Box::::default(), Box::new(basic_remote_cfg_table()), Box::new(TestCheckTimerCreator::new(check_timer_expired)), @@ -1284,7 +1301,8 @@ mod tests { #[test] fn test_basic() { let fault_handler = TestFaultHandler::default(); - let dest_handler = default_dest_handler(fault_handler.clone(), Arc::default()); + let test_sender = TestCfdpSender::default(); + let dest_handler = default_dest_handler(fault_handler.clone(), test_sender, Arc::default()); assert!(dest_handler.transmission_mode().is_none()); assert!(fault_handler.all_queues_empty()); } diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index cf18fe7..7bf6776 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -31,7 +31,23 @@ pub enum EntityType { Receiving, } -/// Generic abstraction for a check timer which has different functionality depending on whether +pub enum TimerContext { + CheckLimit { + local_id: UnsignedByteField, + remote_id: UnsignedByteField, + entity_type: EntityType, + }, + NakActivity(f32), + PositiveAck(f32), +} + +/// Generic abstraction for a check timer which is used by 3 mechanisms of the CFDP protocol. +/// +/// ## 1. Check limit handling +/// +/// The first mechanism is the check limit handling for unacknowledged transfers as specified +/// in 4.6.3.2 and 4.6.3.3 of the CFDP standard. +/// For this mechanism, the timer has different functionality depending on whether /// the using entity is the sending entity or the receiving entity for the unacknowledged /// transmission mode. /// @@ -42,6 +58,18 @@ pub enum EntityType { /// For the receiving entity, this timer determines the expiry period for incrementing a check /// counter after an EOF PDU is received for an incomplete file transfer. This allows out-of-order /// reception of file data PDUs and EOF PDUs. Also see 4.6.3.3 of the CFDP standard. +/// +/// ## 2. NAK activity limit +/// +/// The timer will be used to perform the NAK activity check as specified in 4.6.4.7 of the CFDP +/// standard. The expiration period will be provided by the NAK timer expiration limit of the +/// remote entity configuration. +/// +/// ## 3. Positive ACK procedures +/// +/// The timer will be used to perform the Positive Acknowledgement Procedures as specified in +/// 4.7. 1of the CFDP standard. The expiration period will be provided by the Positive ACK timer +/// interval of the remote entity configuration. pub trait CheckTimer: Debug { fn has_expired(&self) -> bool; fn reset(&mut self); @@ -50,19 +78,14 @@ pub trait CheckTimer: Debug { /// A generic trait which allows CFDP entities to create check timers which are required to /// implement special procedures in unacknowledged transmission mode, as specified in 4.6.3.2 /// and 4.6.3.3. The [CheckTimer] documentation provides more information about the purpose of the -/// check timer. +/// check timer in the context of CFDP. /// -/// This trait also allows the creation of different check timers depending on -/// the ID of the local entity, the ID of the remote entity for a given transaction, and the -/// type of entity. +/// This trait also allows the creation of different check timers depending on context and purpose +/// of the timer, the runtime environment (e.g. standard clock timer vs. timer using a RTC) or +/// other factors. #[cfg(feature = "alloc")] pub trait CheckTimerCreator { - fn get_check_timer_provider( - &self, - local_id: &UnsignedByteField, - remote_id: &UnsignedByteField, - entity_type: EntityType, - ) -> Box; + fn get_check_timer_provider(&self, timer_context: TimerContext) -> Box; } /// Simple implementation of the [CheckTimerCreator] trait assuming a standard runtime. -- 2.43.0 From c5054c323e204d011b1669daabcddc90e21726de Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 22 Dec 2023 19:40:58 +0100 Subject: [PATCH 29/33] add some documentation and other improvements --- satrs-core/src/cfdp/dest.rs | 54 ++++++++++++++++++++++--------------- satrs-core/src/cfdp/mod.rs | 8 ++++-- 2 files changed, 39 insertions(+), 23 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 627a3cd..df28c0b 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -178,21 +178,17 @@ pub trait CfdpPacketSender: Send { /// This is the primary CFDP destination handler. It models the CFDP destination entity, which is /// primarily responsible for receiving files sent from another CFDP entity. It performs the /// reception side of File Copy Operations. - -/// The following core functions are the primary interface for interacting with the destination -/// handler: - -/// 1. [DestinationHandler::state_machine] - Can be used to insert packets into the destination -/// handler and/or advance the state machine. Advancing the state machine might generate new -/// packets to be sent to the remote entity. Please note that the destination handler can also -/// only process Metadata, EOF and Prompt PDUs in addition to ACK PDUs where the acknowledged -/// PDU is the Finished PDU. -/// 2. [DestinationHandler::get_next_packet] - Retrieve next packet to be sent back to the remote -/// CFDP source entity ID. - -/// A new file transfer (Metadata PDU reception) is only be accepted if the handler is in the -/// IDLE state. Furthermore, packet insertion is not allowed until all packets to send were -/// retrieved after a state machine call. +/// +/// The [DestinationHandler::state_machine] function is the primary function to drive the +/// destination handler. It can be used to insert packets into the destination +/// handler and driving the state machine, which might generate new +/// packets to be sent to the remote entity. Please note that the destination handler can also +/// only process Metadata, EOF and Prompt PDUs in addition to ACK PDUs where the acknowledged +/// PDU is the Finished PDU. +/// +/// All generated packets are sent via the [CfdpPacketSender] trait, which is implemented by the +/// user and passed as a constructor parameter. The number of generated packets is returned +/// by the state machine call. pub struct DestinationHandler { local_cfg: LocalEntityConfig, step: TransactionStep, @@ -206,6 +202,25 @@ pub struct DestinationHandler { } impl DestinationHandler { + /// Constructs a new destination handler. + /// + /// # Arguments + /// * `local_cfg` - The local CFDP entity configuration, consisting of the local entity ID, + /// the indication configuration, and the fault handlers. + /// * `max_packet_len` - The maximum expected generated packet size in bytes. Each time a + /// packet is sent, it will be buffered inside an internal buffer. The length of this buffer + /// will be determined by this parameter. This parameter can either be a known upper bound, + /// or it can specifically be determined by the largest packet size parameter of all remote + /// entity configurations in the passed `remote_cfg_table`. + /// * `packet_sender` - All generated packets are sent via this abstraction. + /// * `vfs` - Virtual filestore implementation to decouple the CFDP implementation from the + /// underlying filestore/filesystem. This allows to use this handler for embedded systems + /// where a standard runtime might not be available. + /// * `remote_cfg_table` - A table of all expected remote entities this entity will communicate + /// with. It contains various configuration parameters required for file transfers. + /// * `check_timer_creator` - This is used by the CFDP handler to generate timers required + /// by various tasks. + /// * `` pub fn new( local_cfg: LocalEntityConfig, max_packet_len: usize, @@ -230,12 +245,9 @@ impl DestinationHandler { /// This is the core function to drive the destination handler. It is also used to insert /// packets into the destination handler. /// - /// Please note that this function will fail if there are still packets which need to be - /// retrieved with [Self::get_next_packet]. After each state machine call, the user has to - /// retrieve all packets before calling the state machine again. The state machine should - /// either be called if a packet with the appropriate destination ID is received, or - /// periodically in IDLE periods to perform all CFDP related tasks, for example checking for - /// timeouts or missed file segments. + /// The state machine should either be called if a packet with the appropriate destination ID + /// is received, or periodically in IDLE periods to perform all CFDP related tasks, for example + /// checking for timeouts or missed file segments. pub fn state_machine( &mut self, cfdp_user: &mut impl CfdpUser, diff --git a/satrs-core/src/cfdp/mod.rs b/satrs-core/src/cfdp/mod.rs index 7bf6776..c3bda16 100644 --- a/satrs-core/src/cfdp/mod.rs +++ b/satrs-core/src/cfdp/mod.rs @@ -37,8 +37,12 @@ pub enum TimerContext { remote_id: UnsignedByteField, entity_type: EntityType, }, - NakActivity(f32), - PositiveAck(f32), + NakActivity { + expiry_time_seconds: f32, + }, + PositiveAck { + expiry_time_seconds: f32, + }, } /// Generic abstraction for a check timer which is used by 3 mechanisms of the CFDP protocol. -- 2.43.0 From c2bd862ba4607b64a354887637782fe885d0cdfa Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 22 Dec 2023 19:42:18 +0100 Subject: [PATCH 30/33] typo --- satrs-core/src/cfdp/dest.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index df28c0b..8969e5f 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -205,6 +205,7 @@ impl DestinationHandler { /// Constructs a new destination handler. /// /// # Arguments + /// /// * `local_cfg` - The local CFDP entity configuration, consisting of the local entity ID, /// the indication configuration, and the fault handlers. /// * `max_packet_len` - The maximum expected generated packet size in bytes. Each time a @@ -220,7 +221,6 @@ impl DestinationHandler { /// with. It contains various configuration parameters required for file transfers. /// * `check_timer_creator` - This is used by the CFDP handler to generate timers required /// by various tasks. - /// * `` pub fn new( local_cfg: LocalEntityConfig, max_packet_len: usize, -- 2.43.0 From d8acaaf58020778de43ab8d8caf3e054ed5d53ad Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 23 Jan 2024 18:43:09 +0100 Subject: [PATCH 31/33] add test for requested closure --- satrs-core/src/cfdp/dest.rs | 58 ++++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 10 deletions(-) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 8969e5f..be4eb88 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -815,6 +815,13 @@ mod tests { Ok(()) } } + + impl TestCfdpSender { + pub fn queue_empty(&self) -> bool { + self.packet_queue.lock().unwrap().is_empty() + } + } + #[derive(Default)] struct TestCfdpUser { next_expected_seq_num: u64, @@ -1066,20 +1073,21 @@ mod tests { struct DestHandlerTester { check_timer_expired: Arc, - test_sender: TestCfdpSender, + pdu_sender: TestCfdpSender, handler: DestinationHandler, src_path: PathBuf, dest_path: PathBuf, check_dest_file: bool, check_handler_idle_at_drop: bool, expected_file_size: u64, + closure_requested: bool, pdu_header: PduHeader, expected_full_data: Vec, buf: [u8; 512], } impl DestHandlerTester { - fn new(fault_handler: TestFaultHandler) -> Self { + fn new(fault_handler: TestFaultHandler, closure_requested: bool) -> Self { let check_timer_expired = Arc::new(AtomicBool::new(false)); let test_sender = TestCfdpSender::default(); let dest_handler = default_dest_handler( @@ -1091,9 +1099,10 @@ mod tests { assert!(!Path::exists(&dest_path)); let handler = Self { check_timer_expired, - test_sender, + pdu_sender: test_sender, handler: dest_handler, src_path, + closure_requested, dest_path, check_dest_file: false, check_handler_idle_at_drop: false, @@ -1144,6 +1153,7 @@ mod tests { self.src_path.as_path(), self.dest_path.as_path(), file_size, + self.closure_requested ); let packet_info = create_packet_info(&metadata_pdu, &mut self.buf); self.handler.state_machine(user, Some(&packet_info))?; @@ -1274,13 +1284,14 @@ mod tests { src_name: &'filename Path, dest_name: &'filename Path, file_size: u64, + closure_requested: bool ) -> MetadataPduCreator<'filename, 'filename, 'static> { let checksum_type = if file_size == 0 { ChecksumType::NullChecksum } else { ChecksumType::Crc32 }; - let metadata_params = MetadataGenericParams::new(false, checksum_type, file_size); + let metadata_params = MetadataGenericParams::new(closure_requested, checksum_type, file_size); MetadataPduCreator::new_no_opts( *pdu_header, metadata_params, @@ -1320,9 +1331,9 @@ mod tests { } #[test] - fn test_empty_file_transfer_not_acked() { + fn test_empty_file_transfer_not_acked_no_closure() { let fault_handler = TestFaultHandler::default(); - let mut test_obj = DestHandlerTester::new(fault_handler.clone()); + let mut test_obj = DestHandlerTester::new(fault_handler.clone(), false); let mut test_user = test_obj.test_user_from_cached_paths(0); test_obj .generic_transfer_init(&mut test_user, 0) @@ -1332,6 +1343,8 @@ mod tests { .generic_eof_no_error(&mut test_user, Vec::new()) .expect("EOF no error insertion failed"); assert!(fault_handler.all_queues_empty()); + assert!(test_obj.pdu_sender.queue_empty()); + test_obj.state_check(State::Idle, TransactionStep::Idle); } #[test] @@ -1341,7 +1354,7 @@ mod tests { let file_size = file_data.len() as u64; let fault_handler = TestFaultHandler::default(); - let mut test_obj = DestHandlerTester::new(fault_handler.clone()); + let mut test_obj = DestHandlerTester::new(fault_handler.clone(), false); let mut test_user = test_obj.test_user_from_cached_paths(file_size); test_obj .generic_transfer_init(&mut test_user, file_size) @@ -1354,6 +1367,8 @@ mod tests { .generic_eof_no_error(&mut test_user, file_data.to_vec()) .expect("EOF no error insertion failed"); assert!(fault_handler.all_queues_empty()); + assert!(test_obj.pdu_sender.queue_empty()); + test_obj.state_check(State::Idle, TransactionStep::Idle); } #[test] @@ -1365,7 +1380,7 @@ mod tests { let segment_len = 256; let fault_handler = TestFaultHandler::default(); - let mut test_obj = DestHandlerTester::new(fault_handler.clone()); + let mut test_obj = DestHandlerTester::new(fault_handler.clone(), false); let mut test_user = test_obj.test_user_from_cached_paths(file_size); test_obj .generic_transfer_init(&mut test_user, file_size) @@ -1385,6 +1400,8 @@ mod tests { .generic_eof_no_error(&mut test_user, random_data.to_vec()) .expect("EOF no error insertion failed"); assert!(fault_handler.all_queues_empty()); + assert!(test_obj.pdu_sender.queue_empty()); + test_obj.state_check(State::Idle, TransactionStep::Idle); } #[test] @@ -1396,7 +1413,7 @@ mod tests { let segment_len = 256; let fault_handler = TestFaultHandler::default(); - let mut test_obj = DestHandlerTester::new(fault_handler.clone()); + let mut test_obj = DestHandlerTester::new(fault_handler.clone(), false); let mut test_user = test_obj.test_user_from_cached_paths(file_size); let transaction_id = test_obj .generic_transfer_init(&mut test_user, file_size) @@ -1432,6 +1449,8 @@ mod tests { assert_eq!(cancelled.0, transaction_id); assert_eq!(cancelled.1, ConditionCode::FileChecksumFailure); assert_eq!(cancelled.2, segment_len as u64); + assert!(test_obj.pdu_sender.queue_empty()); + test_obj.state_check(State::Idle, TransactionStep::Idle); } #[test] @@ -1443,7 +1462,7 @@ mod tests { let segment_len = 256; let fault_handler = TestFaultHandler::default(); - let mut test_obj = DestHandlerTester::new(fault_handler.clone()); + let mut test_obj = DestHandlerTester::new(fault_handler.clone(), false); let mut test_user = test_obj.test_user_from_cached_paths(file_size); let transaction_id = test_obj .generic_transfer_init(&mut test_user, file_size) @@ -1498,6 +1517,8 @@ mod tests { drop(cancelled_queue); + assert!(test_obj.pdu_sender.queue_empty()); + // Check that the broken file exists. test_obj.check_dest_file = false; assert!(Path::exists(test_obj.dest_path())); @@ -1506,4 +1527,21 @@ mod tests { assert_eq!(read_content, &random_data[0..segment_len]); assert!(fs::remove_file(test_obj.dest_path().as_path()).is_ok()); } + + #[test] + fn test_file_transfer_with_closure() { + let fault_handler = TestFaultHandler::default(); + let mut test_obj = DestHandlerTester::new(fault_handler.clone(), true); + let mut test_user = test_obj.test_user_from_cached_paths(0); + test_obj + .generic_transfer_init(&mut test_user, 0) + .expect("transfer init failed"); + test_obj.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + test_obj + .generic_eof_no_error(&mut test_user, Vec::new()) + .expect("EOF no error insertion failed"); + assert!(fault_handler.all_queues_empty()); + test_obj.state_check(State::Idle, TransactionStep::Idle); + // assert!(!test_obj.pdu_sender.queue_empty()); + } } -- 2.43.0 From 48b8c6891aa2f2ca231b75fc38222c7f9437f0d1 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 23 Jan 2024 19:43:52 +0100 Subject: [PATCH 32/33] verified first finished PDU success --- satrs-core/Cargo.toml | 2 +- satrs-core/src/cfdp/dest.rs | 130 ++++++++++++++++++++++++------------ 2 files changed, 90 insertions(+), 42 deletions(-) diff --git a/satrs-core/Cargo.toml b/satrs-core/Cargo.toml index c46fc3b..d16f0a0 100644 --- a/satrs-core/Cargo.toml +++ b/satrs-core/Cargo.toml @@ -73,7 +73,7 @@ features = ["all"] optional = true [dependencies.spacepackets] -version = "0.7.0-beta.3" +version = "0.7.0-beta.4" default-features = false # git = "https://egit.irs.uni-stuttgart.de/rust/spacepackets.git" # rev = "297cfad22637d3b07a1b27abe56d9a607b5b82a7" diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index be4eb88..495e5fa 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -21,7 +21,7 @@ use spacepackets::{ CfdpPdu, CommonPduConfig, FileDirectiveType, PduError, PduHeader, WritablePduPacket, }, tlv::{msg_to_user::MsgToUserTlv, EntityIdTlv, GenericTlv, TlvType}, - ConditionCode, FaultHandlerCode, PduType, TransmissionMode, + ChecksumType, ConditionCode, FaultHandlerCode, PduType, TransmissionMode, }, util::UnsignedByteField, }; @@ -47,6 +47,7 @@ struct TransferState { transaction_id: Option, metadata_params: MetadataGenericParams, progress: u64, + metadata_only: bool, condition_code: ConditionCode, delivery_code: DeliveryCode, file_status: FileStatus, @@ -62,6 +63,7 @@ impl Default for TransferState { transaction_id: None, metadata_params: Default::default(), progress: Default::default(), + metadata_only: false, condition_code: ConditionCode::NoError, delivery_code: DeliveryCode::Incomplete, file_status: FileStatus::Unreported, @@ -344,21 +346,29 @@ impl DestinationHandler { // TODO: Support for metadata only PDUs. let src_name = metadata_pdu.src_file_name(); - if src_name.is_empty() { + let dest_name = metadata_pdu.dest_file_name(); + if src_name.is_empty() && dest_name.is_empty() { + self.tparams.tstate.metadata_only = true; + } + if !self.tparams.tstate.metadata_only && src_name.is_empty() { return Err(DestError::EmptySrcFileField); } - self.tparams.file_properties.src_file_name[..src_name.len_value()] - .copy_from_slice(src_name.value()); - self.tparams.file_properties.src_file_name_len = src_name.len_value(); - let dest_name = metadata_pdu.dest_file_name(); - if dest_name.is_empty() { + if !self.tparams.tstate.metadata_only && dest_name.is_empty() { return Err(DestError::EmptyDestFileField); } - self.tparams.file_properties.dest_file_name[..dest_name.len_value()] - .copy_from_slice(dest_name.value()); - self.tparams.file_properties.dest_file_name_len = dest_name.len_value(); - self.tparams.pdu_conf = *metadata_pdu.pdu_header().common_pdu_conf(); - self.tparams.msgs_to_user_size = 0; + if !self.tparams.tstate.metadata_only { + self.tparams.file_properties.src_file_name[..src_name.len_value()] + .copy_from_slice(src_name.value()); + self.tparams.file_properties.src_file_name_len = src_name.len_value(); + if dest_name.is_empty() { + return Err(DestError::EmptyDestFileField); + } + self.tparams.file_properties.dest_file_name[..dest_name.len_value()] + .copy_from_slice(dest_name.value()); + self.tparams.file_properties.dest_file_name_len = dest_name.len_value(); + self.tparams.pdu_conf = *metadata_pdu.pdu_header().common_pdu_conf(); + self.tparams.msgs_to_user_size = 0; + } if !metadata_pdu.options().is_empty() { for option_tlv in metadata_pdu.options_iter().unwrap() { if option_tlv.is_standard_tlv() @@ -476,27 +486,38 @@ impl DestinationHandler { } fn checksum_verify(&mut self, checksum: u32) -> bool { - match self.vfs.checksum_verify( - self.tparams.file_properties.dest_path_buf.to_str().unwrap(), - self.tparams.metadata_params().checksum_type, - checksum, - &mut self.tparams.cksum_buf, - ) { - Ok(checksum_success) => checksum_success, - Err(e) => match e { - FilestoreError::ChecksumTypeNotImplemented(_) => { - self.declare_fault(ConditionCode::UnsupportedChecksumType); - // For this case, the applicable algorithm shall the the null checksum, which - // is always succesful. - true + let mut file_delivery_complete = false; + if self.tparams.metadata_params().checksum_type == ChecksumType::NullChecksum + || self.tparams.tstate.metadata_only + { + file_delivery_complete = true; + self.tparams.tstate.delivery_code = DeliveryCode::Complete; + self.tparams.tstate.condition_code = ConditionCode::NoError; + } else { + match self.vfs.checksum_verify( + self.tparams.file_properties.dest_path_buf.to_str().unwrap(), + self.tparams.metadata_params().checksum_type, + checksum, + &mut self.tparams.cksum_buf, + ) { + Ok(checksum_success) => { + file_delivery_complete = checksum_success; } - _ => { - self.declare_fault(ConditionCode::FilestoreRejection); - // Treat this equivalent to a failed checksum procedure. - false - } - }, + Err(e) => match e { + FilestoreError::ChecksumTypeNotImplemented(_) => { + self.declare_fault(ConditionCode::UnsupportedChecksumType); + // For this case, the applicable algorithm shall be the the null checksum, + // which is always succesful. + file_delivery_complete = true; + } + _ => { + self.declare_fault(ConditionCode::FilestoreRejection); + // Treat this equivalent to a failed checksum procedure. + } + }, + }; } + file_delivery_complete } fn start_check_limit_handling(&mut self) { @@ -771,7 +792,7 @@ mod tests { use spacepackets::{ cfdp::{ lv::Lv, - pdu::{metadata::MetadataPduCreator, WritablePduPacket}, + pdu::{finished::FinishedPduReader, metadata::MetadataPduCreator, WritablePduPacket}, ChecksumType, TransmissionMode, }, util::{UbfU16, UnsignedByteFieldU16}, @@ -794,7 +815,12 @@ mod tests { pub length: usize, } - type SharedPduPacketQueue = Arc, Vec)>>>; + struct SentPdu { + pdu_type: PduType, + file_directive_type: Option, + raw_pdu: Vec, + } + type SharedPduPacketQueue = Arc>>; #[derive(Default, Clone)] struct TestCfdpSender { packet_queue: SharedPduPacketQueue, @@ -807,16 +833,19 @@ mod tests { file_directive_type: Option, raw_pdu: &[u8], ) -> Result<(), PduError> { - self.packet_queue.lock().unwrap().push_back(( + self.packet_queue.lock().unwrap().push_back(SentPdu { pdu_type, file_directive_type, - raw_pdu.to_vec(), - )); + raw_pdu: raw_pdu.to_vec(), + }); Ok(()) } } impl TestCfdpSender { + pub fn retrieve_next_pdu(&self) -> Option { + self.packet_queue.lock().unwrap().pop_front() + } pub fn queue_empty(&self) -> bool { self.packet_queue.lock().unwrap().is_empty() } @@ -1153,7 +1182,7 @@ mod tests { self.src_path.as_path(), self.dest_path.as_path(), file_size, - self.closure_requested + self.closure_requested, ); let packet_info = create_packet_info(&metadata_pdu, &mut self.buf); self.handler.state_machine(user, Some(&packet_info))?; @@ -1284,14 +1313,15 @@ mod tests { src_name: &'filename Path, dest_name: &'filename Path, file_size: u64, - closure_requested: bool + closure_requested: bool, ) -> MetadataPduCreator<'filename, 'filename, 'static> { let checksum_type = if file_size == 0 { ChecksumType::NullChecksum } else { ChecksumType::Crc32 }; - let metadata_params = MetadataGenericParams::new(closure_requested, checksum_type, file_size); + let metadata_params = + MetadataGenericParams::new(closure_requested, checksum_type, file_size); MetadataPduCreator::new_no_opts( *pdu_header, metadata_params, @@ -1528,6 +1558,20 @@ mod tests { assert!(fs::remove_file(test_obj.dest_path().as_path()).is_ok()); } + fn check_finished_pdu_success(sent_pdu: &SentPdu) { + assert_eq!(sent_pdu.pdu_type, PduType::FileDirective); + assert_eq!( + sent_pdu.file_directive_type, + Some(FileDirectiveType::FinishedPdu) + ); + let finished_pdu = FinishedPduReader::from_bytes(&sent_pdu.raw_pdu).unwrap(); + assert_eq!(finished_pdu.file_status(), FileStatus::Retained); + assert_eq!(finished_pdu.condition_code(), ConditionCode::NoError); + assert_eq!(finished_pdu.delivery_code(), DeliveryCode::Complete); + assert!(finished_pdu.fault_location().is_none()); + assert_eq!(finished_pdu.fs_responses_raw(), &[]); + } + #[test] fn test_file_transfer_with_closure() { let fault_handler = TestFaultHandler::default(); @@ -1537,11 +1581,15 @@ mod tests { .generic_transfer_init(&mut test_user, 0) .expect("transfer init failed"); test_obj.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); - test_obj + let sent_packets = test_obj .generic_eof_no_error(&mut test_user, Vec::new()) .expect("EOF no error insertion failed"); + assert_eq!(sent_packets, 1); assert!(fault_handler.all_queues_empty()); + // The Finished PDU was sent, so the state machine is done. test_obj.state_check(State::Idle, TransactionStep::Idle); - // assert!(!test_obj.pdu_sender.queue_empty()); + assert!(!test_obj.pdu_sender.queue_empty()); + let sent_pdu = test_obj.pdu_sender.retrieve_next_pdu().unwrap(); + check_finished_pdu_success(&sent_pdu); } } -- 2.43.0 From bf97a037301824c49ed289b37382230e47fd67d9 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 29 Jan 2024 23:36:34 +0100 Subject: [PATCH 33/33] let's finish this PR for now --- satrs-core/src/cfdp/dest.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/satrs-core/src/cfdp/dest.rs b/satrs-core/src/cfdp/dest.rs index 495e5fa..1899abd 100644 --- a/satrs-core/src/cfdp/dest.rs +++ b/satrs-core/src/cfdp/dest.rs @@ -1592,4 +1592,9 @@ mod tests { let sent_pdu = test_obj.pdu_sender.retrieve_next_pdu().unwrap(); check_finished_pdu_success(&sent_pdu); } + + #[test] + fn test_file_transfer_with_closure_check_limit_reached() { + // TODO: Implement test. + } } -- 2.43.0