YAML parameter parser #235

anup-pem · 2018-05-04T03:43:53Z

A basic yaml parameter parser to parse ros2 node parameters from a yaml file and populate a C data structure

anup-pem · 2018-05-05T00:39:35Z

anup-pem · 2018-05-07T18:19:40Z

@mikaelarguedas @wjwwood Just wanted to touch base and see when you might be able to take a look at this PR? Thanks

mikaelarguedas

Thanks @anup-pem for the PR!

I didn't review the bulk of the code yet. This is mostly cosmetic comments to match the rest of the codebase and get the ball rolling. I added some design questions regarding: e.g. the fact the bytes structures are commented out.

I'll take a closer look at the code tomorrow once I have the system version of libyaml properly detected and used by this package.
At which point we'll have to look how to integrate libyaml to be able to test this code on all platforms in our CI

mikaelarguedas · 2018-05-07T23:24:00Z

rcl_yaml_param_parser/CMakeLists.txt

+
+if(BUILD_TESTING)
+  if(NOT WIN32)
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++14")


we recommend to pass -Wall -Wextra and -Wpedantic to gcc.
The C++ standard use should be defined using CMAKE_CXX_STANDARD (exanple here)

Good point. I have added those flags

Please also address the comment about the way to specify the C++ standard

mikaelarguedas · 2018-05-07T23:24:54Z

rcl_yaml_param_parser/CMakeLists.txt

+
+  find_package(ament_cmake_gmock REQUIRED)
+  find_package(ament_cmake_gtest REQUIRED)
+  find_package(ament_cmake_pytest REQUIRED)


As this package doesnt have any pytest, this line can be removed

mikaelarguedas · 2018-05-07T23:25:16Z

rcl_yaml_param_parser/CMakeLists.txt

+
+  set(extra_test_libraries)
+  set(extra_test_env)
+  set(extra_lib_dirs)


Are any of these variables used ?

No, lingering from rcutils CMake. Removed

mikaelarguedas · 2018-05-07T23:25:37Z

rcl_yaml_param_parser/CMakeLists.txt

+  set(SKIP_TEST_IF_WIN32 "")
+  if(WIN32)  # (memory tools doesn't do anything on Windows)
+    set(SKIP_TEST_IF_WIN32 "SKIP_TEST")
+  endif()


Same question here: is if used anywhere?

No, lingering from rcutils CMake. Removed

mikaelarguedas · 2018-05-07T23:26:35Z

rcl_yaml_param_parser/CMakeLists.txt

+
+  macro(rcutils_custom_add_gmock target)
+    ament_add_gmock(${target} ${ARGN})
+  endmacro()


As this package registers a single gtest, these 2 marcos can be removed as well as find_package(ament_cmake_gmock REQUIRED) on line 39

Ahhh ok. Removed

mikaelarguedas · 2018-05-07T23:40:10Z

rcl_yaml_param_parser/package.xml

+  <test_depend>ament_cmake_pytest</test_depend>
+  <test_depend>ament_lint_common</test_depend>
+  <test_depend>ament_lint_auto</test_depend>
+  <test_depend>launch_testing</test_depend>


this package doesn't use launch_testing so this line can be removed

mikaelarguedas · 2018-05-07T23:40:53Z

rcl_yaml_param_parser/src/parser.c

+#include <stdbool.h>
+#include <stdio.h>
+#include <errno.h>
+#include <yaml.h>


Nitpick: include statements should be in alphabetical order

Good point. Fixed it

mikaelarguedas · 2018-05-07T23:42:14Z

rcl_yaml_param_parser/src/parser.c

+#define PARAMS_KEY "params"
+
+#define MAX_NUM_NODE_ENTRIES 256U
+#define MAX_NUM_PARAMS_PER_NODE 512U


How were these numbers decided (not asking to change them just curious), we should document the values picked

