Add hw and os checks #491
Add hw and os checks #491
Conversation
WalkthroughThe pull request enhances the launch process by introducing hardware configuration and operating system version compatibility checks. In the launch file, a new constant, function, and launch arguments have been added to evaluate and enforce a minimum OS version and proper hardware configuration. Additionally, new actions trigger warnings or halt the process based on the checks. A utility module now provides a standardized way of generating styled error and warning messages through a new error message class with associated helper functions. Changes
Sequence Diagram(s)sequenceDiagram
participant Launcher as Launch File
participant Env as Environment
participant Validator as OS Version Check
participant Msg as ErrorMessages
Launcher->>Env: Retrieve current OS version
Launcher->>Validator: Execute check_version_compatibility(os_version, MIN_REQUIRED_OS_VERSION)
Validator-->>Launcher: Return compatibility status
alt OS version incompatible
Launcher->>Msg: Generate warning message (INCORRECT_OS_VERSION)
else OS version compatible
Note over Launcher, Validator: Continue normal launch actions
end
alt Hardware configuration incorrect and exit_on_wrong_hw True
Launcher->>Msg: Generate error message (INCORRECT_HW_CONFIG)
Launcher-->>Launcher: Trigger indefinite sleep (halt launch)
end
✨ Finishing Touches
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
🧹 Nitpick comments (3)
husarion_ugv_bringup/launch/bringup.launch.py (3)
45-45: Consider externalizing minimum OS version.
DefiningMIN_REQUIRED_OS_VERSION = [2, 2, 0]works. However, storing such constants in a separate config file or param might simplify adjustments without modifying code.
48-68: OS version compatibility checks are clear but may be fragile.
The regex-based parsing and multi-step comparison logic works for the simple semantic formatvX.Y.Z. Consider handling extra characters (e.g.,-rcor build metadata) or missing segments. You might return a more detailed reason for incompatibility to guide users.
191-195: Usingsleep infinityto block might be limiting.
While this effectively prevents node startup, it could consume resources indefinitely. Consider a more graceful fail or a single blocking action that clearly indicates the issue.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
husarion_ugv_bringup/launch/bringup.launch.py(3 hunks)husarion_ugv_utils/husarion_ugv_utils/messages.py(2 hunks)
🔇 Additional comments (15)
husarion_ugv_bringup/launch/bringup.launch.py (12)
18-19: No issues with new imports.
The addition ofosandreappears necessary for environment variable retrieval and version parsing.
21-26: Imports fromhusarion_ugv_utils.messagesare correctly introduced.
These imports unify and standardize the way you log errors, warnings, and welcome messages.
28-34: Expanded imports fromlaunch.actions.
Bringing in these actions (DeclareLaunchArgument,ExecuteProcess,GroupAction,IncludeLaunchDescription,TimerAction) is appropriate for controlling advanced launch flows.
35-35: Conditional imports look good.
UsingIfConditionandUnlessConditionis standard for modular ROS 2 launch logic.
41-41: Use ofPythonExpressionis valid.
It’s a good choice for runtime boolean checks and dynamic substitutions in launch files.
71-77: Launch argument for hardware exit check is appropriate.
Defining"exit_on_wrong_hw"as a launch argument provides flexibility to either block or proceed, aligning with different deployment needs.
189-190: Environment variable default aligns with design.
SettingROBOT_HW_CONFIG_CORRECTto"false"by default ensures the system checks are performed unless overridden.
196-202: Grouping incorrect hardware config actions is well-structured.
Leveragingerror_msgand aGroupActionensures the user is notified and the process halts as intended based on conditions.
204-208: Retrieving OS version from environment is straightforward.
Falling back to"v0.0.0"whenSYSTEM_BUILD_VERSIONis missing or invalid is sensible.
209-218: Warning action for incorrect OS version is sensible.
Using a warning rather than an error acknowledges that the robot might still operate, albeit in an unsupported configuration.
230-237: Conditional driver actions are cleanly toggled.
ThisGroupActionensures the driver launches only when the hardware config is correct, preventing partial or erroneous startups.
240-248: Final actions list neatly integrates new checks.
Appendingincorrect_hw_config_action,incorrect_os_version_action, anddriver_actionsintoactionsseamlessly ties together the updated logic in a single return.husarion_ugv_utils/husarion_ugv_utils/messages.py (3)
48-64: Centralized error/warning strings improve maintainability.
ErrorMessagesprovides well-formatted constants. Consider adding a docstring to clarify their usage and whether more messages might be consolidated here.
112-115:error_msgfunction works well for quick stylized logs.
Currently, it usesLogInfowith red text, which is sufficient for most ROS 2 terminal outputs.
117-119:warning_msgfunction parallelserror_msgconsistently.
The consistency ensures easy integration for both error and warning messages. In future, consider whether more advanced log levels or additional context is needed.
| os_version_correct = PythonExpression( | ||
| f"{check_os_version_compatibility(os_version, MIN_REQUIRED_OS_VERSION)}" | ||
| ) |
There was a problem hiding this comment.
PythonExpression probably can be omitted, because os.environ is just a string
| os_version_correct = PythonExpression( | |
| f"{check_os_version_compatibility(os_version, MIN_REQUIRED_OS_VERSION)}" | |
| ) | |
| os_version_correct = check_os_version_compatibility(os_version, MIN_REQUIRED_OS_VERSION) |
There was a problem hiding this comment.
UnlessCondition doesn't accept bool, that's why this is wrapped with PythonExpression
| exit_on_wrong_hw = LaunchConfiguration("exit_on_wrong_hw") | ||
| declare_exit_on_wrong_hw_arg = DeclareLaunchArgument( | ||
| "exit_on_wrong_hw", | ||
| default_value="false", |
There was a problem hiding this comment.
Why not default true?
There was a problem hiding this comment.
It's the debate what is our default state. I assumed using docker and avoiding unnecessary restarts of it
| MIN_REQUIRED_OS_VERSION = [2, 2, 0] | ||
|
|
||
|
|
||
| def check_os_version_compatibility(version_string: str, min_required_version: list[int]) -> bool: | ||
| match = re.search(r"v(\d+\.\d+\.\d+)", version_string) | ||
|
|
||
| if not match: | ||
| return False | ||
|
|
||
| version_str = match.group(1) | ||
| version = version_str.split(".") | ||
|
|
||
| if int(version[0]) > min_required_version[0]: | ||
| return True | ||
|
|
||
| if int(version[0]) == min_required_version[0]: | ||
| if int(version[1]) > min_required_version[1]: | ||
| return True | ||
|
|
||
| if int(version[1]) == min_required_version[1]: | ||
| return int(version[2]) >= min_required_version[2] | ||
|
|
||
| return False |
There was a problem hiding this comment.
Propose to move it to *_utils and simplify it. Like in following example
import re
MIN_REQUIRED_OS_VERSION = "v2.2.2"
def extract_version_tuple(version_string: str) -> tuple[int, int, int]:
match = re.search(r"v(\d+)\.(\d+)\.(\d+)", version_string)
return tuple(map(int, match.groups())) if match else (0, 0, 0)
def check_os_version_compatibility(version1: str, min_required_version: str) -> bool:
return extract_version_tuple(version1) >= extract_version_tuple(min_required_version)
# Example usage
print(check_os_version_compatibility("v1.9.5", MIN_REQUIRED_OS_VERSION)) # False
print(check_os_version_compatibility("v2.2.2", MIN_REQUIRED_OS_VERSION)) # True
print(check_os_version_compatibility("v2.3.0", MIN_REQUIRED_OS_VERSION)) # True
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
husarion_ugv_utils/husarion_ugv_utils/version_check.py (2)
20-22: Add input validation and improve documentation.The function needs input validation and better documentation:
- Add input validation for None/empty strings
- Add docstring explaining the expected format
- Add more specific type hints
-def extract_version_tuple(version_string: str) -> tuple[int, int, int]: - match = re.search(r"v(\d+)\.(\d+)\.(\d+)", version_string) - return tuple(map(int, match.groups())) if match else (0, 0, 0) +def extract_version_tuple(version_string: str | None) -> tuple[int, int, int]: + """Extract version numbers from a version string. + + Args: + version_string: Version string in format "vX.Y.Z" where X, Y, Z are integers + + Returns: + tuple[int, int, int]: Version numbers as (major, minor, patch), or (0, 0, 0) if invalid + """ + if not version_string: + return (0, 0, 0) + match = re.search(r"v(\d+)\.(\d+)\.(\d+)", version_string) + return tuple(map(int, match.groups())) if match else (0, 0, 0)
25-30: Enhance docstring with parameter and return type documentation.The docstring should include parameter and return type documentation.
def check_version_compatibility(version: str, min_required_version: str) -> bool: """ Check if the version is compatible with the minimum required version. - Assumes versions in format "v0.0.0". + + Args: + version: Version string in format "vX.Y.Z" + min_required_version: Minimum required version string in format "vX.Y.Z" + + Returns: + bool: True if version is compatible (greater or equal), False otherwise """ return extract_version_tuple(version) >= extract_version_tuple(min_required_version)husarion_ugv_bringup/launch/bringup.launch.py (2)
45-45: Consider moving MIN_REQUIRED_OS_VERSION to a central configuration.The version constant should be in a central configuration file to maintain consistency across the codebase.
Consider moving it to a new file like
husarion_ugv_utils/config.py:# husarion_ugv_utils/config.py MIN_REQUIRED_OS_VERSION = "v2.2.0"
169-172: Consider alternative error handling approach.The current approach of using
sleep infinityto prevent exit might not be the best way to handle incorrect hardware configuration:
- It keeps the process running unnecessarily
- It might mask other issues
- It makes it harder to automate error handling
Consider these alternatives:
- Exit with a non-zero status code when
exit_on_wrong_hwis true- Implement a retry mechanism with backoff
- Add proper logging for monitoring and alerting
Also applies to: 174-180
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
husarion_ugv_bringup/launch/bringup.launch.py(3 hunks)husarion_ugv_utils/husarion_ugv_utils/version_check.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run unit tests hardware build type
- GitHub Check: Run unit tests simulation build type
🔇 Additional comments (2)
husarion_ugv_bringup/launch/bringup.launch.py (2)
182-185: Simplify OS version check implementation.Based on the past review comments, PythonExpression is only needed for UnlessCondition. The version check can be simplified.
- os_version = os.environ.get("SYSTEM_BUILD_VERSION", "v0.0.0") - os_version_correct = PythonExpression( - f"{check_version_compatibility(os_version, MIN_REQUIRED_OS_VERSION)}" - ) + os_version = EnvironmentVariable("SYSTEM_BUILD_VERSION", default_value="v0.0.0") + os_version_correct = PythonExpression( + [ + "str('", + os_version, + "') >= '", + MIN_REQUIRED_OS_VERSION, + "'", + ] + )
167-167: Verify the default value for hardware configuration.The default value "false" for
ROBOT_HW_CONFIG_CORRECTmeans the driver won't start by default. This might not be the desired behavior for development or testing environments.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if there are any environment setup files that might override this default. # Search for environment setup files fd -e 'sh' -e 'bash' -e 'env' | xargs grep -l "ROBOT_HW_CONFIG_CORRECT"Length of output: 65
🏁 Script executed:
#!/bin/bash # Search for all occurrences of "ROBOT_HW_CONFIG_CORRECT" in the codebase to check for any additional references or overrides. rg "ROBOT_HW_CONFIG_CORRECT" -n .Length of output: 184
Confirmed: The default value remains "false".
I searched the entire repository for any alternative settings or overrides for the
ROBOT_HW_CONFIG_CORRECTvariable and found only the instance in the filehusarion_ugv_bringup/launch/bringup.launch.py. This confirms that the default remains set to"false", which means the driver will not start by default. If this behavior is not desired in development or testing environments, you may need to update the default value accordingly.
Description
Added following checks in launch files:
Requirements
Tests 🧪
Summary by CodeRabbit