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

Parse parameter descriptors in yaml #533

Open
wants to merge 19 commits into
base: rolling
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2185043
add param_descriptor type
bpwilcox Mar 13, 2021
d5712fd
add parameter descriptors map level and namespace key
bpwilcox Mar 13, 2021
2ab8f09
add param descriptor copy and deallocation
bpwilcox Mar 13, 2021
f337a6a
add node_params_descriptors init and re/deallocation
bpwilcox Mar 13, 2021
702c448
add find_descriptor, parse_desriptor, and modify parse file events an…
bpwilcox Mar 16, 2021
817bce1
modify replace ns with parameter descriptor level
bpwilcox Mar 16, 2021
2640d09
modify parser implementation for parameter descriptors, add get param…
bpwilcox Mar 16, 2021
b299ff2
add new files to CMakeLists
bpwilcox Mar 16, 2021
ce49272
check for new map at MAP_NODE_NAME_LVL: fixes issue to allow both par…
bpwilcox Mar 17, 2021
7836801
fix test now that more calloc calls with param descriptors
bpwilcox Mar 17, 2021
9da5590
add parameter descriptor yaml tests
bpwilcox Mar 17, 2021
0b8a5d7
add parameter to test yaml for extra coverage and verify parsed param…
bpwilcox Mar 17, 2021
c06b47a
add unit tests for param descriptors
bpwilcox Mar 23, 2021
dbfbe87
fix cpplint
bpwilcox Mar 23, 2021
f5338cc
fix uncrustify issues
bpwilcox Mar 23, 2021
a5939aa
change param descriptor key name to ros__parameter_descriptors
bpwilcox Mar 23, 2021
e6cf65d
remove error msg for node name before key to allow for both keys para…
bpwilcox Jun 29, 2021
1d1eb27
address feedback, remove descriptor name, change field naming
bpwilcox Dec 7, 2021
a66f54d
add dynamic_typing field
bpwilcox Dec 7, 2021
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
26 changes: 26 additions & 0 deletions rcl_yaml_param_parser/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ endif()
set(rcl_yaml_parser_sources
src/add_to_arrays.c
src/namespace.c
src/node_params_descriptors.c
src/node_params.c
src/parse.c
src/parser.c
src/yaml_descriptor.c
src/yaml_variant.c
)

Expand Down Expand Up @@ -92,6 +94,18 @@ if(BUILD_TESTING)
target_link_libraries(test_node_params ${PROJECT_NAME})
endif()

ament_add_gtest(test_node_params_descriptors
test/test_node_params_descriptors.cpp
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
)
if(TARGET test_node_params_descriptors)
ament_target_dependencies(test_node_params_descriptors
"rcutils"
"osrf_testing_tools_cpp"
)
target_link_libraries(test_node_params_descriptors ${PROJECT_NAME})
endif()

ament_add_gtest(test_parse_yaml
test/test_parse_yaml.cpp
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
Expand Down Expand Up @@ -166,6 +180,18 @@ if(BUILD_TESTING)
target_link_libraries(test_yaml_variant ${PROJECT_NAME})
endif()

ament_add_gtest(test_yaml_descriptor
test/test_yaml_descriptor.cpp
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
)
if(TARGET test_yaml_descriptor)
ament_target_dependencies(test_yaml_descriptor
"rcutils"
"osrf_testing_tools_cpp"
)
target_link_libraries(test_yaml_descriptor ${PROJECT_NAME})
endif()

add_performance_test(benchmark_parse_yaml test/benchmark/benchmark_parse_yaml.cpp
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}")
if(TARGET benchmark_parse_yaml)
Expand Down
11 changes: 11 additions & 0 deletions rcl_yaml_param_parser/include/rcl_yaml_param_parser/parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,17 @@ rcl_variant_t * rcl_yaml_node_struct_get(
const char * param_name,
rcl_params_t * params_st);

/// \brief Get the descriptor struct for a given parameter
/// \param[in] node_name is the name of the node to which the parameter belongs
/// \param[in] param_name is the name of the parameter whose descriptor is to be retrieved
/// \param[inout] params_st points to the populated (or to be populated) parameter struct
/// \return parameter descriptor struct on success and NULL on failure
Copy link
Contributor

Choose a reason for hiding this comment

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

@bpwilcox what if no descriptor was specified? what will this function do?

Copy link
Author

Choose a reason for hiding this comment

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

If the parameter name or node name does not exist for the descriptor, then it will still return a descriptor (not NULL). I found that this is the same behavior as what happens with rcl_yaml_node_struct_get.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bpwilcox it'd be best to document what the expected behavior is.

RCL_YAML_PARAM_PARSER_PUBLIC
rcl_param_descriptor_t * rcl_yaml_node_struct_get_descriptor(
const char * node_name,
const char * param_name,
rcl_params_t * params_st);

/// \brief Print the parameter structure to stdout
/// \param[in] params_st points to the populated parameter struct
RCL_YAML_PARAM_PARSER_PUBLIC
Expand Down
28 changes: 28 additions & 0 deletions rcl_yaml_param_parser/include/rcl_yaml_param_parser/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,33 @@ typedef struct rcl_node_params_s
size_t capacity_params; ///< Capacity of parameters in the node
} rcl_node_params_t;

