Add validity checks for range and defaults #111

jabbenseth · 2018-05-25T12:40:06Z

a) the range for a parameter is impossible to meet
b) the default is out of range

Having those errors in a cfg file can be tedious work to find/debug

a) the range for a parameter is impossible to meet b) the default is out of range

jabbenseth · 2018-06-21T11:47:10Z

ping

Any chance to fix the two failing builds from my side? IMHO they are in no way related to my changes but were existing before (#103) but I may be wrong here

mikaelarguedas · 2018-08-03T23:55:35Z

@ipa-jba Sorry for the long delay on this. I'll try to get around reviewing pending PRs in the next few days.

the failing tests seem to be unrelated to this PR, it can be reproduced on master and is tracked at #103

jabbenseth · 2018-08-06T14:19:18Z

great to hear. However the impact of this pr might be quite substantial and require work on "unknown buggy" packages/config files

mikaelarguedas · 2018-08-06T15:23:51Z

great to hear. However the impact of this pr might be quite substantial and require work on "unknown buggy" packages/config files

Correct, we will need to build every known ROS package using dynamic_reconfigure to see how many do not comply. If this proves to require many changes in downstream packages we will need to hold off that change for the next ROS release (ROS Noetic) and make sure to file PR on every package that requires a change

mikaelarguedas · 2018-09-14T22:55:14Z

@ros-pull-request-builder retest this please

mikaelarguedas · 2018-09-18T00:31:36Z

@ipa-jba I finally found the hours needed to test this on Kinetic (tested all packages depending directly on dynamic reconfigure).

There are several packages failing, some are warranted failures but for some the check seems a bit too restrictive. For example the comparison is made regardless of the parameter type, for string for example it may not make sense to do a comparison (arguably it may not make much sense to give a range to a string parameter in the first place..).

Some packages seem to set out of range values purposefully, I guess it's to reflect the "uninitialized" state. Though to confirm I guess we should reach out to the maintainers of these packages to see if it's a valid assumption.

List of packages failing with this change (10 out of 159):

aruco_detect
create_node
jsk_pcl_ros
jsk_perception
libuvc_camera
naoqi_driver_py
naoqi_sensors_py
sick_scan
tuw_aruco
tuw_marker_pose_estimation

Diff applied to prerelease job

=== ./aruco_detect (git) ===
diff --git a/cfg/DetectorParams.cfg b/cfg/DetectorParams.cfg
index b953cd3..51b3d06 100755
--- a/cfg/DetectorParams.cfg
+++ b/cfg/DetectorParams.cfg
@@ -71,7 +71,7 @@ gen.add("minMarkerPerimeterRate",                 double_t, 0,
 
 gen.add("maxMarkerPerimeterRate",                 double_t, 0,
         "Determine maximum perimeter for marker contour to be detected. This is defined as a rate respect to the maximum dimension of the input image",
-        4.0, 0, 1)
+        4.0, 0, 4.0)
 
 gen.add("minOtsuStdDev",                          double_t, 0,
         "Minimun standard deviation in pixels values during the decodification step to apply Otsu thresholding (otherwise, all the bits are set to 0 or 1 depending on mean higher than 128 or not)",
=== ./create_node (git) ===
diff --git a/cfg/TurtleBot.cfg b/cfg/TurtleBot.cfg
index 9c5ac08..8caef59 100755
--- a/cfg/TurtleBot.cfg
+++ b/cfg/TurtleBot.cfg
@@ -32,7 +32,7 @@ gen.add("odom_angular_scale_correction", double_t, 0, "A correction applied to t
 
 gen.add("odom_linear_scale_correction", double_t, 0, "A correction applied to the computation of the translation in odometry.", 1.0, 0.0, 3.0)
 
-gen.add("min_abs_yaw_vel", double_t, 0, "Minimum angular velocity of the TurtleBot.", None, 0.0, 3.0)
-gen.add("max_abs_yaw_vel", double_t, 0, "Maximum angular velocity of the TurtleBot.", None, 0.0, 3.0)
+gen.add("min_abs_yaw_vel", double_t, 0, "Minimum angular velocity of the TurtleBot.", 0.0, 0.0, 3.0)
+gen.add("max_abs_yaw_vel", double_t, 0, "Maximum angular velocity of the TurtleBot.", 0.0, 0.0, 3.0)
 
 exit( gen.generate(PKG, "TurtleBot", "TurtleBot"))
=== ./jsk_pcl_ros (git) ===
diff --git a/cfg/ClusterPointIndicesDecomposer.cfg b/cfg/ClusterPointIndicesDecomposer.cfg
index c107427..ba39189 100755
--- a/cfg/ClusterPointIndicesDecomposer.cfg
+++ b/cfg/ClusterPointIndicesDecomposer.cfg
@@ -6,7 +6,7 @@ PACKAGE = 'jsk_pcl_ros'
 from dynamic_reconfigure.parameter_generator_catkin import *;
 
 gen = ParameterGenerator ()
-gen.add("max_size", int_t, 0, "the max number of the points of each cluster", -1, 0, 100000)
-gen.add("min_size", int_t, 0, "the minimum number of the points of each cluster", -1, 0, 1000)
+gen.add("max_size", int_t, 0, "the max number of the points of each cluster", 0, 0, 100000)
+gen.add("min_size", int_t, 0, "the minimum number of the points of each cluster", 0, 0, 1000)
 
 exit (gen.generate (PACKAGE, "jsk_pcl_ros", "ClusterPointIndicesDecomposer"))
diff --git a/cfg/EdgebasedCubeFinder.cfg b/cfg/EdgebasedCubeFinder.cfg
index dcdb23a..1bd8cb5 100755
--- a/cfg/EdgebasedCubeFinder.cfg
+++ b/cfg/EdgebasedCubeFinder.cfg
@@ -12,7 +12,7 @@ gen = ParameterGenerator ()
 gen.add("outlier_threshold", double_t, 0, "threshold to rmeove outliers", 0.01, 0.0, 0.1)
 gen.add("min_inliers", int_t, 0, "the minimum number on a edge", 1000, 0, 100000)
 gen.add("convex_area_threshold", double_t, 0, "minimum area of edge supporting face", 0.01, 0.0, 1.0)
-gen.add("convex_edge_threshold", double_t, 0, "", 0.1, 0.0, 0.0)
+gen.add("convex_edge_threshold", double_t, 0, "", 0.1, 0.0, 0.1)
 gen.add("parallel_edge_distance_min_threshold", double_t, 0, "", 0.1, 0.0, 1.0)
 gen.add("parallel_edge_distance_max_threshold", double_t, 0, "", 0.4, 0.0, 1.0)
 
diff --git a/cfg/PeopleDetection.cfg b/cfg/PeopleDetection.cfg
index 8b8935b..65250a0 100755
--- a/cfg/PeopleDetection.cfg
+++ b/cfg/PeopleDetection.cfg
@@ -10,6 +10,6 @@ gen.add('voxel_size', double_t, 0, "voxel size", 0.03, 0.0, 1.0)
 gen.add('min_confidence', double_t, 0, "Minimum confidence of svm's people detector threshold", -1.5, -10.0, 10.0)
 gen.add('box_width', double_t, 0, "The width of bounding_box", 0.5, 0.0, 10.0)
 gen.add('box_depth', double_t, 0, "The depth of bounding_box ", 0.5, 0.0, 10.0)
-gen.add('people_height_threshold', double_t, 0, "The height threshold", 0.0, 1.2, 3.0)
+gen.add('people_height_threshold', double_t, 0, "The height threshold", 1.2, 1.2, 3.0)
 
 exit (gen.generate (PACKAGE, "jsk_pcl_ros", "PeopleDetection"))
=== ./jsk_perception (git) ===
diff --git a/cfg/ColorHistogram.cfg b/cfg/ColorHistogram.cfg
index 1875612..a3d208b 100755
--- a/cfg/ColorHistogram.cfg
+++ b/cfg/ColorHistogram.cfg
@@ -6,11 +6,11 @@ from dynamic_reconfigure.parameter_generator_catkin import *
 
 gen = ParameterGenerator()
 
-gen.add("red_histogram_bin", int_t,    0, "the size of the bin for red", 1, 10, 256)
-gen.add("green_histogram_bin", int_t,    0, "the size of the bin for green", 1, 10, 256)
-gen.add("blue_histogram_bin", int_t,    0, "the size of the bin for blue", 1, 10, 256)
-gen.add("hue_histogram_bin", int_t,    0, "the size of the bin for hue", 1, 10, 180)
-gen.add("saturation_histogram_bin", int_t,    0, "the size of the bin for saturatoin", 1, 10, 256)
-gen.add("intensity_histogram_bin", int_t,    0, "the size of the bin for intensity", 1, 10, 256)
+gen.add("red_histogram_bin", int_t,    0, "the size of the bin for red", 10, 10, 256)
+gen.add("green_histogram_bin", int_t,    0, "the size of the bin for green", 10, 10, 256)
+gen.add("blue_histogram_bin", int_t,    0, "the size of the bin for blue", 10, 10, 256)
+gen.add("hue_histogram_bin", int_t,    0, "the size of the bin for hue", 10, 10, 180)
+gen.add("saturation_histogram_bin", int_t,    0, "the size of the bin for saturation", 10, 10, 256)
+gen.add("intensity_histogram_bin", int_t,    0, "the size of the bin for intensity", 10, 10, 256)
 
 exit(gen.generate(PACKAGE, "color_histogram", "ColorHistogram"))
diff --git a/cfg/SelectiveSearch.cfg b/cfg/SelectiveSearch.cfg
index 19f5a93..75b521a 100755
--- a/cfg/SelectiveSearch.cfg
+++ b/cfg/SelectiveSearch.cfg
@@ -7,5 +7,5 @@ from dynamic_reconfigure.parameter_generator_catkin import *
 gen = ParameterGenerator()
 
 gen.add("min_size", int_t, 0, "Minimum size of bounding box", 1000, 100, 100000)
-gen.add("max_size", int_t, 0, "Maximum size of bounding box", 1000000, 100, 100000)
+gen.add("max_size", int_t, 0, "Maximum size of bounding box", 1000000, 100, 1000000)
 exit(gen.generate(PACKAGE, "jsk_perception", "SelectiveSearch"))
diff --git a/package.xml b/package.xml
index e35cdc7..e01197e 100644
--- a/package.xml
+++ b/package.xml
@@ -76,13 +76,9 @@
   <run_depend>posedetection_msgs</run_depend>
   <!-- install chainer {{ -->
   <run_depend>python-h5py</run_depend>
-  <run_depend>python-chainer-pip</run_depend>
-  <run_depend>python-chainercv-pip</run_depend>
   <!-- }} install chainer -->
-  <run_depend>python-dlib</run_depend>  <!-- pip -->
   <!-- install fcn {{ -->
   <run_depend>leveldb</run_depend>
-  <run_depend>python-fcn-pip</run_depend>  <!-- pip -->
   <!-- }} install fcn -->
   <run_depend>python-sklearn</run_depend>
   <run_depend>python-yaml</run_depend>
=== ./libuvc_camera (git) ===
diff --git a/cfg/UVCCamera.cfg b/cfg/UVCCamera.cfg
index 7c73e90..9feb42b 100755
--- a/cfg/UVCCamera.cfg
+++ b/cfg/UVCCamera.cfg
@@ -92,7 +92,7 @@ gen.add("auto_exposure_priority", int_t, RECONFIGURE_RUNNING,
         0, 0, 1)
 
 gen.add("exposure_absolute", double_t, RECONFIGURE_RUNNING,
-        "Length of exposure, seconds.", 0., 0.0001, 10.0)
+        "Length of exposure, seconds.", 0.0001, 0.0001, 10.0)
 
 # TODO: relative exposure time
=== ./naoqi_driver_py (git) ===
diff --git a/cfg/NaoqiSpeech.cfg b/cfg/NaoqiSpeech.cfg
index cbd4afe..cd861ef 100755
--- a/cfg/NaoqiSpeech.cfg
+++ b/cfg/NaoqiSpeech.cfg
@@ -47,7 +47,7 @@ params = {
     "volume" : {
         "type": pg.int_t,
         "description" : "NAOqi's volume",
-        "default" : -1,
+        "default" : 0,
         "min" : 0,
         "max" : 100
     },
=== ./naoqi_sensors_py (git) ===
diff --git a/cfg/NaoqiCamera.cfg b/cfg/NaoqiCamera.cfg
index 95ac214..7af9680 100755
--- a/cfg/NaoqiCamera.cfg
+++ b/cfg/NaoqiCamera.cfg
@@ -102,7 +102,7 @@ gen.add("auto_exposition", int_t, SensorLevels.RECONFIGURE_RUNNING,
         "Auto exposition.", 1, 0, 1, edit_method = controls)
 
 gen.add("auto_exposure_algo", int_t, SensorLevels.RECONFIGURE_RUNNING,
-        "Auto Exposure Algorithm.", 1, 0, 0, edit_method = exposure_algos)
+        "Auto Exposure Algorithm.", 1, 0, 3, edit_method = exposure_algos)
 
 gen.add("exposure", int_t, SensorLevels.RECONFIGURE_RUNNING,
         "Exposure (time in ms = (value * 2) / 5) (disables auto exposition)", 128, 0, 512)
=== ./sick_scan (git) ===
diff --git a/cfg/SickScan.cfg b/cfg/SickScan.cfg
index d1f4e59..6f33def 100755
--- a/cfg/SickScan.cfg
+++ b/cfg/SickScan.cfg
@@ -62,7 +62,7 @@ gen.add("skip",           int_t,    0, "The number of scans to skip between each
 gen.add("frame_id",       str_t,    0, "The TF frame in which laser scans will be returned.",                        "laser")
 gen.add("time_offset",    double_t, 0, "An offset to add to the time stamp before publication of a scan [s].",       -0.001,     -0.25, 0.25)
 gen.add("auto_reboot",    bool_t,   0, "Whether or not to reboot laser if it reports an error.",                      True)
-gen.add("filter_echos", int_t,  0, "Bitmask to filter first [0], all [1], or last echos [2]", 0, 1, 2)
+gen.add("filter_echos", int_t,  0, "Bitmask to filter first [0], all [1], or last echos [2]", 1, 0, 2)
 gen.add("powerOnCount", int_t	,0, "Read only Power On counter at start up.",                            0,0,65536)
 gen.add("operationHours", double_t	,0, "Read only operationg hours [h].",                            0,0,6553.6)
 gen.add("locationName", str_t,0, "Device Location Name",""),
=== ./tuw_aruco (git) ===
diff --git a/cfg/ArUco.cfg b/cfg/ArUco.cfg
index 303a8db..a45af54 100755
--- a/cfg/ArUco.cfg
+++ b/cfg/ArUco.cfg
@@ -23,7 +23,7 @@ marker_directory_enum = gen.enum([
     gen.const("TAG36h10", str_t, "TAG36h10", "")
 ], "Marker dictonary type")  
 
-gen.add("marker_dictonary", str_t, 0, "Marker dictonary type", "ARTOOLKITPLUSBCH", "ARUCO", "TAG36h10", edit_method=marker_directory_enum)
+gen.add("marker_dictonary", str_t, 0, "Marker dictonary type", "ARUTOOLKITPLUSBCH", "ARUCO", "TAG36h10", edit_method=marker_directory_enum)
 gen.add("marker_size", double_t, 0, "Marker size in meters", 0.06)
 gen.add("publish_tf", bool_t, 0, "", True)
 gen.add("publish_markers", bool_t, 0, "", True)
=== ./tuw_marker_pose_estimation (git) ===
diff --git a/cfg/MarkerMapPoseEstimation.cfg b/cfg/MarkerMapPoseEstimation.cfg
index 6de939d..4963531 100755
--- a/cfg/MarkerMapPoseEstimation.cfg
+++ b/cfg/MarkerMapPoseEstimation.cfg
@@ -8,7 +8,7 @@ pose_estimation_enum = gen.enum([
     gen.const("OPENCV_ITERATIVE", int_t, 0, "OpenCV ITERATIVE"),
 ], "Pose estimation type")
 
-gen.add("pose_estimation_type", int_t, 0, "Pose estimation type", 0, 0, 0, edit_method=pose_estimation_enum)
+gen.add("pose_estimation_type", int_t, 0, "Pose estimation type", 0, 0, 1, edit_method=pose_estimation_enum)
 gen.add("publish_tf", bool_t, 0, "", True)
 gen.add("publish_markers", bool_t, 0, "", True)
 
diff --git a/cfg/MarkerPoseEstimation.cfg b/cfg/MarkerPoseEstimation.cfg
index 3bc6dea..4d394a8 100755
--- a/cfg/MarkerPoseEstimation.cfg
+++ b/cfg/MarkerPoseEstimation.cfg
@@ -8,7 +8,7 @@ pose_estimation_enum = gen.enum([
     gen.const("OPENCV_ITERATIVE", int_t, 0, "OpenCV ITERATIVE"),
 ], "Pose estimation type")
 
-gen.add("pose_estimation_type", int_t, 0, "Pose estimation type", 0, 0, 0, edit_method=pose_estimation_enum)
+gen.add("pose_estimation_type", int_t, 0, "Pose estimation type", 0, 0, 1, edit_method=pose_estimation_enum)
 gen.add("publish_tf", bool_t, 0, "", True)
 gen.add("publish_markers", bool_t, 0, "", True)

mikaelarguedas · 2018-09-18T23:25:44Z

RE:

For example the comparison is made regardless of the parameter type, for string for example it may not make sense to do a comparison (arguably it may not make much sense to give a range to a string parameter in the first place..).

This was actually a broken chekc in dynamic reconfigure and should have failed regardless of this PR (addressed in #122)

mikaelarguedas · 2020-05-01T10:12:58Z

@mjcarroll do you think this can get merged for noetic ? this would be a great improvement for debugging / improving robustness of systems

raise an exception if

7743b85

a) the range for a parameter is impossible to meet b) the default is out of range

jabbenseth changed the title ~~raise an exception if~~ Add validity checks for range and defaults May 25, 2018

jabbenseth changed the base branch from master to melodic-devel June 21, 2018 11:07

jabbenseth changed the base branch from melodic-devel to master June 21, 2018 11:08

mikaelarguedas added this to the ROS Noetic milestone Sep 18, 2018

mikaelarguedas mentioned this pull request Sep 18, 2018

fix check preventing range for string and bool parameters #122

Merged

mikaelarguedas mentioned this pull request Sep 19, 2018

fix range of auto_exposure_algo parameter ros-naoqi/naoqi_bridge#92

Open

mikaelarguedas changed the base branch from master to melodic-devel October 2, 2018 19:01

mjcarroll changed the base branch from melodic-devel to noetic-devel May 1, 2020 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validity checks for range and defaults #111

Add validity checks for range and defaults #111

jabbenseth commented May 25, 2018 •

edited

Loading

jabbenseth commented Jun 21, 2018

mikaelarguedas commented Aug 3, 2018

jabbenseth commented Aug 6, 2018

mikaelarguedas commented Aug 6, 2018

mikaelarguedas commented Sep 14, 2018

mikaelarguedas commented Sep 18, 2018

mikaelarguedas commented Sep 18, 2018

mikaelarguedas commented May 1, 2020

Add validity checks for range and defaults #111

Are you sure you want to change the base?

Add validity checks for range and defaults #111

Conversation

jabbenseth commented May 25, 2018 • edited Loading

jabbenseth commented Jun 21, 2018

mikaelarguedas commented Aug 3, 2018

jabbenseth commented Aug 6, 2018

mikaelarguedas commented Aug 6, 2018

mikaelarguedas commented Sep 14, 2018

mikaelarguedas commented Sep 18, 2018

mikaelarguedas commented Sep 18, 2018

mikaelarguedas commented May 1, 2020

jabbenseth commented May 25, 2018 •

edited

Loading