Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use latest stable Rust CI + Fix Test Errors #420

Merged
merged 13 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ jobs:
use-ros2-testing: ${{ matrix.ros_distribution == 'rolling' }}

- name: Setup Rust
uses: dtolnay/rust-toolchain@1.74.0
uses: dtolnay/rust-toolchain@stable
with:
components: clippy, rustfmt

Expand Down
7 changes: 6 additions & 1 deletion rclrs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ authors = ["Esteve Fernandez <[email protected]>", "Nikolai Morin <nnmmgit@gmail
edition = "2021"
license = "Apache-2.0"
description = "A ROS 2 client library for developing robotics applications in Rust"
rust-version = "1.63"
rust-version = "1.70"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually depend on u128 FFI functions (and suppress warnings currently), so it'd be nice if our minimum was actually 1.78 so we could remove that.

#398

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've bumped the minimum version to 1.78 because I agree that it's good to declare that the minimum version we support is the one where u128 is ABI safe.

But sadly that's not enough to let us remove the lint because the lint is still being applied for the foreseeable future, I guess because it's still not guaranteed to be ABI safe across all build targets.


[lib]
path = "src/lib.rs"
Expand All @@ -29,6 +29,10 @@ libloading = { version = "0.8", optional = true }
# Needed for the Message trait, among others
rosidl_runtime_rs = "0.4"

# Needed for serliazation and deserialization of vendored messages
serde = { version = "1", optional = true, features = ["derive"] }
serde-big-array = { version = "0.5.1", optional = true }

[dev-dependencies]
# Needed for e.g. writing yaml files in tests
tempfile = "3.3.0"
Expand All @@ -46,6 +50,7 @@ cfg-if = "1.0.0"
[features]
default = []
dyn_msg = ["ament_rs", "libloading"]
serde = ["dep:serde", "dep:serde-big-array", "rosidl_runtime_rs/serde"]
# This feature is solely for the purpose of being able to generate documetation without a ROS installation
# The only intended usage of this feature is for docs.rs builders to work, and is not intended to be used by end users
generate_docs = ["rosidl_runtime_rs/generate_docs"]
Expand Down
18 changes: 9 additions & 9 deletions rclrs/src/parameter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,18 @@ use std::{
// Among the most relevant ones:
//
// * Parameter declaration returns an object which will be the main accessor to the parameter,
// providing getters and, except for read only parameters, setters. Object destruction will
// undeclare the parameter.
// providing getters and, except for read only parameters, setters. Object destruction will
// undeclare the parameter.
// * Declaration uses a builder pattern to specify ranges, description, human readable constraints
// instead of an ParameterDescriptor argument.
// instead of an ParameterDescriptor argument.
// * Parameters properties of read only and dynamic are embedded in their type rather than being a
// boolean parameter.
// boolean parameter.
// * There are no runtime exceptions for common cases such as undeclared parameter, already
// declared, or uninitialized.
// declared, or uninitialized.
// * There is no "parameter not set" type, users can instead decide to have a `Mandatory` parameter
// that must always have a value or `Optional` parameter that can be unset.
// that must always have a value or `Optional` parameter that can be unset.
// * Explicit API for access to undeclared parameters by having a
// `node.use_undeclared_parameters()` API that allows access to all parameters.
// `node.use_undeclared_parameters()` API that allows access to all parameters.

#[derive(Clone, Debug)]
struct ParameterOptionsStorage {
Expand Down Expand Up @@ -668,9 +668,9 @@ impl<'a> Parameters<'a> {
/// Returns:
/// * `Ok(())` if setting was successful.
/// * [`Err(DeclarationError::TypeMismatch)`] if the type of the requested value is different
/// from the parameter's type.
/// from the parameter's type.
/// * [`Err(DeclarationError::OutOfRange)`] if the requested value is out of the parameter's
/// range.
/// range.
/// * [`Err(DeclarationError::ReadOnly)`] if the parameter is read only.
pub fn set<T: ParameterVariant>(
&self,
Expand Down
2 changes: 1 addition & 1 deletion rclrs/src/parameter/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ impl From<()> for ParameterRanges {
/// Usually only one of these ranges will be applied, but all have to be stored since:
///
/// * A dynamic parameter can change its type at runtime, in which case a different range could be
/// applied.
/// applied.
/// * Introspection through service calls requires all the ranges to be reported to the user.
#[derive(Clone, Debug, Default)]
pub struct ParameterRanges {
Expand Down
10 changes: 6 additions & 4 deletions rclrs/src/subscription.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,9 +272,7 @@ where
}

fn execute(&self) -> Result<(), RclrsError> {
// Immediately evaluated closure, to handle SubscriptionTakeFailed
// outside this match
match (|| {
let evaluate = || {
match &mut *self.callback.lock().unwrap() {
AnySubscriptionCallback::Regular(cb) => {
let (msg, _) = self.take()?;
Expand Down Expand Up @@ -302,7 +300,11 @@ where
}
}
Ok(())
})() {
};

// Immediately evaluated closure, to handle SubscriptionTakeFailed
// outside this match
match evaluate() {
Err(RclrsError::RclError {
code: RclReturnCode::SubscriptionTakeFailed,
..
Expand Down
36 changes: 19 additions & 17 deletions rclrs/src/subscription/message_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,28 @@ use crate::rcl_bindings::*;

/// An identifier for a publisher in the local context.
///
/// To quote the `rmw` documentation:
/// To quote the [`rmw` documentation][1]:
///
/// > The identifier uniquely identifies the publisher for the local context, but
/// it will not necessarily be the same identifier given in other contexts or processes
/// for the same publisher.
/// Therefore the identifier will uniquely identify the publisher within your application
/// but may disagree about the identifier for that publisher when compared to another
/// application.
/// Even with this limitation, when combined with the publisher sequence number it can
/// uniquely identify a message within your local context.
/// Publisher GIDs generated by the RMW implementation could collide at some point, in which
/// case it is not possible to distinguish which publisher sent the message.
/// The details of how GIDs are generated are RMW implementation dependent.
///
/// > it will not necessarily be the same identifier given in other contexts or processes
/// > for the same publisher.
/// > Therefore the identifier will uniquely identify the publisher within your application
/// > but may disagree about the identifier for that publisher when compared to another
/// > application.
/// > Even with this limitation, when combined with the publisher sequence number it can
/// > uniquely identify a message within your local context.
/// > Publisher GIDs generated by the RMW implementation could collide at some point, in which
/// > case it is not possible to distinguish which publisher sent the message.
/// > The details of how GIDs are generated are RMW implementation dependent.
/// >
/// > It is possible the the RMW implementation needs to reuse a publisher GID,
/// due to running out of unique identifiers or some other constraint, in which case
/// the RMW implementation may document what happens in that case, but that
/// behavior is not defined here.
/// However, this should be avoided, if at all possible, by the RMW implementation,
/// and should be unlikely to happen in practice.
/// > due to running out of unique identifiers or some other constraint, in which case
/// > the RMW implementation may document what happens in that case, but that
/// > behavior is not defined here.
/// > However, this should be avoided, if at all possible, by the RMW implementation,
/// > and should be unlikely to happen in practice.
///
/// [1]: https://docs.ros.org/en/rolling/p/rmw/generated/structrmw__message__info__s.html#_CPPv4N18rmw_message_info_s13publisher_gidE
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct PublisherGid {
/// Bytes identifying a publisher in the RMW implementation.
Expand Down
2 changes: 1 addition & 1 deletion rclrs/src/wait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,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.
///
Expand Down
Loading