-
Notifications
You must be signed in to change notification settings - Fork 163
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
base: rolling
Are you sure you want to change the base?
Changes from all commits
2185043
d5712fd
2ab8f09
f337a6a
702c448
817bce1
2640d09
b299ff2
ce49272
7836801
9da5590
0b8a5d7
c06b47a
dbfbe87
f5338cc
a5939aa
e6cf65d
1d1eb27
a66f54d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wjwwood meta: not a comment for you @bpwilcox, but replicating There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parallelism? I think I had intended to parallel the structure of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the name field in |
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bpwilcox wouldn't adding an There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
|
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 | ||
/// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_ |
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bpwilcox did you mean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This parallels the function naming in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bpwilcox Hmm, I see. Though if we were to match |
||
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_ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.