Add validity checks for range and defaults

[jabbenseth]

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

[jabbenseth]

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]

@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 https://github.com/ros/dynamic_reconfigure/issues/103

[jabbenseth]

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

[mikaelarguedas]

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]

@ros-pull-request-builder retest this please

[mikaelarguedas]

@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 @@ posedetection_msgs python-h5py - python-chainer-pip - python-chainercv-pip - python-dlib leveldb - python-fcn-pip python-sklearn python-yaml === ./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]

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 https://github.com/ros/dynamic_reconfigure/pull/122)

[mikaelarguedas]

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