Skip to content

Commit

Permalink
Make the order of parameters deterministic
Browse files Browse the repository at this point in the history
GUI tools for working with parameters (at least Foxglove Studio, rqt
and rig_reconfigure) show the parameters in the same order as returned
by the ListParameters service. r2r stores the parameters in a HashMap,
which iterates keys and values in arbitrary order. The result is that
the GUI tools show the parameters in different order after every node
invocation, which can be quite annoying.

To make the order deterministic, we change the parameter storage from
HashMap to IndexMap, which iterates the map in insertion order.
According to the indexmap documentation, IndexMap is a drop-in
replacement of HashMap so this change should not require code changes
in applications using r2r. At least r2r examples and my projects
needed no changes.
  • Loading branch information
wentasah authored and m-dahl committed Apr 29, 2024
1 parent b46e374 commit e9f375e
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 9 deletions.
1 change: 1 addition & 0 deletions r2r/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ uuid = { version = "1.2.2", features = ["serde", "v4"] }
futures = "0.3.25"
log = "0.4.18"
phf = "0.11.1"
indexmap = "2.2.6"

[dev-dependencies]
serde_json = "1.0.89"
Expand Down
3 changes: 2 additions & 1 deletion r2r/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@
//! }
//! ```

// otherwise crates using r2r needs to specify the same version of uuid as
// otherwise crates using r2r needs to specify the same version of indexmap and uuid as
// this crate depend on, which seem like bad user experience.
pub extern crate indexmap;
pub extern crate uuid;

mod error;
Expand Down
9 changes: 5 additions & 4 deletions r2r/src/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use futures::{
future::{self, join_all, FutureExt, TryFutureExt},
stream::{Stream, StreamExt},
};
use indexmap::IndexMap;
use std::{
collections::HashMap,
ffi::{CStr, CString},
Expand Down Expand Up @@ -43,7 +44,7 @@ use crate::{
pub struct Node {
context: Context,
/// ROS parameters.
pub params: Arc<Mutex<HashMap<String, Parameter>>>,
pub params: Arc<Mutex<IndexMap<String, Parameter>>>,
pub(crate) node_handle: Box<rcl_node_t>,
// the node owns the subscribers
pub(crate) subscribers: Vec<Box<dyn Subscriber_>>,
Expand Down Expand Up @@ -198,7 +199,7 @@ impl Node {
};

let mut node = Node {
params: Arc::new(Mutex::new(HashMap::new())),
params: Arc::new(Mutex::new(IndexMap::new())),
context: ctx,
node_handle,
subscribers: Vec::new(),
Expand Down Expand Up @@ -460,7 +461,7 @@ impl Node {

fn handle_list_parameters(
req: ServiceRequest<rcl_interfaces::srv::ListParameters::Service>,
params: &Arc<Mutex<HashMap<String, Parameter>>>,
params: &Arc<Mutex<IndexMap<String, Parameter>>>,
) -> future::Ready<()> {
use rcl_interfaces::srv::ListParameters;

Expand Down Expand Up @@ -503,7 +504,7 @@ impl Node {

fn handle_desc_parameters(
req: ServiceRequest<rcl_interfaces::srv::DescribeParameters::Service>,
params: &Arc<Mutex<HashMap<String, Parameter>>>,
params: &Arc<Mutex<IndexMap<String, Parameter>>>,
) -> future::Ready<()> {
use rcl_interfaces::{msg::ParameterDescriptor, srv::DescribeParameters};
let mut descriptors = Vec::<ParameterDescriptor>::new();
Expand Down
7 changes: 4 additions & 3 deletions r2r/src/parameters.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use crate::{Error, Result};
use std::{collections::HashMap, ffi::CStr};
use std::ffi::CStr;

use crate::msg_types::generated_msgs::rcl_interfaces;
use indexmap::IndexMap;
use r2r_rcl::*;

/// ROS parameter value.
Expand Down Expand Up @@ -190,7 +191,7 @@ impl Parameter {
/// `parameters_derive.rs` example.
pub trait RosParams {
fn register_parameters(
&mut self, prefix: &str, param: Option<Parameter>, params: &mut HashMap<String, Parameter>,
&mut self, prefix: &str, param: Option<Parameter>, params: &mut IndexMap<String, Parameter>,
) -> Result<()>;
fn get_parameter(&mut self, param_name: &str) -> Result<ParameterValue>;
fn set_parameter(&mut self, param_name: &str, param_val: &ParameterValue) -> Result<()>;
Expand All @@ -202,7 +203,7 @@ macro_rules! impl_ros_params {
impl RosParams for $type {
fn register_parameters(
&mut self, prefix: &str, param: Option<Parameter>,
params: &mut HashMap<String, Parameter>,
params: &mut IndexMap<String, Parameter>,
) -> Result<()> {
if let Some(cli_param) = params.get(prefix) {
// Apply parameter value if set from command line or launch file
Expand Down
2 changes: 1 addition & 1 deletion r2r_macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub fn derive_r2r_params(input: proc_macro::TokenStream) -> proc_macro::TokenStr
&mut self,
prefix: &str,
desc: ::std::option::Option<::r2r::Parameter>,
params: &mut ::std::collections::hash_map::HashMap<String, ::r2r::Parameter>,
params: &mut ::r2r::indexmap::IndexMap<String, ::r2r::Parameter>,
) -> ::r2r::Result<()> {
let prefix = if prefix.is_empty() {
String::from("")
Expand Down

0 comments on commit e9f375e

Please sign in to comment.