/// \typedef rcl_param_descriptor_t
/// \brief param_descriptor_t stores the descriptor of a parameter
typedef struct rcl_param_descriptor_s
{
bool * read_only;
bool * dynamic_typing;
uint8_t * type;
char * description;
char * additional_constraints;
double * min_value_double;
double * max_value_double;
double * step_double;
int64_t * min_value_int;
int64_t * max_value_int;
int64_t * step_int;
} rcl_param_descriptor_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

@wjwwood meta: not a comment for you @bpwilcox, but replicating rcl_interfaces/msg/ParameterDescriptor structure is not ideal. The same goes with rcl_interfaces/msg/Parameter, so I wonder if we should be re-using the message C representation instead (like we partially do in rcl_action already). Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

In a related note, I understand pointers are used here to have indication of absence. At the same time, ROS 2 interfaces have no optional fields, so something, somewhere, will choose defaults. If we had all of that here, we could get rid of many heap allocations and simplify the code.


/// \typedef rcl_node_params_descriptors_t
/// \brief node_params_descriptors_t stores all the parameter descriptors of a single node
typedef struct rcl_node_params_descriptors_s
{
char ** parameter_names; ///< Array of parameter names (keys)
Copy link
Contributor

Choose a reason for hiding this comment

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

@bpwilcox I don't follow why there's an array of parameter names here and a name on each descriptor.

Copy link
Author

Choose a reason for hiding this comment

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

Parallelism? I think I had intended to parallel the structure of rcl_node_params and rcl_node_params_descriptors. It made the code more convenient to write, I believe, though I suppose we could remove one or the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I'm not entirely convinced it's worth the duplication. You can get away with just an rcl_param_descriptor_t array.

Copy link
Author

Choose a reason for hiding this comment

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

Fair point

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment still applies i.e. no need to have the parameter name in three (3) places.

Copy link
Author

Choose a reason for hiding this comment

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

I removed the name field in rcl_param_descriptor_t instead since it made more sense following the parallel logic of the code with the parameter values.

rcl_param_descriptor_t * parameter_descriptors; ///< Array of corresponding parameter descriptors
size_t num_descriptors; ///< Number of parameters in the node
size_t capacity_descriptors; ///< Capacity of parameters in the node
} rcl_node_params_descriptors_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

@bpwilcox wouldn't adding an rcl_param_descriptor_t ** parameter_descriptors; field to the rcl_node_params_s struct simplify the code a bit? Using NULL to indicate when a descriptor is not available, precluding parameter descriptors without an associated parameter value.

Copy link
Author

Choose a reason for hiding this comment

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

I was initially going to put the together as you recommend, however I do not think we would want the parameter descriptors to require association with a parameter value. In my view, it should be perfectly acceptable to have a yaml file with descriptors only and declared parameters in code may comply with these descriptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bpwilcox hmm, that's a strange use case. I'd expect the parameter file to specify both, or the descriptor to be specified in code. In what scenario would you want to externally change the parameter descriptor of a parameter declared in code?


