Add log level argument to launch files #473
Conversation
|
Warning Rate limit exceeded@miloszlagan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThis pull request introduces a standardized Changes
Sequence DiagramsequenceDiagram
participant User
participant LaunchSystem
participant ROSNodes
User->>LaunchSystem: Specify log_level (optional)
LaunchSystem->>LaunchSystem: Set default to "INFO"
LaunchSystem->>ROSNodes: Pass log_level argument
ROSNodes->>ROSNodes: Configure logging verbosity
Possibly related PRs
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: 2
🧹 Nitpick comments (11)
husarion_ugv_battery/launch/battery.launch.py (1)
44-44: Consider making RCL log level configurableThe RCL log level is hardcoded to "INFO". Consider making it configurable if different RCL logging levels might be needed.
- arguments=["--ros-args", "--log-level", log_level, "--log-level", "rcl:=INFO"], + arguments=["--ros-args", "--log-level", log_level],husarion_ugv_gazebo/launch/simulation.launch.py (1)
42-47: LGTM! Consider adding description choicesThe log level implementation looks good. Consider enhancing the description by documenting the available log level choices (debug, info, warn, error, fatal).
- description="Logging level", + description="Logging level (debug, info, warn, error, fatal)",husarion_ugv_gazebo/launch/spawn_robot.launch.py (1)
33-38: LGTM! Consider adding description choicesThe log level implementation looks good. Consider enhancing the description by documenting the available log level choices.
- description="Logging level", + description="Logging level (debug, info, warn, error, fatal)",husarion_ugv_manager/launch/manager.launch.py (1)
61-66: LGTM! Consider adding description choicesThe log level implementation looks good. Consider enhancing the description by documenting the available log level choices.
- description="Logging level", + description="Logging level (debug, info, warn, error, fatal)",lynx_description/launch/load_urdf.launch.py (2)
87-92: Enhance the log level argument descriptionThe description could be more detailed to specify:
- Available log levels (debug, info, warn, error, fatal)
- Impact on node verbosity
declare_log_level_arg = DeclareLaunchArgument( "log_level", default_value="info", - description="Logging level", + description="Logging level (debug, info, warn, error, fatal). Controls verbosity of the nodes.", )
187-187: Consider extracting duplicate log level argumentsThe log level configuration is duplicated across nodes. Consider extracting it into a variable for better maintainability.
+ log_args = ["--ros-args", "--log-level", log_level, "--log-level", "rcl:=INFO"] + robot_state_pub_node = Node( ... - arguments=["--ros-args", "--log-level", log_level, "--log-level", "rcl:=INFO"], + arguments=log_args, ... ) joint_state_publisher_node = Node( ... - arguments=["--ros-args", "--log-level", log_level, "--log-level", "rcl:=INFO"], + arguments=log_args, ... )Also applies to: 179-179
husarion_ugv_gazebo/launch/simulate_robot.launch.py (2)
89-94: Enhance the log level argument descriptionSimilar to other launch files, the description could be more detailed.
declare_log_level_arg = DeclareLaunchArgument( "log_level", default_value="info", - description="Logging level", + description="Logging level (debug, info, warn, error, fatal). Controls verbosity of the nodes.", )
242-246: Improve log level arguments formatting in world_transformThe log level arguments are split across multiple lines, which differs from the more concise format used in other nodes. Consider maintaining consistency.
world_transform = Node( ... arguments=[ ... - "--ros-args", - "--log-level", - log_level, - "--log-level", - "rcl:=INFO", + "--ros-args", "--log-level", log_level, "--log-level", "rcl:=INFO", ], ... )husarion_ugv_controller/launch/controller.launch.py (2)
120-125: Enhance log level argument descriptionConsider expanding the description to include available log levels (debug, info, warn, error, fatal) to improve usability.
declare_log_level_arg = DeclareLaunchArgument( "log_level", default_value="info", - description="Logging level", + description="Logging level (debug, info, warn, error, fatal)", )
292-296: Consider refactoring repeated log level configurationThe log level configuration pattern is repeated across all spawner nodes. Consider creating a helper function to generate the common arguments.
def get_spawner_arguments(controller_name, log_level): return [ controller_name, "--controller-manager", "controller_manager", "--controller-manager-timeout", "10", "--ros-args", "--log-level", log_level, "--log-level", "rcl:=INFO", ]Also applies to: 319-323
README.md (1)
105-105: Enhance log level documentation and fix table formatting
- Consider expanding the log level documentation to include available options.
- Fix the table formatting by adding the trailing pipe.
-| ✅ | ✅ | `log_level` | Sets verbosity of launched nodes. <br/> ***string:*** `info` +| ✅ | ✅ | `log_level` | Sets verbosity of launched nodes (debug, info, warn, error, fatal). <br/> ***string:*** `info` |🧰 Tools
🪛 Markdownlint (0.37.0)
105-105: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style(MD055, table-pipe-style)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(1 hunks)husarion_ugv_battery/launch/battery.launch.py(2 hunks)husarion_ugv_bringup/launch/bringup.launch.py(8 hunks)husarion_ugv_controller/launch/controller.launch.py(5 hunks)husarion_ugv_diagnostics/launch/system_monitor.launch.py(2 hunks)husarion_ugv_gazebo/launch/simulate_robot.launch.py(9 hunks)husarion_ugv_gazebo/launch/simulation.launch.py(2 hunks)husarion_ugv_gazebo/launch/spawn_robot.launch.py(3 hunks)husarion_ugv_lights/launch/lights.launch.py(3 hunks)husarion_ugv_localization/launch/localization.launch.py(5 hunks)husarion_ugv_localization/launch/nmea_navsat.launch.py(2 hunks)husarion_ugv_manager/launch/manager.launch.py(3 hunks)lynx_description/launch/load_urdf.launch.py(3 hunks)panther_description/launch/load_urdf.launch.py(3 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
105-105: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style
(MD055, table-pipe-style)
🔇 Additional comments (18)
husarion_ugv_battery/launch/battery.launch.py (1)
24-29: LGTM: Log level argument properly implemented
The log level configuration follows ROS2 best practices with appropriate default value and description.
husarion_ugv_diagnostics/launch/system_monitor.launch.py (2)
29-34: LGTM: Consistent log level implementation
The log level configuration matches the pattern used across other launch files.
63-63: Consider making RCL log level configurable
Similar to other launch files, the RCL log level is hardcoded to "INFO".
husarion_ugv_localization/launch/nmea_navsat.launch.py (3)
29-34: LGTM: Consistent log level implementation
The log level configuration maintains consistency with other launch files.
71-71: Consider making RCL log level configurable
Similar to other launch files, the RCL log level is hardcoded to "INFO".
Line range hint 53-71: Verify logging behavior with remappings
The node has complex topic remappings. Let's verify that log messages properly reflect the remapped topics.
✅ Verification successful
Topic remappings are safe and logging is properly configured
The node's logging configuration is properly set up with:
- A configurable log level via the
log_levellaunch argument - Default RCL logging set to "INFO" level
- The remapped topics will be correctly reflected in the logs as the remappings are applied at the ROS node level
The remappings follow the standard ROS 2 convention and will be properly handled by the ROS 2 logging system, which automatically uses the remapped topic names in log messages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing logging statements that might reference the remapped topics
# Search for logging statements in the nmea_navsat_driver package
rg -t py "(?i)(logger|log|debug|info|warn|error|fatal).*?(?:fix|time_reference|vel|heading)"
# Search for any potential logging configuration in the package
rg -t yaml "logger|log_level" $(fd -t f ".*\.yaml$")
Length of output: 11931
Script:
#!/bin/bash
# Let's check the nmea_navsat_driver package for any logging related to topics
rg -t py "(?i)(logger|log|debug|info|warn|error|fatal).*?(fix|time_reference|vel|heading)" $(fd -t f ".*" -d 1 /opt/ros/humble/share/nmea_navsat_driver/)
# Let's also check if the package is installed and its source code is available
fd "nmea_navsat_driver" /opt/ros/humble/share/
# Let's check the configuration file for any logging settings
cat husarion_ugv_localization/config/nmea_navsat.yaml
Length of output: 10593
husarion_ugv_gazebo/launch/simulation.launch.py (1)
101-101: LGTM! Proper propagation of log level
The log level is correctly passed down to the simulate_robots launch description.
husarion_ugv_manager/launch/manager.launch.py (1)
Line range hint 1-1: Verify consistent log level implementation across all launch files
The PR mentions updating multiple launch files. Let's verify the consistency of the log level implementation across all of them.
✅ Verification successful
Let me gather more information to check if there are any inconsistencies in how the log level parameter is declared and used across these files.
Log level implementation is consistent across all launch files
The verification shows that log level implementation is consistent across all launch files:
- All files declare the log_level argument with the same default value "info"
- The pattern
arguments=["--ros-args", "--log-level", log_level, "--log-level", "rcl:=INFO"]is consistently used - The declaration and usage of log_level follows the same structure in all files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent log level implementation across launch files
# Expected: All launch files should have similar log level argument declarations and usage
# Find all launch files
echo "Checking launch files for log level implementation..."
fd -e launch.py | xargs rg -l "log_level"
# Check for potential inconsistencies in log level usage
echo "\nChecking for potential inconsistencies in log level arguments..."
fd -e launch.py | xargs rg "log_level.*rcl:=INFO"
Length of output: 2647
Script:
#!/bin/bash
# Check how log_level parameter is declared and used across launch files
echo "Checking log_level parameter declarations..."
fd -e launch.py | xargs rg "DeclareLaunchArgument.*log_level"
echo "\nChecking log_level parameter default values..."
fd -e launch.py | xargs rg "default_value.*log_level"
echo "\nChecking for any other log level patterns that might be inconsistent..."
fd -e launch.py | xargs rg "log[_-]level" -A 1
Length of output: 14136
husarion_ugv_bringup/launch/bringup.launch.py (2)
47-52: LGTM! Well-structured log level argument declaration.
The log level argument is properly declared with a clear description and sensible default value.
72-76: LGTM! Consistent propagation of log level to all included launches.
The log_level argument is consistently passed to all included launch files, ensuring uniform logging configuration across components.
Also applies to: 89-89, 98-102, 111-111, 120-124, 134-138
husarion_ugv_lights/launch/lights.launch.py (2)
63-68: LGTM! Well-structured log level argument declaration.
The log level argument is properly declared with a clear description and sensible default value.
134-134: LGTM! Smart combination of configurable and fixed log levels.
Good practice: Setting rcl:=INFO alongside the configurable log level ensures that RCL framework logging remains at INFO while allowing component-specific logging to be configured.
husarion_ugv_localization/launch/localization.launch.py (2)
60-65: LGTM! Well-structured log level argument declaration.
The log level argument is properly declared with a clear description and sensible default value.
135-135: LGTM! Consistent log level configuration across nodes.
The log level configuration is consistently applied to all nodes and included launches:
- ekf_filter_node
- nmea_navsat_launch
- navsat_transform_node
Good practice: Setting rcl:=INFO alongside the configurable log level ensures that RCL framework logging remains at INFO while allowing component-specific logging to be configured.
Also applies to: 145-145, 160-160
panther_description/launch/load_urdf.launch.py (1)
86-91: Same improvements apply as in lynx_description
The changes in this file mirror those in lynx_description/launch/load_urdf.launch.py and would benefit from the same improvements.
Also applies to: 179-179, 187-187
husarion_ugv_gazebo/launch/simulate_robot.launch.py (1)
118-118: LGTM: Consistent log level propagation
The log_level argument is consistently propagated to all included launch files and nodes, which ensures uniform logging configuration across the system.
Also applies to: 129-133, 142-146, 161-161, 178-182, 215-215
husarion_ugv_controller/launch/controller.launch.py (2)
349-349: LGTM!
The log level argument is correctly integrated into the launch sequence.
273-277: Review duplicate log level configuration
The node has two --log-level arguments. While setting a fixed rcl level is valid, consider documenting why rcl logs are fixed to INFO level to avoid confusion.
✅ Verification successful
Pattern is consistent across the codebase
The duplicate --log-level configuration with rcl:=INFO is a consistent pattern used throughout the codebase in multiple launch files. This appears to be an intentional design choice to allow user-specified log levels for components while maintaining a fixed INFO level for RCL logs. No changes needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other spawner nodes in the codebase follow similar pattern
rg -A 2 "spawner.*--log-level.*rcl:=INFO"
Length of output: 43
Script:
#!/bin/bash
# Search for any controller spawner nodes with log level configuration
rg -B 2 -A 2 "controller.*spawner.*--log-level"
# Also search for any log level configurations in launch files
rg -B 2 -A 2 '"--log-level"' '*.launch.py'
Length of output: 146
Script:
#!/bin/bash
# Find launch files first
fd ".launch.py$"
# Then search for log level configurations in all Python files
rg -B 2 -A 2 '"--log-level"' -g '*.py'
Length of output: 8761
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
husarion_ugv_controller/launch/controller.launch.py (1)
120-125: Enhance the log level argument descriptionThe description could be more helpful by listing the available log levels (debug, info, warn, error, fatal).
- description="Logging level", + description="Logging level (debug, info, warn, error, fatal)",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
husarion_ugv_controller/launch/controller.launch.py(6 hunks)husarion_ugv_gazebo/launch/simulate_robot.launch.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- husarion_ugv_gazebo/launch/simulate_robot.launch.py
🔇 Additional comments (3)
husarion_ugv_controller/launch/controller.launch.py (3)
268-280: LGTM! Appropriate log level configuration
The implementation correctly applies the user-specified log level while maintaining INFO level for critical components, which aligns with the PR objectives.
305-309: Consider keeping spawner at INFO level
Since the spawner is a short-lived process, debug logs might not provide significant value. Consider removing the configurable log level for spawners and keeping them at INFO level.
376-376: LGTM! Proper launch argument registration
The log level argument is correctly registered in the launch description.
|
I've reworked ros2_control_node verbosity. It should work in debug level now without flooding console with unnecessary logs |
|
LGTM but please resolve the conflicts |
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (2)
105-105: Enhance the log_level argument documentationThe description for the new
log_levelargument could be more detailed. Consider adding:
- Available log level choices (e.g., debug, info, warn, error)
- Example usage in command line
- Note about excluded nodes (robot_state_publisher, ros2_control_node) as mentioned in PR objectives
-| ✅ | ✅ | `log_level` | Sets verbosity of launched nodes. <br/> ***string:*** `info` +| ✅ | ✅ | `log_level` | Sets verbosity level for launched nodes. Some nodes (robot_state_publisher, ros2_control_node) maintain their default logging level. <br/> ***string:*** `info` (choices: `debug`, `info`, `warn`, `error`)🧰 Tools
🪛 Markdownlint (0.37.0)
105-105: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style(MD055, table-pipe-style)
105-105: Fix table formatting: Add missing trailing pipeFor consistency with the rest of the table formatting, add a trailing pipe to the line.
-| ✅ | ✅ | `log_level` | Sets verbosity of launched nodes. <br/> ***string:*** `info` +| ✅ | ✅ | `log_level` | Sets verbosity of launched nodes. <br/> ***string:*** `info` |🧰 Tools
🪛 Markdownlint (0.37.0)
105-105: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style(MD055, table-pipe-style)
husarion_ugv_gazebo/launch/simulation.launch.py (2)
42-47: Enhance the log level argument descriptionWhile the implementation is correct, consider enhancing the description to list the available log levels (debug, info, warn, error, fatal) to improve usability.
declare_log_level_arg = DeclareLaunchArgument( "log_level", default_value="info", - description="Logging level", + description="Logging level (debug, info, warn, error, fatal)", )
Line range hint
82-82: Consider making gz_log_level configurableCurrently, gz_log_level is hardcoded to "1". Consider making it configurable through a launch argument, similar to log_level, to provide consistent logging control across both ROS2 and Gazebo components.
gz_sim = IncludeLaunchDescription( PythonLaunchDescriptionSource( PathJoinSubstitution( [FindPackageShare("husarion_gz_worlds"), "launch", "gz_sim.launch.py"] ) ), - launch_arguments={"gz_gui": namespaced_gz_gui, "gz_log_level": "1"}.items(), + launch_arguments={ + "gz_gui": namespaced_gz_gui, + "gz_log_level": LaunchConfiguration("gz_log_level", default="1") + }.items(), )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)husarion_ugv_controller/launch/controller.launch.py(6 hunks)husarion_ugv_gazebo/launch/simulate_robot.launch.py(8 hunks)husarion_ugv_gazebo/launch/simulation.launch.py(2 hunks)husarion_ugv_gazebo/launch/spawn_robot.launch.py(3 hunks)husarion_ugv_lights/launch/lights.launch.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- husarion_ugv_lights/launch/lights.launch.py
- husarion_ugv_gazebo/launch/simulate_robot.launch.py
- husarion_ugv_gazebo/launch/spawn_robot.launch.py
- husarion_ugv_controller/launch/controller.launch.py
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
105-105: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style
(MD055, table-pipe-style)
🔇 Additional comments (4)
README.md (2)
Line range hint 109-109: LGTM: Clear and well-documented robot_model argument
The updated description clearly specifies the purpose and available choices for the robot model configuration.
🧰 Tools
🪛 Markdownlint (0.37.0)
105-105: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style
(MD055, table-pipe-style)
Line range hint 1-150: LGTM: Well-structured and comprehensive documentation
The README maintains excellent organization and clarity while incorporating the new changes. The use of compatibility indicators (🤖/🖥️) and detailed descriptions helps users understand the available configuration options.
🧰 Tools
🪛 Markdownlint (0.37.0)
105-105: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style
(MD055, table-pipe-style)
husarion_ugv_gazebo/launch/simulation.launch.py (2)
106-106: LGTM!
The declare_log_level_arg is correctly placed in the actions list, ensuring proper initialization before usage.
101-101: Verify log_level propagation
The log_level is correctly passed to simulate_robot.launch.py. Let's verify that the downstream launch file properly handles this argument.
✅ Verification successful
Log level argument is properly declared and utilized
The verification shows that simulate_robot.launch.py correctly:
- Declares the log_level argument with a default value of "info"
- Propagates the log_level to multiple downstream components:
- Robot spawn configuration
- Lights launch
- Manager launch
- ROS args configuration (--log-level parameter)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that simulate_robot.launch.py accepts and uses the log_level argument
# Check if simulate_robot.launch.py declares the log_level argument
rg -l "DeclareLaunchArgument.*log_level" $(fd -t f "simulate_robot.launch.py")
# Check how the log_level is used in the launch file
rg "log_level" $(fd -t f "simulate_robot.launch.py") -A 5
Length of output: 1786
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
husarion_ugv_utils/husarion_ugv_utils/logging.py (1)
21-21: Add input validation for unit parameter.The function should validate that the
unitparameter is not empty or None to prevent runtime errors.Consider adding validation:
def limit_log_level_to_info(unit: SomeSubstitutionsType, log_level: SomeSubstitutionsType): if not unit: raise ValueError("Unit parameter cannot be empty")husarion_ugv_diagnostics/launch/system_monitor.launch.py (1)
64-70: Document the dual log level configuration.The node is configured with two log level settings:
- ROS-level logging
- RCL-level logging with potential info-level limitation
Consider adding a comment explaining this behavior for future maintainers.
Add this comment above the arguments:
# Configure both ROS and RCL logging levels # RCL logging is limited to info level when debug is specifiedhusarion_ugv_controller/launch/controller.launch.py (1)
20-20: Consider optimizing namespace handling for log units.While the implementation is correct, the namespace handling for log units could be optimized by creating a single reusable namespace expression.
- joint_state_broadcaster_log_unit = PythonExpression( - [ - "'", - namespace, - "' + '.joint_state_broadcaster' if '", - namespace, - "' else 'joint_state_broadcaster'", - ] - ) - controller_manager_log_unit = PythonExpression( - [ - "'", - namespace, - "' + '.controller_manager' if '", - namespace, - "' else 'controller_manager'", - ] - ) + namespace_with_dot = PythonExpression(["'", namespace, "' + '.' if '", namespace, "' else ''"]) + joint_state_broadcaster_log_unit = PythonExpression([namespace_with_dot, "'joint_state_broadcaster'"]) + controller_manager_log_unit = PythonExpression([namespace_with_dot, "'controller_manager'"])Also applies to: 121-126, 218-235
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
husarion_ugv_battery/launch/battery.launch.py(2 hunks)husarion_ugv_controller/launch/controller.launch.py(7 hunks)husarion_ugv_controller/package.xml(1 hunks)husarion_ugv_diagnostics/launch/system_monitor.launch.py(3 hunks)husarion_ugv_gazebo/launch/simulate_robot.launch.py(9 hunks)husarion_ugv_gazebo/launch/spawn_robot.launch.py(4 hunks)husarion_ugv_lights/launch/lights.launch.py(4 hunks)husarion_ugv_localization/launch/localization.launch.py(6 hunks)husarion_ugv_localization/launch/nmea_navsat.launch.py(3 hunks)husarion_ugv_localization/package.xml(1 hunks)husarion_ugv_manager/launch/manager.launch.py(4 hunks)husarion_ugv_utils/husarion_ugv_utils/logging.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- husarion_ugv_gazebo/launch/spawn_robot.launch.py
- husarion_ugv_gazebo/launch/simulate_robot.launch.py
- husarion_ugv_battery/launch/battery.launch.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pre-commit / pre-commit
🔇 Additional comments (8)
husarion_ugv_localization/package.xml (1)
18-18: LGTM!The addition of
husarion_ugv_utilsas an execution dependency is correct and necessary for the new logging functionality.husarion_ugv_controller/package.xml (1)
23-23: LGTM!The addition of
husarion_ugv_utilsas an execution dependency is correct and necessary for the new logging functionality.husarion_ugv_diagnostics/launch/system_monitor.launch.py (1)
30-35: LGTM!The log level configuration is well-structured with a sensible default value.
husarion_ugv_localization/launch/nmea_navsat.launch.py (1)
17-17: LGTM! Clean implementation of log level configuration.The changes correctly implement the log level configuration:
- Proper import of the logging utility
- Clear declaration of the log level argument with a sensible default
- Correct usage in the node configuration with rcl level limiting
Also applies to: 30-35, 72-78, 81-88
husarion_ugv_manager/launch/manager.launch.py (1)
17-17: LGTM! Consistent implementation of log level configuration.The changes correctly implement the log level configuration for both nodes, properly utilizing the
limit_log_level_to_infoutility to manage rcl logging levels.Also applies to: 62-67, 114-120, 136-142, 149-149
husarion_ugv_lights/launch/lights.launch.py (1)
19-19: LGTM! Well-structured log level configuration with plugin loader consideration.The changes correctly implement the log level configuration with appropriate handling of both rcl and pluginlib.ClassLoader logging levels.
Also applies to: 64-69, 135-143, 152-152
husarion_ugv_localization/launch/localization.launch.py (1)
17-17: LGTM! Comprehensive log level configuration with proper propagation.The changes correctly implement the log level configuration:
- Proper configuration for all nodes
- Correct propagation to included launch files
- Consistent use of the logging utility
Also applies to: 61-66, 136-142, 152-152, 167-173, 183-183
husarion_ugv_controller/launch/controller.launch.py (1)
265-277: LGTM! Comprehensive log level configuration with proper component-specific handling.The changes correctly implement the log level configuration with appropriate handling for:
- ROS client library (rcl)
- Plugin loader
- Joint state broadcaster
- Controller manager
Also applies to: 302-306, 343-347, 373-373
| def limit_log_level_to_info(unit: SomeSubstitutionsType, log_level: SomeSubstitutionsType): | ||
| log_level = PythonExpression(["'", log_level, "'.lower()'"]) | ||
|
|
||
| if PythonExpression(["'", log_level, "' == 'debug'"]): | ||
| return PythonExpression([unit, "+ ':=' + 'info'"]) | ||
| else: | ||
| return PythonExpression([unit, "+ ':=' + ", log_level]) |
There was a problem hiding this comment.
Fix string concatenation and comparison in PythonExpressions.
The current implementation has issues with string handling in PythonExpressions:
- Incorrect string concatenation for lowercase conversion
- Incorrect string comparison for debug level check
Apply this diff to fix the issues:
def limit_log_level_to_info(unit: SomeSubstitutionsType, log_level: SomeSubstitutionsType):
- log_level = PythonExpression(["'", log_level, "'.lower()'"])
+ log_level = PythonExpression(['"' + str(log_level) + '".lower()'])
- if PythonExpression(["'", log_level, "' == 'debug'"]):
+ if PythonExpression([str(log_level) + ' == "debug"']):
return PythonExpression([unit, "+ ':=' + 'info'"])
else:
return PythonExpression([unit, "+ ':=' + ", log_level])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def limit_log_level_to_info(unit: SomeSubstitutionsType, log_level: SomeSubstitutionsType): | |
| log_level = PythonExpression(["'", log_level, "'.lower()'"]) | |
| if PythonExpression(["'", log_level, "' == 'debug'"]): | |
| return PythonExpression([unit, "+ ':=' + 'info'"]) | |
| else: | |
| return PythonExpression([unit, "+ ':=' + ", log_level]) | |
| def limit_log_level_to_info(unit: SomeSubstitutionsType, log_level: SomeSubstitutionsType): | |
| log_level = PythonExpression(['"' + str(log_level) + '".lower()']) | |
| if PythonExpression([str(log_level) + ' == "debug"']): | |
| return PythonExpression([unit, "+ ':=' + 'info'"]) | |
| else: | |
| return PythonExpression([unit, "+ ':=' + ", log_level]) |
|
I have limited |
|
Releated: #477 |
|
Please resolve conflicts |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
husarion_ugv_utils/husarion_ugv_utils/logging.py (1)
21-27: 🛠️ Refactor suggestionRefactor string handling in PythonExpression.
This code uses string concatenations like["'", log_level, "'.upper()'"]and["'", log_level, "' == 'DEBUG'"], which can produce unintended expressions, especially iflog_levelis not precisely set to"DEBUG"or if it contains mixed-case text. Likewise, the'usage makes the expression awkward to read and prone to errors.As a best practice, unify the string handling to produce a cleaner, more reliable Python expression. For example:
-def limit_log_level_to_info(unit: SomeSubstitutionsType, log_level: SomeSubstitutionsType): - log_level = PythonExpression(["'", log_level, "'.upper()'"]) - if PythonExpression(["'", log_level, "' == 'DEBUG'"]): - return PythonExpression(["'", unit, "' + ':=' + 'INFO'"]) - else: - return PythonExpression(["'", unit, "' + ':=' + ", log_level]) +def limit_log_level_to_info(unit: SomeSubstitutionsType, log_level: SomeSubstitutionsType): + # Convert the log level to uppercase reliably + log_level_upper = PythonExpression([f'"{{}}".format(', log_level, ').upper()']) + + # Compare against "DEBUG" explicitly + return PythonExpression([ + "({0} == 'DEBUG') and ('{1}:=INFO') or ('{1}:=' + ".format(log_level_upper, unit), + log_level_upper, + ")" + ])This approach ensures:
- The string conversion and comparison are straightforward.
- Mixed or inconsistent usage of quotes is avoided.
husarion_ugv_gazebo/launch/spawn_robot.launch.py (1)
127-131:⚠️ Potential issueFix duplicate log level argument
The
--log-levelargument appears twice in the spawn_robot node arguments. The second occurrence might override the user-specified log level.Apply this diff to fix the issue:
"--ros-args", "--log-level", log_level, - "--log-level", - limit_log_level_to_info("rcl", log_level),
🧹 Nitpick comments (3)
husarion_ugv_battery/launch/battery.launch.py (1)
46-52: Good usage of dynamic ROS log-level arguments.
Passing both--log-leveland thelimit_log_level_to_info("rcl", log_level)argument helps keep RCL logging from exploding at debug level. Consider following the same pattern for other deep internals (ClassLoader, etc.) if needed in the future.husarion_ugv_controller/launch/controller.launch.py (1)
219-236: LGTM: Well-structured namespace-aware log units.The implementation properly handles namespaced logging units. Consider extracting the namespace concatenation logic into a helper function to reduce duplication.
+def create_namespaced_log_unit(namespace: LaunchConfiguration, unit_name: str) -> PythonExpression: + return PythonExpression([ + "'", + namespace, + "' + '." + unit_name + "' if '", + namespace, + "' else '" + unit_name + "'", + ]) -joint_state_broadcaster_log_unit = PythonExpression(...) -controller_manager_log_unit = PythonExpression(...) +joint_state_broadcaster_log_unit = create_namespaced_log_unit(namespace, "joint_state_broadcaster") +controller_manager_log_unit = create_namespaced_log_unit(namespace, "controller_manager")README.md (1)
105-105: Fix table formatting.Add a trailing pipe to maintain consistent table formatting:
-| ✅ | ✅ | `log_level` | Sets verbosity of launched nodes. <br/> ***string:*** `INFO` +| ✅ | ✅ | `log_level` | Sets verbosity of launched nodes. <br/> ***string:*** `INFO` |🧰 Tools
🪛 Markdownlint (0.37.0)
105-105: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style(MD055, table-pipe-style)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(1 hunks)husarion_ugv_battery/launch/battery.launch.py(2 hunks)husarion_ugv_bringup/launch/bringup.launch.py(8 hunks)husarion_ugv_controller/launch/controller.launch.py(7 hunks)husarion_ugv_diagnostics/launch/system_monitor.launch.py(3 hunks)husarion_ugv_gazebo/launch/simulate_robot.launch.py(9 hunks)husarion_ugv_gazebo/launch/simulation.launch.py(2 hunks)husarion_ugv_gazebo/launch/spawn_robot.launch.py(4 hunks)husarion_ugv_lights/launch/lights.launch.py(4 hunks)husarion_ugv_localization/launch/localization.launch.py(6 hunks)husarion_ugv_localization/launch/nmea_navsat.launch.py(3 hunks)husarion_ugv_manager/launch/manager.launch.py(4 hunks)husarion_ugv_utils/husarion_ugv_utils/logging.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- husarion_ugv_gazebo/launch/simulation.launch.py
- husarion_ugv_gazebo/launch/simulate_robot.launch.py
- husarion_ugv_manager/launch/manager.launch.py
- husarion_ugv_lights/launch/lights.launch.py
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
105-105: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style
(MD055, table-pipe-style)
🔇 Additional comments (32)
husarion_ugv_utils/husarion_ugv_utils/logging.py (1)
1-16: Good license header and file setup.
No issues noted in the shebang, license header, and import statements.husarion_ugv_battery/launch/battery.launch.py (3)
17-17: Import statement looks correct.
The import oflimit_log_level_to_infois properly referenced.
25-31: Clear and concise declaration of thelog_levelargument.
Thechoicesparameter improves user experience by restricting inputs to recognized logging levels.
57-57: Actions list is well organized.
Including the declared arguments and node in the final actions ensures clarity and maintainability in the launch description.husarion_ugv_diagnostics/launch/system_monitor.launch.py (4)
17-17: Correct import for logging helper.
Looks good.
30-36: Consistentlog_leveldeclaration with choices.
Appears consistent with the rest of the launch files, ensuring a uniform user interface.
65-71: Proper dynamic logging configuration for the system monitor.
Similar to the battery node, this ensuresrcllogs are constrained if the user selects a debug setting.
76-76: Actions list is clear and comprehensive.
No issues found.husarion_ugv_localization/launch/nmea_navsat.launch.py (5)
17-17: Import pattern matches other files.
Ensures uniform usage of the utility function.
30-36: Well-defined set of valid log levels.
Keeps usage consistent throughout the codebase.
73-79: NMEA driver node logging arguments.
The addition of--ros-argsandlimit_log_level_to_info("rcl", log_level)is well-integrated.
82-87: Actions list includes newlog_levelargument.
Clearly presented and mirrors other files.
89-89: Final return statement is concise and readable.
This finalLaunchDescription(actions)pattern is consistent with typical ROS 2 launch standards.husarion_ugv_gazebo/launch/spawn_robot.launch.py (3)
18-18: LGTM!The import statement for the logging utility is correctly placed and follows Python import conventions.
33-39: LGTM!The log level argument is properly declared with appropriate default value and valid choices.
100-100: LGTM!The log level argument is correctly passed to the included launch file.
husarion_ugv_bringup/launch/bringup.launch.py (8)
47-54: LGTM!The log level argument is properly declared with appropriate default value and valid choices.
73-77: LGTM!The log level argument is correctly passed to the controller launch file.
90-90: LGTM!The log level argument is correctly passed to the system monitor launch file.
99-103: LGTM!The log level argument is correctly passed to the lights launch file.
112-112: LGTM!The log level argument is correctly passed to the battery launch file.
121-125: LGTM!The log level argument is correctly passed to the EKF launch file.
135-139: LGTM!The log level argument is correctly passed to the manager launch file.
155-155: LGTM!The log level argument declaration is correctly added to the actions list.
husarion_ugv_localization/launch/localization.launch.py (4)
61-67: LGTM: Well-structured log level argument declaration.The log level argument is properly declared with appropriate default value and valid choices.
137-143: LGTM: Proper log level configuration for ekf_filter_node.The implementation correctly applies the log level and limits rcl logger as specified.
153-153: LGTM: Log level properly propagated to included launch file.
168-174: LGTM: Consistent log level configuration for navsat_transform_node.husarion_ugv_controller/launch/controller.launch.py (4)
121-127: LGTM: Consistent log level argument declaration.
266-278: LGTM: Comprehensive log level configuration for control_node.The implementation properly handles multiple logging units and follows the requirement to limit certain loggers to INFO level.
303-307: LGTM: Proper log level configuration for drive_controller_spawner.
344-348: LGTM: Consistent log level configuration for imu_broadcaster_spawner.
Description
robot_state_publisheri andros2_control_nodenodes were ommited as they publish a lot of information under debug level.Requirements
Tests 🧪
Summary by CodeRabbit
log_levellaunch argument across various components, allowing users to configure logging verbosity with a default value of "INFO".log_leveland modifications torobot_model.husarion_ugv_utilsfor bothhusarion_ugv_controllerandhusarion_ugv_localizationpackages.