search

fujitatomoya / ros2_persist_parameter_server

ROS2 parameter server which caches parameter into the storage if necessary and reload it during initialization.
Apache License 2.0
79 stars 14 forks source link

Error when loading a persistent parameter of type array<double> #13

Open sukhrajklair opened 1 year ago

sukhrajklair commented 1 year ago

@fujitatomoya I appreciate the work you've done here.

I ran into an issue when saving and loading an array of doubles. Problem is that the YAML emitter automatically converts the double values whose decimal part is 0 (ex. 1.0) to int. For example, if I set a parameter {1.0, 1.1}, the first number gets saved as 1 in the yaml file. When the parameter_server loads the yaml file, the rcl_yaml_param_parser throws an error saying Failed to parse parameters from custom yaml file '<file-path': Sequence should be of same type. Value type 'integer' do not belong at line_num 5, at ./src/parse.c:337.

The rcl_yaml_param_parser checks that when loading a yaml node that is a sequence, each value of that sequence should have the same type as the sequence itself. https://github.com/ros2/rcl/blob/41b83c8ebfe8344a80c0093083b7abdfded79337/rcl_yaml_param_parser/src/parse.c#L325

I have tried to set tag of the Sequence Node to "!!float" in the StoreYamlFile() function but that doesn't help.

I believe this can be solved by forcing the YAML emitter to write decimal numbers as decimals even if the decimal part is 0. I couldn't figure out how to do it.

fujitatomoya commented 1 year ago

@sukhrajklair thanks for creating issue. this obviously sounds the unexpected behavior, i will take a look when i can find some spare time. in the meantime, could you provide reproducible parameter yaml to load the server or procedure? that would be helpful.

sukhrajklair commented 1 year ago

@fujitatomoya Here are the steps to reproduce the issue:

  1. launch parameter server ros2 launch parameter_server parameter_server.launch.py
  2. set a parameter containing array of doubles with at least one double value ending in .0 ros2 param set /parameter_server persistent.d_array '[1.0,1.1]'
  3. Kill parameter_server and relaunch. At this point, it shows the following error: Catch exception: Failed to parse parameters from custom yaml file '/tmp/parameter_server.yaml': Sequence should be of same type. Value type 'double' do not belong at line_num 5
fujitatomoya commented 1 year ago

@sukhrajklair thanks, i will look into it.

fujitatomoya commented 12 months ago

I believe this can be solved by forcing the YAML emitter to write decimal numbers as decimals even if the decimal part is 0. I couldn't figure out how to do it.

yeah, this is not the problem for either ROS 2 nor parameter server, after pushing the double value to YAML::Node from rclcpp::Parameter, it is already gone wrong [1, 1.1000000000000001]. this hurts us when reloading the saved file to the parameter server, since it deduces 1 as integer, then eventually fails to load the parameter file.

see https://github.com/jbeder/yaml-cpp/issues/1016

fujitatomoya commented 12 months ago

this is obviously yaml-cpp fix required. but if the fix does not backport to v0.7.0 (ubuntu 22.04), we might end up having the patched yaml-cpp in this repository. 1st, i can try the fix to yaml-cpp anyway.

note, this should be described as known issue with possible work-around for user application.

sukhrajklair commented 11 months ago

Thanks for looking into it. I am probably going to work around this by saving the values as a string and then parsing them.

I agree, yaml-cpp needs a fix. However, I don't see the need to check/throw an error when an int is provided where a double is expected by the rcl_yamal_param_parser. Why not just cast the int as a double? Am I missing something?

fujitatomoya commented 11 months ago

I am probably going to work around this by saving the values as a string and then parsing them.

yeah i was thinking the same work-around just like this, which is not really helpful for the application since they need to covert the parameters during read/write. but doable...

I agree, yaml-cpp needs a fix.

a few of consideration.

However, I don't see the need to check/throw an error when an int is provided where a double is expected by the rcl_yamal_param_parser. Why not just cast the int as a double? Am I missing something?

that is very good question, yes and no.

as far as i see current implementation (i am not the one who implemented this 😅 ), in sequence, 1st value type prevails. saing

on the other hand,

i think we can make the above change, but downside is not consistent behavior for user application. for application view point, it sometimes can be parsed, but sometime cannot. to keep the consistent behavior, i say in the sequence 1st one prevails and the following values must be the same type of that. (and i think that is aligned with https://yaml.org/spec/1.2.2/)

sukhrajklair commented 11 months ago

I am probably going to work around this by saving the values as a string and then parsing them.

yeah i was thinking the same work-around just like this, which is not really helpful for the application since they need to covert the parameters during read/write. but doable...

I agree, yaml-cpp needs a fix.

a few of consideration.

  • fix yaml-cpp and backport right away. (best scenario)

looks like the fix mentioned in https://github.com/jbeder/yaml-cpp/issues/1016 was never added. I think this will be helpful to other people as well.

let me know if I can help.

  • change the backend storage into lightweight database such as https://github.com/google/leveldb (opt-in option, because we lose the ROS 2 parameter file compatibility.)

this could have other uses such as centralized configuration management for a fleet of robots.

However, I don't see the need to check/throw an error when an int is provided where a double is expected by the rcl_yamal_param_parser. Why not just cast the int as a double? Am I missing something?

that is very good question, yes and no.

as far as i see current implementation (i am not the one who implemented this 😅 ), in sequence, 1st value type prevails. saing

  • [1, 1.1000000000000001] -> it deduces sequence type is int, because 1st one is the int. and double cannot be handled since we end up the accuracy. and it is likely that this is not user's intention, so fail to parse to notify the user application. i think that is correct behavior.

on the other hand,

  • [1.1000000000000001, 1] -> it deduces sequence type is double, because 1st one is the double. in this case, we could continue parsing since we are not losing any accuracy here. and user might think this can be parsed as double.

i think we can make the above change, but downside is not consistent behavior for user application. for application view point, it sometimes can be parsed, but sometime cannot. to keep the consistent behavior, i say in the sequence 1st one prevails and the following values must be the same type of that. (and i think that is aligned with https://yaml.org/spec/1.2.2/)

That makes sense now. thanks for the explanation :)

fujitatomoya commented 9 months ago

actually i tried the following patch, which does not fix the problem.

diff --git a/include/yaml-cpp/node/convert.h b/include/yaml-cpp/node/convert.h
index 596898d..502deac 100644
--- a/include/yaml-cpp/node/convert.h
+++ b/include/yaml-cpp/node/convert.h
@@ -9,6 +9,7 @@

 #include <array>
 #include <cmath>
+#include <iomanip>
 #include <limits>
 #include <list>
 #include <map>
@@ -109,7 +110,7 @@ inner_encode(const T& rhs, std::stringstream& stream){
       stream << ".inf";
     }
   } else {
-    stream << rhs;
+    stream << std::setprecision(12) << std::fixed << rhs;
   }
 }
fujitatomoya commented 9 months ago

The problem stays in the master: https://github.com/jbeder/yaml-cpp/commit/eaf72053724814be3b99d38e292fca5797a57b7b

fujitatomoya commented 8 months ago

TODO: I guess at least we need to add in documentation as known issue.