/// stores all the parameters of all nodes of a process
/*
* \typedef rcl_params_t
Expand All @@ -103,6 +130,7 @@ typedef struct rcl_params_s
{
char ** node_names; ///< List of names of the node
rcl_node_params_t * params; ///< Array of parameters
rcl_node_params_descriptors_t * descriptors; ///< Array of parameter descriptors
size_t num_nodes; ///< Number of nodes
size_t capacity_nodes; ///< Capacity of nodes
rcutils_allocator_t allocator; ///< Allocator used
Expand Down
69 changes: 69 additions & 0 deletions rcl_yaml_param_parser/src/impl/node_params_descriptors.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Copyright 2018 Apex.AI, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef IMPL__NODE_PARAMS_DESCRIPTORS_H_
#define IMPL__NODE_PARAMS_DESCRIPTORS_H_

#include "rcl_yaml_param_parser/types.h"
#include "rcl_yaml_param_parser/visibility_control.h"

#ifdef __cplusplus
extern "C"
{
#endif

///
/// Create rcl_node_params_descriptors_t structure
///
Copy link
Contributor

Choose a reason for hiding this comment

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

@bpwilcox here and elsewhere in this file, can we document return values and parameters?

RCL_YAML_PARAM_PARSER_PUBLIC
RCUTILS_WARN_UNUSED
rcutils_ret_t node_params_descriptors_init(
rcl_node_params_descriptors_t * node_descriptors,
const rcutils_allocator_t allocator);

///
/// Create rcl_node_params_descriptors_t structure with a capacity
///
RCL_YAML_PARAM_PARSER_PUBLIC
RCUTILS_WARN_UNUSED
rcutils_ret_t node_params_descriptors_init_with_capacity(
rcl_node_params_descriptors_t * node_descriptors,
size_t capacity,
const rcutils_allocator_t allocator);

///
/// Reallocate rcl_node_params_descriptors_t structure with a new capacity
/// \post the address of \p parameter_names in \p node_params might be changed
/// even if the result value is `RCL_RET_BAD_ALLOC`.
///
RCL_YAML_PARAM_PARSER_PUBLIC
RCUTILS_WARN_UNUSED
rcutils_ret_t node_params_descriptors_reallocate(
rcl_node_params_descriptors_t * node_descriptors,
size_t new_capacity,
const rcutils_allocator_t allocator);

///
/// Finalize rcl_node_params_descriptors_t structure
///
RCL_YAML_PARAM_PARSER_PUBLIC
void rcl_yaml_node_params_descriptors_fini(
rcl_node_params_descriptors_t * node_descriptors,
const rcutils_allocator_t allocator);

#ifdef __cplusplus
}
#endif

#endif // IMPL__NODE_PARAMS_DESCRIPTORS_H_
18 changes: 18 additions & 0 deletions rcl_yaml_param_parser/src/impl/parse.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ rcutils_ret_t parse_value(
data_types_t * seq_data_type,
rcl_params_t * params_st);

RCL_YAML_PARAM_PARSER_PUBLIC
RCUTILS_WARN_UNUSED
rcutils_ret_t parse_descriptor(
namespace_tracker_t * ns_tracker,
const yaml_event_t event,
const bool is_seq,
const size_t node_idx,
const size_t parameter_idx,
rcl_params_t * params_st);

RCL_YAML_PARAM_PARSER_PUBLIC
RCUTILS_WARN_UNUSED
rcutils_ret_t parse_key(
Expand Down Expand Up @@ -87,6 +97,14 @@ rcutils_ret_t find_parameter(
rcl_params_t * param_st,
size_t * parameter_idx);

RCL_YAML_PARAM_PARSER_PUBLIC
RCUTILS_WARN_UNUSED
rcutils_ret_t find_descriptor(
const size_t node_idx,
const char * parameter_name,
rcl_params_t * param_st,
size_t * parameter_idx);

#ifdef __cplusplus
}
#endif
Expand Down
7 changes: 6 additions & 1 deletion rcl_yaml_param_parser/src/impl/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ extern "C"
#endif

#define PARAMS_KEY "ros__parameters"
#define PARAMS_DESCRIPTORS_KEY "ros__parameter_descriptors"
#define NODE_NS_SEPERATOR "/"
#define PARAMETER_NS_SEPERATOR "."

Expand All @@ -36,6 +37,7 @@ typedef enum yaml_map_lvl_e
MAP_UNINIT_LVL = 0U,
MAP_NODE_NAME_LVL = 1U,
MAP_PARAMS_LVL = 2U,
MAP_PARAMS_DESCRIPTORS_LVL = 3U,
} yaml_map_lvl_t;

/// Basic supported data types in the yaml file
Expand All @@ -51,7 +53,8 @@ typedef enum data_types_e
typedef enum namespace_type_e
{
NS_TYPE_NODE = 1U,
NS_TYPE_PARAM = 2U
NS_TYPE_PARAM = 2U,
NS_TYPE_DESCRIPTOR = 3U
} namespace_type_t;

/// Keep track of node and parameter name spaces
Expand All @@ -61,6 +64,8 @@ typedef struct namespace_tracker_s
uint32_t num_node_ns;
char * parameter_ns;
uint32_t num_parameter_ns;
char * descriptor_key_ns;
uint32_t num_descriptor_key_ns;
} namespace_tracker_t;

#ifdef __cplusplus
Expand Down
51 changes: 51 additions & 0 deletions rcl_yaml_param_parser/src/impl/yaml_descriptor.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright 2018 Apex.AI, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef IMPL__YAML_DESCRIPTOR_H_
#define IMPL__YAML_DESCRIPTOR_H_

#include "rcutils/allocator.h"

#include "./types.h"
#include "rcl_yaml_param_parser/types.h"
#include "rcl_yaml_param_parser/visibility_control.h"

#ifdef __cplusplus
extern "C"
{
#endif

///
/// Finalize an rcl_param_descriptor_t
///
RCL_YAML_PARAM_PARSER_PUBLIC
void rcl_yaml_descriptor_fini(
Copy link
Contributor

Choose a reason for hiding this comment

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

@bpwilcox did you mean rcl_param_descriptor_fini? Same elsewhere. YAML is implied.

Copy link
Author

Choose a reason for hiding this comment

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

This parallels the function naming in yaml_variant.h so I just kept it. If we change the name to your suggestion, I'd recommend we do the same for the variant version. I don't know if this is a big concern, however.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bpwilcox Hmm, I see. Though if we were to match rcl_variant_t APIs naming pattern it should be rcl_yaml_param_descriptor_fini().

rcl_param_descriptor_t * param_desc,
const rcutils_allocator_t allocator);

///
/// Copy a rcl_param_descriptor_t from param_desc to out_param_desc
///
RCL_YAML_PARAM_PARSER_PUBLIC
RCUTILS_WARN_UNUSED
bool rcl_yaml_descriptor_copy(
rcl_param_descriptor_t * out_param_desc,
const rcl_param_descriptor_t * param_desc,
rcutils_allocator_t allocator);

#ifdef __cplusplus
}
#endif

#endif // IMPL__YAML_DESCRIPTOR_H_
10 changes: 10 additions & 0 deletions rcl_yaml_param_parser/src/namespace.c
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,16 @@ rcutils_ret_t replace_ns(
}
ns_tracker->num_parameter_ns = new_ns_count;
break;
case NS_TYPE_DESCRIPTOR:
if (NULL != ns_tracker->descriptor_key_ns) {
allocator.deallocate(ns_tracker->descriptor_key_ns, allocator.state);
}
ns_tracker->descriptor_key_ns = rcutils_strdup(new_ns, allocator);
if (NULL == ns_tracker->descriptor_key_ns) {
return RCUTILS_RET_BAD_ALLOC;
}
ns_tracker->num_descriptor_key_ns = new_ns_count;
break;
default:
res = RCUTILS_RET_ERROR;
break;
Expand Down
Loading