optional config dir #460
optional config dir #460
Conversation
WalkthroughThe changes introduce a new launch argument, Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
husarion_ugv_manager/launch/manager.launch.py (1)
38-48: Consider normalizing paths to prevent issues with trailing slashesThe current path construction could lead to issues if common_dir_path ends with a slash, potentially resulting in paths with double slashes.
Consider using os.path.join in the PythonExpression:
- husarion_ugv_manager_common_dir = PythonExpression( - [ - "'", - common_dir_path, - "/husarion_ugv_manager' if '", - common_dir_path, - "' else '", - FindPackageShare("husarion_ugv_manager"), - "'", - ] - ) + husarion_ugv_manager_common_dir = PythonExpression( + [ + "os.path.join('", + common_dir_path, + "', 'husarion_ugv_manager') if '", + common_dir_path, + "' else '", + FindPackageShare("husarion_ugv_manager"), + "'", + ] + )husarion_ugv_lights/launch/lights.launch.py (1)
42-52: Consider normalizing paths to prevent issues with trailing slashesSimilar to manager.launch.py, the path construction could be improved to handle trailing slashes.
Consider using os.path.join in the PythonExpression:
- husarion_ugv_lights_common_dir = PythonExpression( - [ - "'", - common_dir_path, - "/husarion_ugv_lights' if '", - common_dir_path, - "' else '", - FindPackageShare("husarion_ugv_lights"), - "'", - ] - ) + husarion_ugv_lights_common_dir = PythonExpression( + [ + "os.path.join('", + common_dir_path, + "', 'husarion_ugv_lights') if '", + common_dir_path, + "' else '", + FindPackageShare("husarion_ugv_lights"), + "'", + ] + )husarion_ugv_localization/launch/localization.launch.py (1)
95-105: Consider simplifying the path construction expressionWhile functional, the nested string concatenation in PythonExpression could be simplified for better readability.
Consider this alternative:
- husarion_ugv_localization_common_dir = PythonExpression( - [ - "'", - common_dir_path, - "/husarion_ugv_localization' if '", - common_dir_path, - "' else '", - FindPackageShare("husarion_ugv_localization"), - "'", - ] - ) + husarion_ugv_localization_common_dir = PythonExpression([ + f"'{common_dir_path}/husarion_ugv_localization' if '{common_dir_path}' else '{FindPackageShare('husarion_ugv_localization')}'" + ])husarion_ugv_controller/launch/controller.launch.py (1)
37-70: Consider extracting common path construction logicThe path construction pattern is repeated across multiple launch files. Consider creating a helper function or shared module.
This would:
- Reduce code duplication
- Ensure consistent path handling
- Make future updates easier
- Centralize the logic for better maintainability
Would you like me to help create a shared module for this common functionality?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
husarion_ugv_bringup/launch/bringup.launch.py(6 hunks)husarion_ugv_controller/launch/controller.launch.py(2 hunks)husarion_ugv_lights/launch/lights.launch.py(2 hunks)husarion_ugv_localization/launch/localization.launch.py(2 hunks)husarion_ugv_manager/launch/manager.launch.py(2 hunks)
🔇 Additional comments (8)
husarion_ugv_manager/launch/manager.launch.py (1)
32-37: LGTM: Launch argument declaration is well-structured
The common_dir_path argument is properly declared with appropriate default value and description.
husarion_ugv_bringup/launch/bringup.launch.py (2)
32-37: LGTM: Launch argument declaration matches other files
The common_dir_path argument declaration is consistent with other launch files.
65-65: LGTM: Consistent propagation of common_dir_path to child launches
The common_dir_path argument is correctly propagated to all relevant child launch files (controller, lights, ekf, and manager).
Also applies to: 87-87, 105-105, 115-115
husarion_ugv_lights/launch/lights.launch.py (1)
36-41: LGTM: Launch argument declaration is consistent
The common_dir_path argument declaration maintains consistency with other launch files.
husarion_ugv_localization/launch/localization.launch.py (2)
89-94: LGTM: Launch argument declaration is well-structured
The new common_dir_path argument is properly declared with a clear description and appropriate default value.
111-111: Verify the impact on existing configurations
The path construction changes might affect existing setups. Please ensure:
- Backward compatibility with existing configurations
- Proper handling of empty/invalid common_dir_path values
Also applies to: 156-156
✅ Verification successful
Let me gather more information about the configuration files and their usage.
Based on the gathered information, I can now provide the final response:
Path construction changes are safely handled with proper fallbacks
The code implements proper handling of configuration paths with built-in safeguards:
- All standard configuration files exist in the package (
relative_localization.yaml,enu_localization.yaml, and their GPS variants) - The
common_dir_pathargument has a safe empty default value - When
common_dir_pathis empty, it automatically falls back to the package's share directory usingFindPackageShare - The path construction is done through ROS 2's
PathJoinSubstitution, ensuring proper path handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing config file references
rg -l "localization_config_path|husarion_ugv_localization.*config" --type python
# Check for potential path construction patterns
ast-grep --pattern 'PathJoinSubstitution([FindPackageShare("husarion_ugv_localization")$$$])'
Length of output: 1772
Script:
#!/bin/bash
# Find all configuration files in the package
fd -e yaml -e xml -e json . husarion_ugv_localization/config/
# Check for any references to configuration files in launch files
rg -t launch "config.*\.(yaml|xml|json)" -A 2 -B 2
# Check the content of the launch file to understand the variable usage
cat husarion_ugv_localization/launch/localization.launch.py
Length of output: 6597
husarion_ugv_controller/launch/controller.launch.py (2)
37-53: LGTM: Launch argument and path construction are consistent
The implementation follows the same pattern as other launch files, maintaining consistency across the codebase.
58-70: Verify dynamic package resolution
The dynamic construction of robot_description_pkg combined with common_dir_path needs careful verification:
- Package resolution when
common_dir_pathis empty - Path construction for different robot models
✅ Verification successful
Based on the search results, I can now verify the dynamic package resolution pattern. The code shows consistent usage across multiple launch files where robot_description_pkg is constructed similarly. The pattern is safe because:
- When
common_dir_pathis empty, it correctly falls back to usingFindPackageShare(robot_description_pkg). - The path construction is consistent with other similar patterns in the codebase (e.g., in
husarion_ugv_lights,husarion_ugv_manager, etc.). - The
PythonExpressionconstruction follows the same pattern used throughout the codebase for conditional path resolution.
Dynamic package resolution verified
The implementation is correct and follows established patterns in the codebase for handling both direct paths and package-based resolution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check robot model package references
rg "robot_description_pkg.*FindPackageShare" --type python
# Verify package resolution patterns
ast-grep --pattern 'PythonExpression([
$$$
FindPackageShare($pkg)
$$$
])'
Length of output: 173
Script:
#!/bin/bash
# Check robot model package references with correct file type
rg "robot_description_pkg.*FindPackageShare" -t py
# Search for PythonExpression usage patterns
rg "PythonExpression.*FindPackageShare" -t py -A 3
# Look for robot_description_pkg variable usage
rg "robot_description_pkg\s*=" -t py -A 2
# Find launch files that might use similar package resolution
fd -e launch.py --exec cat {}
Length of output: 66677
BOOTCFG
force-pushed
the
ros2-devel-better-config-dir
branch
from
919eb28 to
95b49be
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
husarion_ugv_controller/launch/controller.launch.py (1)
37-69: Consider adding validation for common_dir_pathWhile the path construction is correct, consider adding validation to ensure the provided directory exists when non-empty.
common_dir_path = LaunchConfiguration("common_dir_path") +def validate_common_dir(context): + path = context.launch_configurations.get('common_dir_path', '') + if path and not os.path.isdir(path): + raise RuntimeError(f"Specified common_dir_path '{path}' does not exist") + declare_common_dir_path_arg = DeclareLaunchArgument( "common_dir_path", default_value="", description="Path to the common configuration directory.", + validator=validate_common_dir )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
husarion_ugv_bringup/launch/bringup.launch.py(6 hunks)husarion_ugv_controller/launch/controller.launch.py(2 hunks)husarion_ugv_lights/launch/lights.launch.py(2 hunks)husarion_ugv_localization/launch/localization.launch.py(2 hunks)husarion_ugv_manager/launch/manager.launch.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- husarion_ugv_bringup/launch/bringup.launch.py
- husarion_ugv_localization/launch/localization.launch.py
- husarion_ugv_manager/launch/manager.launch.py
🔇 Additional comments (3)
husarion_ugv_lights/launch/lights.launch.py (1)
36-52: LGTM: Well-structured path configuration implementation
The implementation correctly handles both cases:
- When
common_dir_pathis provided: appends/husarion_ugv_lights - When empty: falls back to package share path
The default empty string is appropriate as it maintains backward compatibility.
Let's verify the path construction logic:
✅ Verification successful
Path configuration implementation is correctly structured and safe
The verification confirms that:
- The package has a proper config directory structure with all required YAML files
- The path construction logic is sound:
- When
common_dir_pathis empty (default), it uses package files fromhusarion_ugv_lights/config/ - When
common_dir_pathis provided, it will look for config files under<common_dir_path>/husarion_ugv_lights/config/
- When
- All referenced config files exist in the package:
- Robot-specific:
panther_animations.yaml,lynx_animations.yaml,panther_driver.yaml,lynx_driver.yaml - Common:
user_animations.yaml
- Robot-specific:
The implementation maintains backward compatibility and provides flexibility for custom configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the package structure and potential config locations
# Check if the package exists and its structure
fd --type d "husarion_ugv_lights"
# Look for any existing config files that might be affected
fd --type f --extension yaml "husarion_ugv_lights/config"
Length of output: 571
Script:
#!/bin/bash
# Let's verify the config files and their locations with corrected commands
# Check for YAML files in the package
fd --extension yaml . husarion_ugv_lights
# Check the launch file to understand the config file usage
cat husarion_ugv_lights/launch/lights.launch.py
Length of output: 5696
husarion_ugv_controller/launch/controller.launch.py (2)
37-52: LGTM: Consistent implementation of common directory configuration
The implementation follows the same pattern as other launch files, maintaining consistency across the codebase.
57-69: Verify the dynamic path construction for robot description
The path construction looks correct but involves dynamic package names. We should verify:
- The package naming convention for different robot models
- The existence of config directories in these packages
✅ Verification successful
Let me verify the content of these configuration files to ensure they contain the expected robot descriptions.
Dynamic path construction for robot description is correctly implemented
The verification confirms that:
- Both
lynx_descriptionandpanther_descriptionpackages exist with the expected naming convention - Both packages contain proper
configdirectories with valid YAML configurations:- Wheel configurations (WH*.yaml) with correct robot-specific parameters
- Component configurations for optional sensors/accessories
- The mesh package references in the configs align with the dynamic path construction logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify robot description package naming and structure
# Check for robot description packages
fd --type d "(lynx|panther)_description$"
# Verify config directories in these packages
fd --type d "config$" -p "(lynx|panther)_description"
# Look for any existing controller configurations
fd --type f --extension yaml -p "(lynx|panther)_description/config"
Length of output: 550
Script:
#!/bin/bash
# Check content of config files to verify robot descriptions
for file in lynx_description/config/*.yaml panther_description/config/*.yaml; do
echo "=== $file ==="
cat "$file"
echo
done
Length of output: 3523
Description
Requirements
Tests 🧪
Summary by CodeRabbit
New Features
common_dir_pathacross multiple launch files, enhancing the configurability of the system by allowing users to specify a common configuration directory.common_dir_pathfor improved flexibility.Bug Fixes