battery node reads from /dev/battery | lights are read from symlink #347
Conversation
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
WalkthroughThis update enhances several components of the Panther system by adding a new method for retrieving battery paths, improving error handling, and refining device path updates. Key changes include the addition of the Changes
Sequence Diagram(s)Battery Node Initialization with ADC BatterysequenceDiagram
autonote over User, BatteryNode: Initialize with ADC Battery (Updated)
User ->> BatteryNode: Initialize()
BatteryNode ->> BatteryNode: declare_parameter("adc/device0", "/dev/battery1")
BatteryNode ->> BatteryNode: declare_parameter("adc/device1", "/dev/battery2")
BatteryNode ->> BatteryNode: get_parameter("adc/device0")
BatteryNode ->> BatteryNode: get_parameter("adc/device1")
BatteryNode ->> BatteryNode: GetBatteryPath("/dev/battery1")
BatteryNode ->> BatteryNode: GetBatteryPath("/dev/battery2")
BatteryNode ->> ADCDataReader: Instantiate with adc0_device_path
BatteryNode ->> ADCDataReader: Instantiate with adc1_device_path
autonote over User, BatteryNode: Initialization complete
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
panther_battery/include/panther_battery/battery_node.hpp (1)
46-46: Add a method comment toGetBatteryPath.A method comment explaining the purpose and the expected behavior of
GetBatteryPathwould enhance the readability and maintainability of the code, especially for future developers or for those unfamiliar with the codebase.panther_battery/src/battery_node.cpp (1)
85-97: Ensure parameter naming consistency.The parameters for device paths (
adc/device0andadc/device1) are well-named, but consider ensuring consistency in naming convention across all parameters to enhance readability and prevent confusion. For example,adc/ma_window_len/tempcould be renamed to maintain the same level of detail as the device path parameters.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- panther_battery/include/panther_battery/battery_node.hpp (1 hunks)
- panther_battery/src/battery_node.cpp (3 hunks)
- panther_battery/src/battery_publisher.cpp (1 hunks)
- panther_diagnostics/src/system_status_node.cpp (1 hunks)
- panther_hardware_interfaces/src/canopen_controller.cpp (4 hunks)
- panther_hardware_interfaces/src/gpio_driver.cpp (2 hunks)
- panther_hardware_interfaces/src/panther_system_ros_interface.cpp (1 hunks)
- panther_hardware_interfaces/src/utils.cpp (1 hunks)
- panther_lights/src/driver_node.cpp (1 hunks)
Files skipped from review due to trivial changes (6)
- panther_battery/src/battery_publisher.cpp
- panther_diagnostics/src/system_status_node.cpp
- panther_hardware_interfaces/src/canopen_controller.cpp
- panther_hardware_interfaces/src/gpio_driver.cpp
- panther_hardware_interfaces/src/panther_system_ros_interface.cpp
- panther_hardware_interfaces/src/utils.cpp
Additional comments not posted (1)
panther_lights/src/driver_node.cpp (1)
43-44: Verify device path updates.Ensure that the updated device paths in
DriverNodeconstructor are correctly configured and accessible. This is crucial for the proper operation of the lighting system.
panther_battery/src/battery_node.cpp
Outdated
| this->get_logger(), | ||
| "An exception occurred while initializing with ADC: " << e.what() | ||
| << " Falling back to " | ||
| "using Roboteq " | ||
| "drivers to " | ||
| "publish battery " | ||
| "data."); |
There was a problem hiding this comment.
Refactor exception handling for clarity.
The exception handling block within BatteryNode::Initialize could be refactored for better clarity and maintainability. Consider encapsulating the fallback logic into a separate method or improving the logging details to provide more context about the failure.
| @@ -64,7 +64,7 @@ void BatteryNode::Initialize() | |||
| return; | |||
| } catch (const std::runtime_error & e) { | |||
| RCLCPP_WARN_STREAM( | |||
| this->get_logger(), "An exception ocurred while initializing with ADC: " | |||
| this->get_logger(), "An exception occurred while initializing with ADC: " | |||
There was a problem hiding this comment.
Refactor exception handling for clarity.
The exception handling block within BatteryNode::Initialize could be refactored for better clarity and maintainability. Consider encapsulating the fallback logic into a separate method or improving the logging details to provide more context about the failure.
| std::string BatteryNode::GetBatteryPath(const std::string & adc_device_path) const | ||
| { | ||
| std::string battery_device_name; | ||
|
|
||
| if (std::filesystem::is_symlink(adc_device_path)) { | ||
| battery_device_name = std::filesystem::read_symlink(adc_device_path).string(); | ||
| } else { | ||
| battery_device_name = std::filesystem::path(adc_device_path).filename().string(); | ||
| } | ||
|
|
||
| return std::string("/sys/bus/iio/devices/") + battery_device_name; | ||
| } |
There was a problem hiding this comment.
LGTM! But consider adding error handling for invalid paths.
The code changes are approved.
However, consider adding error handling for invalid paths to enhance robustness.
std::string BatteryNode::GetBatteryPath(const std::string & adc_device_path) const
{
std::string battery_device_name;
if (std::filesystem::is_symlink(adc_device_path)) {
battery_device_name = std::filesystem::read_symlink(adc_device_path).string();
} else {
battery_device_name = std::filesystem::path(adc_device_path).filename().string();
}
// Add error handling for invalid paths
if (battery_device_name.empty()) {
RCLCPP_ERROR(this->get_logger(), "Invalid ADC device path: %s", adc_device_path.c_str());
throw std::runtime_error("Invalid ADC device path");
}
return std::string("/sys/bus/iio/devices/") + battery_device_name;
}| this->declare_parameter<std::string>("adc/device0", "/dev/battery1"); | ||
| this->declare_parameter<std::string>("adc/device1", "/dev/battery2"); |
There was a problem hiding this comment.
Tip
Codebase Verification
Device paths /dev/battery1 and /dev/battery2 do not exist.
The device paths specified in the code (/dev/battery1 and /dev/battery2) are not present in the system. This will cause the initialization to fail.
- Ensure that the device paths
/dev/battery1and/dev/battery2are correctly set up and accessible before running the code.
Analysis chain
LGTM! But verify the device paths.
The code changes are approved.
However, ensure that the new device paths (/dev/battery1 and /dev/battery2) are correctly set up and accessible.
Also applies to: 86-87, 89-93
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new device paths are correctly set up and accessible.
# Test: Check if the device paths exist. Expect: Paths exist.
test -e /dev/battery1 && echo "/dev/battery1 exists" || echo "/dev/battery1 does not exist"
test -e /dev/battery2 && echo "/dev/battery2 exists" || echo "/dev/battery2 does not exist"
Length of output: 181
Description
Added symlinks to the os image https://github.com/husarion/panther-rpi-os-img/pull/94.
Updating devices and params parcsing after adding symlinks to leds and batteries ADS measurements.
Summary by CodeRabbit
New Features
GetBatteryPathmethod.Bug Fixes
Improvements
Refactor