diff options
author | Ayush Jain <ayushjain@google.com> | 2023-04-27 22:02:39 +0000 |
---|---|---|
committer | Presubmit Automerger Backend <android-build-presubmit-automerger-backend@system.gserviceaccount.com> | 2023-04-27 22:02:39 +0000 |
commit | 4f8b34ee4d87022ed46896f78b65ec1379257876 (patch) | |
tree | 3c950164255ec5640c147b872f7dd671ce6970ab | |
parent | 6da0bdfee9b21c5d27c3f6840056b43cdb0da4a2 (diff) | |
parent | bf84153f90713a2bdad6a5c271dc2e3f6932ea3b (diff) | |
download | uwb-4f8b34ee4d87022ed46896f78b65ec1379257876.tar.gz |
[automerge] Implement retries for DataSnd packets. 2p: bf84153f90
Original change: https://googleplex-android-review.googlesource.com/c/platform/external/uwb/+/22913370
Bug: 273376343
Change-Id: Ie77d0c3457538b1b4b4cb293071032a8c65e3f9d
Merged-In: Ifb813c4b3cd0e0bd8fadb8fce9c631a52a824eca
-rw-r--r-- | src/rust/uwb_core/src/uci/uci_manager.rs | 196 |
1 files changed, 176 insertions, 20 deletions
diff --git a/src/rust/uwb_core/src/uci/uci_manager.rs b/src/rust/uwb_core/src/uci/uci_manager.rs index ee88d6c..d74d7fa 100644 --- a/src/rust/uwb_core/src/uci/uci_manager.rs +++ b/src/rust/uwb_core/src/uci/uci_manager.rs @@ -523,11 +523,15 @@ struct UciManagerActor<T: UciHal, U: UciLogger> { // Used for the logic of retrying the command. Only valid when waiting for the response of a // UCI command. - retryer: Option<Retryer>, + uci_cmd_retryer: Option<UciCmdRetryer>, // The timeout of waiting for the response. Only used when waiting for the response of a UCI // command. wait_resp_timeout: PinSleep, + // Used for the logic of retrying the DataSnd packet. Only valid when waiting for the + // DATA_TRANSFER_STATUS_NTF. + uci_data_snd_retryer: Option<UciDataSndRetryer>, + // Used to identify if response corresponds to the last vendor command, if so return // a raw packet as a response to the sender. last_raw_cmd: Option<RawUciControlPacket>, @@ -560,7 +564,8 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { data_credit_map: HashMap::new(), data_packet_fragments_map: HashMap::new(), wait_device_status_timeout: PinSleep::new(Duration::MAX), - retryer: None, + uci_cmd_retryer: None, + uci_data_snd_retryer: None, wait_resp_timeout: PinSleep::new(Duration::MAX), last_raw_cmd: None, core_notf_sender: mpsc::unbounded_channel().0, @@ -595,7 +600,7 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { // Timeout waiting for the response of the UCI command. _ = &mut self.wait_resp_timeout, if self.is_waiting_resp() => { - self.retryer.take().unwrap().send_result(Err(Error::Timeout)); + self.uci_cmd_retryer.take().unwrap().send_result(Err(Error::Timeout)); } // Timeout waiting for the notification of the device status. @@ -690,7 +695,7 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { } UciManagerCmd::SendUciCommand { cmd } => { - debug_assert!(self.retryer.is_none()); + debug_assert!(self.uci_cmd_retryer.is_none()); // Remember that this command is a raw UCI command, we'll use this later // to send a raw UCI response. @@ -716,8 +721,9 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { }); } - self.retryer = Some(Retryer { cmd, result_sender, retry_count: MAX_RETRY_COUNT }); - self.retry_command().await; + self.uci_cmd_retryer = + Some(UciCmdRetryer { cmd, result_sender, retry_count: MAX_RETRY_COUNT }); + self.retry_uci_cmd().await; } UciManagerCmd::SendUciData { data_snd_packet } => { @@ -727,20 +733,47 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { } } - async fn retry_command(&mut self) { - if let Some(mut retryer) = self.retryer.take() { - if !retryer.could_retry() { - retryer.send_result(Err(Error::Timeout)); + async fn retry_uci_cmd(&mut self) { + if let Some(mut uci_cmd_retryer) = self.uci_cmd_retryer.take() { + if !uci_cmd_retryer.could_retry() { + error!("Out of retries for Uci Cmd packet"); + uci_cmd_retryer.send_result(Err(Error::Timeout)); return; } - match self.send_uci_command(retryer.cmd.clone()).await { + match self.send_uci_command(uci_cmd_retryer.cmd.clone()).await { Ok(_) => { self.wait_resp_timeout = PinSleep::new(Duration::from_millis(UCI_TIMEOUT_MS)); - self.retryer = Some(retryer); + self.uci_cmd_retryer = Some(uci_cmd_retryer); + } + Err(e) => { + error!("Uci Cmd send resulted in error:{}", e); + uci_cmd_retryer.send_result(Err(e)); + } + } + } + } + + async fn retry_uci_data_snd(&mut self) { + if let Some(mut uci_data_snd_retryer) = self.uci_data_snd_retryer.take() { + let data_packet_session_id = uci_data_snd_retryer.data_packet_session_id; + if !uci_data_snd_retryer.could_retry() { + error!( + "Out of retries for Uci DataSnd packet, last DataSnd packet session_id:{}", + data_packet_session_id + ); + return; + } + + match self.hal.send_packet(uci_data_snd_retryer.data_packet.clone().to_vec()).await { + Ok(_) => { + self.uci_data_snd_retryer = Some(uci_data_snd_retryer); } Err(e) => { - retryer.send_result(Err(e)); + error!( + "DataSnd packet fragment session_id:{} retry failed with error:{}", + data_packet_session_id, e + ); } } } @@ -837,6 +870,13 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { } }; + // Create and save a retryer for sending this data packet fragment. + self.uci_data_snd_retryer = Some(UciDataSndRetryer { + data_packet: hal_data_packet_fragment.clone(), + data_packet_session_id, + retry_count: MAX_RETRY_COUNT, + }); + let result = self.hal.send_packet(hal_data_packet_fragment.to_vec()).await; if result.is_err() { error!( @@ -911,12 +951,12 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { async fn handle_response(&mut self, resp: UciResponse) { if resp.need_retry() { - self.retry_command().await; + self.retry_uci_cmd().await; return; } - if let Some(retryer) = self.retryer.take() { - retryer.send_result(Ok(resp)); + if let Some(uci_cmd_retryer) = self.uci_cmd_retryer.take() { + uci_cmd_retryer.send_result(Ok(resp)); } else { warn!("Received an UCI response unexpectedly: {:?}", resp); } @@ -924,7 +964,10 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { async fn handle_notification(&mut self, notf: UciNotification) { if notf.need_retry() { - self.retry_command().await; + // Retry sending both last sent UCI CMD and UCI DataSnd packet since the notification + // could be for either of them. + self.retry_uci_cmd().await; + self.retry_uci_data_snd().await; return; } @@ -978,6 +1021,15 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { } return; // We consume these here and don't need to send to upper layer. } + if let SessionNotification::DataTransferStatus { + session_id: _, + uci_sequence_number: _, + status: _, + } = session_notf + { + // Reset the UciDataSnd Retryer since we received a DataTransferStatusNtf. + let _ = self.uci_data_snd_retryer.take(); + } let _ = self.session_notf_sender.send(session_notf); } UciNotification::Vendor(vendor_notf) => { @@ -1032,7 +1084,7 @@ impl<T: UciHal, U: UciLogger> UciManagerActor<T, U> { } fn is_waiting_resp(&self) -> bool { - self.retryer.is_some() + self.uci_cmd_retryer.is_some() } fn is_waiting_device_status(&self) -> bool { self.open_hal_result_sender.is_some() @@ -1046,13 +1098,13 @@ impl<T: UciHal, U: UciLogger> Drop for UciManagerActor<T, U> { } } -struct Retryer { +struct UciCmdRetryer { cmd: UciCommand, result_sender: oneshot::Sender<Result<UciResponse>>, retry_count: usize, } -impl Retryer { +impl UciCmdRetryer { fn could_retry(&mut self) -> bool { if self.retry_count == 0 { return false; @@ -1066,6 +1118,28 @@ impl Retryer { } } +struct UciDataSndRetryer { + // Store the last-sent DataSnd packet fragment across all the active UWB session, as the UCI + // spec states that the "last UCI packet should be re-transmitted from Host". + // + // TODO(b/273376343): The spec is open to a race condition in the scenario of multiple active + // sessions, as there can be outstanding DataSnd packet fragments across them. We could do an + // alternative implementation of sending all of them. + data_packet: UciDataPacketHal, + data_packet_session_id: u32, + retry_count: usize, +} + +impl UciDataSndRetryer { + fn could_retry(&mut self) -> bool { + if self.retry_count == 0 { + return false; + } + self.retry_count -= 1; + true + } +} + #[derive(Debug)] enum UciManagerCmd { SetLoggerMode { @@ -2113,6 +2187,14 @@ mod tests { hal.expected_send_command(cmd, responses, Ok(())); } + // TODO(b/276320369): Listing down the Data Packet Rx scenarios below, will add unit tests + // for them in subsequent CLs. + #[tokio::test] + async fn test_data_packet_recv_ok() {} + + #[tokio::test] + async fn test_data_packet_recv_fragmented_packet_ok() {} + #[tokio::test] async fn test_data_packet_send_ok() { // Test Data packet send for a single packet (on a UWB session). @@ -2274,6 +2356,80 @@ mod tests { assert!(mock_hal.wait_expected_calls_done().await); } + #[tokio::test] + async fn test_data_packet_send_retry_ok() { + // Test Data packet send for a single packet (on a UWB session). + let mt_data = 0x0; + let pbf = 0x0; + let dpf = 0x1; + let oid = 0x0; + let session_id = 0x5; + let dest_mac_address = vec![0xa0, 0xb0, 0xc0, 0xd0, 0xa1, 0xb1, 0xc1, 0xd1]; + let dest_fira_component = FiraComponent::Host; + let uci_sequence_number = 0xa; + let app_data = vec![0x01, 0x02, 0x03]; + let expected_data_snd_payload = vec![ + 0x05, 0x00, 0x00, 0x00, // SessionID + 0xa0, 0xb0, 0xc0, 0xd0, 0xa1, 0xb1, 0xc1, 0xd1, // MacAddress + 0x01, // FiraComponent + 0x0a, // UciSequenceNumber + 0x03, 0x00, // AppDataLen + 0x01, 0x02, 0x03, // AppData + ]; + let status = DataTransferNtfStatusCode::UciDataTransferStatusRepetitionOk; + + let (uci_manager, mut mock_hal) = setup_uci_manager_with_open_hal( + move |hal| { + setup_active_session(hal, session_id); + + // Setup receiving a CORE_GENERIC_ERROR_NTF with STATUS_COMMAND_RETRY after a + // failed Data packet send attempt. + let data_packet_snd = + build_uci_packet(mt_data, pbf, dpf, oid, expected_data_snd_payload); + let error_ntf = into_uci_hal_packets(uwb_uci_packets::GenericErrorBuilder { + status: StatusCode::UciStatusCommandRetry, + }); + hal.expected_send_packet(data_packet_snd.clone(), error_ntf, Ok(())); + + // Setup the notifications that should be received after the Data packet send + // is successfully retried. + let mut ntfs = into_uci_hal_packets(uwb_uci_packets::DataCreditNtfBuilder { + session_id, + credit_availability: CreditAvailability::CreditAvailable, + }); + ntfs.append(&mut into_uci_hal_packets( + uwb_uci_packets::DataTransferStatusNtfBuilder { + session_id, + uci_sequence_number, + status, + }, + )); + hal.expected_send_packet(data_packet_snd, ntfs, Ok(())); + }, + UciLoggerMode::Disabled, + mpsc::unbounded_channel::<UciLogEvent>().0, + ) + .await; + + let result = uci_manager.range_start(session_id).await; + assert!(result.is_ok()); + + let result = uci_manager + .send_data_packet( + session_id, + dest_mac_address, + dest_fira_component, + uci_sequence_number, + app_data, + ) + .await; + assert!(result.is_ok()); + assert!(mock_hal.wait_expected_calls_done().await); + + // TODO(b/276320369): Verify that session_notf_sender is called (once implemented), as a + // DataTransferStatusNtf is received in this test scenario. + } + // TODO(b/276320369): Listing down the Data Packet Tx scenarios below, will add unit tests // for them in subsequent CLs. |