Use a private netft-cpp core snapshot - #9
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis release replaces the legacy driver API with a vendored ChangesNetFT core and build
Validation and integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CMakeLists.txt`:
- Around line 11-12: Update the CURL requirement in the CMake package
configuration from 7.62 to a minimum of 7.63, ensuring builds using
CURLOPT_CURLU satisfy the dependency required by discovery.cpp.
In `@src/core/src/detail/posix_transport.cpp`:
- Around line 98-108: Update the EINTR retry loop around ::poll in receive() to
track elapsed time and recompute the remaining timeout before each retry, rather
than reusing the original timeout_milliseconds(timeout). Return 0 once the
caller-specified timeout has been exhausted, while preserving the existing error
handling for non-EINTR failures.
In `@test/cpp/test_ros1_node.cpp`:
- Around line 32-37: Initialize ROS at the beginning of
MapsEveryAdapterParameterWithManualCalibration with an anonymous node name,
before calling ros::master::check() or constructing the private NodeHandle,
mirroring the initialization in the first test so it runs independently.
In `@test/support/fake_sensor.py`:
- Around line 43-44: Rename the log_message method’s format parameter to a
non-conflicting name while preserving its positional signature and the existing
no-op behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69ebc753-610d-4be8-8296-ebec5545f933
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (101)
.github/workflows/coverage.ymlCHANGELOG.rstCMakeLists.txtCONTRIBUTING.mdREADME.mdconfig/netft_ros1.yamlconfig/netft_ros2.yamldocs/architecture.mdinclude/netft_driver/client.hppinclude/netft_driver/status.hppinclude/netft_driver/types.hpplaunch/netft.launchlaunch/netft.launch.pylaunch/netft_ros2_control.launch.pypackage.xmlpixi.tomlsrc/check_main.cppsrc/client.cppsrc/core/LICENSEsrc/core/UPSTREAMsrc/core/include/netft/client.hppsrc/core/include/netft/discovery.hppsrc/core/include/netft/export.hppsrc/core/include/netft/status.hppsrc/core/include/netft/types.hppsrc/core/src/client.cppsrc/core/src/detail/client_impl.cppsrc/core/src/detail/client_impl.hppsrc/core/src/detail/fault_latch.cppsrc/core/src/detail/fault_latch.hppsrc/core/src/detail/posix_transport.cppsrc/core/src/detail/posix_transport.hppsrc/core/src/detail/protocol.cppsrc/core/src/detail/protocol.hppsrc/core/src/detail/sequence.cppsrc/core/src/detail/sequence.hppsrc/core/src/detail/xml_config.cppsrc/core/src/detail/xml_config.hppsrc/core/src/discovery.cppsrc/core/src/status.cppsrc/core/src/types.cppsrc/netft_hardware_interface.cppsrc/protocol.cppsrc/ros/adapter_config.hppsrc/ros/diagnostics.cppsrc/ros/diagnostics.hppsrc/ros/ros2_control_compat.hppsrc/ros/ros2_control_test_access.hppsrc/ros/unit_conversion.cppsrc/ros/unit_conversion.hppsrc/ros1_node.cppsrc/ros2_node.cppsrc/status.cpptest/core/CMakeLists.txttest/core/support/fake_http_server.cpptest/core/support/fake_http_server.hpptest/core/support/fake_sensor.cpptest/core/support/fake_sensor.hpptest/core/test_client_lifecycle.cpptest/core/test_client_recovery.cpptest/core/test_client_stream.cpptest/core/test_discovery.cpptest/core/test_fault_latch.cpptest/core/test_protocol.cpptest/core/test_sequence.cpptest/core/test_status.cpptest/core/test_types.cpptest/cpp/CMakeLists.txttest/cpp/netft_hardware_test_plugins.xmltest/cpp/support/fake_sensor.cpptest/cpp/support/fake_sensor.hpptest/cpp/support/ros2_control_test_access.cpptest/cpp/test_adapter_config.cpptest/cpp/test_check.cpptest/cpp/test_client.cpptest/cpp/test_diagnostics.cpptest/cpp/test_hardware_interface.cpptest/cpp/test_protocol.cpptest/cpp/test_ros1_node.cpptest/cpp/test_ros2_node.cpptest/cpp/test_status.cpptest/cpp/test_types.cpptest/cpp/test_unit_conversion.cpptest/integration/audit_ros2_control_symbols.shtest/integration/build_package.shtest/integration/fake_sensor_process.pytest/integration/ros1_smoke.shtest/integration/ros2_control_isolation_test.shtest/integration/ros2_control_smoke.shtest/integration/ros2_smoke.shtest/integration/ros_graph_assertions.pytest/support/fake_sensor.pytest/test_config_validation.pytest/test_fake_sensor.pytest/test_gitignore.pytest/test_launch_validation.pytest/test_manifest.pytest/test_ros_graph_assertions.pytest/test_urdf_validation.pytest/test_workflow_validation.pyurdf/netft.ros2_control.xacro
💤 Files with no reviewable changes (13)
- test/cpp/test_protocol.cpp
- test/cpp/test_status.cpp
- test/cpp/test_types.cpp
- include/netft_driver/client.hpp
- src/client.cpp
- test/cpp/test_client.cpp
- include/netft_driver/types.hpp
- src/status.cpp
- src/protocol.cpp
- src/ros/ros2_control_compat.hpp
- test/cpp/support/fake_sensor.hpp
- include/netft_driver/status.hpp
- test/cpp/support/fake_sensor.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/compat/xml_config.cpp (1)
15-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftShare the XML tokenizer with the canonical parser
src/compat/xml_config.cppstill duplicates the XML tag-scanning and validation logic fromsrc/core/src/detail/xml_config.cpp; onlyparse_positive_countneeds the compat-specific path. Movingextract_required_fields,find_tag_end, and the markup checks into a shared unit would reduce drift risk in the calibration parser.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compat/xml_config.cpp` around lines 15 - 274, The XML scanning and validation logic in extract_required_fields, find_tag_end, and their associated markup helpers duplicates the canonical parser. Move or reuse these symbols through a shared tokenizer/parser unit, updating both compat and canonical parsing paths to use it, while retaining parse_positive_count as the compat-specific implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/compat/xml_config.cpp`:
- Around line 15-274: The XML scanning and validation logic in
extract_required_fields, find_tag_end, and their associated markup helpers
duplicates the canonical parser. Move or reuse these symbols through a shared
tokenizer/parser unit, updating both compat and canonical parsing paths to use
it, while retaining parse_positive_count as the compat-specific implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42178889-878e-4cba-b037-195a69f77c6c
📒 Files selected for processing (12)
CHANGELOG.rstCMakeLists.txtCONTRIBUTING.mdREADME.mddocs/architecture.mdsrc/compat/xml_config.cppsrc/netft_hardware_interface.cpptest/core/CMakeLists.txttest/core/test_discovery.cpptest/cpp/support/ros2_control_test_access.cpptest/cpp/test_check.cpptest/integration/audit_ros2_control_symbols.sh
💤 Files with no reviewable changes (1)
- test/integration/audit_ros2_control_symbols.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- CHANGELOG.rst
- test/core/CMakeLists.txt
- test/cpp/support/ros2_control_test_access.cpp
- CONTRIBUTING.md
- README.md
- docs/architecture.md
- test/cpp/test_check.cpp
- test/core/test_discovery.cpp
- CMakeLists.txt
- src/netft_hardware_interface.cpp
Summary
Replace the original ROS-specific Net F/T runtime with a private, byte-identical snapshot of
netft-cppv0.1.2 and adapt the ROS 1, ROS 2, diagnostic, CLI, and ros2_control layers to it.The snapshot comes from
netft/netft-cpptagv0.1.2, commitbb7df8d211ecc36ebc1434da3686897d7b3d73cc. It remains in the upstreamnetftnamespace. The core is built as an internal static target and is not installed, exported, discovered, or fetched as a separate dependency. This release also adopts Apache-2.0 for the complete repository, matching the upstream core.This also changes the default runtime behavior to read calibration from the sensor when available and converts native Net F/T samples to ROS SI units (
N,N*m). Manual counts-per-force and counts-per-torque settings remain available when sensor calibration is disabled.Related issue
None.
Validation
pixi run unitTested ROS distributions and commands:
pixi run -e test core-test(14/14 passed) andpixi run unit(143 passed, 1 skipped).pixi run -e noetic buildandpixi run -e noetic smoke.pixi run -e humble build,pixi run -e humble smoke, andpixi run -e humble ros2-control-smoke.pixi run -e jazzy build,pixi run -e jazzy smoke, andpixi run -e jazzy ros2-control-smoke..gitgit archivesource tree passed the manifest audit.Checklist
Hardware validation, if any:
No physical sensor was used. All network integration tests used loopback fake sensors, and no software bias command was issued.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes