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

Add preliminary support for parameters #332

Merged
merged 83 commits into from
Oct 27, 2023
Merged
Show file tree
Hide file tree
Changes from 66 commits
Commits
Show all changes
83 commits
Select commit Hold shift + click to select a range
17145d2
WIP Add minimal TimeSource implementation
luca-della-vedova Jul 31, 2023
5de661a
Minor cleanup and code reorganization
luca-della-vedova Jul 31, 2023
7676a41
Minor cleanups, add debug code
luca-della-vedova Jul 31, 2023
8f2e1b8
Minor cleanup
luca-della-vedova Jul 31, 2023
978fd2e
Change safety note to reflect get_now safety
luca-della-vedova Aug 1, 2023
eda7103
Fix comment spelling
luca-della-vedova Aug 1, 2023
213978f
Cleanup and add some docs / tests
luca-della-vedova Aug 1, 2023
1f5ea5c
Avoid duplicated clocks in TimeSource
luca-della-vedova Aug 1, 2023
19a73c3
Change _rcl_clock to just Mutex
luca-della-vedova Aug 2, 2023
fde0f35
Remove Mutex from node clock
luca-della-vedova Aug 2, 2023
b808cc4
Change Mutex<bool> to AtomicBool
luca-della-vedova Aug 2, 2023
f11b4b0
Avoid cloning message
luca-della-vedova Aug 2, 2023
90b0938
Minor cleanup, add dependency
luca-della-vedova Aug 2, 2023
debaac9
Remove hardcoded use_sim_time
luca-della-vedova Aug 2, 2023
cc59f0a
Cleanup remaining warnings / clippy
luca-della-vedova Aug 2, 2023
d7bb9f2
Fix tests
luca-della-vedova Aug 2, 2023
5df379f
Refactor API to use ClockSource
luca-della-vedova Aug 3, 2023
0fcd296
Fix Drop implementation, finish builder and add comments
luca-della-vedova Aug 3, 2023
524475d
Minor cleanups
luca-della-vedova Aug 3, 2023
013cce5
Fix graph_tests
luca-della-vedova Aug 3, 2023
a5bdb07
Remove Arc from time source
luca-della-vedova Aug 3, 2023
d02ccc5
Remove Sync trait, format
luca-della-vedova Aug 3, 2023
9c3d987
Add comparison function for times, use reference to clock
luca-della-vedova Aug 4, 2023
99401aa
Implement Add<Duration> and Sub<Duration> for Time
luca-della-vedova Aug 4, 2023
90fe308
Make node pointer Weak to avoid memory leaks
luca-della-vedova Aug 7, 2023
0ee8954
Cleanups
luca-della-vedova Sep 12, 2023
4645935
WIP change clock type when use_sim_time parameter is changed
luca-della-vedova Sep 12, 2023
edce20e
Remove need for mutex in node TimeSource
luca-della-vedova Sep 13, 2023
632e082
Change get_clock() to return cloned Clock object
luca-della-vedova Sep 13, 2023
7e536e0
Minor cleanup
luca-della-vedova Sep 13, 2023
2fed18e
Refactor spin and spin_once to mimic rclcpp's Executor API (#324)
esteve Aug 7, 2023
744f4aa
add serde big array support (fixed #327) (#328)
fawdlstty Aug 22, 2023
df4010d
Fixed an issue where cpu usage remained at 100% (#330)
fawdlstty Aug 28, 2023
7286ce1
Make get_parameter pub(crate)
luca-della-vedova Sep 15, 2023
ed583f6
Address review feedback
luca-della-vedova Sep 18, 2023
382f780
Make time_source pub(crate), remove unused APIs
luca-della-vedova Sep 18, 2023
e665dfd
WIP parameter interface implementation
luca-della-vedova Sep 19, 2023
878a500
Migrate to proposed API
luca-della-vedova Sep 20, 2023
cbb9aa9
Add API for mandatory and optional parameters
luca-della-vedova Sep 20, 2023
a84e375
Merge branch 'main' into luca/parameter_services
luca-della-vedova Sep 20, 2023
0bd5052
First round of unit tests for parameter API
luca-della-vedova Sep 20, 2023
c62ca72
Make complex parameter types Arc
luca-della-vedova Sep 21, 2023
9512807
First attempt at undeclared parameter API
luca-della-vedova Sep 22, 2023
d5ffc59
First attempt at automatic parameter undeclaring
luca-della-vedova Sep 22, 2023
2ca517c
Change undeclared_parameter API to be more explicit
luca-della-vedova Sep 22, 2023
35e5d94
Minor cleanup, use atomic for undeclared params
luca-della-vedova Sep 22, 2023
b3788e7
Add docs
luca-della-vedova Sep 22, 2023
0a77427
Add more tests
luca-della-vedova Sep 22, 2023
c140672
Update setter for optional parameters
luca-della-vedova Sep 25, 2023
01eb4dc
Add unset api for optional parameters
luca-della-vedova Sep 25, 2023
3febf78
Cleanup and add helper functions for arrays
luca-della-vedova Sep 25, 2023
8bb26c5
Add type alias
luca-della-vedova Sep 25, 2023
660bce5
Reduce number of generics, add more tests
luca-della-vedova Sep 25, 2023
a069d41
Refactor to include parameter ranges and read_only
luca-della-vedova Sep 28, 2023
347320e
Refactor ranges
luca-della-vedova Sep 29, 2023
efaf8d7
Revert API change for range builder
luca-della-vedova Sep 29, 2023
4bd53d9
Add ParameterValueError variant
luca-della-vedova Sep 29, 2023
e7af874
First implementation of tentative API
luca-della-vedova Oct 2, 2023
26bf4ce
Rework API for unified declaration syntax
luca-della-vedova Oct 2, 2023
79404ff
Add missing APIs
luca-della-vedova Oct 2, 2023
71e7e42
Add error variant for default out of range
luca-della-vedova Oct 2, 2023
b95008c
Refactor and remove duplicated code
luca-della-vedova Oct 2, 2023
7b66d9b
Revert change to parameter service file
luca-della-vedova Oct 5, 2023
ccc91e9
Merge branch 'luca/add_time_source' into luca/parameter_services
luca-della-vedova Oct 5, 2023
6dcc21f
Remove time related changes
luca-della-vedova Oct 5, 2023
3f1b9ac
Clarify doc for unset API
luca-della-vedova Oct 5, 2023
8d3096a
Fix comments / TODOs
luca-della-vedova Oct 11, 2023
4b9fb5d
Remove Arc<RwLock> wrapper from undeclared params
luca-della-vedova Oct 11, 2023
b142cec
Add short description of deviations from other client libraries
luca-della-vedova Oct 11, 2023
25b1bb8
Address minor feedback
luca-della-vedova Oct 16, 2023
3e2a7bf
Remove duplicated range
luca-della-vedova Oct 16, 2023
25cd2a8
Change maybe_from to try_from
luca-della-vedova Oct 16, 2023
be4b977
WIP converting to default value builder argument
luca-della-vedova Oct 16, 2023
768535a
Add type for test
luca-della-vedova Oct 18, 2023
29d6c76
Infer arrays and allow custom discriminators
mxgrey Oct 18, 2023
67b0994
fix merge
mxgrey Oct 18, 2023
bc9477c
Fix CI
luca-della-vedova Oct 18, 2023
e20ea35
Fix some documentation
mxgrey Oct 18, 2023
e6636dd
Merge branch 'luca/parameter_services' of ssh://github.com/luca-della…
mxgrey Oct 18, 2023
df55acc
Fix doc link
luca-della-vedova Oct 19, 2023
009d4ae
Minor cleanups and optimizations
luca-della-vedova Oct 23, 2023
190cf0d
Fix outdated docstrings
luca-della-vedova Oct 23, 2023
1c845db
Add links in documentation / address clippy warnings
luca-della-vedova Oct 23, 2023
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
88 changes: 83 additions & 5 deletions rclrs/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ pub use self::builder::*;
pub use self::graph::*;
use crate::rcl_bindings::*;
use crate::{
Client, ClientBase, Context, GuardCondition, ParameterOverrideMap, Publisher, QoSProfile,
RclrsError, Service, ServiceBase, Subscription, SubscriptionBase, SubscriptionCallback,
ToResult,
Client, ClientBase, Context, Declarable, GuardCondition, ParameterBuilder, ParameterInterface,
ParameterVariant, Parameters, Publisher, QoSProfile, RclrsError, Service, ServiceBase,
Subscription, SubscriptionBase, SubscriptionCallback, ToResult,
};

impl Drop for rcl_node_t {
Expand Down Expand Up @@ -71,7 +71,7 @@ pub struct Node {
pub(crate) guard_conditions_mtx: Mutex<Vec<Weak<GuardCondition>>>,
pub(crate) services_mtx: Mutex<Vec<Weak<dyn ServiceBase>>>,
pub(crate) subscriptions_mtx: Mutex<Vec<Weak<dyn SubscriptionBase>>>,
_parameter_map: ParameterOverrideMap,
_parameter: ParameterInterface,
Copy link
Collaborator

@esteve esteve Oct 10, 2023

Choose a reason for hiding this comment

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

I like this pattern from rclcpp (interfaces for each group of features), we should do the same for services, subscriptions, etc.

}

impl Eq for Node {}
Expand Down Expand Up @@ -360,6 +360,84 @@ impl Node {
domain_id
}

/// Creates a `ParameterBuilder` that can be used to set parameter declaration options and
/// declare a parameter as `Optional`, `Mandatory` or `ReadOnly`.
///
/// # Example
/// ```
/// # use rclrs::{Context, ParameterRange, RclrsError};
/// let context = Context::new([])?;
/// let node = rclrs::create_node(&context, "domain_id_node")?;
/// // Set it to a range of 0-100, with a step of 2
luca-della-vedova marked this conversation as resolved.
Show resolved Hide resolved
/// let range = ParameterRange {
/// lower: Some(-100),
/// upper: Some(100),
/// step: Some(2),
/// };
/// let param = node.declare_parameter("int_param", 10)
/// .range(range)
/// .mandatory()
/// .unwrap();
/// assert_eq!(param.get(), 10);
/// param.set(50).unwrap();
/// assert_eq!(param.get(), 50);
/// // Out of range, will return an error
/// assert!(param.set(200).is_err());
/// # Ok::<(), RclrsError>(())
/// ```
pub fn declare_parameter<T: Declarable>(
&self,
name: &str,
default_value: T,
luca-della-vedova marked this conversation as resolved.
Show resolved Hide resolved
) -> ParameterBuilder<'_, T> {
self._parameter.declare(name, default_value)
}

/// Creates a `ParameterBuilder` that can be used to set parameter declaration options and
/// declare a parameter from an iterable as `Optional`, `Mandatory` or `ReadOnly`.
pub fn declare_parameter_from_iter<U: IntoIterator>(
&self,
name: &str,
default_value: U,
) -> ParameterBuilder<'_, Arc<[U::Item]>>
where
Arc<[U::Item]>: ParameterVariant,
{
self._parameter.declare_from_iter(name, default_value)
}

/// Creates a `ParameterBuilder` that can be used to set parameter declaration options and
/// declare a string array parameter as `Optional`, `Mandatory` or `ReadOnly`.
pub fn declare_string_array_parameter<U>(
&self,
name: &str,
default_value: U,
) -> ParameterBuilder<'_, Arc<[Arc<str>]>>
where
U: IntoIterator,
U::Item: Into<Arc<str>>,
{
self._parameter.declare_string_array(name, default_value)
}

/// Helper function to declare an `Optional` parameter with a default unset value.
pub fn declare_unset_parameter<T: ParameterVariant>(
&self,
name: &str,
) -> ParameterBuilder<'_, Option<T>> {
self._parameter.declare::<Option<T>>(name, None)
}

/// Enables usage of undeclared parameters for this node.
///
/// Returns a `Parameter` struct that can be used to get and set all parameters.
pub fn use_undeclared_parameters(&self) -> Parameters {
self._parameter.allow_undeclared();
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this get reset? In fact, is it used at all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the current API, once undeclared parameters are allowed, they're allowed for the rest of the node's lifespan. It's hard for me to imagine a use case for undoing that allowance or even what the behavior should be to undo it.

In rclcpp this decision is set in stone at initialization, so I think it's reasonable for us to not offer a way to reverse it.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I don't see the allow_undeclared atomic being read anywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It has been added in anticipation of parameter services. When parameter services are implemented on top of this in the future, this flag will determine whether to reject parameter service requests on undeclared parameters.

Until then it's true that it doesn't have any effect, simply because we've designed the rclrs API in a way that makes it unnecessary to check this for rclrs API calls.

Parameters {
interface: &self._parameter,
}
}

/// Creates a [`NodeBuilder`][1] with the given name.
///
/// Convenience function equivalent to [`NodeBuilder::new()`][2].
Expand All @@ -384,7 +462,7 @@ impl Node {
// function, which is why it's not merged into Node::call_string_getter().
// This function is unsafe since it's possible to pass in an rcl_node_t with dangling
// pointers etc.
unsafe fn call_string_getter_with_handle(
pub(crate) unsafe fn call_string_getter_with_handle(
rcl_node: &rcl_node_t,
getter: unsafe extern "C" fn(*const rcl_node_t) -> *const c_char,
) -> String {
Expand Down
21 changes: 7 additions & 14 deletions rclrs/src/node/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@ use std::ffi::CString;
use std::sync::{Arc, Mutex};

use crate::rcl_bindings::*;
use crate::{
node::call_string_getter_with_handle, resolve_parameter_overrides, Context, Node, RclrsError,
ToResult,
};
use crate::{Context, Node, ParameterInterface, RclrsError, ToResult};

/// A builder for creating a [`Node`][1].
///
Expand Down Expand Up @@ -262,24 +259,20 @@ impl NodeBuilder {
.ok()?;
};

let _parameter_map = unsafe {
let fqn = call_string_getter_with_handle(&rcl_node, rcl_node_get_fully_qualified_name);
resolve_parameter_overrides(
&fqn,
&rcl_node_options.arguments,
&rcl_context.global_arguments,
)?
};
let rcl_node_mtx = Arc::new(Mutex::new(rcl_node));

let _parameter = ParameterInterface::new(
&rcl_node_mtx,
&rcl_node_options.arguments,
&rcl_context.global_arguments,
)?;
Ok(Node {
rcl_node_mtx,
rcl_context_mtx: self.context.clone(),
clients_mtx: Mutex::new(vec![]),
guard_conditions_mtx: Mutex::new(vec![]),
services_mtx: Mutex::new(vec![]),
subscriptions_mtx: Mutex::new(vec![]),
_parameter_map,
_parameter,
})
}

Expand Down
Loading
Loading