Don't have a great reason. Wanted to put a MAX value instead of having infinity. Picked 512 nodes per process, because If a process decides to have more than that it might be better to split the process. If there is a max limit of nodes per process restricted by ROS2, I can use it too.

mikaelarguedas · 2018-05-07T23:46:49Z

rcl_yaml_param_parser/src/parser.c

+  rcl_node_params_t * node_params,
+  const rcutils_allocator_t allocator);
+
+static rcutils_ret_t param_struct_initialize(


do create and initialize have different meanings here ?
I recommend using _init or get_zero_initialized_XX to match the rest of the stack. Examples here and here

Not really. Changed all initialization to _init

mikaelarguedas · 2018-05-07T23:47:25Z

rcl_yaml_param_parser/src/parser.c

+#include "rcl_yaml_param_parser/parser.h"
+#include "rcl_yaml_param_parser/types.h"
+
+#include "rcutils/error_handling.h"


All the functions in this file should use rcl/error_handling
rcutils_ret_t and rcutils_allocators_t should be replaced by their rcl equivalent where appropriate

Since it was initially decided to be implemented as part of rcutils, I used the rcutils API. Will change it

yeah that's my bad I missed that during my first search / replace sweep

anup-pem · 2018-05-08T01:19:28Z

@mikaelarguedas

bytes structures are commented out.
Have not provided the byte support in this phase. Was thinking of adding it later as it requires more discussion about identifying/differentiating a byte type. As per my understanding from the yaml doc (http://yaml.org/type/binary.html) an additional "!!binary" tag may have to be added before the byte value in the yaml file.
At which point we'll have to look how to integrate libyaml to be able to test this code on all platforms in our CI
For now I was thinking the user download the libyaml. Once we have the parser merged with all the supported API's can discuss about integrating libyaml into the CI. In my development, have a libyaml built as part of ament and since we will be modifying libyaml will have a copy of libyaml in apex repo. In case of ROS2 repo, might need some direction about is it better to download and compile on the fly before compiling the parser (like poco_vendor) or is it better to have a copy of libyaml in the CI infrastructure.

mikaelarguedas · 2018-05-08T21:53:34Z

For now I was thinking the user download the libyaml. Once we have the parser merged with all the supported API's can discuss about integrating libyaml into the CI.

Unfortunately merging this without having it working on CI means that our CI will fail for any new PR and nightly builds. If we merge this before, we need to add extra logic in CI to ignore this package.
This can be done quite easily but that means that people cloning our repository will not be able to build the workspace either. So I think this needs to build and pass on all platforms before being merged (that's our general policy for any new code).

In case of ROS2 repo, might need some direction about is it better to download and compile on the fly before compiling the parser (like poco_vendor) or is it better to have a copy of libyaml in the CI infrastructure.

Yes, vendoring it ourselves in a first time is definitely a possibility 👍 . We can later look at how to avoid having to compile it every time.

mikaelarguedas · 2018-05-08T21:54:34Z

Have not provided the byte support in this phase. Was thinking of adding it later as it requires more discussion about identifying/differentiating a byte type.

Yeah that's fine, we should put a TODO in the code and open a follow-up ticket to make sure we come back to it after the first version is merged

…l_param_parser - rename folders from rcutils_yaml_param_parser to rcl_yaml_param_parser - rename project, header guards and include statements from rcutils_yaml_param_parser to rcl_yaml_param_parser - move type declaration in separat file and namespace new structures - Fix the code review comments from Mikael

mikaelarguedas · 2018-05-11T01:36:48Z

Ok I got this to compile on Linux with the linked PRs.

This generates some warnings on MacOS (https://ci.ros2.org/job/ci_osx/3592/warnings11Result/)

I didnt get this to compile on Windows so far. Weirdly Fast-RTPS is failing to build (shouldn't have any interaction with this code...).

mikaelarguedas · 2018-05-11T02:08:59Z

rcl_yaml_param_parser/CMakeLists.txt

+  # Gtests
+  rcutils_custom_add_gtest(test_parse_yaml
+    test/test_parse_yaml.cpp
+  )


can you please remove the custom rcutils ... macro and simply call

ament_add_gtest(test_parse_yaml test/test_parse_yaml.cpp )

Good point!

mikaelarguedas · 2018-05-11T02:13:29Z

rcl_yaml_param_parser/src/parser.c

+  MAP_NODE_NAME_LVL = 3U,
+  MAP_PARAMS_LVL = 4U,
+  MAP_PARAMS_NS_LVL = 5U
+} yaml_map_lvl_t;


Is this implementation robust to multiple namespacing levels? e.g.:

stereo_cameras_ns_top: stereo_cameras_ns_low: stereo_camera1: params: is_cam_on: [true, true, false, true, false, false] brand: Bosch stereo_camera2: params: stereo_camera_dr: stereo_camera_left: image_type: ir stereo_camera_right: image_type: rbg

Yes it supports. The file test/correct_config.yaml has this

Oh I must have messed up something then. I tried just adding one level of namespacing to the correct_config.yaml file and it fails to parse on my machine. Do you have an idea of what I did wrong ?

diff --git a/rcl_yaml_param_parser/test/correct_config.yaml b/rcl_yaml_param_parser/test/correct_config.yaml index 4b5e50d..51954f8 100644 --- a/rcl_yaml_param_parser/test/correct_config.yaml +++ b/rcl_yaml_param_parser/test/correct_config.yaml @@ -38,6 +38,7 @@ new_camera_ns: brand: Bosch new_camera2: params: - camera_dr: - dr_name: default - brand: Mobius + camera_left: + camera_dr: + dr_name: default + brand: Mobius

The test fail with the following message:

1: Will not support deeper mappings at line 43, at /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c:1079 1: /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/test/test_parse_yaml.cpp:37: Failure 1: Value of: res 1: Actual: false 1: Expected: true

Sorry my earlier answer is not entirely true. I restrict it to one namespacing level after params but can have mutilpe namespaces under params. Things can get crazy allowing multiple namespace levels. Looked at quite a few examples in the config files in various places like autoware stacks, our internal lidar stack, and did not find a need for many namespace levels. Trying to keep simple

It is very common in ROS to have several levels of namespacing for parameters.
For example, a minimalistic pan-tilt camera controller:

gimbal_controller: pan_joint: pid: {p: 50.0, i: 0, d: 2} tilt_joint: pid: {p: 50.0, i: 0, d: 2}

It supports just one mapping level(params namespace mapping) after params. In the above example it has two mapping levels after params camera_left and camera_dr. It can have

new_camera2: params: camera_left_dr: dr_name: default brand: Mobius camera_right_dr: dr_name: default brand: Mobius

We in the design decided with the following format. A parameter_namespace_string and series of <field_name> : <value> pairs.

<node_namespace_string>: # optional <node1_name>: params: <field_name>: <field_value> <parameter_namespace_string>: # optional <field1_name>: <field1_value> <field2_name>: <field2_value> <node2_name>: params: <field_name>: <field_value> <parameter_namespace_string>: # optional <field1_name>: <field1_value> <field2_name>: <field2_value>

So the parameter name we decided was parameter_namespace_string.field_name. If we want to change this behavior, we can put it as a TODO and add more support after the basic support is done.

Just to keep in mind, a libyaml C parser library is a very very minimalistic parser. It is more of a tokeniser. Just parses each word and returns it. It does not provide any more info and the rcl parser code has to keep track of everything else a yaml specification talks about. It does not even tell if a returned token is a key or a value. Doing multiple levels of mappings, can make the rcl_yaml_parser code very cumbersome. That is why there are lot of other wrappers written around libyamlc to parse fancy things. The biggest advantage of using libyaml C is, it will be comparatively easier to get it through MISRA compared to other parsers. It still has around 3000 MISRA complaints.

Again, if you think we should have mutiple namespace levels and mutiple layers of key:value pairs inside a value like pid: {p: 50.0, i: 0, d: 2}, we can put as a TODO and support more adavanced features later on

Mikael, where can I find the code where mutiple level of parameter namespace gets used (Eg: the gimbal_controller) . Just for me to get an understanding. Do you have a pointer to the code? Thanks

mikaelarguedas · 2018-05-11T02:15:16Z

rcl_yaml_param_parser/src/parser.c

+} data_types_t;
+
+#define MAX_STRING_SZ 128U
+#define PARAMS_KEY "params"


After thinking about it a bit more. A name that cannot be a node name may be more appropriate for this key.
As we forbid double underscores in ROS node/topic names, ros__parameters may be a better candidate

Makes sense. WIll change it

Changed it!

anup-pem · 2018-05-11T02:26:10Z

Ok I got this to compile on Linux with the linked PRs.

Awesome! I saw you libyaml_vendor file

This generates some warnings on MacOS (https://ci.ros2.org/job/ci_osx/3592/warnings11Result/)

Dont have access to Mac and windows build here. WIll look into it. Thanks for running.

I didnt get this to compile on Windows so far. Weirdly Fast-RTPS is failing to build (shouldn't have any interaction with this code...).

Hmmm.... Might be can try re-start the build and see it.

mikaelarguedas · 2018-05-11T02:31:10Z

I didnt get this to compile on Windows so far. Weirdly Fast-RTPS is failing to build (shouldn't have any interaction with this code...).

Hmmm.... Might be can try re-start the build and see it.

I'll look into it with more details but I ruled out the fact it could be host specific or due to internet connectivity as it failed on all our windows machines:
https://ci.ros2.org/job/ci_windows/4449/
https://ci.ros2.org/job/ci_windows/4450/
https://ci.ros2.org/job/ci_windows/4451/

But a build from master at the same time on the same machine succeeded: https://ci.ros2.org/job/ci_windows/4447/

mikaelarguedas · 2018-05-11T02:36:14Z

rcl_yaml_param_parser/include/rcl_yaml_param_parser/types.h

+typedef struct rcl_params_s
+{
+  char ** node_namespaces;  ///< List of namespaces the corresponding node belongs to
+  char ** node_names;  ///< List of names of the node


Actually (sorry for going back and forth on this), there is no real benefit in keeping the node_namespaces and node_names separated so we could do the same as for rcl_node_params_s and have a single char ** node_names that contains node_namespace/node_name instead of 2 different fields

I need to look into it.

Should not be bad

Have removed node_namespaces and just kept nodes names as discussed

mikaelarguedas · 2018-05-11T03:00:38Z

rcl_yaml_param_parser/CMakeLists.txt

+find_package(rcl REQUIRED)
+
+if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic")


Actually this is a C library so we need to set the flags for the C compiler.
A more generic way to do it is:

if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID MATCHES "Clang") add_compile_options(-Wall -Wextra -Wpedantic)

This way the option is added for both C and C++.

Compiling with this generates warnings:

/home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c: In function ‘rcl_yaml_node_struct_print’: /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c:318:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for (int i = 0; i < param_var->bool_array_value->size; i++) { ^ /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c:327:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for (int i = 0; i < param_var->integer_array_value->size; i++) { ^ /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c:335:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for (int i = 0; i < param_var->double_array_value->size; i++) { ^ /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c:343:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for (int i = 0; i < param_var->string_array_value->size; i++) { ^ /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c: In function ‘parse_value’: /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c:619:24: warning: pointer targets in initialization differ in signedness [-Wpointer-sign] const char * value = event.data.scalar.value; ^ /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c: In function ‘parse_key’: /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl_yaml_param_parser/src/parser.c:797:24: warning: pointer targets in initialization differ in signedness [-Wpointer-sign] const char * value = event.data.scalar.value;

Ooops! you are right. Will do that

Fixed the CmakeLists.txt and the warnings

- Changed "params" string to "ros__parameters" - Add -Wall, -Wextra and -Wpedantic falgs - Fix the compile warning with the new flags

anup-pem · 2018-05-11T21:05:16Z

This generates some warnings on MacOS (https://ci.ros2.org/job/ci_osx/3592/warnings11Result/)

Changed to use PRId64 to print int64_t. Hopefully it will stop the warning in Mac

- Remove the node_namespaces entry in rcl_params_t - Change the type of num_nodes and num_params to size_t

- Add install command into CMakelists.txt - Replace one of the zero_allocate with reallocate - Pass allocator state - Fix int32_t -> rcl_ret_t return value

All issues addressed

mikaelarguedas · 2018-05-25T09:45:20Z

New round of CI on the current state of this and ros2/libyaml_vendor#1

Linux
Linux-aarch64
macOS
Windows

Using the yaml_param_parser_shane_review_2 branch:

Windows

sloretz

I'd like to see a change of how the allocator gets passed around, but I'm of the opinion that could happen in a follow up PR. After a couple windows fixes in #241 I think this could be merged.

sloretz · 2018-05-24T01:15:20Z

rcl_yaml_param_parser/include/rcl_yaml_param_parser/parser.h

+RCL_YAML_PARAM_PARSER_PUBLIC
+void rcl_yaml_node_struct_fini(
+  rcl_params_t * params_st,
+  const rcutils_allocator_t allocator);


Storing the allocator in rcl_params_t instead of passing the allocator in to *_fini() prevents accidentally deallocating with a different allocator.

Agree. I think we can move the allocator field in rcl_variant_t into rcl_params_t and initialize it in rcl_yaml_node_struct_init(). Then remove "allocator" variable from other public API's except rcl_yaml_node_struct_init() and use the internal one. If you already have a fix can go ahead with it. If not I can make that change.

Sounds perfect. I don't have the fix already.

Thinking about it, because of the user specific state parameter in allocators, just passing the allocator in init() and cahing it in rcl_params_t and not able to pass the allocator in the fini() and other API's might defeat the purpose of the state field in the allocator. To overcome that, we can store the allocator in rcl_params_t, and let the user pass the allocator in other API's like fini and parse_yaml_file as it is currently implemented.. But, have to check if the incoming allocator is same as the stored allocator except the state value. Does it make sense?

Thinking about it, because of the user specific state parameter in allocators, just passing the allocator in init() and cahing it in rcl_params_t and not able to pass the allocator in the fini() and other API's might defeat the purpose of the state field in the allocator.

I think the expectation is that it's the allocator's state rather than a user's state. For example, someone might implement an allocator that only uses big chunk of memory at the start of the program. Calling allocator.allocate(..., allocator.state) would make the allocator record how much of that initial pool it's used. Since allocator.state is a void * the state pointer itself must be the same when init, parse, and fini are called.

OK, so are you suggesting, allow allocator to be passed in init, parse and fini and check all the fields of the allocator in the beginning of the API of parse and fini to that stored in rcl_params_t? If so, is there a API in rcutils or rcl which checks if two allocators are same or should I just implement one in parser.c to compare two allocators. I could not find a API in rcutils to compare two allocators.

so are you suggesting, allow allocator to be passed in init, parse and fini and check all the fields of the allocator in the beginning of the API of parse and fini to that stored in rcl_params_t?

I recommend only allowing the allocator to be passed into init.

sloretz · 2018-05-24T18:51:12Z

rcl_yaml_param_parser/src/parser.c

+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>


What's this header used for? It's not available on windows. Is it safe to just remove the include? The build succeeds on windows if I comment it out.

I used it for some print formatting stuff but looks like it may not be needed.

sloretz · 2018-05-25T15:27:52Z

rcl_yaml_param_parser/src/parser.c

+  if (NULL == value) {
+    return RCL_RET_INVALID_ARGUMENT;
+  }
+  num_nodes = params_st->num_nodes;  // New node index


On windows I see a warning about a possible loss of data here because of the conversion from size_t to uint32_t

Yaml param parser shane review 2

sloretz · 2018-05-25T18:10:06Z

One more CI run now that #241 was merged

Linux
Linux-aarch64
macOS
Windows

sloretz · 2018-05-25T22:41:29Z

CI

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2018-05-29T15:25:48Z

@sloretz If this now looks good to you,
Do you mind reviewing ros2/libyaml_vendor#1 when you get a chance so that we can go ahead and merge the first version of this?

Thanks!

mikaelarguedas · 2018-05-29T16:04:50Z

Thanks @anup-pem the contribution and for iterating with us on this 👍

Merging this and ros2/libyaml_vendor#1

mikaelarguedas · 2018-05-29T16:26:13Z

I opened a few follow-up tickets. @sloretz @anup-pem feel free to open tickets for the things I missed. Thanks!

anup-pem · 2018-05-29T18:15:54Z

@mikaelarguedas @sloretz Thank you for the great support and also thank you for opening the follow up tickets. I have few more follow up tickets and will fix them in the next phase

dirk-thomas · 2018-05-29T23:58:44Z

rcl_yaml_param_parser/src/parser.c

+            return RCL_RET_BAD_ALLOC;
+          }
+        } else {
+          if (*seq_data_type != val_type) {


This line resulted in a new compiler warning in this job. Please address the new warning as soon as feasible. Thanks.

Looking at it now

mikaelarguedas · 2018-05-31T06:58:37Z

I have few more follow up tickets and will fix them in the next phase

@anup-pem Can you please open issues detailing the work to be done in these tickets ? This will allow us to be sure not to lose track of them as well as allowing other contributors to be aware of them / pick them up.

Thanks!

anup-pem · 2018-06-01T19:13:59Z

@mikaelarguedas Have created new follow up tickets

* Implement a basic YAML based parameter parser for ros2 nodes * Add README file * Fix the issues after moving from rcutils_yaml_param_parser to rcl_yaml_param_parser - rename folders from rcutils_yaml_param_parser to rcl_yaml_param_parser - rename project, header guards and include statements from rcutils_yaml_param_parser to rcl_yaml_param_parser - move type declaration in separat file and namespace new structures - Fix the code review comments from Mikael * Few minor changes - Changed "params" string to "ros__parameters" - Add -Wall, -Wextra and -Wpedantic falgs - Fix the compile warning with the new flags * Fix the changes made in the design of C structure - Remove the node_namespaces entry in rcl_params_t - Change the type of num_nodes and num_params to size_t * depend on libyaml_vendor (#236) * Fix cmake setting standard(C and C++) and add byte_array in C struct * Remove C11 so that it defaults to C99 * [rcl_yaml_param_parser] fix export symbols (#237) * add visibility macros * remove unused macro * Support for multi level node and parameter name spaces * Additional fixes and cleanups - Support for string namespace seperator - Provide parameter structure init function API - name cleanups * off by 1 * Call yaml_parser_delete() * fclose(yaml_file) * free() allocated paths * Call yaml_event_delete() * completely deallcoate string array * Few cleanup changes - Add install command into CMakelists.txt - Replace one of the zero_allocate with reallocate - Pass allocator state - Fix int32_t -> rcl_ret_t return value * Don't include unistd.h * Use size_t for array indices * Just pass the allocator in the init function

rcl_yaml_param_parser/CMakeLists.txt

Anup Pemmaiah and others added 3 commits May 3, 2018 17:36

Implement a basic YAML based parameter parser for ros2 nodes

c19f921

Add README file

7fc07dd

Merge remote-tracking branch 'bar/tmp'

3bbdb6e

anup-pem requested review from wjwwood and mikaelarguedas May 4, 2018 03:43

tfoote added the in progress Actively being worked on (Kanban column) label May 4, 2018

anup-pem self-assigned this May 4, 2018

mikaelarguedas reviewed May 7, 2018

View reviewed changes

anup-pem force-pushed the yaml_param_parser branch from 7c7c387 to 5324a47 Compare May 8, 2018 22:03

This was referenced May 11, 2018

[WIP] add libyaml_vendor to repos file ros2/ros2#491

Merged

create libyaml_vendor ros2/libyaml_vendor#1

Merged

depend on libyaml_vendor #236

Merged

mikaelarguedas reviewed May 11, 2018

View reviewed changes

Few minor changes

dbed970

- Changed "params" string to "ros__parameters" - Add -Wall, -Wextra and -Wpedantic falgs - Fix the compile warning with the new flags

Anup Pemmaiah and others added 2 commits May 11, 2018 16:59

Fix the changes made in the design of C structure

73e912c

- Remove the node_namespaces entry in rcl_params_t - Change the type of num_nodes and num_params to size_t

depend on libyaml_vendor (#236)

8368f8f

Few cleanup changes

787dae4

- Add install command into CMakelists.txt - Replace one of the zero_allocate with reallocate - Pass allocator state - Fix int32_t -> rcl_ret_t return value

sloretz mentioned this pull request May 23, 2018

[WIP] Initialize parameters ros2/rclcpp#477

Closed

10 tasks

sloretz added 2 commits May 24, 2018 12:07

Don't include unistd.h

cc36b7a

Use size_t for array indices

9e54504

sloretz reviewed May 25, 2018

View reviewed changes

Merge pull request #241 from ros2/yaml_param_parser_shane_review_2

b4c2161

Yaml param parser shane review 2

Just pass the allocator in the init function

530bd2b

sloretz approved these changes May 29, 2018

View reviewed changes

mikaelarguedas merged commit 5edf27b into master May 29, 2018

mikaelarguedas deleted the yaml_param_parser branch May 29, 2018 16:08

This was referenced May 29, 2018

[YAML parser] Support bytes type #242

Open

[Parameters] document the yaml format used for parameters ros2/design#175

Open

[YAML parser] Improve print function #243

Open

dirk-thomas reviewed May 29, 2018

View reviewed changes

sloretz mentioned this pull request May 30, 2018

Split ParameterVariant into Parameter and ParameterValue ros2/rclcpp#481

Merged

This was referenced Jun 1, 2018

[YAML parser] Support command line yaml parameter string #246

Closed

[YAML parser] Support for multiple parameter files #247

Closed

clalancette mentioned this pull request Oct 15, 2019

[YAML parser] Configurable maximum values during parsing #244

Closed

dirk-thomas reviewed Apr 17, 2020

View reviewed changes

rcl_yaml_param_parser/CMakeLists.txt Show resolved Hide resolved

YAML parameter parser #235

YAML parameter parser #235

Conversation

anup-pem commented May 4, 2018

anup-pem commented May 5, 2018

anup-pem commented May 7, 2018

mikaelarguedas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anup-pem commented May 8, 2018

mikaelarguedas commented May 8, 2018

mikaelarguedas commented May 8, 2018

mikaelarguedas commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anup-pem May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anup-pem commented May 11, 2018

mikaelarguedas commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anup-pem commented May 11, 2018 • edited Loading

mikaelarguedas commented May 25, 2018 • edited Loading

sloretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz commented May 25, 2018

sloretz commented May 25, 2018

mikaelarguedas commented May 29, 2018

mikaelarguedas commented May 29, 2018

anup-pem May 11, 2018 •

edited

Loading

anup-pem commented May 11, 2018 •

edited

Loading

mikaelarguedas commented May 25, 2018 •

edited

Loading