diff --git a/.github/workflows/rust-minimal.yml b/.github/workflows/rust-minimal.yml index f6b13d988..ed21f184b 100644 --- a/.github/workflows/rust-minimal.yml +++ b/.github/workflows/rust-minimal.yml @@ -14,6 +14,10 @@ on: env: CARGO_TERM_COLOR: always +defaults: + run: + shell: bash + jobs: build: strategy: @@ -82,6 +86,20 @@ jobs: sudo pip3 install git+https://github.com/colcon/colcon-cargo.git sudo pip3 install git+https://github.com/colcon/colcon-ros-cargo.git + - name: Search Rust packages in this repository + id: list_repo_rust_packages + run: | + mapfile -t package_paths < <(colcon list | awk '$3 == "(ros.ament_cargo)" { print $2 }') + if (( ${#package_paths[@]} == 0 )); then + echo "::error::No ros.ament_cargo packages found in the repository" + exit 1 + fi + { + echo 'package_paths<> "$GITHUB_OUTPUT" + # test_msgs recently added ament_mypy as a test dependency, but rosdep # may fail to install it when building from source on rolling - name: Install ament_cmake_mypy for rolling @@ -91,13 +109,16 @@ jobs: sudo apt-get install -y ros-rolling-ament-cmake-mypy - name: Check formatting of Rust packages + env: + PACKAGE_PATHS: ${{ steps.list_repo_rust_packages.outputs.package_paths }} run: | - for path in $(colcon list | awk '$3 == "(ament_cargo)" { print $2 }'); do - cd $path rustup toolchain install nightly - cargo +nightly fmt -- --check - cd - - done + while IFS= read -r path; do + [ -n "$path" ] || continue + cd "$path" + cargo +nightly fmt -- --check + cd - > /dev/null + done <<< "$PACKAGE_PATHS" - name: Build and test id: build @@ -107,45 +128,53 @@ jobs: target-ros2-distro: ${{ matrix.ros_distribution }} vcs-repo-file-url: ros2_rust_${{ matrix.ros_distribution }}.repos - - name: Run clippy on Rust packages + - name: Search target Rust packages in the built workspace + id: list_workspace_rust_packages + env: + PACKAGE_NAMES: ${{ steps.list_packages.outputs.package_list }} run: | cd ${{ steps.build.outputs.ros-workspace-directory-name }} - . /opt/ros/${{ matrix.ros_distribution }}/setup.sh - for path in $(colcon list | awk '$3 == "(ament_cargo)" { print $2 }'); do - cd $path - echo "Running clippy in $path" - # Run clippy for all features except use_ros_shim (needed for docs.rs) - if [ "$(basename $path)" = "rclrs" ]; then - cargo clippy --no-deps --all-targets -F default -- -D warnings - else - cargo clippy --no-deps --all-targets --all-features -- -D warnings + mapfile -t package_names <<< "$PACKAGE_NAMES" + mapfile -t package_paths < <(colcon list --packages-select "${package_names[@]}" | awk '$3 == "(ros.ament_cargo)" { print $2 }') + if (( ${#package_paths[@]} == 0 )); then + echo "::error::No target ros.ament_cargo packages found in the built workspace" + exit 1 fi - cd - - done + { + echo 'package_paths<> "$GITHUB_OUTPUT" - - name: Run cargo test on Rust packages + - name: Run clippy on Rust packages + env: + PACKAGE_PATHS: ${{ steps.list_workspace_rust_packages.outputs.package_paths }} run: | cd ${{ steps.build.outputs.ros-workspace-directory-name }} . install/setup.sh - for path in $(colcon list | awk '$3 == "(ament_cargo)" && $1 != "examples_rclrs_minimal_pub_sub" && $1 != "examples_rclrs_minimal_client_service" && $1 != "rust_pubsub" { print $2 }'); do - cd $path - echo "Running cargo test in $path" - # Run cargo test for all features except use_ros_shim (needed for docs.rs) - if [ "$(basename $path)" = "rclrs" ]; then - cargo test -F default,serde - else - cargo test --all-features - fi - cd - - done + while IFS= read -r path; do + [ -n "$path" ] || continue + cd "$path" + echo "Running clippy in $path" + # Run clippy for all features except use_ros_shim (needed for docs.rs) + if [ "$(basename "$path")" = "rclrs" ]; then + cargo clippy --no-deps --all-targets -F default -- -D warnings + else + cargo clippy --no-deps --all-targets --all-features -- -D warnings + fi + cd - > /dev/null + done <<< "$PACKAGE_PATHS" - name: Rustdoc check + env: + PACKAGE_PATHS: ${{ steps.list_workspace_rust_packages.outputs.package_paths }} run: | cd ${{ steps.build.outputs.ros-workspace-directory-name }} - . /opt/ros/${{ matrix.ros_distribution }}/setup.sh - for path in $(colcon list | awk '$3 == "(ament_cargo)" && $1 != "examples_rclrs_minimal_pub_sub" && $1 != "examples_rclrs_minimal_client_service" && $1 != "rust_pubsub" { print $2 }'); do - cd $path - echo "Running rustdoc check in $path" - cargo rustdoc -- -D warnings - cd - - done + . install/setup.sh + while IFS= read -r path; do + [ -n "$path" ] || continue + cd "$path" + echo "Running rustdoc check in $path" + cargo rustdoc -- -D warnings + cd - > /dev/null + done <<< "$PACKAGE_PATHS" diff --git a/.github/workflows/rust-stable.yml b/.github/workflows/rust-stable.yml index da0644703..e589e729b 100644 --- a/.github/workflows/rust-stable.yml +++ b/.github/workflows/rust-stable.yml @@ -14,6 +14,10 @@ on: env: CARGO_TERM_COLOR: always +defaults: + run: + shell: bash + jobs: build: strategy: @@ -82,6 +86,20 @@ jobs: sudo pip3 install git+https://github.com/colcon/colcon-cargo.git sudo pip3 install git+https://github.com/colcon/colcon-ros-cargo.git + - name: Search Rust packages in this repository + id: list_repo_rust_packages + run: | + mapfile -t package_paths < <(colcon list | awk '$3 == "(ros.ament_cargo)" { print $2 }') + if (( ${#package_paths[@]} == 0 )); then + echo "::error::No ros.ament_cargo packages found in the repository" + exit 1 + fi + { + echo 'package_paths<> "$GITHUB_OUTPUT" + # test_msgs recently added ament_mypy as a test dependency, but rosdep # may fail to install it when building from source on rolling - name: Install ament_cmake_mypy for rolling @@ -91,13 +109,16 @@ jobs: sudo apt-get install -y ros-rolling-ament-cmake-mypy - name: Check formatting of Rust packages + env: + PACKAGE_PATHS: ${{ steps.list_repo_rust_packages.outputs.package_paths }} run: | - for path in $(colcon list | awk '$3 == "(ament_cargo)" { print $2 }'); do - cd $path rustup toolchain install nightly - cargo +nightly fmt -- --check - cd - - done + while IFS= read -r path; do + [ -n "$path" ] || continue + cd "$path" + cargo +nightly fmt -- --check + cd - > /dev/null + done <<< "$PACKAGE_PATHS" - name: Build and test id: build @@ -107,45 +128,53 @@ jobs: target-ros2-distro: ${{ matrix.ros_distribution }} vcs-repo-file-url: ros2_rust_${{ matrix.ros_distribution }}.repos - - name: Run clippy on Rust packages + - name: Search target Rust packages in the built workspace + id: list_workspace_rust_packages + env: + PACKAGE_NAMES: ${{ steps.list_packages.outputs.package_list }} run: | cd ${{ steps.build.outputs.ros-workspace-directory-name }} - . /opt/ros/${{ matrix.ros_distribution }}/setup.sh - for path in $(colcon list | awk '$3 == "(ament_cargo)" { print $2 }'); do - cd $path - echo "Running clippy in $path" - # Run clippy for all features except use_ros_shim (needed for docs.rs) - if [ "$(basename $path)" = "rclrs" ]; then - cargo clippy --no-deps --all-targets -F default -- -D warnings - else - cargo clippy --no-deps --all-targets --all-features -- -D warnings + mapfile -t package_names <<< "$PACKAGE_NAMES" + mapfile -t package_paths < <(colcon list --packages-select "${package_names[@]}" | awk '$3 == "(ros.ament_cargo)" { print $2 }') + if (( ${#package_paths[@]} == 0 )); then + echo "::error::No target ros.ament_cargo packages found in the built workspace" + exit 1 fi - cd - - done + { + echo 'package_paths<> "$GITHUB_OUTPUT" - - name: Run cargo test on Rust packages + - name: Run clippy on Rust packages + env: + PACKAGE_PATHS: ${{ steps.list_workspace_rust_packages.outputs.package_paths }} run: | cd ${{ steps.build.outputs.ros-workspace-directory-name }} . install/setup.sh - for path in $(colcon list | awk '$3 == "(ament_cargo)" && $1 != "examples_rclrs_minimal_pub_sub" && $1 != "examples_rclrs_minimal_client_service" && $1 != "rust_pubsub" { print $2 }'); do - cd $path - echo "Running cargo test in $path" - # Run cargo test for all features except use_ros_shim (needed for docs.rs) - if [ "$(basename $path)" = "rclrs" ]; then - cargo test -F default,serde - else - cargo test --all-features - fi - cd - - done + while IFS= read -r path; do + [ -n "$path" ] || continue + cd "$path" + echo "Running clippy in $path" + # Run clippy for all features except use_ros_shim (needed for docs.rs) + if [ "$(basename "$path")" = "rclrs" ]; then + cargo clippy --no-deps --all-targets -F default -- -D warnings + else + cargo clippy --no-deps --all-targets --all-features -- -D warnings + fi + cd - > /dev/null + done <<< "$PACKAGE_PATHS" - name: Rustdoc check + env: + PACKAGE_PATHS: ${{ steps.list_workspace_rust_packages.outputs.package_paths }} run: | cd ${{ steps.build.outputs.ros-workspace-directory-name }} - . /opt/ros/${{ matrix.ros_distribution }}/setup.sh - for path in $(colcon list | awk '$3 == "(ament_cargo)" && $1 != "examples_rclrs_minimal_pub_sub" && $1 != "examples_rclrs_minimal_client_service" && $1 != "rust_pubsub" { print $2 }'); do - cd $path - echo "Running rustdoc check in $path" - cargo rustdoc -- -D warnings - cd - - done + . install/setup.sh + while IFS= read -r path; do + [ -n "$path" ] || continue + cd "$path" + echo "Running rustdoc check in $path" + cargo rustdoc -- -D warnings + cd - > /dev/null + done <<< "$PACKAGE_PATHS" diff --git a/.github/workflows/rust-win.yml b/.github/workflows/rust-win.yml index 13b91e5ab..8cf88f96f 100644 --- a/.github/workflows/rust-win.yml +++ b/.github/workflows/rust-win.yml @@ -75,10 +75,12 @@ jobs: run: | call C:\pixi_ws\ros2-windows\setup.bat cd C:\workspace + set /a rust_package_count=0 for /f "tokens=1,2,3" %%A in ('pixi run --manifest-path C:\pixi_ws\pixi.toml colcon list') do ( - if "%%C"=="(ament_cargo)" ( + if "%%C"=="(ros.ament_cargo)" ( if /I not "%%A"=="examples_rclrs_minimal_pub_sub" if /I not "%%A"=="examples_rclrs_minimal_client_service" if /I not "%%A"=="rust_pubsub" ( - cd %%B + set /a rust_package_count+=1 + cd /d %%B echo Running cargo test in %%B if /I "%%~nxB"=="rclrs" ( cargo test -F default @@ -87,9 +89,14 @@ jobs: ) else ( cargo test --all-features ) - cd .. + if errorlevel 1 exit /b 1 + cd /d C:\workspace ) ) ) + if %rust_package_count% EQU 0 ( + echo No target ros.ament_cargo packages found 1>&2 + exit /b 1 + ) shell: cmd working-directory: C:\workspace diff --git a/Cargo.toml b/Cargo.toml index afac85b2a..ac1d16890 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,4 +2,10 @@ members = [ "rclrs", ] +# action-ros-ci builds the colcon workspace as `ros_ws/` inside this checkout. Without this +# exclusion every package under it inherits this manifest as its workspace root and cargo +# refuses to build them ("current package believes it's in a workspace when it's not"). +exclude = [ + "ros_ws", +] resolver = "2" diff --git a/docs/building.md b/docs/building.md index 667b74588..b128d63a4 100644 --- a/docs/building.md +++ b/docs/building.md @@ -110,7 +110,7 @@ colcon list without errors and see a line like this in the output: ``` -rclrs src/ros2_rust/rclrs (ament_cargo) +rclrs src/ros2_rust/rclrs (ros.ament_cargo) ``` The build type `ament_cargo` means that the `colcon-ros-cargo` plugin works as expected. diff --git a/rclrs/src/action.rs b/rclrs/src/action.rs index 1daecd91f..62676fcae 100644 --- a/rclrs/src/action.rs +++ b/rclrs/src/action.rs @@ -103,12 +103,12 @@ impl CancelResponseCode { impl From for CancelResponseCode { fn from(value: i8) -> Self { - if 0 <= value && value <= 3 { + if (0..=3).contains(&value) { unsafe { // SAFETY: We have already ensured that the integer value is // within the acceptable range for the enum, so transmuting is // safe. - return std::mem::transmute(value); + return std::mem::transmute::(value); } } @@ -205,12 +205,12 @@ impl GoalStatusCode { impl From for GoalStatusCode { fn from(value: i8) -> Self { - if 0 <= value && value <= 6 { + if (0..=6).contains(&value) { unsafe { // SAFETY: We have already ensured that the integer value is // within the acceptable range for the enum, so transmuting is // safe. - return std::mem::transmute(value); + return std::mem::transmute::(value); } } @@ -579,7 +579,7 @@ mod tests { let mut current = 1; for _ in 0..goal_order { - if let Err(_) = sender.send(current) { + if sender.send(current).is_err() { // The action has been cancelled early, so just drop this thread. return; } diff --git a/rclrs/src/action/action_client.rs b/rclrs/src/action/action_client.rs index 0d2967dab..a26583186 100644 --- a/rclrs/src/action/action_client.rs +++ b/rclrs/src/action/action_client.rs @@ -41,7 +41,7 @@ pub use requested_goal_client::*; /// `ActionClientOptions` are used by [`Node::create_action_client`][1] to initialize an /// [`ActionClient`]. /// -/// [1]: crate::Node::create_action_client +/// [1]: crate::NodeState::create_action_client #[derive(Debug, Clone)] #[non_exhaustive] pub struct ActionClientOptions<'a> { @@ -147,7 +147,7 @@ impl<'a> From<&'_ ActionClientOptions<'a>> for rcl_action_client_options_t { /// Receiving feedback and results requires the node's executor to [spin][2]. /// /// [1]: crate::NodeState::create_action_client -/// [2]: crate::spin +/// [2]: crate::Executor::spin pub type ActionClient = Arc>; /// The inner state of an [`ActionClient`]. @@ -348,7 +348,7 @@ impl ActionClientState { let handle = Arc::new(ActionClientHandle { rcl_action_client: Mutex::new(rcl_action_client), - node_handle: Arc::clone(&node.handle()), + node_handle: Arc::clone(node.handle()), }); let board = Arc::new(ActionClientGoalBoard { @@ -384,7 +384,7 @@ struct ActionClientGoalBoard { status_senders: Mutex>>>, status_posters: Mutex>>, cancel_response_senders: Mutex>, - result_senders: Mutex>>, + result_senders: Mutex>>, handle: Arc, client: Mutex>>, /// Ensure the parent node remains alive as long as the subscription is held. @@ -393,6 +393,8 @@ struct ActionClientGoalBoard { node: Node, } +type ActionResultSender = Sender<(GoalStatusCode, ::Result)>; + enum CancelResponseSender { /// Used when only a single goal is being cancelled SingleGoalCancel(Sender), @@ -580,7 +582,7 @@ impl ActionClientGoalBoard { ) -> Result, RclrsError> { let goal_id: GoalUuid = uuid::Uuid::new_v4().as_bytes().into(); let goal_rmw = ::into_rmw_message(Cow::Owned(goal)).into_owned(); - let request = A::create_goal_request(&*goal_id, goal_rmw); + let request = A::create_goal_request(&goal_id, goal_rmw); let mut seq: i64 = 0; unsafe { @@ -730,7 +732,7 @@ impl ActionClientGoalBoard { client: ActionClient, goal_id: GoalUuid, ) -> Result, RclrsError> { - let request_rmw = A::create_result_request(&*goal_id); + let request_rmw = A::create_result_request(&goal_id); let mut seq: i64 = 0; unsafe { let handle = self.handle.lock(); diff --git a/rclrs/src/action/action_client/goal_client.rs b/rclrs/src/action/action_client/goal_client.rs index f06f9d051..bcffd13c3 100644 --- a/rclrs/src/action/action_client/goal_client.rs +++ b/rclrs/src/action/action_client/goal_client.rs @@ -66,7 +66,7 @@ impl GoalClient { let initial_value = (*status.borrow_and_update()).clone(); yield GoalEvent::Status(initial_value); - while let Ok(_) = status.changed().await { + while status.changed().await.is_ok() { let value = (*status.borrow_and_update()).clone(); yield GoalEvent::Status(value); } diff --git a/rclrs/src/action/action_goal_receiver.rs b/rclrs/src/action/action_goal_receiver.rs index cc30f107b..fc056e017 100644 --- a/rclrs/src/action/action_goal_receiver.rs +++ b/rclrs/src/action/action_goal_receiver.rs @@ -43,7 +43,7 @@ impl ActionGoalReceiver { /// /// It is unusual to switch from an action goal receiver to an action server, /// so consider carefully whether this is what you really want to do. Usually - /// an action server is created by [`NodeState::create_action_server`]. + /// an action server is created by [`crate::NodeState::create_action_server`]. #[must_use] pub fn into_action_server( self, diff --git a/rclrs/src/action/action_server.rs b/rclrs/src/action/action_server.rs index 9c3747552..a598e0a24 100644 --- a/rclrs/src/action/action_server.rs +++ b/rclrs/src/action/action_server.rs @@ -29,7 +29,7 @@ mod cancellation_state; use cancellation_state::*; mod cancelling_goal; -use cancelling_goal::*; +pub use cancelling_goal::*; mod executing_goal; pub use executing_goal::*; @@ -232,7 +232,7 @@ impl ActionServerState { /// /// It is unusual to switch from an action server to an action goal receiver, /// so consider carefully whether this is what you really want to do. Usually - /// an action goal receiver is created by [`NodeState::create_action_goal_receiver`] + /// an action goal receiver is created by [`crate::NodeState::create_goal_receiver`] /// when the action server is being initialized. #[must_use] pub fn into_goal_receiver(self) -> ActionGoalReceiver { @@ -318,8 +318,8 @@ impl ActionServerState { let handle = Arc::new(ActionServerHandle { rcl_action_server: Mutex::new(rcl_action_server), - node_handle: Arc::clone(&node.handle()), - clock: clock, + node_handle: Arc::clone(node.handle()), + clock, goals: Default::default(), }); @@ -373,7 +373,7 @@ impl ActionServerState { // replaced by the callback. while let Ok(requested_goal) = receiver.try_recv() { let f = (*callback)(requested_goal); - let _ = self.board.node.commands().run(f); + drop(self.board.node.commands().run(f)); } *dispatch = GoalDispatch::Callback(callback); @@ -422,7 +422,7 @@ impl ActionServerGoalBoard { match &mut *self.dispatch.lock()? { GoalDispatch::Callback(callback) => { let f = callback(requested_goal); - let _ = self.node.commands().run(f); + drop(self.node.commands().run(f)); } GoalDispatch::Sender(sender) => { // A send error means the user has dropped their receiver, so @@ -509,8 +509,10 @@ impl ActionServerGoalBoard { // We have a special response type for this specific request. // Either the goal has been terminated or we don't know about // it at all. - let mut response = CancelGoal_Response::default(); - response.return_code = response_code as i8; + let response = CancelGoal_Response { + return_code: response_code as i8, + ..Default::default() + }; let mut response_rmw = CancelGoal_Response::into_rmw_message(Cow::Owned(response)).into_owned(); return unsafe { @@ -539,15 +541,13 @@ impl ActionServerGoalBoard { for goal in waiting_for { if let Some(live_goal) = live_goals.get(&goal).and_then(|goal| goal.upgrade()) { live_goal.request_cancellation(cancellation_request.clone()); - } else { - if let Some(handle) = self.handle.goals.lock()?.get(&goal) { - // If the goal is already cancelled then we will say that we - // accept the cancellation request. There is no need to - // check for the cancelling state since non-live goals must - // be in a terminal state. - if handle.is_cancelled() { - cancellation_request.accept(goal); - } + } else if let Some(handle) = self.handle.goals.lock()?.get(&goal) { + // If the goal is already cancelled then we will say that we + // accept the cancellation request. There is no need to + // check for the cancelling state since non-live goals must + // be in a terminal state. + if handle.is_cancelled() { + cancellation_request.accept(goal); } } } diff --git a/rclrs/src/action/action_server/accepted_goal.rs b/rclrs/src/action/action_server/accepted_goal.rs index d24e3878f..cca313d7d 100644 --- a/rclrs/src/action/action_server/accepted_goal.rs +++ b/rclrs/src/action/action_server/accepted_goal.rs @@ -65,6 +65,8 @@ impl AcceptedGoal { /// until the goal reaches a terminal state. // // TODO(@mxgrey): Add a doctest and example for this. + // The unit error is intentional: cancellation is the only error state and carries no data. + #[allow(clippy::result_unit_err)] pub async fn unless_cancel_requested(&self, f: F) -> Result { self.live.cancellation().unless_cancel_requested(f).await } diff --git a/rclrs/src/action/action_server/action_server_goal_handle.rs b/rclrs/src/action/action_server/action_server_goal_handle.rs index dc133be6b..e7614e6e6 100644 --- a/rclrs/src/action/action_server/action_server_goal_handle.rs +++ b/rclrs/src/action/action_server/action_server_goal_handle.rs @@ -148,8 +148,8 @@ impl ResponseState { let action_server = action_server_handle.lock(); // Respond to all queued requests. - for mut result_request in result_requests { - Self::send_result(&*action_server, &mut result_request, &mut result)?; + for result_request in result_requests { + Self::send_result(&action_server, result_request, &mut result)?; } } @@ -168,7 +168,7 @@ impl ResponseState { } Self::Available(result) => { let action_server = action_server_handle.lock(); - Self::send_result(&*action_server, &mut result_request, result)?; + Self::send_result(&action_server, &mut result_request, result)?; } } Ok(()) diff --git a/rclrs/src/action/action_server/cancellation_state.rs b/rclrs/src/action/action_server/cancellation_state.rs index e34c043c9..8afac5d4f 100644 --- a/rclrs/src/action/action_server/cancellation_state.rs +++ b/rclrs/src/action/action_server/cancellation_state.rs @@ -93,7 +93,7 @@ impl CancellationState { *mode = CancellationMode::None; // We do not need to worry about errors from sending this state // since it is okay for the receiver to be dropped. - let _ = self.change_cancel_requested_status(false); + self.change_cancel_requested_status(false); } CancellationMode::None => { // Do nothing @@ -120,7 +120,7 @@ impl CancellationState { // a true value in the cancel requested channel. We can ignore // errors from this because it is okay for the receiver to be // dropped. - let _ = self.change_cancel_requested_status(true); + self.change_cancel_requested_status(true); } CancellationMode::None => { // Skip straight to cancellation mode since the user has accepted @@ -128,7 +128,7 @@ impl CancellationState { *mode = CancellationMode::Cancelling; // Make sure the cancellation is signalled. We can ignore errors // from this because it is okay for the receiver to be dropped. - let _ = self.change_cancel_requested_status(true); + self.change_cancel_requested_status(true); } CancellationMode::Cancelling => { // Do nothing @@ -261,13 +261,16 @@ impl CancellationRequestInner { self.response_sent = true; - let mut response = CancelGoal_Response::default(); - response.goals_canceling = self.accepted.drain(..).collect(); - if response.goals_canceling.is_empty() { - response.return_code = CancelResponseCode::Reject as i8; + let goals_canceling = std::mem::take(&mut self.accepted); + let return_code = if goals_canceling.is_empty() { + CancelResponseCode::Reject as i8 } else { - response.return_code = CancelResponseCode::Accept as i8; - } + CancelResponseCode::Accept as i8 + }; + let response = CancelGoal_Response { + return_code, + goals_canceling, + }; let mut response_rmw = CancelGoal_Response::into_rmw_message(Cow::Owned(response)).into_owned(); diff --git a/rclrs/src/action/action_server/executing_goal.rs b/rclrs/src/action/action_server/executing_goal.rs index 670e5f749..2cc605bd4 100644 --- a/rclrs/src/action/action_server/executing_goal.rs +++ b/rclrs/src/action/action_server/executing_goal.rs @@ -68,6 +68,8 @@ impl ExecutingGoal { /// until the goal reaches a terminal state. // // TODO(@mxgrey): Add a doctest and example for this. + // The unit error is intentional: cancellation is the only error state and carries no data. + #[allow(clippy::result_unit_err)] pub async fn unless_cancel_requested(&self, f: F) -> Result { self.live.cancellation().unless_cancel_requested(f).await } diff --git a/rclrs/src/action/action_server/live_action_server_goal.rs b/rclrs/src/action/action_server/live_action_server_goal.rs index a9e236d98..bdced3d43 100644 --- a/rclrs/src/action/action_server/live_action_server_goal.rs +++ b/rclrs/src/action/action_server/live_action_server_goal.rs @@ -166,7 +166,7 @@ impl LiveActionServerGoal { let feedback_rmw = <::Feedback as Message>::into_rmw_message(Cow::Owned(feedback)); let mut feedback_msg = - ::create_feedback_message(&*self.goal_id(), feedback_rmw.into_owned()); + ::create_feedback_message(self.goal_id(), feedback_rmw.into_owned()); let r = unsafe { // SAFETY: The action server is locked through the handle, meaning that no other // non-thread-safe functions can be called on it at the same time. The feedback_msg is diff --git a/rclrs/src/action/action_server/requested_goal.rs b/rclrs/src/action/action_server/requested_goal.rs index c9f6d1c38..1979c58ff 100644 --- a/rclrs/src/action/action_server/requested_goal.rs +++ b/rclrs/src/action/action_server/requested_goal.rs @@ -36,7 +36,6 @@ impl RequestedGoal { /// An alternative to [`RequestedGoal::accept`] which does not panic in the /// event of an error. - #[must_use] pub fn try_accept(mut self) -> Result, RclrsError> { let handle = { let mut goal_info = unsafe { diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs index a229bac13..e7fd256b0 100644 --- a/rclrs/src/client.rs +++ b/rclrs/src/client.rs @@ -353,7 +353,7 @@ where let commands = node.commands().async_worker_commands(); let handle = Arc::new(ClientHandle { rcl_client: Mutex::new(rcl_client), - node: Arc::clone(&node), + node: Arc::clone(node), }); let board = Arc::new(Mutex::new(ClientRequestBoard::new())); @@ -363,7 +363,7 @@ where handle: Arc::clone(&handle), board: Arc::clone(&board), }), - Some(Arc::clone(&commands.get_guard_condition())), + Some(Arc::clone(commands.get_guard_condition())), ); commands.add_to_wait_set(waitable); diff --git a/rclrs/src/drop_guard.rs b/rclrs/src/drop_guard.rs index bd57d7074..a976943dd 100644 --- a/rclrs/src/drop_guard.rs +++ b/rclrs/src/drop_guard.rs @@ -26,13 +26,13 @@ impl Deref for DropGuard { type Target = T; fn deref(&self) -> &T { - &*self.value + &self.value } } impl DerefMut for DropGuard { fn deref_mut(&mut self) -> &mut T { - &mut *self.value + &mut self.value } } diff --git a/rclrs/src/dynamic_message.rs b/rclrs/src/dynamic_message.rs index 956b88bc6..e9b595992 100644 --- a/rclrs/src/dynamic_message.rs +++ b/rclrs/src/dynamic_message.rs @@ -74,7 +74,7 @@ impl DynamicMessageLibraryCache { pub fn get_dynamic_message_package_cache() -> &'static Mutex { static DYNAMIC_MESSAGE_PACKAGE_CACHE: OnceLock> = OnceLock::new(); - DYNAMIC_MESSAGE_PACKAGE_CACHE.get_or_init(|| Default::default()) + DYNAMIC_MESSAGE_PACKAGE_CACHE.get_or_init(Default::default) } /// A parsed/validated message type name of the form `/msg/`. @@ -147,7 +147,7 @@ fn get_type_support_library( #[cfg(all(not(target_os = "windows"), not(target_os = "macos")))] let library_path = prefix.join("lib").join(format!( "lib{}__{}.so", - &package_name, type_support_identifier + package_name, type_support_identifier )); Ok({ // SAFETY: This function is unsafe because it may execute initialization/termination routines @@ -169,7 +169,7 @@ unsafe fn get_type_support_handle( ) -> Result<*const rosidl_message_type_support_t, DynamicMessageError> { let symbol_name = format!( "{}__get_message_type_support_handle__{}__msg__{}", - type_support_identifier, &message_type.package_name, &message_type.type_name + type_support_identifier, message_type.package_name, message_type.type_name ); // SAFETY: We know that the symbol has this type, from the safety requirement of this function. @@ -225,7 +225,7 @@ impl TryFrom<&str> for MessageTypeName { impl Display for MessageTypeName { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}/msg/{}", &self.package_name, &self.type_name) + write!(f, "{}/msg/{}", self.package_name, self.type_name) } } @@ -260,7 +260,7 @@ impl DynamicMessageMetadata { .get_or_load(&message_type.package_name)?; let type_support_ptr = unsafe { get_type_support_handle( - &*library, + &library, INTROSPECTION_TYPE_SUPPORT_IDENTIFIER, &message_type, )? @@ -459,6 +459,8 @@ impl DynamicMessage { /// /// If the RMW-native message type does not match the underlying message type of this `DynamicMessage`, /// it is not converted but instead returned unchanged. + // Returning `Self` lets callers recover the original dynamic message without allocation. + #[allow(clippy::result_large_err)] pub fn convert_into_rmw_message(mut self) -> Result where T: RmwMessage, diff --git a/rclrs/src/dynamic_message/dynamic_subscription.rs b/rclrs/src/dynamic_message/dynamic_subscription.rs index ddc3de290..a7f01f123 100644 --- a/rclrs/src/dynamic_message/dynamic_subscription.rs +++ b/rclrs/src/dynamic_message/dynamic_subscription.rs @@ -56,15 +56,18 @@ pub(crate) struct NodeDynamicSubscriptionCallback( Box, ); +type AsyncDynamicSubscriptionCallback = + dyn FnMut(DynamicMessage, MessageInfo) -> BoxFuture<'static, ()> + Send + Sync; +type WorkerDynamicCallback = + dyn FnMut(&mut Payload, DynamicMessage, MessageInfo) + Send + Sync; + impl NodeDynamicSubscriptionCallback { pub(crate) fn new(f: impl Fn(DynamicMessage, MessageInfo) + Send + Sync + 'static) -> Self { NodeDynamicSubscriptionCallback(Box::new(f)) } } -pub(crate) struct NodeAsyncDynamicSubscriptionCallback( - Box BoxFuture<'static, ()> + Send + Sync>, -); +pub(crate) struct NodeAsyncDynamicSubscriptionCallback(Box); impl NodeAsyncDynamicSubscriptionCallback { pub(crate) fn new( @@ -74,9 +77,7 @@ impl NodeAsyncDynamicSubscriptionCallback { } } -pub(crate) struct WorkerDynamicSubscriptionCallback( - Box, -); +pub(crate) struct WorkerDynamicSubscriptionCallback(Box>); impl WorkerDynamicSubscriptionCallback { pub(crate) fn new( @@ -94,8 +95,7 @@ impl Deref for NodeDynamicSubscriptionCallback { } impl Deref for NodeAsyncDynamicSubscriptionCallback { - type Target = - Box BoxFuture<'static, ()> + Send + Sync>; + type Target = Box; fn deref(&self) -> &Self::Target { &self.0 } @@ -226,14 +226,14 @@ impl RclPrimitive for DynamicSubscriptionExecutable { self.callback .lock() .unwrap() - .execute(&self, payload, &self.commands) + .execute(self, payload, &self.commands) } fn kind(&self) -> RclPrimitiveKind { RclPrimitiveKind::Subscription } - fn handle(&self) -> RclPrimitiveHandle { + fn handle(&self) -> RclPrimitiveHandle<'_> { RclPrimitiveHandle::Subscription(self.handle.lock()) } } diff --git a/rclrs/src/dynamic_message/error.rs b/rclrs/src/dynamic_message/error.rs index 777305b4c..55ec371a6 100644 --- a/rclrs/src/dynamic_message/error.rs +++ b/rclrs/src/dynamic_message/error.rs @@ -58,7 +58,7 @@ impl PartialEq for DynamicMessageError { return false; } // TODO(luca) this is not very efficient, revisit - return self.to_string() == other.to_string(); + self.to_string() == other.to_string() } } diff --git a/rclrs/src/dynamic_message/field_access.rs b/rclrs/src/dynamic_message/field_access.rs index bcddc0b1a..5f0575261 100644 --- a/rclrs/src/dynamic_message/field_access.rs +++ b/rclrs/src/dynamic_message/field_access.rs @@ -238,6 +238,8 @@ macro_rules! define_value_types { // * This function does not transmute & to &mut // * This is only used for primitive values and rosidl_runtime_rs types marked as repr(C), // so there is no risk of reinterpreting as a type with undefined layout. + // The macro needs an explicit lifetime token to select `&'a` or `&'a mut`. + #[allow(clippy::needless_lifetimes)] unsafe fn reinterpret<'a, T>(bytes: make_ref!('a, [u8])) -> make_ref!('a, T) { check::(bytes); $select!( @@ -253,6 +255,8 @@ macro_rules! define_value_types { // // std::slice::from_raw_parts is the correct way to transmute a slice. // We can't rely on the internal representation of slices (or other stdlib types). + // The macro needs an explicit lifetime token to select `&'a` or `&'a mut`. + #[allow(clippy::needless_lifetimes)] unsafe fn reinterpret_array<'a, T>(bytes: make_ref!('a, [u8]), array_size: usize) -> make_ref!('a, [T]) { check::(bytes); $select!( diff --git a/rclrs/src/dynamic_message/field_access/dynamic_bounded_string.rs b/rclrs/src/dynamic_message/field_access/dynamic_bounded_string.rs index 9e011168b..330ed3b34 100644 --- a/rclrs/src/dynamic_message/field_access/dynamic_bounded_string.rs +++ b/rclrs/src/dynamic_message/field_access/dynamic_bounded_string.rs @@ -63,14 +63,14 @@ pub struct DynamicBoundedWStringMut<'msg> { // ========================= impl for DynamicBounded(W)String ========================= -impl<'msg> Deref for DynamicBoundedString<'msg> { +impl Deref for DynamicBoundedString<'_> { type Target = rosidl_runtime_rs::String; fn deref(&self) -> &Self::Target { self.inner } } -impl<'msg> Display for DynamicBoundedString<'msg> { +impl Display for DynamicBoundedString<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { self.inner.fmt(f) } @@ -92,28 +92,28 @@ unsafe impl<'msg> Proxy<'msg> for DynamicBoundedString<'msg> { } } -impl<'msg> DynamicBoundedString<'msg> { +impl DynamicBoundedString<'_> { /// Returns the maximum length of this string. pub fn upper_bound(&self) -> NonZeroUsize { self.upper_bound } } -impl<'msg> DynamicBoundedWString<'msg> { +impl DynamicBoundedWString<'_> { /// Returns the maximum length of this string. pub fn upper_bound(&self) -> NonZeroUsize { self.upper_bound } } -impl<'msg> Deref for DynamicBoundedWString<'msg> { +impl Deref for DynamicBoundedWString<'_> { type Target = rosidl_runtime_rs::WString; fn deref(&self) -> &Self::Target { self.inner } } -impl<'msg> Display for DynamicBoundedWString<'msg> { +impl Display for DynamicBoundedWString<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { self.inner.fmt(f) } @@ -137,20 +137,20 @@ unsafe impl<'msg> Proxy<'msg> for DynamicBoundedWString<'msg> { // ========================= impl for DynamicBounded(W)StringMut ========================= -impl<'msg> AsMut<[std::os::raw::c_char]> for DynamicBoundedStringMut<'msg> { +impl AsMut<[std::os::raw::c_char]> for DynamicBoundedStringMut<'_> { fn as_mut(&mut self) -> &mut [std::os::raw::c_char] { self.inner.deref_mut() } } -impl<'msg> Deref for DynamicBoundedStringMut<'msg> { +impl Deref for DynamicBoundedStringMut<'_> { type Target = rosidl_runtime_rs::String; fn deref(&self) -> &Self::Target { self.inner } } -impl<'msg> Display for DynamicBoundedStringMut<'msg> { +impl Display for DynamicBoundedStringMut<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { self.inner.fmt(f) } @@ -176,7 +176,7 @@ unsafe impl<'msg> ProxyMut<'msg> for DynamicBoundedStringMut<'msg> { } } -impl<'msg> DynamicBoundedStringMut<'msg> { +impl DynamicBoundedStringMut<'_> { /// Returns the maximum length of this string. pub fn upper_bound(&self) -> NonZeroUsize { self.upper_bound @@ -197,20 +197,20 @@ impl<'msg> DynamicBoundedStringMut<'msg> { } } -impl<'msg> AsMut<[std::os::raw::c_ushort]> for DynamicBoundedWStringMut<'msg> { +impl AsMut<[std::os::raw::c_ushort]> for DynamicBoundedWStringMut<'_> { fn as_mut(&mut self) -> &mut [std::os::raw::c_ushort] { self.inner.deref_mut() } } -impl<'msg> Deref for DynamicBoundedWStringMut<'msg> { +impl Deref for DynamicBoundedWStringMut<'_> { type Target = rosidl_runtime_rs::WString; fn deref(&self) -> &Self::Target { self.inner } } -impl<'msg> Display for DynamicBoundedWStringMut<'msg> { +impl Display for DynamicBoundedWStringMut<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { self.inner.fmt(f) } @@ -236,7 +236,7 @@ unsafe impl<'msg> ProxyMut<'msg> for DynamicBoundedWStringMut<'msg> { } } -impl<'msg> DynamicBoundedWStringMut<'msg> { +impl DynamicBoundedWStringMut<'_> { /// Returns the maximum length of this string. pub fn upper_bound(&self) -> NonZeroUsize { self.upper_bound diff --git a/rclrs/src/dynamic_message/field_access/dynamic_message_view.rs b/rclrs/src/dynamic_message/field_access/dynamic_message_view.rs index 2c1153756..add92a03b 100644 --- a/rclrs/src/dynamic_message/field_access/dynamic_message_view.rs +++ b/rclrs/src/dynamic_message/field_access/dynamic_message_view.rs @@ -28,7 +28,7 @@ pub struct DynamicMessageViewMut<'msg> { // ========================= impl for DynamicMessageView ========================= -impl<'msg> Debug for DynamicMessageView<'msg> { +impl Debug for DynamicMessageView<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { let mut struct_ = f.debug_struct(&self.structure().type_name); for field in &self.structure().fields { @@ -39,7 +39,7 @@ impl<'msg> Debug for DynamicMessageView<'msg> { } } -impl<'msg> Deref for DynamicMessageView<'msg> { +impl Deref for DynamicMessageView<'_> { type Target = MessageStructure; fn deref(&self) -> &Self::Target { self.structure @@ -90,7 +90,7 @@ impl<'msg> DynamicMessageView<'msg> { // ========================= impl for DynamicMessageViewMut ========================= -impl<'msg> Debug for DynamicMessageViewMut<'msg> { +impl Debug for DynamicMessageViewMut<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { DynamicMessageView { structure: self.structure, @@ -100,7 +100,7 @@ impl<'msg> Debug for DynamicMessageViewMut<'msg> { } } -impl<'msg> Deref for DynamicMessageViewMut<'msg> { +impl Deref for DynamicMessageViewMut<'_> { type Target = MessageStructure; fn deref(&self) -> &Self::Target { self.structure diff --git a/rclrs/src/dynamic_message/field_access/dynamic_sequence.rs b/rclrs/src/dynamic_message/field_access/dynamic_sequence.rs index e768f1fc6..7809ef6c2 100644 --- a/rclrs/src/dynamic_message/field_access/dynamic_sequence.rs +++ b/rclrs/src/dynamic_message/field_access/dynamic_sequence.rs @@ -135,7 +135,7 @@ where } } -impl<'msg, T> InnerSequence for &'msg mut Sequence +impl InnerSequence for &mut Sequence where T: PartialEq + SequenceAlloc, { @@ -277,7 +277,7 @@ where { type Target = [T]; fn deref(&self) -> &Self::Target { - &*self.elements + &self.elements } } @@ -298,22 +298,22 @@ where /// /// [1]: rosidl_runtime_rs::Sequence::as_slice pub fn as_slice(&self) -> &[T] { - &*self.elements + &self.elements } } // ------------------------- impl for DynamicBoundedSequence ------------------------- -impl<'msg, T> BooSlice<'msg, T> { +impl BooSlice<'_, T> { fn as_slice(&self) -> &[T] { match self { BooSlice::Borrowed(slice) => slice, - BooSlice::Owned(boxed_slice) => &**boxed_slice, + BooSlice::Owned(boxed_slice) => boxed_slice, } } } -impl<'msg, T> Debug for DynamicBoundedSequence<'msg, T> +impl Debug for DynamicBoundedSequence<'_, T> where T: Debug, { @@ -322,7 +322,7 @@ where } } -impl<'msg, T> Deref for DynamicBoundedSequence<'msg, T> { +impl Deref for DynamicBoundedSequence<'_, T> { type Target = [T]; fn deref(&self) -> &Self::Target { self.boo.as_slice() @@ -360,7 +360,7 @@ where } } -impl<'msg, T: SequenceAlloc> DynamicBoundedSequence<'msg, T> { +impl DynamicBoundedSequence<'_, T> { /// See [`Sequence::as_slice()`][1]. /// /// [1]: rosidl_runtime_rs::Sequence::as_slice @@ -386,13 +386,22 @@ pub(super) type ResizeFunction = /// An unbounded sequence. /// /// This type dereferences to `&[T]` and `&mut [T]`. -#[derive(PartialEq)] pub struct DynamicSequenceMut<'msg, T: DynamicSequenceElementMut<'msg>> { // This is either &mut Sequence or ProxySequence sequence: T::InnerSequence, resize_function: ResizeFunction, } +impl<'msg, T> PartialEq for DynamicSequenceMut<'msg, T> +where + T: DynamicSequenceElementMut<'msg>, +{ + fn eq(&self, other: &Self) -> bool { + self.sequence == other.sequence + && std::ptr::fn_addr_eq(self.resize_function, other.resize_function) + } +} + /// A bounded sequence whose upper bound is only known at runtime. /// /// This is conceptually the same as a [`BoundedSequence`][1]. diff --git a/rclrs/src/dynamic_message/message_structure.rs b/rclrs/src/dynamic_message/message_structure.rs index 5ca403457..9dfafd7ab 100644 --- a/rclrs/src/dynamic_message/message_structure.rs +++ b/rclrs/src/dynamic_message/message_structure.rs @@ -1,4 +1,8 @@ -use std::{ffi::CStr, mem, num::NonZeroUsize, os::raw::c_char, os::raw::c_void}; +use std::{ + ffi::CStr, + num::NonZeroUsize, + os::raw::{c_char, c_void}, +}; use super::TypeErasedSequence; use crate::rcl_bindings::{ @@ -47,7 +51,7 @@ pub enum BaseType { /// That is, the base types exist as single values, arrays, bounded sequences and unbounded sequences. /// /// [1]: crate::dynamic_message::DynamicMessage -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug)] pub struct MessageFieldInfo { /// The field name. pub name: String, @@ -63,6 +67,29 @@ pub struct MessageFieldInfo { type ResizeFunction = Option bool>; +impl PartialEq for MessageFieldInfo { + fn eq(&self, other: &Self) -> bool { + let resize_functions_equal = match (self.resize_function, other.resize_function) { + (Some(left), Some(right)) => std::ptr::fn_addr_eq(left, right), + (None, None) => true, + _ => false, + }; + + self.name == other.name + && self.base_type == other.base_type + && self.value_kind == other.value_kind + && self.string_upper_bound == other.string_upper_bound + && resize_functions_equal + && self.offset == other.offset + } +} + +impl Eq for MessageFieldInfo {} + +#[cfg(any( + test, + not(any(ros_distro = "humble", ros_distro = "jazzy", ros_distro = "kilted")) +))] #[repr(C)] #[derive(Clone, Copy)] struct MessageMemberPrefix { @@ -84,6 +111,10 @@ struct MessageMemberPrefix { resize_function: *const c_void, } +#[cfg(any( + test, + not(any(ros_distro = "humble", ros_distro = "jazzy", ros_distro = "kilted")) +))] impl MessageMemberPrefix { fn is_valid_for_message(&self, message_size: usize) -> bool { use rosidl_typesupport_introspection_c_field_types::*; @@ -247,11 +278,15 @@ impl MessageFieldInfo { ) } + #[cfg(any( + test, + not(any(ros_distro = "humble", ros_distro = "jazzy", ros_distro = "kilted")) + ))] unsafe fn from_prefix(rosidl_message_member: &MessageMemberPrefix) -> Self { let resize_function: ResizeFunction = if rosidl_message_member.resize_function.is_null() { None } else { - Some(mem::transmute::< + Some(std::mem::transmute::< *const c_void, unsafe extern "C" fn(*mut c_void, usize) -> bool, >(rosidl_message_member.resize_function)) @@ -270,6 +305,7 @@ impl MessageFieldInfo { ) } + #[allow(clippy::too_many_arguments)] unsafe fn from_parts( name: *const c_char, type_id: u8, @@ -326,9 +362,13 @@ impl MessageFieldInfo { // ========================= impl for MessageStructure ========================= impl MessageStructure { + #[cfg(any( + test, + not(any(ros_distro = "humble", ros_distro = "jazzy", ros_distro = "kilted")) + ))] unsafe fn message_member_stride(message_members: &rosidl_message_members_t) -> usize { - let current_stride = mem::size_of::(); - let prefix_stride = mem::size_of::(); + let current_stride = std::mem::size_of::(); + let prefix_stride = std::mem::size_of::(); if message_members.member_count_ < 2 { return current_stride; } diff --git a/rclrs/src/error.rs b/rclrs/src/error.rs index 6f66086a7..b1c4b505b 100644 --- a/rclrs/src/error.rs +++ b/rclrs/src/error.rs @@ -649,7 +649,7 @@ impl TakeFailedAsNone for Result { return Ok(None); } - return Err(err); + Err(err) } } } diff --git a/rclrs/src/executor/basic_executor.rs b/rclrs/src/executor/basic_executor.rs index 4df0db9a1..c17312d73 100644 --- a/rclrs/src/executor/basic_executor.rs +++ b/rclrs/src/executor/basic_executor.rs @@ -24,8 +24,7 @@ use crate::{ Waitable, WeakActivityListener, WorkerChannel, }; -static FAILED_TO_SEND_WORKER: &'static str = - "Failed to send the new runner. This should never happen. \ +static FAILED_TO_SEND_WORKER: &str = "Failed to send the new runner. This should never happen. \ Please report this to the rclrs maintainers with a minimal reproducible example."; /// The implementation of this runtime is based off of the async Rust reference book: @@ -76,11 +75,7 @@ impl AllGuardConditions { fn push(&self, guard_condition: Weak) { let mut inner = self.inner.lock().unwrap(); - if inner - .iter() - .find(|other| guard_condition.ptr_eq(other)) - .is_some() - { + if inner.iter().any(|other| guard_condition.ptr_eq(other)) { // This guard condition is already known return; } @@ -351,7 +346,7 @@ impl TaskSender { task_sender: self.task_sender.clone(), }); - if let Err(_) = self.task_sender.send(task) { + if self.task_sender.send(task).is_err() { // This is a debug log because it is normal for this to happen while // an executor is winding down. log_debug!( diff --git a/rclrs/src/lib.rs b/rclrs/src/lib.rs index d16bb56d8..f5be6b26b 100644 --- a/rclrs/src/lib.rs +++ b/rclrs/src/lib.rs @@ -235,7 +235,7 @@ pub use timer::*; pub use wait_set::*; pub use worker::*; -pub use rosidl_runtime_rs; pub use rosidl_runtime_rs::{ - Action as ActionIDL, Message as MessageIDL, RmwMessage as RmwMessageIDL, Service as ServiceIDL, + self, Action as ActionIDL, Message as MessageIDL, RmwMessage as RmwMessageIDL, + Service as ServiceIDL, }; diff --git a/rclrs/src/logging/log_params.rs b/rclrs/src/logging/log_params.rs index 99eed7cab..9081966a7 100644 --- a/rclrs/src/logging/log_params.rs +++ b/rclrs/src/logging/log_params.rs @@ -182,7 +182,7 @@ pub enum LoggerName<'a> { // of RCUTILS_LOG_SEVERITY to just LogSeverity so it's more idiomatic and then // export it from the rclrs module. #[doc(hidden)] -#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] pub enum LogSeverity { /// Use the severity level of the parent logger (or the root logger if the /// current logger has no parent) @@ -191,6 +191,7 @@ pub enum LogSeverity { Debug, /// For messages that provide useful information about the state of the /// application. + #[default] Info, /// For messages that indicate something unusual or unintended might have happened. Warn, @@ -236,16 +237,11 @@ impl LogSeverity { } } -impl Default for LogSeverity { - fn default() -> Self { - Self::Info - } -} - /// Specify when a log message should be published -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Default)] pub enum LogOccurrence { /// Every message will be published if all other conditions are met + #[default] All, /// The message will only be published on the first occurrence (Note: no other conditions apply) Once, @@ -266,12 +262,6 @@ pub enum ThrottleClock<'a> { Clock(&'a Clock), } -impl Default for LogOccurrence { - fn default() -> Self { - Self::All - } -} - // Anything that we can borrow a string from can be used as if it's a logger and // turned into LogParams impl<'a, T: Borrow> ToLogParams<'a> for &'a T { diff --git a/rclrs/src/logging/logger.rs b/rclrs/src/logging/logger.rs index c6193ad10..9ab0f76d4 100644 --- a/rclrs/src/logging/logger.rs +++ b/rclrs/src/logging/logger.rs @@ -58,7 +58,7 @@ impl Logger { if self.name.is_empty() { Self::new(child_name) } else { - Self::new(format!("{}.{}", &self.name, child_name.borrow())) + Self::new(format!("{}.{}", self.name, child_name.borrow())) } } diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs index 8fdccad79..973c4e255 100644 --- a/rclrs/src/node.rs +++ b/rclrs/src/node.rs @@ -323,7 +323,7 @@ impl NodeState { /// /// In the above example, `addition_client` and `result_publisher` can be /// created later inside a subscription or service callback using the [`Node`]. - pub fn create_worker<'a, Payload>( + pub fn create_worker( self: &Arc, options: impl Into>, ) -> Worker @@ -895,7 +895,7 @@ impl NodeState { /// /// - The message type is determined at runtime through the `topic_type` function parameter. /// - Only one type of callback is supported (returning both [`crate::DynamicMessage`] and - /// [`crate::MessageInfo`]). + /// [`crate::MessageInfo`]). /// /// # Message type passing /// @@ -951,7 +951,7 @@ impl NodeState { /// /// - The message type is determined at runtime through the `topic_type` function parameter. /// - Only one type of callback is supported (returning both [`crate::DynamicMessage`] and - /// [`crate::MessageInfo`]. + /// [`crate::MessageInfo`]. /// /// # Message type passing /// diff --git a/rclrs/src/node/node_options.rs b/rclrs/src/node/node_options.rs index 62855d240..e51f87840 100644 --- a/rclrs/src/node/node_options.rs +++ b/rclrs/src/node/node_options.rs @@ -407,10 +407,10 @@ impl<'a> NodeOptions<'a> { ) }; commands.add_to_wait_set(graph_change_waitable); - let _ = commands.run(node_graph_task( + drop(commands.run(node_graph_task( graph_change_receiver, graph_change_guard_condition, - )); + ))); let node = Arc::new(NodeState { time_source: TimeSource::builder(self.clock_type) @@ -419,7 +419,7 @@ impl<'a> NodeOptions<'a> { parameter, logger: Logger::new(logger_name)?, graph_change_action, - commands: Arc::clone(&commands), + commands: Arc::clone(commands), handle, }); node.time_source.attach_node(&node); diff --git a/rclrs/src/parameter.rs b/rclrs/src/parameter.rs index 1ff3415ae..1e19735f6 100644 --- a/rclrs/src/parameter.rs +++ b/rclrs/src/parameter.rs @@ -95,7 +95,7 @@ pub struct ParameterBuilder<'a, T: ParameterVariant> { discriminator: DiscriminatorFunction<'a, T>, options: ParameterOptions, interface: &'a ParameterInterface, - validate: Option Result<(), String> + Send + Sync>>, + validate: Option>, } impl<'a, T: ParameterVariant> ParameterBuilder<'a, T> { @@ -275,6 +275,7 @@ pub fn default_initial_value_discriminator( } type DiscriminatorFunction<'a, T> = Box) -> Option + 'a>; +type TypedValidateCallback = Arc Result<(), String> + Send + Sync>; /// Wraps a typed validate callback into a type-erased one that operates on `ParameterValue`. /// @@ -282,7 +283,7 @@ type DiscriminatorFunction<'a, T> = Box) -> Option /// `validate_parameter_setting` which checks the type discriminant first, /// and from `Parameters::set()` which checks `T::kind() == param.kind`. fn wrap_validate_callback( - callback: Arc Result<(), String> + Send + Sync>, + callback: TypedValidateCallback, ) -> ValidateCallback { Arc::new(move |pv: &ParameterValue| { let typed: T = pv.clone().try_into().ok().expect( @@ -352,7 +353,7 @@ pub struct MandatoryParameter { ranges: ParameterRanges, map: Weak>, change_tx: watch::Sender<()>, - validate: Option Result<(), String> + Send + Sync>>, + validate: Option>, _marker: PhantomData, } @@ -439,7 +440,7 @@ pub struct OptionalParameter { ranges: ParameterRanges, map: Weak>, change_tx: watch::Sender<()>, - validate: Option Result<(), String> + Send + Sync>>, + validate: Option>, _marker: PhantomData, } @@ -1032,12 +1033,12 @@ impl Parameters<'_> { /// /// Returns: /// * `Ok(())` if setting was successful. - /// * [`Err(ParameterValueError::TypeMismatch)`] if the type of the requested value is different + /// * `Err(`[`ParameterValueError::TypeMismatch`]`)` if the type of the requested value is different /// from the parameter's type. - /// * [`Err(ParameterValueError::OutOfRange)`] if the requested value is out of the parameter's + /// * `Err(`[`ParameterValueError::OutOfRange`]`)` if the requested value is out of the parameter's /// range. - /// * [`Err(ParameterValueError::ReadOnly)`] if the parameter is read only. - /// * [`Err(ParameterValueError::ValidationFailed)`] if the validate callback rejects the value. + /// * `Err(`[`ParameterValueError::ReadOnly`]`)` if the parameter is read only. + /// * `Err(`[`ParameterValueError::ValidationFailed`]`)` if the validate callback rejects the value. pub fn set( &self, name: impl Into>, diff --git a/rclrs/src/parameter/service.rs b/rclrs/src/parameter/service.rs index 79ea85e96..08d4ea6b6 100644 --- a/rclrs/src/parameter/service.rs +++ b/rclrs/src/parameter/service.rs @@ -133,14 +133,21 @@ fn list_parameters(req: ListParameters_Request, map: &ParameterMap) -> ListParam if req.depth == ListParameters_Request::DEPTH_RECURSIVE { return true; } - u64::try_from(substring.iter().filter(|c| **c == ('.' as _)).count()).unwrap() < req.depth + u64::try_from( + substring + .iter() + .filter(|c| i32::from(**c) == i32::from(b'.')) + .count(), + ) + .unwrap() + < req.depth }; let names: Sequence<_> = map .storage .keys() .filter_map(|name| { let name: rosidl_runtime_rs::String = name.clone().into(); - if req.prefixes.len() == 0 && check_parameter_name_depth(&name[..]) { + if req.prefixes.is_empty() && check_parameter_name_depth(&name[..]) { return Some(name); } req.prefixes diff --git a/rclrs/src/service.rs b/rclrs/src/service.rs index a30ec8dc4..55fc95868 100644 --- a/rclrs/src/service.rs +++ b/rclrs/src/service.rs @@ -152,7 +152,7 @@ where Box::new(ServiceExecutable:: { handle: Arc::clone(&handle), callback: Arc::clone(&callback), - commands: Arc::clone(&commands), + commands: Arc::clone(commands), }), Some(Arc::clone(commands.get_guard_condition())), ); @@ -321,6 +321,7 @@ unsafe impl Send for rcl_service_t {} pub struct ServiceHandle { rcl_service: Mutex, node_handle: Arc, + #[cfg_attr(ros_distro = "humble", allow(dead_code))] clock: Clock, } diff --git a/rclrs/src/service/any_service_callback.rs b/rclrs/src/service/any_service_callback.rs index 6a4cc6198..87782b82a 100644 --- a/rclrs/src/service/any_service_callback.rs +++ b/rclrs/src/service/any_service_callback.rs @@ -30,7 +30,7 @@ where commands: &Arc, ) -> Result<(), RclrsError> { match self { - Self::Node(node) => node.execute(Arc::clone(&handle), commands), + Self::Node(node) => node.execute(Arc::clone(handle), commands), Self::Worker(worker) => worker.execute(handle, payload), } } diff --git a/rclrs/src/service/node_service_callback.rs b/rclrs/src/service/node_service_callback.rs index 6713396d3..b8c259f4d 100644 --- a/rclrs/src/service/node_service_callback.rs +++ b/rclrs/src/service/node_service_callback.rs @@ -9,17 +9,28 @@ use futures::future::BoxFuture; use std::sync::Arc; +type OnlyRequestCallback = + Box::Request) -> BoxFuture<'static, ::Response> + Send>; +type WithIdCallback = Box< + dyn FnMut(::Request, RequestId) -> BoxFuture<'static, ::Response> + + Send, +>; +type WithInfoCallback = Box< + dyn FnMut(::Request, ServiceInfo) -> BoxFuture<'static, ::Response> + + Send, +>; + /// An enum capturing the various possible function signatures for service callbacks. pub enum NodeServiceCallback where T: Service, { /// A callback that only takes in the request value - OnlyRequest(Box BoxFuture<'static, T::Response> + Send>), + OnlyRequest(OnlyRequestCallback), /// A callback that takes in the request value and the ID of the request - WithId(Box BoxFuture<'static, T::Response> + Send>), + WithId(WithIdCallback), /// A callback that takes in the request value and all available - WithInfo(Box BoxFuture<'static, T::Response> + Send>), + WithInfo(WithInfoCallback), } impl NodeServiceCallback { @@ -37,7 +48,7 @@ impl NodeServiceCallback { if let Err(err) = handle.send_response::(&mut rmw_request_id, response.await) { - log_service_send_error(&*handle, rmw_request_id, err); + log_service_send_error(&handle, rmw_request_id, err); } }); } @@ -49,7 +60,7 @@ impl NodeServiceCallback { if let Err(err) = handle.send_response::(&mut rmw_request_id, response.await) { - log_service_send_error(&*handle, rmw_request_id, err); + log_service_send_error(&handle, rmw_request_id, err); } }); } @@ -62,7 +73,7 @@ impl NodeServiceCallback { if let Err(err) = handle.send_response::(&mut rmw_request_id, response.await) { - log_service_send_error(&*handle, rmw_request_id, err); + log_service_send_error(&handle, rmw_request_id, err); } }); } diff --git a/rclrs/src/service/worker_service_callback.rs b/rclrs/src/service/worker_service_callback.rs index fd935b7e5..ec5ab46a3 100644 --- a/rclrs/src/service/worker_service_callback.rs +++ b/rclrs/src/service/worker_service_callback.rs @@ -4,6 +4,16 @@ use crate::{RclrsError, RclrsErrorFilter, RequestId, ServiceHandle, ServiceInfo} use std::{any::Any, sync::Arc}; +type OnlyRequestCallback = + Box::Request) -> ::Response + Send>; +type WithIdCallback = Box< + dyn FnMut(&mut Payload, ::Request, RequestId) -> ::Response + Send, +>; +type WithInfoCallback = Box< + dyn FnMut(&mut Payload, ::Request, ServiceInfo) -> ::Response + + Send, +>; + /// An enum capturing the various possible function signatures for service /// callbacks that can be used by a [`Worker`][crate::Worker]. /// @@ -16,11 +26,11 @@ where Payload: 'static + Send, { /// A callback that only takes in the request value - OnlyRequest(Box T::Response + Send>), + OnlyRequest(OnlyRequestCallback), /// A callback that takes in the request value and the ID of the request - WithId(Box T::Response + Send>), + WithId(WithIdCallback), /// A callback that takes in the request value and all available - WithInfo(Box T::Response + Send>), + WithInfo(WithInfoCallback), } impl WorkerServiceCallback diff --git a/rclrs/src/subscription.rs b/rclrs/src/subscription.rs index 16a800a9c..55a3525f4 100644 --- a/rclrs/src/subscription.rs +++ b/rclrs/src/subscription.rs @@ -585,7 +585,7 @@ mod tests { // conditions can settle down. std::thread::sleep(std::time::Duration::from_millis(10)); - let _ = commands.run(async move { + drop(commands.run(async move { let (sender, mut receiver) = mpsc::unbounded(); let _subscription = node .create_subscription("test_delayed_subscription", move |_: Empty| { @@ -600,13 +600,13 @@ mod tests { // Publish the message, which should trigger the executor to stop spinning publisher.publish(Empty::default()).unwrap(); - if let Some(_) = receiver.next().await { + if receiver.next().await.is_some() { send_success.store(true, Ordering::Release); if let Some(promise) = promise.lock().unwrap().take() { promise.send(()).unwrap(); } } - }); + })); }); let r = executor.spin( diff --git a/rclrs/src/subscription/worker_subscription_callback.rs b/rclrs/src/subscription/worker_subscription_callback.rs index 58cfc6180..98cde6830 100644 --- a/rclrs/src/subscription/worker_subscription_callback.rs +++ b/rclrs/src/subscription/worker_subscription_callback.rs @@ -7,6 +7,14 @@ use crate::{ use std::{any::Any, sync::Arc}; +type RegularCallback = Box; +type RegularWithInfoCallback = Box; +type BoxedCallback = Box) + Send>; +type BoxedWithInfoCallback = Box, MessageInfo) + Send>; +type LoanedCallback = Box) + Send>; +type LoanedWithInfoCallback = + Box, MessageInfo) + Send>; + /// An enum capturing the various possible function signatures for subscription /// callbacks that can be used by a [`Worker`][crate::Worker]. /// @@ -15,19 +23,17 @@ use std::{any::Any, sync::Arc}; /// [1]: crate::IntoWorkerSubscriptionCallback pub enum WorkerSubscriptionCallback { /// A callback that only takes the payload and the message as arguments. - Regular(Box), + Regular(RegularCallback), /// A callback with the payload, message, and the message info as arguments. - RegularWithMessageInfo(Box), + RegularWithMessageInfo(RegularWithInfoCallback), /// A callback with only the payload and boxed message as arguments. - Boxed(Box) + Send>), + Boxed(BoxedCallback), /// A callback with the payload, boxed message, and the message info as arguments. - BoxedWithMessageInfo(Box, MessageInfo) + Send>), + BoxedWithMessageInfo(BoxedWithInfoCallback), /// A callback with only the payload and loaned message as arguments. - Loaned(Box) + Send>), + Loaned(LoanedCallback), /// A callback with the payload, loaned message, and the message info as arguments. - LoanedWithMessageInfo( - Box, MessageInfo) + Send>, - ), + LoanedWithMessageInfo(LoanedWithInfoCallback), } impl WorkerSubscriptionCallback { diff --git a/rclrs/src/timer.rs b/rclrs/src/timer.rs index 9d64e99c2..28ed39fae 100644 --- a/rclrs/src/timer.rs +++ b/rclrs/src/timer.rs @@ -109,7 +109,7 @@ impl TimerState { /// a cancelled state. [`TimerState::reset`] can be used to revert the timer /// out of the cancelled state. pub fn cancel(&self) -> Result<(), RclrsError> { - let cancel_result = unsafe { + unsafe { // SAFETY: The unwrap is safe here since we never use the rcl_timer // in a way that could panic while the mutex is locked. let mut rcl_timer = self.handle.rcl_timer.lock().unwrap(); @@ -120,7 +120,7 @@ impl TimerState { rcl_timer_cancel(&mut *rcl_timer) } .ok()?; - Ok(cancel_result) + Ok(()) } /// Checks whether the timer is canceled or not @@ -256,7 +256,7 @@ impl TimerState { /// Creates a new timer. Users should call one of [`Node::create_timer`], /// [`Node::create_timer_repeating`], [`Node::create_timer_oneshot`], or /// [`Node::create_timer_inert`]. - pub(crate) fn create<'a>( + pub(crate) fn create( period: Duration, clock: Clock, callback: AnyTimerCallback, @@ -375,7 +375,7 @@ impl TimerState { match callback { AnyTimerCallback::Repeating(mut callback) => { callback(payload, self); - self.restore_callback(AnyTimerCallback::Repeating(callback).into()); + self.restore_callback(AnyTimerCallback::Repeating(callback)); } AnyTimerCallback::OneShot(callback) => { callback(payload, self); diff --git a/rclrs/src/timer/any_timer_callback.rs b/rclrs/src/timer/any_timer_callback.rs index 3c4493d72..8ca7cfc8a 100644 --- a/rclrs/src/timer/any_timer_callback.rs +++ b/rclrs/src/timer/any_timer_callback.rs @@ -1,14 +1,19 @@ use crate::{TimerState, WorkScope}; use std::sync::Arc; +type RepeatingCallback = + Box::Payload, &Arc>) + Send>; +type OneShotCallback = + Box::Payload, &Arc>) + Send>; + /// A callback that can be triggered when a timer elapses. pub enum AnyTimerCallback { /// This callback will be triggered repeatedly, each time the period of the /// timer elapses. - Repeating(Box>) + Send>), + Repeating(RepeatingCallback), /// This callback will be triggered exactly once, the first time the period /// of the timer elapses. - OneShot(Box>) + Send>), + OneShot(OneShotCallback), /// Do nothing when the timer elapses. This can be replaced later so that /// the timer does something. Inert, diff --git a/rclrs/src/timer/into_node_timer_callback.rs b/rclrs/src/timer/into_node_timer_callback.rs index 5ef3772fb..c50ba4226 100644 --- a/rclrs/src/timer/into_node_timer_callback.rs +++ b/rclrs/src/timer/into_node_timer_callback.rs @@ -11,7 +11,7 @@ where Func: FnMut() + 'static + Send, { fn into_node_timer_repeating_callback(mut self) -> AnyTimerCallback { - AnyTimerCallback::Repeating(Box::new(move |_, _| self())).into() + AnyTimerCallback::Repeating(Box::new(move |_, _| self())) } } @@ -20,7 +20,7 @@ where Func: FnMut(&Timer) + 'static + Send, { fn into_node_timer_repeating_callback(mut self) -> AnyTimerCallback { - AnyTimerCallback::Repeating(Box::new(move |_, t| self(t))).into() + AnyTimerCallback::Repeating(Box::new(move |_, t| self(t))) } } @@ -29,7 +29,7 @@ where Func: FnMut(Time) + 'static + Send, { fn into_node_timer_repeating_callback(mut self) -> AnyTimerCallback { - AnyTimerCallback::Repeating(Box::new(move |_, t| self(t.handle.clock.now()))).into() + AnyTimerCallback::Repeating(Box::new(move |_, t| self(t.handle.clock.now()))) } } @@ -44,7 +44,7 @@ where Func: FnOnce() + 'static + Send, { fn into_node_timer_oneshot_callback(self) -> AnyTimerCallback { - AnyTimerCallback::OneShot(Box::new(move |_, _| self())).into() + AnyTimerCallback::OneShot(Box::new(move |_, _| self())) } } @@ -53,7 +53,7 @@ where Func: FnOnce(&Timer) + 'static + Send, { fn into_node_timer_oneshot_callback(self) -> AnyTimerCallback { - AnyTimerCallback::OneShot(Box::new(move |_, t| self(t))).into() + AnyTimerCallback::OneShot(Box::new(move |_, t| self(t))) } } @@ -62,6 +62,6 @@ where Func: FnOnce(Time) + 'static + Send, { fn into_node_timer_oneshot_callback(self) -> AnyTimerCallback { - AnyTimerCallback::OneShot(Box::new(move |_, t| self(t.handle.clock.now()))).into() + AnyTimerCallback::OneShot(Box::new(move |_, t| self(t.handle.clock.now()))) } } diff --git a/rclrs/src/timer/into_worker_timer_callback.rs b/rclrs/src/timer/into_worker_timer_callback.rs index bdb752411..eb2d2be4e 100644 --- a/rclrs/src/timer/into_worker_timer_callback.rs +++ b/rclrs/src/timer/into_worker_timer_callback.rs @@ -12,7 +12,7 @@ where Func: FnMut() + 'static + Send, { fn into_worker_timer_repeating_callback(mut self) -> AnyTimerCallback { - AnyTimerCallback::Repeating(Box::new(move |_, _| self())).into() + AnyTimerCallback::Repeating(Box::new(move |_, _| self())) } } @@ -21,7 +21,7 @@ where Func: FnMut(&mut Scope::Payload) + 'static + Send, { fn into_worker_timer_repeating_callback(mut self) -> AnyTimerCallback { - AnyTimerCallback::Repeating(Box::new(move |payload, _| self(payload))).into() + AnyTimerCallback::Repeating(Box::new(move |payload, _| self(payload))) } } @@ -31,7 +31,7 @@ where Func: FnMut(&mut Scope::Payload, &Arc>) + 'static + Send, { fn into_worker_timer_repeating_callback(self) -> AnyTimerCallback { - AnyTimerCallback::Repeating(Box::new(self)).into() + AnyTimerCallback::Repeating(Box::new(self)) } } @@ -44,7 +44,6 @@ where AnyTimerCallback::Repeating(Box::new(move |payload, t| { self(payload, t.handle.clock.now()) })) - .into() } } @@ -59,7 +58,7 @@ where Func: FnOnce() + 'static + Send, { fn into_worker_timer_oneshot_callback(self) -> AnyTimerCallback { - AnyTimerCallback::OneShot(Box::new(move |_, _| self())).into() + AnyTimerCallback::OneShot(Box::new(move |_, _| self())) } } @@ -68,7 +67,7 @@ where Func: FnOnce(&mut Scope::Payload) + 'static + Send, { fn into_worker_timer_oneshot_callback(self) -> AnyTimerCallback { - AnyTimerCallback::OneShot(Box::new(move |payload, _| self(payload))).into() + AnyTimerCallback::OneShot(Box::new(move |payload, _| self(payload))) } } @@ -78,7 +77,7 @@ where Func: FnOnce(&mut Scope::Payload, &Arc>) + 'static + Send, { fn into_worker_timer_oneshot_callback(self) -> AnyTimerCallback { - AnyTimerCallback::OneShot(Box::new(self)).into() + AnyTimerCallback::OneShot(Box::new(self)) } } @@ -90,6 +89,5 @@ where AnyTimerCallback::OneShot(Box::new(move |payload, t| { self(payload, t.handle.clock.now()) })) - .into() } } diff --git a/rclrs/src/wait_set.rs b/rclrs/src/wait_set.rs index 2e129157c..c60da9504 100644 --- a/rclrs/src/wait_set.rs +++ b/rclrs/src/wait_set.rs @@ -94,7 +94,7 @@ impl WaitSet { /// /// - Passing a wait set with no wait-able items in it will return an error. /// - The timeout must not be so large so as to overflow an `i64` with its nanosecond - /// representation, or an error will occur. + /// representation, or an error will occur. /// /// This list is not comprehensive, since further errors may occur in the `rmw` or `rcl` layers. /// @@ -140,10 +140,10 @@ impl WaitSet { } // Do not check the readiness if an error was reported. - if !r.is_err() { + if r.is_ok() { // For the remaining entities, check if they were activated and then run // the callback for those that were. - for waiter in self.primitives.values_mut().flat_map(|v| v) { + for waiter in self.primitives.values_mut().flatten() { if let Some(ready) = waiter.is_ready(&self.handle.rcl_wait_set) { f(ready, &mut *waiter.primitive)?; } @@ -208,7 +208,7 @@ impl WaitSet { /// /// [1]: crate::RclReturnCode fn register_rcl_primitives(&mut self) -> Result<(), RclrsError> { - for entity in self.primitives.values_mut().flat_map(|c| c) { + for entity in self.primitives.values_mut().flatten() { entity.add_to_wait_set(&mut self.handle.rcl_wait_set)?; } Ok(()) diff --git a/rclrs/src/wait_set/guard_condition.rs b/rclrs/src/wait_set/guard_condition.rs index 030874aa7..4d3abd726 100644 --- a/rclrs/src/wait_set/guard_condition.rs +++ b/rclrs/src/wait_set/guard_condition.rs @@ -75,7 +75,7 @@ impl GuardCondition { let handle = Arc::new(GuardConditionHandle { rcl_guard_condition, - context_handle: Arc::clone(&context), + context_handle: Arc::clone(context), }); let (waitable, lifecycle) = Waitable::new( @@ -105,7 +105,7 @@ impl GuardCondition { let handle = Arc::new(GuardConditionHandle { rcl_guard_condition, - context_handle: Arc::clone(&context), + context_handle: Arc::clone(context), }); let (waitable, lifecycle) = Waitable::new( diff --git a/rclrs/src/wait_set/rcl_primitive.rs b/rclrs/src/wait_set/rcl_primitive.rs index c59efc54c..0a3998695 100644 --- a/rclrs/src/wait_set/rcl_primitive.rs +++ b/rclrs/src/wait_set/rcl_primitive.rs @@ -11,7 +11,9 @@ pub trait RclPrimitive: Send + Sync { /// primitives sent through a [`WorkerChannel`][2] by a [`Worker`][3] this must be /// the same type as the `Worker`'s generic argument. /// - /// SAFETY: Make sure the type of the payload always matches what the primitive + /// # Safety + /// + /// Make sure the type of the payload always matches what the primitive /// expects to receive. For now we will return an error if there is a mismatch. /// In the future we may use `std::Any::downcast_mut_unchecked` once it /// stabilizes, which would give undefined behavior in a mismatch, making it @@ -138,7 +140,7 @@ impl ReadyKind { /// Action servers provide multiple services bundled together. When a wait set /// wakes up it is possible for any number of those services to be ready for /// processing. This struct conveys which of an action's services are ready. -#[derive(Debug, Copy, Clone, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq, Default)] pub struct ActionServerReady { /// True if there is a goal request message ready to take, false otherwise. pub goal_request: bool, @@ -150,17 +152,6 @@ pub struct ActionServerReady { pub goal_expired: bool, } -impl Default for ActionServerReady { - fn default() -> Self { - Self { - goal_request: false, - cancel_request: false, - result_request: false, - goal_expired: false, - } - } -} - impl ActionServerReady { /// Check whether any primitives in an action server are ready to be processed. /// @@ -216,7 +207,7 @@ impl ActionServerReady { /// some subscribers. When a wait set wakes up it is possible for any number of /// those services or subscriptions to be ready for processing. This struct /// conveys which of an action client's messages are ready. -#[derive(Debug, Copy, Clone, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq, Default)] pub struct ActionClientReady { /// True if there is a feedback message ready to take, false otherwise. pub feedback: bool, @@ -276,15 +267,3 @@ impl ActionClientReady { || self.result_response } } - -impl Default for ActionClientReady { - fn default() -> Self { - Self { - feedback: false, - status: false, - goal_response: false, - cancel_response: false, - result_response: false, - } - } -} diff --git a/rclrs/src/wait_set/wait_set_runner.rs b/rclrs/src/wait_set/wait_set_runner.rs index 62d39c9fb..7c281c728 100644 --- a/rclrs/src/wait_set/wait_set_runner.rs +++ b/rclrs/src/wait_set/wait_set_runner.rs @@ -111,7 +111,7 @@ impl WaitSetRunner { let (sender, promise) = channel(); std::thread::spawn(move || { let result = self.run_blocking(conditions); - if let Err(_) = sender.send((self, result)) { + if sender.send((self, result)).is_err() { // This is a debug log because this is a normal thing to occur // when an executor is winding down. log_debug!( diff --git a/rclrs/src/wait_set/waitable.rs b/rclrs/src/wait_set/waitable.rs index be771736d..077263ba5 100644 --- a/rclrs/src/wait_set/waitable.rs +++ b/rclrs/src/wait_set/waitable.rs @@ -151,7 +151,7 @@ impl Waitable { rcl_wait_set_add_subscription(wait_set, &*handle, &mut index) } RclPrimitiveHandle::GuardCondition(handle) => handle.use_handle(|handle| { - rcl_wait_set_add_guard_condition(wait_set, &*handle, &mut index) + rcl_wait_set_add_guard_condition(wait_set, handle, &mut index) }), RclPrimitiveHandle::Service(handle) => { rcl_wait_set_add_service(wait_set, &*handle, &mut index) diff --git a/rclrs/src/worker.rs b/rclrs/src/worker.rs index dd411399a..76da72d33 100644 --- a/rclrs/src/worker.rs +++ b/rclrs/src/worker.rs @@ -273,12 +273,12 @@ impl WorkerState { /// Creates a [`WorkerDynamicSubscription`], whose message type is only known at runtime. /// - /// Refer to ['Worker::create_subscription`] for the API and behavior except two key + /// Refer to [`WorkerState::create_subscription`] for the API and behavior except two key /// differences: /// /// - The message type is determined at runtime through the `topic_type` function parameter. /// - Only one type of callback is supported (returning both [`crate::DynamicMessage`] and - /// [`crate::MessageInfo`]). + /// [`crate::MessageInfo`]). /// /// ``` /// # use rclrs::*; @@ -615,7 +615,7 @@ impl WorkerState { callback: AnyTimerCallback>, ) -> Result, RclrsError> { let options = options.into_timer_options(); - let clock = options.clock.as_clock(&*self.node); + let clock = options.clock.as_clock(&self.node); let node = options.clock.is_node_time().then(|| Arc::clone(&self.node)); TimerState::create( options.period, @@ -732,10 +732,12 @@ impl ActivityListener { /// This type is used by executor runtimes to keep track of listeners. pub type WeakActivityListener = Weak>>; +type ActivityCallback = Box; + /// Enum for the different types of callbacks that a listener may have pub enum ActivityListenerCallback { /// The listener is listening - Listen(Box), + Listen(ActivityCallback), /// The listener is inert Inert, } @@ -817,7 +819,9 @@ mod tests { let client = node .create_client::("test_worker_service") .unwrap(); - let _: Promise = client.call(Empty_Request::default()).unwrap(); + let response_promise: Promise = + client.call(Empty_Request::default()).unwrap(); + drop(response_promise); let (mut promise, notice) = executor.commands().create_notice(promise);