feat(holocat): Add EtherCAT application #1284
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new "holocat" application for EtherCAT real-time integration: registers the app, adds EC‑Master CMake find support, Docker packaging and SDK verification, YAML/ENI configs, Holoscan operators and app wiring, and associated metadata and docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (15)
applications/holocat/README.md (1)
26-26: Replace hardcoded path with placeholder.The ECMASTER_ROOT path is user-specific and should use a placeholder instead.
Apply this diff:
-export ECMASTER_ROOT=/home/hking/devel/ethercat/ecm +export ECMASTER_ROOT=/path/to/ec-master-sdkapplications/holocat/cpp/hc_data_rx_op.hpp (1)
19-20: Clarify data source in documentation.The documentation states the operator "receives integer data from the ECat bus," but based on the operator architecture, it actually receives data from another Holoscan operator (HolocatOp). Consider clarifying the data flow.
Example:
- * A Holoscan operator that receives integer data from the ECat bus. - * This operator acts as a data receiver, receiving integer values from the ECat bus. + * A Holoscan operator that receives integer data from the HolocatOp. + * This operator acts as a data receiver in the Holocat data flow pipeline.applications/holocat/configs/holocat_config.yaml (2)
5-5: Use generic adapter name in example configuration.The adapter name appears to be a specific hardware MAC-based identifier. For an example configuration file, use a more generic placeholder.
Apply this diff:
- adapter_name: "enxc84bd6d2788c" + adapter_name: "eth0" # Change to your EtherCAT interfaceNote: This matches the placeholder used in the README example.
9-9: Replace hardcoded path with relative or placeholder path.The ENI file path contains a user-specific absolute path. Use a relative path or generic placeholder.
Apply this diff:
- eni_file: "/home/hking/devel/holo/holohub/applications/holocat/configs/eni2.xml" + eni_file: "applications/holocat/configs/eni2.xml"Or use an environment variable placeholder as suggested in the comment above.
applications/holocat/CMakeLists.txt (1)
1-30: Add testing section for functional tests.Per coding guidelines, applications should include a testing section in CMakeLists.txt for functional testing using CTest.
As per coding guidelines, applications should include testing.
Consider adding after the
add_subdirectory(cpp)line:# Testing if(BUILD_TESTING) add_subdirectory(tests) endif()applications/holocat/configs/eni2.xml (1)
1-1: Document the example ENI configuration.This ENI file appears to be for specific hardware (simco drive 40028083-00-0 and WAGO 750-354). Consider adding a comment or separate README in the configs directory explaining that this is an example configuration and users must generate their own ENI file for their specific EtherCAT hardware setup.
Consider adding
applications/holocat/configs/README.md:# EtherCAT Configuration Files ## eni2.xml This is an example EtherCAT Network Information (ENI) file for: - SubDevice_1001: simco drive 40028083-00-0 - SubDevice_1002: WAGO 750-354 I/O module **Important**: You must generate your own ENI file matching your specific EtherCAT hardware configuration using EtherCAT configuration tools (e.g., TwinCAT, EC-Engineer, or acontis EC-Engineer).applications/holocat/cpp/CMakeLists.txt (1)
1-89: Missing CTest integration for functional testing.Per coding guidelines, applications should include a testing section in CMakeLists.txt for functional testing using CTest.
Consider adding a testing section:
# Testing if(BUILD_TESTING) enable_testing() # Add test cases here endif()applications/holocat/cpp/hc_data_tx_op.cpp (1)
28-30: Counter starts at 1, not 0.The counter increments before use, so the first emitted value is
1, not0. If starting at0is intended, consider swapping the order:- // Increment counter - counter_++; - counter_ = counter_ % kMaxCount; - // Emit current counter value op_output.emit<int>(counter_, "count_out"); + + // Increment counter for next cycle + counter_ = (counter_ + 1) % kMaxCount;applications/holocat/cpp/hc_data_rx_op.cpp (1)
33-34: INFO-level logging on every cycle may be excessive.In a real-time system with fast cycle times, logging at INFO level on every received value will flood logs and potentially impact performance. Consider using DEBUG level similar to
HcDataTxOp.last_count_ = maybe_count.value(); - HOLOSCAN_LOG_INFO("HcDataRxOp: Received count: {}", last_count_); + // Log periodically to avoid spam + if (last_count_ % 50 == 0) { + HOLOSCAN_LOG_DEBUG("HcDataRxOp: 50x Received count = {}", last_count_); + }applications/holocat/cpp/holocat_op.cpp (3)
175-182: Blockingsleep(1)instop()delays shutdown.Using
sleep(1)blocks the calling thread unconditionally. Consider using a non-blocking approach or reducing the delay if waiting for async completion.pending_state_transition_ = std::async(std::launch::async, [this]() { return ecatSetMasterState(kEthercatStateChangeTimeout_, eEcatState_INIT); }); - sleep(1); + // Wait for state transition to complete with timeout + if (pending_state_transition_.wait_for(std::chrono::seconds(5)) == std::future_status::timeout) { + HOLOSCAN_LOG_WARN("State transition to INIT timed out during stop"); + } + // deinitialize master ecatDeinitMaster();
350-364: Static local variable incompute()is not thread-safe.The
static struct timespec t_lastis shared across all invocations. If multipleHolocatOpinstances run concurrently or the operator is restarted, timing calculations will be incorrect.Consider making
t_lasta member variable initialized instart():+// In class declaration (holocat_op.hpp): + struct timespec t_last_{}; + +// In start(): + clock_gettime(CLOCK_MONOTONIC, &t_last_); + // In compute(): struct timespec t_now; clock_gettime(CLOCK_MONOTONIC, &t_now); - static struct timespec t_last{t_now.tv_sec, t_now.tv_nsec}; ... - t_last = t_now; + t_last_ = t_now;
311-347: EtherCAT job results are silently discarded.The return values from
ecatExecJobcalls are assigned todwResbut then immediately suppressed withEC_UNREFPARM. While this may be intentional for polling mode, consider logging errors at DEBUG level to aid troubleshooting.dwRes = ecatExecJob(eUsrJob_ProcessAllRxFrames, EC_NULL); - EC_UNREFPARM(dwRes); + if (dwRes != EC_E_NOERROR) { + HOLOSCAN_LOG_DEBUG("ProcessAllRxFrames returned: {} (0x{:x})", ecatGetText(dwRes), dwRes); + }applications/holocat/cpp/holocat_app.cpp (2)
72-78: Clarify comment about uninitialized values.The comment "target remains uninitialized" is misleading since
HolocatConfig config = {}value-initializes all members (empty strings, zeros, false). The behavior is correct but the comment should reflect the actual semantics.auto try_extract = [this](const std::string& key, auto& target) { try { target = from_config(key).as<std::decay_t<decltype(target)>>(); } catch (...) { - // Key not found or conversion failed - target remains uninitialized + // Key not found or conversion failed - target retains default value } };
42-47: Consider making the counter update period configurable.The 100ms period for
HcDataTxOpis hardcoded. For flexibility, consider adding this as a configuration parameter similar tocycle_time_us.applications/holocat/cpp/holocat_op.hpp (1)
10-11: Consider using conventional include guard naming.The include guard
INC_HOLOCAT_Hwith value1is unconventional. A more standard pattern would match the filename.-#ifndef INC_HOLOCAT_H -#define INC_HOLOCAT_H 1 +#ifndef HOLOCAT_OP_HPP +#define HOLOCAT_OP_HPPAnd update the closing guard:
-#endif /* INC_HOLOCAT_H */ +#endif // HOLOCAT_OP_HPP
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
applications/CMakeLists.txt(1 hunks)applications/holocat/CMakeLists.txt(1 hunks)applications/holocat/Dockerfile(1 hunks)applications/holocat/FindEcMaster.cmake(1 hunks)applications/holocat/README.md(1 hunks)applications/holocat/configs/eni2.xml(1 hunks)applications/holocat/configs/holocat_config.yaml(1 hunks)applications/holocat/cpp/CMakeLists.txt(1 hunks)applications/holocat/cpp/hc_data_rx_op.cpp(1 hunks)applications/holocat/cpp/hc_data_rx_op.hpp(1 hunks)applications/holocat/cpp/hc_data_tx_op.cpp(1 hunks)applications/holocat/cpp/hc_data_tx_op.hpp(1 hunks)applications/holocat/cpp/holocat_app.cpp(1 hunks)applications/holocat/cpp/holocat_app.hpp(1 hunks)applications/holocat/cpp/holocat_op.cpp(1 hunks)applications/holocat/cpp/holocat_op.hpp(1 hunks)applications/holocat/cpp/main.cpp(1 hunks)applications/holocat/metadata.json(1 hunks)applications/holocat/scripts/verify_ecmaster.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
{operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
{operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp}: All code must adhere to Holoscan SDK coding standards including proper error handling and validation
Use descriptive English naming for functions, variables, and components; minimize acronyms and brand names
Include inline comments for complex logic in code
Files:
applications/holocat/cpp/hc_data_rx_op.hppapplications/holocat/cpp/hc_data_tx_op.hppapplications/holocat/cpp/main.cppapplications/holocat/cpp/holocat_op.cppapplications/holocat/cpp/holocat_op.hppapplications/holocat/cpp/holocat_app.cppapplications/holocat/cpp/hc_data_tx_op.cppapplications/holocat/cpp/hc_data_rx_op.cppapplications/holocat/cpp/holocat_app.hpp
{operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp,cmake}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All code must compile and build successfully on target platforms before submission
Files:
applications/holocat/cpp/hc_data_rx_op.hppapplications/holocat/cpp/hc_data_tx_op.hppapplications/holocat/cpp/main.cppapplications/holocat/cpp/holocat_op.cppapplications/holocat/cpp/holocat_op.hppapplications/holocat/cpp/holocat_app.cppapplications/holocat/cpp/hc_data_tx_op.cppapplications/holocat/cpp/hc_data_rx_op.cppapplications/holocat/cpp/holocat_app.hppapplications/holocat/FindEcMaster.cmake
{operators,applications,workflows,gxf_extensions,tutorials}/**/README.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
README.md must include purpose, usage instructions, requirements, examples, and architecture overview
Files:
applications/holocat/README.md
**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use Holoscan SDK glossary terms when referring to HoloHub-specific locations in documentation
Files:
applications/holocat/README.md
**/metadata.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When reviewing PRs that modify or add
metadata.jsonfiles, verify that the category (first tag in thetagsarray) matches one of the approved categories: Benchmarking, Camera, Computer Vision and Perception, Converter, Deployment, Development, Extended Reality, Healthcare AI, Image Processing, Inference, Interoperability, Medical Imaging, Natural Language and Conversational AI, Networking and Distributed Computing, Optimization, Quantum Computing, Rendering, Robotics, Scheduler, Signal Processing, Streaming, Threading, Video, Video Capture, Visualization, XR
**/metadata.json: Useinterfaceor specific metadata schema for defining contribution structure (metadata.json)
metadata.json must include name, authors, language, version, changelog, holoscan_sdk info, platforms, tags, ranking, dependencies, and run command
metadata.json ranking levels must be self-assessed from 0 (production-ready) to 5 (deprecated) with corresponding requirements
Dependencies must be correctly specified in metadata.json dependencies section
Files:
applications/holocat/metadata.json
{applications,workflows}/**/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
CMakeLists.txt must include build system integration using add_holohub_application() for applications and workflows
Files:
applications/holocat/CMakeLists.txtapplications/CMakeLists.txtapplications/holocat/cpp/CMakeLists.txt
applications/**/CMakeLists.txt
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Applications should include a testing section in CMakeLists.txt for functional testing using CTest
Files:
applications/holocat/CMakeLists.txtapplications/CMakeLists.txtapplications/holocat/cpp/CMakeLists.txt
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to operators/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_operator() for operators
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to {applications,workflows}/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_application() for applications and workflows
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: operators/video_streaming/streaming_client_enhanced/python/CMakeLists.txt:16-24
Timestamp: 2025-10-22T16:53:45.393Z
Learning: The pybind11_add_holohub_module CMake macro in cmake/pybind11_add_holohub_module.cmake encapsulates all pybind11 setup internally, including finding pybind11, linking against holoscan::pybind11 conditionally, and linking the C++ operator target. Operator Python bindings in holohub should only call this macro without additional pybind11 setup.
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to operators/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_operator() for operators
Applied to files:
applications/holocat/cpp/hc_data_rx_op.hppapplications/holocat/cpp/hc_data_tx_op.hppapplications/holocat/CMakeLists.txtapplications/CMakeLists.txtapplications/holocat/cpp/holocat_op.hppapplications/holocat/cpp/hc_data_tx_op.cppapplications/holocat/cpp/hc_data_rx_op.cppapplications/holocat/cpp/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to {operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp} : All code must adhere to Holoscan SDK coding standards including proper error handling and validation
Applied to files:
applications/holocat/cpp/hc_data_rx_op.hppapplications/holocat/cpp/holocat_op.hppapplications/holocat/cpp/holocat_app.cppapplications/holocat/cpp/hc_data_rx_op.cpp
📚 Learning: 2025-11-24T16:28:06.281Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.281Z
Learning: Applies to **/metadata.json : metadata.json must include name, authors, language, version, changelog, holoscan_sdk info, platforms, tags, ranking, dependencies, and run command
Applied to files:
applications/holocat/metadata.json
📚 Learning: 2025-11-24T16:27:43.600Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:27:43.600Z
Learning: Applies to **/metadata.json : When reviewing PRs that modify or add `metadata.json` files, verify that the **category** (first tag in the `tags` array) matches one of the approved categories: Benchmarking, Camera, Computer Vision and Perception, Converter, Deployment, Development, Extended Reality, Healthcare AI, Image Processing, Inference, Interoperability, Medical Imaging, Natural Language and Conversational AI, Networking and Distributed Computing, Optimization, Quantum Computing, Rendering, Robotics, Scheduler, Signal Processing, Streaming, Threading, Video, Video Capture, Visualization, XR
Applied to files:
applications/holocat/metadata.json
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to {applications,workflows}/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_application() for applications and workflows
Applied to files:
applications/holocat/CMakeLists.txtapplications/CMakeLists.txtapplications/holocat/cpp/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to gxf_extensions/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_extension() for GXF extensions
Applied to files:
applications/holocat/CMakeLists.txtapplications/CMakeLists.txtapplications/holocat/cpp/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to applications/**/CMakeLists.txt : Applications should include a testing section in CMakeLists.txt for functional testing using CTest
Applied to files:
applications/holocat/CMakeLists.txtapplications/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.281Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.281Z
Learning: Applies to applications/**/ : Applications should follow the directory structure with metadata.json, README.md, implementation files, and CMakeLists.txt
Applied to files:
applications/holocat/CMakeLists.txtapplications/CMakeLists.txt
📚 Learning: 2025-10-22T16:53:45.393Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: operators/video_streaming/streaming_client_enhanced/python/CMakeLists.txt:16-24
Timestamp: 2025-10-22T16:53:45.393Z
Learning: The pybind11_add_holohub_module CMake macro in cmake/pybind11_add_holohub_module.cmake encapsulates all pybind11 setup internally, including finding pybind11, linking against holoscan::pybind11 conditionally, and linking the C++ operator target. Operator Python bindings in holohub should only call this macro without additional pybind11 setup.
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Each operator should have at least one associated application demonstrating its capabilities
Applied to files:
applications/CMakeLists.txt
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to operators/**/*.{py,cpp,hpp} : Use TitleCase + 'Op' suffix for operator class names
Applied to files:
applications/holocat/cpp/holocat_op.hpp
🧬 Code graph analysis (3)
applications/holocat/cpp/hc_data_rx_op.hpp (2)
applications/holocat/cpp/hc_data_tx_op.hpp (2)
spec(40-40)op_input(48-50)applications/holocat/cpp/holocat_op.hpp (2)
spec(63-63)op_input(65-67)
applications/holocat/cpp/hc_data_tx_op.hpp (2)
applications/holocat/cpp/hc_data_rx_op.hpp (2)
spec(38-38)op_input(46-48)applications/holocat/cpp/holocat_op.hpp (2)
spec(63-63)op_input(65-67)
applications/holocat/cpp/holocat_op.hpp (3)
applications/holocat/cpp/holocat_app.hpp (1)
config(25-25)applications/holocat/cpp/hc_data_rx_op.hpp (2)
spec(38-38)op_input(46-48)applications/holocat/cpp/hc_data_tx_op.hpp (2)
spec(40-40)op_input(48-50)
🪛 Cppcheck (2.18.0)
applications/holocat/cpp/main.cpp
[error] 55-55: There is an unknown macro here somewhere. Configuration is required. If HOLOSCAN_OPERATOR_FORWARD_ARGS is a macro then please configure it.
(unknownMacro)
applications/holocat/cpp/holocat_op.cpp
[error] 55-55: There is an unknown macro here somewhere. Configuration is required. If HOLOSCAN_OPERATOR_FORWARD_ARGS is a macro then please configure it.
(unknownMacro)
applications/holocat/cpp/holocat_app.cpp
[error] 55-55: There is an unknown macro here somewhere. Configuration is required. If HOLOSCAN_OPERATOR_FORWARD_ARGS is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (14)
applications/CMakeLists.txt (1)
152-152: LGTM!The holocat application is correctly integrated using
add_holohub_application()and positioned in proper alphabetical order.As per coding guidelines, CMakeLists.txt correctly uses add_holohub_application().
applications/holocat/cpp/holocat_app.hpp (1)
1-28: LGTM!The application class is properly structured, inheriting from holoscan::Application with appropriate method declarations for configuration extraction and validation.
applications/holocat/cpp/hc_data_tx_op.cpp (1)
17-22: LGTM!The setup method correctly configures the output port with appropriate logging.
applications/holocat/scripts/verify_ecmaster.sh (1)
1-221: Well-structured verification script.Comprehensive SDK validation covering directory structure, libraries, version extraction, and system requirements. The colored output and clear next steps guidance are helpful.
applications/holocat/cpp/main.cpp (1)
54-74: LGTM!Clean argument parsing with proper error handling for missing values and unknown options.
applications/holocat/cpp/holocat_op.cpp (1)
32-59: LGTM!The logging bridge correctly maps EC-Master severity levels to Holoscan logging with appropriate prefixing.
applications/holocat/Dockerfile (2)
29-31: User permissions are appropriately configured.The holocat user creation with scoped NOPASSWD access for
setcapis appropriate for granting real-time capabilities to the application without requiring full root access during normal operation.
45-49: Verification fallback logic is sensible.The approach of running the full verification script with a fallback to minimal SDK checks (header and library existence) provides build resilience while still ensuring essential components are present.
applications/holocat/FindEcMaster.cmake (2)
123-137: Imported target is correctly defined.The
EcMaster::EcMasterimported target follows CMake conventions with proper interface properties for include directories and link libraries.
1-16: Well-structured CMake find module.Good use of policy handling for CMP0144 (environment variable handling) and clear documentation of the module's outputs.
applications/holocat/cpp/holocat_app.cpp (1)
140-154: Priority validation handles defaults correctly.The validation properly detects zero-initialized priorities (from missing config) and applies sensible defaults with appropriate warning logs.
applications/holocat/cpp/holocat_op.hpp (3)
24-38: Configuration struct is well-organized.The
HolocatConfigstruct clearly groups related configuration parameters. Includingerror_messagefor validation feedback is a pragmatic approach.
53-68: Operator class follows Holoscan conventions.The
HolocatOpclass correctly derives fromholoscan::Operator, uses theHOLOSCAN_OPERATOR_FORWARD_ARGSmacro for constructor forwarding, and follows the TitleCase + 'Op' naming convention per coding guidelines.
105-118: Async state transition design is appropriate for real-time operation.Using
std::futurefor pending state transitions allows the operator to maintain real-time scheduling while managing the EtherCAT state machine asynchronously.
| # Create executable | ||
| add_executable(holocat ${HOLOCAT_SOURCES} ${HOLOCAT_HEADERS}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing add_holohub_application() build system integration.
Per HoloHub coding guidelines, applications must use add_holohub_application() for proper build system integration instead of raw add_executable(). This ensures consistent build behavior and integration with the HoloHub infrastructure.
Based on learnings, the CMakeLists.txt should include build system integration using add_holohub_application().
🤖 Prompt for AI Agents
In applications/holocat/cpp/CMakeLists.txt around lines 47-48, the build
currently calls add_executable(holocat ${HOLOCAT_SOURCES} ${HOLOCAT_HEADERS})
which violates HoloHub guidelines; replace this raw add_executable call with the
prescribed add_holohub_application() invocation, passing the same target name
and source/header lists (and any required properties or options the HoloHub
macro expects) so the holocat target is registered through the HoloHub build
integration rather than created directly.
| option(HOLOCAT_SET_CAPABILITIES "Set Linux capabilities during build" ON) | ||
| if(HOLOCAT_SET_CAPABILITIES) | ||
| add_custom_command(TARGET holocat POST_BUILD | ||
| COMMAND sudo setcap 'cap_net_raw,cap_sys_nice,cap_ipc_lock=ep' $<TARGET_FILE:holocat> | ||
| COMMENT "Setting capabilities for real-time EtherCAT operation" | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
sudo in POST_BUILD will break non-privileged builds.
Using sudo in a CMake post-build command requires interactive authentication and will fail in CI/CD pipelines, containers without sudo, or when users lack sudo access. Consider making this a separate script or installation step rather than part of the build.
-option(HOLOCAT_SET_CAPABILITIES "Set Linux capabilities during build" ON)
+option(HOLOCAT_SET_CAPABILITIES "Set Linux capabilities during build" OFF)
if(HOLOCAT_SET_CAPABILITIES)
+ message(WARNING "HOLOCAT_SET_CAPABILITIES requires sudo privileges - ensure you have appropriate access")
add_custom_command(TARGET holocat POST_BUILD
COMMAND sudo setcap 'cap_net_raw,cap_sys_nice,cap_ipc_lock=ep' $<TARGET_FILE:holocat>
COMMENT "Setting capabilities for real-time EtherCAT operation"
)
endif()Alternatively, provide a separate install script that users can run manually with appropriate privileges.
📝 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.
| option(HOLOCAT_SET_CAPABILITIES "Set Linux capabilities during build" ON) | |
| if(HOLOCAT_SET_CAPABILITIES) | |
| add_custom_command(TARGET holocat POST_BUILD | |
| COMMAND sudo setcap 'cap_net_raw,cap_sys_nice,cap_ipc_lock=ep' $<TARGET_FILE:holocat> | |
| COMMENT "Setting capabilities for real-time EtherCAT operation" | |
| ) | |
| endif() | |
| option(HOLOCAT_SET_CAPABILITIES "Set Linux capabilities during build" OFF) | |
| if(HOLOCAT_SET_CAPABILITIES) | |
| message(WARNING "HOLOCAT_SET_CAPABILITIES requires sudo privileges - ensure you have appropriate access") | |
| add_custom_command(TARGET holocat POST_BUILD | |
| COMMAND sudo setcap 'cap_net_raw,cap_sys_nice,cap_ipc_lock=ep' $<TARGET_FILE:holocat> | |
| COMMENT "Setting capabilities for real-time EtherCAT operation" | |
| ) | |
| endif() |
🤖 Prompt for AI Agents
In applications/holocat/cpp/CMakeLists.txt around lines 83-89, remove the use of
sudo in the POST_BUILD add_custom_command (it breaks non-privileged/CI builds)
and instead move capability-setting out of the build phase: implement either (a)
an install-time step (e.g. add a CMake install(CODE) or install script that runs
setcap) or (b) a separate optional install target/script (e.g.
add_custom_target(setcap INSTALL_COMMAND ...) or a packaged script in CMake
config) and document that it must be run with appropriate privileges; ensure the
build does not require sudo, optionally add a CMake option to enable automatic
install-time setcap and emit a clear message when capabilities are not set so
users know to run the privileged script.
| /*----------------------------------------------------------------------------- | ||
| * hc_data_rx_op.cpp | ||
| * HoloCat Data Receive Operator | ||
| * | ||
| * This file implements the HcDataTxOp operator that generates incrementing | ||
| * counter data for testing and demonstration purposes. | ||
| *---------------------------------------------------------------------------*/ |
There was a problem hiding this comment.
Copy-paste error in file header comment.
The header describes HcDataTxOp but this file implements HcDataRxOp.
/*-----------------------------------------------------------------------------
* hc_data_rx_op.cpp
* HoloCat Data Receive Operator
*
- * This file implements the HcDataTxOp operator that generates incrementing
- * counter data for testing and demonstration purposes.
+ * This file implements the HcDataRxOp operator that receives counter
+ * data from the EtherCAT bus for testing and demonstration purposes.
*---------------------------------------------------------------------------*/📝 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.
| /*----------------------------------------------------------------------------- | |
| * hc_data_rx_op.cpp | |
| * HoloCat Data Receive Operator | |
| * | |
| * This file implements the HcDataTxOp operator that generates incrementing | |
| * counter data for testing and demonstration purposes. | |
| *---------------------------------------------------------------------------*/ | |
| /*----------------------------------------------------------------------------- | |
| * hc_data_rx_op.cpp | |
| * HoloCat Data Receive Operator | |
| * | |
| * This file implements the HcDataRxOp operator that receives counter | |
| * data from the EtherCAT bus for testing and demonstration purposes. | |
| *---------------------------------------------------------------------------*/ |
🤖 Prompt for AI Agents
In applications/holocat/cpp/hc_data_rx_op.cpp around lines 1 to 7, the file
header comment incorrectly references HcDataTxOp and describes transmitter
behavior; update the header to correctly name HcDataRxOp and describe that this
file implements the HcDataRxOp (the data receive operator) and its purpose
(e.g., receiving/parsing incoming data for HoloCat), ensuring the header text
accurately reflects the implementation and purpose.
|
|
||
| /** | ||
| * @brief Compute method called on each execution cycle | ||
| * @param op_input Input context (unused for this source operator) |
There was a problem hiding this comment.
Fix incorrect comment about operator type.
The comment states "unused for this source operator" but HcDataRxOp is a receiver/sink operator, not a source operator.
Apply this diff:
- * @param op_input Input context (unused for this source operator)
+ * @param op_input Input context for receiving counter data📝 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.
| * @param op_input Input context (unused for this source operator) | |
| * @param op_input Input context for receiving counter data |
🤖 Prompt for AI Agents
In applications/holocat/cpp/hc_data_rx_op.hpp around line 42, the param comment
incorrectly refers to HcDataRxOp as a "source operator"; this operator is a
receiver/sink. Update the comment text for the op_input parameter to reflect the
correct operator type (for example: "op_input Input context (unused for this
receiver/sink operator)" or "unused for this sink operator") so the docstring
accurately describes HcDataRxOp.
| if(ECMASTER_VERSION_MAJOR AND ECMASTER_VERSION_MINOR AND ECMASTER_VERSION_PATCH) | ||
| set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}.${ECMASTER_VERSION_BUILD}") | ||
| endif() |
There was a problem hiding this comment.
Version string may include empty component if BUILD is not found.
If EC_VERSION_BUILD is not defined in the header, ECMASTER_VERSION_BUILD will be empty, resulting in a version string like "3.2.1." with a trailing dot.
Consider conditionally including the build number:
if(ECMASTER_VERSION_MAJOR AND ECMASTER_VERSION_MINOR AND ECMASTER_VERSION_PATCH)
- set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}.${ECMASTER_VERSION_BUILD}")
+ if(ECMASTER_VERSION_BUILD)
+ set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}.${ECMASTER_VERSION_BUILD}")
+ else()
+ set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}")
+ endif()
endif()📝 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.
| if(ECMASTER_VERSION_MAJOR AND ECMASTER_VERSION_MINOR AND ECMASTER_VERSION_PATCH) | |
| set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}.${ECMASTER_VERSION_BUILD}") | |
| endif() | |
| if(ECMASTER_VERSION_MAJOR AND ECMASTER_VERSION_MINOR AND ECMASTER_VERSION_PATCH) | |
| if(ECMASTER_VERSION_BUILD) | |
| set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}.${ECMASTER_VERSION_BUILD}") | |
| else() | |
| set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}") | |
| endif() | |
| endif() |
🤖 Prompt for AI Agents
In applications/holocat/FindEcMaster.cmake around lines 100-102, the version
string unconditionally appends ECMASTER_VERSION_BUILD which can be empty and
yield a trailing dot; change the logic to build a base version from
ECMASTER_VERSION_MAJOR.MINOR.PATCH and then only append
".${ECMASTER_VERSION_BUILD}" if ECMASTER_VERSION_BUILD is defined and not empty
(e.g., check using IF(DEFINED ECMASTER_VERSION_BUILD AND NOT
"${ECMASTER_VERSION_BUILD}" STREQUAL "") or similar) so the final
ECMASTER_VERSION has no trailing dot when build is missing.
| "tags": [ | ||
| "EtherCAT" | ||
| ], |
There was a problem hiding this comment.
Invalid category tag - use approved category.
The first tag in the tags array serves as the category and must be from the approved list. "EtherCAT" is not an approved category. Based on the application's purpose, consider using "Interoperability" or "Networking and Distributed Computing".
As per coding guidelines, the category must match one of the approved categories.
Apply this diff to use an appropriate category:
"tags": [
- "EtherCAT"
+ "Interoperability",
+ "EtherCAT"
],Or if networking focus is more appropriate:
"tags": [
- "EtherCAT"
+ "Networking and Distributed Computing",
+ "EtherCAT"
],📝 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.
| "tags": [ | |
| "EtherCAT" | |
| ], | |
| "tags": [ | |
| "Interoperability", | |
| "EtherCAT" | |
| ], |
| "tags": [ | |
| "EtherCAT" | |
| ], | |
| "tags": [ | |
| "Networking and Distributed Computing", | |
| "EtherCAT" | |
| ], |
🤖 Prompt for AI Agents
In applications/holocat/metadata.json around lines 25 to 27, the first element
of the "tags" array ("EtherCAT") is not an approved category; replace it with an
approved category that reflects the app purpose — e.g. change the first tag to
"Interoperability" (or "Networking and Distributed Computing" if networking is
more accurate) so the tags[0] matches the approved category list and keep any
additional tags after it.
| # HoloCat - EtherCAT Real-time Integration | ||
|
|
||
|  | ||
|
|
||
| HoloCat is an EtherCAT master application that integrates the acontis EC-Master SDK with NVIDIA's Holoscan platform. | ||
|
|
||
| ## Overview | ||
|
|
||
| HoloCat provides deterministic EtherCAT communication capabilities within the Holoscan ecosystem, enabling: | ||
|
|
||
| - **Real-time Control** | ||
| - **Holoscan Native** | ||
|
|
||
| ## Prerequisites | ||
|
|
||
| ### Required Dependencies | ||
|
|
||
| 1. **acontis EC-Master SDK** (Commercial License) | ||
| - Version 3.2.3 or later | ||
|
|
||
| ## Usage | ||
|
|
||
| ### Prerequisites | ||
| ```bash | ||
| # Set EC-Master SDK path | ||
| export ECMASTER_ROOT=/home/hking/devel/ethercat/ecm | ||
|
|
||
| # Verify installation (optional) | ||
| ./applications/holocat/scripts/verify_ecmaster.sh | ||
| ``` | ||
|
|
||
| ### Build | ||
| ```bash | ||
| # Build using HoloHub CLI (recommended) | ||
| ./holohub build holocat --local | ||
| ``` | ||
|
|
||
| ### Run | ||
| ```bash | ||
| # Run with configuration file | ||
| ./build/holocat/applications/holocat/cpp/holocat --config ./applications/holocat/configs/holocat_config.yaml | ||
| ``` | ||
|
|
||
| ## Configuration | ||
|
|
||
| ### Basic Configuration | ||
|
|
||
| Create `holocat_config.yaml`: | ||
|
|
||
| ```yaml | ||
| holocat: | ||
| # Network adapter for EtherCAT | ||
| adapter_name: "eth0" # Change to your EtherCAT interface | ||
|
|
||
| # EtherCAT configuration file | ||
| eni_file: "/tmp/holocat_config.xml" | ||
|
|
||
| # Cycle time in microseconds | ||
| cycle_time_us: 1000 # 1ms cycle time | ||
|
|
||
| # Real-time priorities (1-99) | ||
| rt_priority: 39 | ||
| job_thread_priority: 98 | ||
|
|
||
| # Enable real-time scheduling | ||
| enable_rt: true | ||
|
|
||
| # Holoscan application configuration | ||
| holoscan: | ||
| logging: | ||
| level: "info" | ||
| ``` | ||
| ### ENI File Generation | ||
| Use EtherCAT configuration tools to generate your ENI file. | ||
| ## Troubleshooting | ||
| ### Common Issues | ||
| 1. **Permission Denied** | ||
| ```bash | ||
| # Ensure capabilities are set | ||
| sudo setcap 'cap_net_raw=ep' ./build/holocat/applications/holocat/holocat | ||
| ``` | ||
|
|
||
| 3. **EC-Master SDK Not Found** | ||
| ```bash | ||
| # Verify ECMASTER_ROOT environment variable | ||
| echo $ECMASTER_ROOT | ||
| ls -la $ECMASTER_ROOT/SDK/INC/EcMaster.h | ||
| ``` No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add architecture overview section.
Per coding guidelines, the README should include an architecture overview. Add a section explaining the operator architecture (HolocatOp, HcDataTxOp, HcDataRxOp) and how they interact with the EtherCAT master and Holoscan framework.
As per coding guidelines, README.md must include architecture overview.
🤖 Prompt for AI Agents
applications/holocat/README.md lines 1-94: The README lacks the required
"Architecture Overview" section; add a concise subsection titled "Architecture
Overview" describing the operator architecture: list and briefly describe
HolocatOp, HcDataTxOp, and HcDataRxOp, explain that HolocatOp manages lifecycle
and interfaces with the acontis EC‑Master SDK (initialization, ENI loading,
cycle scheduling), HcDataTxOp handles outgoing process data to EtherCAT slaves
and timing control, and HcDataRxOp handles incoming process data and publishes
to Holoscan; include a simple diagram or sequence description of how data flows
between Holoscan application -> HolocatOp -> (HcDataTxOp -> EtherCAT master ->
slaves) and (slaves -> EtherCAT master -> HcDataRxOp -> Holoscan), and mention
real‑time considerations (cycle timing and priorities) and configuration points
(adapter_name, eni_file, cycle_time_us).
| for lib in "${LINK_LIBS[@]}"; do | ||
| if [ -f "$LIB_DIR/$lib" ]; then | ||
| print_status "OK" "$lib found" | ||
| ((FOUND_LIBS++)) | ||
| else | ||
| print_status "WARN" "$lib not found (may be optional)" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Arithmetic increment may cause premature exit with set -e.
When FOUND_LIBS is 0, the expression ((FOUND_LIBS++)) evaluates to 0 (falsy), which bash treats as a failure. Combined with set -e, this will exit the script prematurely on the first found library.
if [ -f "$LIB_DIR/$lib" ]; then
print_status "OK" "$lib found"
- ((FOUND_LIBS++))
+ FOUND_LIBS=$((FOUND_LIBS + 1))
else
print_status "WARN" "$lib not found (may be optional)"
fi📝 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.
| for lib in "${LINK_LIBS[@]}"; do | |
| if [ -f "$LIB_DIR/$lib" ]; then | |
| print_status "OK" "$lib found" | |
| ((FOUND_LIBS++)) | |
| else | |
| print_status "WARN" "$lib not found (may be optional)" | |
| fi | |
| done | |
| for lib in "${LINK_LIBS[@]}"; do | |
| if [ -f "$LIB_DIR/$lib" ]; then | |
| print_status "OK" "$lib found" | |
| FOUND_LIBS=$((FOUND_LIBS + 1)) | |
| else | |
| print_status "WARN" "$lib not found (may be optional)" | |
| fi | |
| done |
🤖 Prompt for AI Agents
In applications/holocat/scripts/verify_ecmaster.sh around lines 111-118, the use
of the post-increment expression ((FOUND_LIBS++)) can produce a zero exit status
when FOUND_LIBS is 0 and cause the script to exit under set -e; change the
increment to a form that yields a non-zero arithmetic result (for example use
FOUND_LIBS=$((FOUND_LIBS+1)) or ((FOUND_LIBS+=1))) so the operation does not
return a failing status and the loop won't prematurely exit.
Greptile OverviewGreptile SummaryThis PR adds a new HoloCat application that integrates acontis EC-Master SDK for real-time EtherCAT communication with NVIDIA's Holoscan streaming framework. Key additions:
Issues requiring attention:
The implementation follows good architectural patterns with proper separation of concerns, async state transitions to maintain real-time performance, and thorough configuration validation. However, the previously identified critical issues (buffer overflow, race condition, Dockerfile build failure) must be resolved before merge. Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Main as main.cpp
participant App as HolocatApp
participant TxOp as HcDataTxOp
participant EcOp as HolocatOp
participant RxOp as HcDataRxOp
participant EcMaster as EC-Master SDK
Main->>App: Create application
Main->>App: Load config YAML
App->>App: extract_config()
App->>App: validate_config()
Note over App: Resolve ENI path<br/>Validate adapter, cycle time
App->>TxOp: Create operator (100ms periodic)
App->>EcOp: Create operator (cycle_time_us periodic)
App->>RxOp: Create operator (data-driven)
App->>App: Add flows: TxOp->EcOp->RxOp
Main->>App: run()
App->>EcOp: start()
EcOp->>EcOp: InitializeEthercatParams()
EcOp->>EcMaster: ecatInitMaster()
EcMaster-->>EcOp: Master initialized
loop Every cycle_time_us
EcOp->>EcOp: compute()
EcOp->>EcOp: BusStartupStateMachine()
Note over EcOp: INIT->CONFIGURE_NETWORK<br/>->PREOP->SAFEOP->OP
EcOp->>EcMaster: ecatConfigureNetwork(ENI)
EcOp->>EcMaster: ecatSetMasterState(async)
EcOp->>EcMaster: ecatExecJob(ProcessRxFrames)
EcOp->>EcMaster: ecatGetProcessImageInputPtr()
EcMaster-->>EcOp: Input process data
EcOp->>EcOp: Read input (inval)
EcOp->>EcMaster: ecatGetProcessImageOutputPtr()
EcOp->>EcMaster: Write output (outval)
EcOp->>EcMaster: ecatExecJob(SendCycFrames)
EcOp->>EcMaster: ecatExecJob(MasterTimer)
EcOp->>RxOp: emit(inval, "count_out")
end
loop Every 100ms
TxOp->>TxOp: compute()
TxOp->>TxOp: Increment counter
TxOp->>EcOp: emit(counter, "count_out")
end
RxOp->>RxOp: compute() [when data available]
Note over RxOp: Receive and log count
Main->>App: Application terminates
App->>EcOp: stop()
EcOp->>EcMaster: ecatSetMasterState(INIT, async)
Note over EcOp: Sleep 1s (race condition)
EcOp->>EcMaster: ecatDeinitMaster()
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (2)
-
applications/holocat/Dockerfile, line 3-4 (link)syntax:
BASE_IMAGEis used but never defined. The ARG declaresBASE_SDK_VERSIONbut the FROM uses${BASE_IMAGE}. This will cause the Docker build to fail. -
applications/holocat/CMakeLists.txt, line 26-30 (link)syntax: Unreachable code.
message(FATAL_ERROR ...)already terminates CMake execution, soexit(1)will never run.
19 files reviewed, 2 comments
|
Hi @hawkeyeking, thanks for this contribution. There are a few comments from the AI reviewer, which worth checking if not relevant please make the resolve with a short comment. Once they are addressed, we'd be happy to do the full review. Thaks |
There was a problem hiding this comment.
Additional Comments (3)
-
applications/holocat/CMakeLists.txt, line 26-29 (link)logic: Unreachable code -
message(FATAL_ERROR ...)terminates CMake execution immediately, soexit(1)will never run. -
applications/holocat/Dockerfile, line 3-4 (link)syntax:
BASE_IMAGEis used in the FROM directive but onlyBASE_SDK_VERSIONis declared as an ARG. Either use the declared variable or addARG BASE_IMAGE. -
applications/holocat/cpp/holocat_op.cpp, line 163-165 (link)logic: Unreachable code -
returnstatement afterthrowwill never execute.
19 files reviewed, 3 comments
There was a problem hiding this comment.
Pull request overview
This PR introduces HoloCat, a new application that integrates EtherCAT real-time industrial communication with NVIDIA's Holoscan SDK. It enables deterministic fieldbus communication for industrial automation applications using the commercial acontis EC-Master SDK.
Key Changes:
- Complete EtherCAT master implementation with Holoscan operator architecture
- Three operators: HolocatOp (main EtherCAT master), HcDataTxOp (data transmission), and HcDataRxOp (data reception)
- Docker containerization with verification scripts and CMake build system
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| applications/holocat/metadata.json | Application metadata with dependencies and configuration modes; category needs correction |
| applications/holocat/cpp/main.cpp | Main entry point with CLI argument parsing; missing return statement |
| applications/holocat/cpp/holocat_op.{hpp,cpp} | Core EtherCAT operator with state machine; unreachable code present |
| applications/holocat/cpp/holocat_app.{hpp,cpp} | Application composition and config validation; format string inconsistency |
| applications/holocat/cpp/hc_data_tx_op.{hpp,cpp} | Counter data generator operator; minor typo in comment |
| applications/holocat/cpp/hc_data_rx_op.{hpp,cpp} | Data receiver operator; incorrect file header documentation |
| applications/holocat/cpp/CMakeLists.txt | C++ build configuration with EC-Master SDK integration |
| applications/holocat/CMakeLists.txt | Top-level build script; unreachable exit statement |
| applications/holocat/FindEcMaster.cmake | CMake module for locating EC-Master SDK installation |
| applications/holocat/configs/holocat_config.yaml | Runtime configuration; contains hardcoded developer-specific paths |
| applications/holocat/configs/eni2.xml | EtherCAT Network Information configuration file |
| applications/holocat/scripts/verify_ecmaster.sh | SDK verification utility; hardcoded path in example |
| applications/holocat/Dockerfile | Container build definition; critical BASE_IMAGE variable issue |
| applications/holocat/README.md | Documentation with setup and usage instructions; list numbering and hardcoded path issues |
| applications/CMakeLists.txt | Adds holocat to build system |
| "aarch64" | ||
| ], | ||
| "tags": [ | ||
| "EtherCAT" |
There was a problem hiding this comment.
The category "EtherCAT" is not in the approved category list. Based on the README description, this application provides "deterministic EtherCAT communication capabilities" for "Real-time Control" and appears to be focused on networking and industrial communication.
Please replace "EtherCAT" with an existing category from the approved category list. Here are some suitable alternatives:
- For industrial real-time communication:
Networking and Distributed Computing - For real-time control aspects:
Robotics - For communication protocols:
Interoperability
The most appropriate would be Networking and Distributed Computing or Interoperability given the EtherCAT protocol's focus on industrial networking.
| holoscan::ExecutionContext& context) override; | ||
|
|
||
| private: | ||
| // coutner from 0 to maxcount for output on ECat bus |
There was a problem hiding this comment.
Spelling error: "coutner" should be "counter".
| // coutner from 0 to maxcount for output on ECat bus | |
| // counter from 0 to maxcount for output on ECat bus |
| HOLOSCAN_LOG_ERROR("Cannot initialize EtherCAT-Master: {} (0x{:x})", | ||
| ecatGetText(dwRes), dwRes); | ||
| throw std::runtime_error("Cannot initialize EtherCAT-Master: " + std::string(ecatGetText(dwRes))); | ||
| return; |
There was a problem hiding this comment.
Unreachable code: The return statement on line 165 will never be executed because line 164 throws an exception. Remove the return statement.
| return; |
| adapter_name: "enxc84bd6d2788c" | ||
|
|
||
| # EtherCAT Network Information (ENI) file path | ||
| # Use environment variable HOLOCAT_ENI_FILE to override, or falls back to config dir | ||
| eni_file: "/home/hking/devel/holo/holohub/applications/holocat/configs/eni2.xml" |
There was a problem hiding this comment.
Hardcoded user-specific paths in configuration file. The adapter_name "enxc84bd6d2788c" and eni_file path "/home/hking/devel/holo/holohub/applications/holocat/configs/eni2.xml" contain developer-specific information that should be replaced with generic placeholders or relative paths. Consider using:
- adapter_name: "eth0" (as shown in the README example)
- eni_file: A relative path or environment variable-based path
| adapter_name: "enxc84bd6d2788c" | |
| # EtherCAT Network Information (ENI) file path | |
| # Use environment variable HOLOCAT_ENI_FILE to override, or falls back to config dir | |
| eni_file: "/home/hking/devel/holo/holohub/applications/holocat/configs/eni2.xml" | |
| adapter_name: "eth0" | |
| # EtherCAT Network Information (ENI) file path | |
| # Use environment variable HOLOCAT_ENI_FILE to override, or falls back to config dir | |
| eni_file: "eni2.xml" |
applications/holocat/README.md
Outdated
| ### Prerequisites | ||
| ```bash | ||
| # Set EC-Master SDK path | ||
| export ECMASTER_ROOT=/home/hking/devel/ethercat/ecm |
There was a problem hiding this comment.
Hardcoded user-specific path in documentation. The path "/home/hking/devel/ethercat/ecm" is developer-specific and should be replaced with a generic placeholder like "/path/to/ec-master" or "/opt/acontis/ecmaster".
| export ECMASTER_ROOT=/home/hking/devel/ethercat/ecm | |
| export ECMASTER_ROOT=/opt/acontis/ecmaster # Adjust to your EC-Master install path |
applications/holocat/README.md
Outdated
| sudo setcap 'cap_net_raw=ep' ./build/holocat/applications/holocat/holocat | ||
| ``` | ||
|
|
||
| 3. **EC-Master SDK Not Found** |
There was a problem hiding this comment.
List numbering is incorrect. The list jumps from "1. Permission Denied" to "3. EC-Master SDK Not Found", skipping item 2. Either add the missing item 2 or renumber item 3 as item 2.
| 3. **EC-Master SDK Not Found** | |
| 2. **EC-Master SDK Not Found** |
| print_status "ERROR" "ECMASTER_ROOT environment variable not set" | ||
| echo " Set it to your EC-Master installation path:" | ||
| echo " export ECMASTER_ROOT=/path/to/ecm" | ||
| echo " Example: export ECMASTER_ROOT=/home/hking/devel/ethercat/ecm" |
There was a problem hiding this comment.
Hardcoded user-specific path in example. Line 38 contains a developer-specific path "/home/hking/devel/ethercat/ecm" in the example that should use a generic placeholder path like "/path/to/ec-master" or "/opt/acontis/ecmaster".
| echo " Example: export ECMASTER_ROOT=/home/hking/devel/ethercat/ecm" | |
| echo " Example: export ECMASTER_ROOT=/opt/acontis/ecmaster" |
applications/holocat/CMakeLists.txt
Outdated
|
|
||
| # Exit with failure | ||
| exit(1) |
There was a problem hiding this comment.
The exit(1) command on line 29 is unreachable because message(FATAL_ERROR) on line 26 already terminates the CMake configuration process. Remove lines 28-29 as they serve no purpose.
| # Exit with failure | |
| exit(1) |
| std::shared_ptr<HolocatOp> holocat_op = make_operator<HolocatOp>("holocat_op", ecat_bus_periodic_cond); | ||
| holocat_op->set_config(config); | ||
| add_operator(holocat_op); | ||
| HOLOSCAN_LOG_INFO("HoloCat operator created with %dus periodic condition", config.cycle_time_us); |
There was a problem hiding this comment.
Inconsistent log formatting. Line 39 uses printf-style format string "%dus" while other log statements in the same function (lines 30, 31) use modern C++ format string style with "{}". Use the consistent format string style: HOLOSCAN_LOG_INFO("HoloCat operator created with {}us periodic condition", config.cycle_time_us);
| HOLOSCAN_LOG_INFO("HoloCat operator created with %dus periodic condition", config.cycle_time_us); | |
| HOLOSCAN_LOG_INFO("HoloCat operator created with {}us periodic condition", config.cycle_time_us); |
| usermod -aG sudo holocat && \ | ||
| echo "holocat ALL=(ALL) NOPASSWD: /usr/sbin/setcap" >> /etc/sudoers | ||
|
|
There was a problem hiding this comment.
The Dockerfile grants the holocat user passwordless sudo access to /usr/sbin/setcap, which allows that user (and any code running as it) to assign arbitrary Linux capabilities to any binary in the container. If an attacker gains code execution as holocat, they can use sudo setcap to escalate privileges and grant powerful capabilities (e.g., raw network, sys_admin) to other binaries, breaking the intended least-privilege isolation. Consider removing this sudoers entry and instead setting the minimal required capabilities at build time or via the container runtime, without allowing arbitrary setcap invocation from inside the container.
| usermod -aG sudo holocat && \ | |
| echo "holocat ALL=(ALL) NOPASSWD: /usr/sbin/setcap" >> /etc/sudoers | |
| usermod -aG sudo holocat |
There was a problem hiding this comment.
Additional Comments (9)
-
applications/CMakeLists.txt, line 152 (link)style: The holocat application should be placed in alphabetical order per the comment on line 16. It should be positioned between 'high_speed_endoscopy' and 'holochat'.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
applications/holocat/CMakeLists.txt, line 28-29 (link)logic: unreachable code after
FATAL_ERROR- CMake stops processing after the fatal error message -
applications/holocat/cpp/hc_data_tx_op.hpp, line 53 (link)syntax: Typo: 'coutner' should be 'counter'
-
applications/holocat/cpp/holocat_op.cpp, line 165 (link)syntax: unreachable code after throw statement
-
applications/holocat/cpp/hc_data_rx_op.hpp, line 42 (link)style: Documentation inconsistency - comment states op_input is unused but this is a receiver operator, not a source operator
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
applications/holocat/Dockerfile, line 4 (link)logic: BASE_IMAGE variable is undefined. Change to BASE_SDK_VERSION or add ARG BASE_IMAGE declaration.
-
applications/holocat/metadata.json, line 5-7 (link)style: missing author name field - only affiliation is specified. Should an author name be added to the metadata?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
applications/holocat/cpp/holocat_app.cpp, line 42-43 (link)style: Condition name 'ethercat_cycle' is used for both EtherCAT bus and counter update conditions, but they have different periods (configurable vs 100ms). Consider using distinct names. Should these conditions have the same name when they serve different purposes with different timing?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
applications/holocat/cpp/hc_data_rx_op.cpp, line 5-6 (link)syntax: Comment incorrectly describes HcDataTxOp instead of HcDataRxOp
19 files reviewed, 9 comments
|
Hi @hawkeyeking, it would be great if you could address the comments on this PR. Please don’t hesitate to reach out if you need any help. Thanks! |
| # HoloCat EtherCAT Application Dockerfile | ||
|
|
||
| ARG BASE_SDK_VERSION | ||
| FROM ${BASE_IMAGE} |
There was a problem hiding this comment.
the variable BASE_IMAGE is undefined and will cause Docker build failure. The ARG BASE_SDK_VERSION is declared on line 3 but never used.
| FROM ${BASE_IMAGE} | |
| FROM nvcr.io/nvidia/clara-holoscan/holoscan:${BASE_SDK_VERSION} |
| char buffer[1024]; | ||
| const char* prefix = "[EC-Master] "; | ||
| strcpy(buffer, prefix); | ||
| vsnprintf(buffer + strlen(prefix), sizeof(buffer) - strlen(prefix), szFormat, vaArgs); |
There was a problem hiding this comment.
buffer overflow vulnerability: strcpy does not perform bounds checking. If prefix length + formatted string length exceeds 1024 bytes, this will write beyond buffer bounds causing memory corruption.
| char buffer[1024]; | |
| const char* prefix = "[EC-Master] "; | |
| strcpy(buffer, prefix); | |
| vsnprintf(buffer + strlen(prefix), sizeof(buffer) - strlen(prefix), szFormat, vaArgs); | |
| char buffer[1024]; | |
| const char* prefix = "[EC-Master] "; | |
| size_t prefix_len = strlen(prefix); | |
| strncpy(buffer, prefix, sizeof(buffer) - 1); | |
| buffer[sizeof(buffer) - 1] = '\0'; | |
| vsnprintf(buffer + prefix_len, sizeof(buffer) - prefix_len - 1, szFormat, vaArgs); |
| throw std::runtime_error("Cannot initialize EtherCAT-Master: " + std::string(ecatGetText(dwRes))); | ||
| return; |
There was a problem hiding this comment.
the return statement after throw is unreachable code and should be removed
| throw std::runtime_error("Cannot initialize EtherCAT-Master: " + std::string(ecatGetText(dwRes))); | |
| return; | |
| throw std::runtime_error("Cannot initialize EtherCAT-Master: " + std::string(ecatGetText(dwRes))); |
applications/holocat/CMakeLists.txt
Outdated
| message(FATAL_ERROR "EC-Master SDK not found") | ||
|
|
||
| # Exit with failure | ||
| exit(1) |
There was a problem hiding this comment.
exit(1) after FATAL_ERROR is unreachable. CMake's message(FATAL_ERROR ...) immediately terminates configuration, so the exit(1) call will never execute.
| message(FATAL_ERROR "EC-Master SDK not found") | |
| # Exit with failure | |
| exit(1) | |
| message(FATAL_ERROR "EC-Master SDK not found") |
|
|
||
| # EtherCAT Network Information (ENI) file path | ||
| # Use environment variable HOLOCAT_ENI_FILE to override, or falls back to config dir | ||
| eni_file: "/home/hking/devel/holo/holohub/applications/holocat/configs/eni2.xml" |
There was a problem hiding this comment.
hardcoded developer-specific absolute path /home/hking/... will fail for other users. Use a relative path or environment variable instead.
| eni_file: "/home/hking/devel/holo/holohub/applications/holocat/configs/eni2.xml" | |
| eni_file: "${HOLOCAT_CONFIG_DIR}/eni2.xml" |
| pending_state_transition_ = std::async(std::launch::async, [this]() { | ||
| return ecatSetMasterState(kEthercatStateChangeTimeout_, eEcatState_INIT); | ||
| }); | ||
|
|
||
| sleep(1); | ||
| // deinitialize master | ||
| ecatDeinitMaster(); |
There was a problem hiding this comment.
race condition: the async state transition is launched but there's only a 1-second sleep() before calling ecatDeinitMaster(). If the state transition takes longer than 1 second, ecatDeinitMaster() will be called while the async operation is still accessing the master, potentially causing a crash or undefined behavior. Should wait for the future to complete before deinitializing.
| pending_state_transition_ = std::async(std::launch::async, [this]() { | |
| return ecatSetMasterState(kEthercatStateChangeTimeout_, eEcatState_INIT); | |
| }); | |
| sleep(1); | |
| // deinitialize master | |
| ecatDeinitMaster(); | |
| HOLOSCAN_LOG_INFO("HolocatOp: stopping EtherCAT Master"); | |
| // reset master to INIT | |
| pending_state_transition_ = std::async(std::launch::async, [this]() { | |
| return ecatSetMasterState(kEthercatStateChangeTimeout_, eEcatState_INIT); | |
| }); | |
| // Wait for state transition to complete before deinitializing | |
| if (pending_state_transition_.valid()) { | |
| pending_state_transition_.wait(); | |
| } | |
| // deinitialize master | |
| ecatDeinitMaster(); |
| holoscan::ExecutionContext& context) override; | ||
|
|
||
| private: | ||
| // coutner from 0 to maxcount for output on ECat bus |
There was a problem hiding this comment.
'coutner' is mis-spelled
| // coutner from 0 to maxcount for output on ECat bus | |
| // counter from 0 to maxcount for output on ECat bus |
applications/holocat/README.md
Outdated
| ### Prerequisites | ||
| ```bash | ||
| # Set EC-Master SDK path | ||
| export ECMASTER_ROOT=/home/hking/devel/ethercat/ecm |
There was a problem hiding this comment.
hardcoded developer-specific path /home/hking/... in example documentation. Should use a generic placeholder path.
| export ECMASTER_ROOT=/home/hking/devel/ethercat/ecm | |
| export ECMASTER_ROOT=/path/to/ecmaster |
Add HoloCat application that integrates EtherCAT real-time communication with NVIDIA's Holoscan streaming framework. Key Features: - Real-time EtherCAT master implementation using EC-Master SDK - Holoscan operator architecture with periodic scheduling - Data flow integration between EtherCAT and Holoscan operators - Configurable cycle times and real-time priorities - Docker containerization with proper capabilities (in-progress) Components: - HolocatOp: Main EtherCAT operator with state machine management - HcDataTxOp: Data transmission operator for counter generation - HcDataRxOp: Data reception operator for processing received data - Comprehensive configuration system with validation - Build system integration with CMake and Docker This enables real-time industrial automation applications using EtherCAT fieldbus communication within the Holoscan ecosystem.
- Fix CMake and Dockerfile build configuration errors - Address buffer overflow vulnerability in LogWrapper - Extract HolocatConfig to separate header file - Add relative path resolution for ENI configuration files - Remove dead code and unused member variables - Update documentation - Removed specific example paths from the verify_ecmaster.sh script to enhance portability.
hawkeyeking
force-pushed
the
hking/holocat_hs
branch
from
be06ee0 to
473f21e
Compare
| * This file implements the HcDataTxOp operator that generates incrementing | ||
| * counter data for testing and demonstration purposes. |
There was a problem hiding this comment.
Comment incorrectly states this is HcDataTxOp when it's actually HcDataRxOp
| * This file implements the HcDataTxOp operator that generates incrementing | |
| * counter data for testing and demonstration purposes. | |
| * This file implements the HcDataRxOp operator that receives and processes | |
| * counter data from the EtherCAT bus for testing and demonstration purposes. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@applications/CMakeLists.txt`:
- Around line 159-161: The application list in the CMake file is out of
alphabetical order: the add_holohub_application(holocat) entry is appended at
the end next to vila_live and holochat; move the holocat entry into the proper
"holo*" section so the "holo*" entries remain alphabetized and keep
add_holohub_application(holochat) adjacent to it (i.e., reorder the
add_holohub_application(...) lines so all holo* entries are sorted
alphabetically).
In `@applications/holocat/cpp/holocat_config.hpp`:
- Around line 9-13: The header is missing the <cstdint> include required for
fixed-width types used in this file; add `#include` <cstdint> near the top
(alongside `#include` <string>) so usages of uint32_t and uint64_t in the structs
or declarations compile correctly (update in holocat_config.hpp where those
types are referenced).
In `@applications/holocat/cpp/holocat_op.cpp`:
- Around line 354-357: The static timespec t_last in compute() causes shared
state across HolocatOp instances; make it an instance member (e.g., add struct
timespec t_last_{0,0}; and bool first_compute_ = true; to HolocatOp's class) and
replace the static usage in compute() with these members: on first call use
first_compute_ to initialize t_last_ from current time and set first_compute_ =
false, otherwise compute elapsed using t_last_ and then update t_last_ with the
current time; update any references to t_last in compute() accordingly.
In `@applications/holocat/cpp/holocat_op.hpp`:
- Around line 67-70: The declarations EcMasterJobTask() and
EcMasterJobTaskStatic(EC_T_VOID*) in holocat_op.hpp are unused; either delete
both declarations to remove dead code or keep them but add a clear TODO comment
indicating they are intentionally unimplemented and planned for future use
(e.g., "TODO: implement EcMasterJobTask and EcMasterJobTaskStatic" with expected
behavior), and if kept ensure the static signature matches the expected
OsCreateThread callback convention; update any related comments near the
declarations accordingly.
In `@applications/holocat/cpp/main.cpp`:
- Around line 94-103: Replace the use of std::filesystem::canonical(argv[0])
(which can throw) with std::filesystem::weakly_canonical or otherwise guard the
call so it won’t throw before your exists check: when args.config_file is empty,
compute config_path using
std::filesystem::weakly_canonical(argv[0]).parent_path() / "holocat_config.yaml"
(or catch filesystem_error around canonical and fall back to weakly_canonical)
so that the subsequent std::filesystem::exists(config_path) runs reliably and
your thrown runtime_error contains the intended path information (references:
config_path, args.config_file, argv, std::filesystem::canonical,
std::filesystem::weakly_canonical, std::filesystem::exists).
In `@applications/holocat/Dockerfile`:
- Around line 24-26: The Dockerfile currently hardcodes
"/opt/acontis/ecmaster/Bin/Linux/x64" in ENV LD_LIBRARY_PATH; change it to
detect/set the proper arch at build or runtime: add ARG TARGETARCH (or use an
entrypoint script) and map TARGETARCH to the vendor folder name, e.g. set ENV
ECMASTER_ARCH after mapping ("amd64"→"x64", "arm64" or "aarch64"→ appropriate
folder), then set ENV
LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:${ECMASTER_ROOT}/lib:${ECMASTER_ROOT}/Bin/Linux/${ECMASTER_ARCH};
update references to ENV ECMASTER_ROOT and LD_LIBRARY_PATH accordingly so the
path is not hardcoded to x64.
♻️ Duplicate comments (12)
applications/holocat/cpp/hc_data_rx_op.hpp (1)
40-44: Fix receiver docstring (not a source operator).The parameter comment still describes this as a “source operator.” Also,
op_outputisn’t used by the receiver.📝 Doc fix
- * `@param` op_input Input context (unused for this source operator) - * `@param` op_output Output context for emitting counter data + * `@param` op_input Input context for receiving counter data + * `@param` op_output Output context (unused for this receiver operator)applications/holocat/cpp/CMakeLists.txt (2)
47-48: Useadd_holohub_application()for app integration.This uses
add_executable(holocat ...), which may bypass Holohub’s application integration macro. Please align with existing application patterns in the repo.
Based on learnings, applications should useadd_holohub_application().#!/bin/bash # Inspect existing application CMakeLists for expected macro usage. rg -n "add_holohub_application\\(" applications -g "CMakeLists.txt"
83-87: Avoidsudoin POST_BUILD; default to non-privileged builds.Running
sudo setcapduring build will fail in CI or non-privileged environments. Prefer moving this to an install/manual step and default the option OFF.🔧 Safer default
-option(HOLOCAT_SET_CAPABILITIES "Set Linux capabilities during build" ON) +option(HOLOCAT_SET_CAPABILITIES "Set Linux capabilities during build" OFF) if(HOLOCAT_SET_CAPABILITIES) + message(WARNING "HOLOCAT_SET_CAPABILITIES requires sudo privileges; consider running setcap manually after build") add_custom_command(TARGET holocat POST_BUILD COMMAND sudo setcap 'cap_net_raw,cap_sys_nice,cap_ipc_lock=ep' $<TARGET_FILE:holocat> COMMENT "Setting capabilities for real-time EtherCAT operation" ) endif()applications/holocat/cpp/hc_data_tx_op.hpp (1)
20-23: Fix counter range docs and typo.The docs imply 0–256, but with
kMaxCount = 256the range is 0–255; also “coutner” is misspelled.✏️ Doc correction
- * A Holoscan operator that generates incrementing counter data from 0 to 256. - * This operator acts as a data source, emitting integer values that increment - * on each compute cycle. When the counter reaches 256, it wraps back to 0. + * A Holoscan operator that generates incrementing counter data from 0 to 255. + * This operator acts as a data source, emitting integer values that increment + * on each compute cycle. When the counter reaches 255, it wraps back to 0. @@ - // coutner from 0 to maxcount for output on ECat bus + // counter from 0 to kMaxCount-1 for output on EtherCAT busAlso applies to: 53-55
applications/holocat/scripts/verify_ecmaster.sh (1)
110-117: Arithmetic increment may cause premature exit withset -e.When
FOUND_LIBSis0, the expression((FOUND_LIBS++))evaluates to0(falsy), which bash treats as a failure underset -e, causing premature script termination on the first found library.Proposed fix
if [ -f "$LIB_DIR/$lib" ]; then print_status "OK" "$lib found" - ((FOUND_LIBS++)) + FOUND_LIBS=$((FOUND_LIBS + 1)) else print_status "WARN" "$lib not found (may be optional)" fiapplications/holocat/cpp/hc_data_rx_op.cpp (1)
1-7: Copy-paste error in file header comment.The header describes
HcDataTxOp("generates incrementing counter data") but this file implementsHcDataRxOp(receives data).Proposed fix
/*----------------------------------------------------------------------------- * hc_data_rx_op.cpp * HoloCat Data Receive Operator * - * This file implements the HcDataTxOp operator that generates incrementing - * counter data for testing and demonstration purposes. + * This file implements the HcDataRxOp operator that receives counter + * data from the EtherCAT bus for testing and demonstration purposes. *---------------------------------------------------------------------------*/applications/holocat/Dockerfile (1)
28-31: Granting passwordless sudo for setcap is a security risk.Allowing the
holocatuser to runsetcapvia sudo without a password enables privilege escalation if the container is compromised. Consider setting capabilities at build time or via container runtime flags (--cap-add) instead.applications/holocat/cpp/main.cpp (1)
125-133: Missingreturn 0at end ofmain().After successful execution of
app->run(), the function falls through without an explicit return statement. While C++11 and later implicitly return 0 formain(), an explicit return is clearer.Proposed fix
// Run the main application HOLOSCAN_LOG_INFO("Starting HoloCat application..."); app->run(); - - + + return 0; } catch (const std::exception& e) { HOLOSCAN_LOG_ERROR("Application error: {}", e.what()); std::cerr << "Error: " << e.what() << std::endl; return 1; } }applications/holocat/cpp/holocat_op.cpp (1)
172-185: Race condition concern instop()- async state transition not awaited.The
sleep(1)is a fixed wait that doesn't guarantee the async state transition completes before callingecatDeinitMaster(). The comment acknowledges this may "never happen," but this could lead to undefined behavior if the transition is still accessing the master.Consider using a timed wait with a reasonable timeout instead of a fixed sleep:
Proposed fix
void HolocatOp::stop() { HOLOSCAN_LOG_INFO("HolocatOp: stopping EtherCAT Master"); // reset master to INIT pending_state_transition_ = std::async(std::launch::async, [this]() { return ecatSetMasterState(kEthercatStateChangeTimeout_, eEcatState_INIT); }); - // Don't wait for state transition to complete, since that may never happen. - sleep(1); + // Wait for state transition with timeout - if it doesn't complete, proceed anyway + if (pending_state_transition_.valid()) { + auto status = pending_state_transition_.wait_for(std::chrono::seconds(2)); + if (status == std::future_status::timeout) { + HOLOSCAN_LOG_WARN("State transition to INIT timed out during shutdown"); + } + } // deinitialize master ecatDeinitMaster(); }applications/holocat/FindEcMaster.cmake (1)
100-102: Version string may have trailing dot when BUILD is not defined.If
EC_VERSION_BUILDis not defined inEcVersion.h,ECMASTER_VERSION_BUILDwill be empty, resulting in a version like"3.2.1.".Proposed fix
if(ECMASTER_VERSION_MAJOR AND ECMASTER_VERSION_MINOR AND ECMASTER_VERSION_PATCH) - set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}.${ECMASTER_VERSION_BUILD}") + if(ECMASTER_VERSION_BUILD) + set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}.${ECMASTER_VERSION_BUILD}") + else() + set(ECMASTER_VERSION "${ECMASTER_VERSION_MAJOR}.${ECMASTER_VERSION_MINOR}.${ECMASTER_VERSION_PATCH}") + endif() endif()applications/holocat/cpp/holocat_app.cpp (1)
39-39: Inconsistent log format style.Line 39 uses printf-style
%dformat specifier while all other log statements use spdlog's{}placeholders. This inconsistency should be fixed for consistency and potential runtime issues.- HOLOSCAN_LOG_INFO("HoloCat operator created with %dus periodic condition", config.cycle_time_us); + HOLOSCAN_LOG_INFO("HoloCat operator created with {}μs periodic condition", config.cycle_time_us);applications/holocat/cpp/holocat_op.hpp (1)
75-75: Misleading constant name: value is microseconds per second, not millisecond.
kUsPerMs = 1000000represents microseconds per second (1,000,000 μs = 1 s), not per millisecond (which would be 1,000 μs).- static constexpr uint64_t kUsPerMs = 1000000; // Microseconds per millisecond + static constexpr uint64_t kUsPerSec = 1000000; // Microseconds per second
🧹 Nitpick comments (13)
applications/holocat/CMakeLists.txt (1)
16-27: Add a CTest section for the application.This CMakeLists lacks the standard CTest hook expected for applications. Consider adding a guarded tests section so functional tests can be integrated when available.
✅ Suggested CTest hook
+include(CTest) +if(BUILD_TESTING AND EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/tests") + add_subdirectory(tests) +endif()As per coding guidelines, applications should include a testing section in CMakeLists.txt for functional testing using CTest.
applications/holocat/scripts/verify_ecmaster.sh (2)
134-146: Version extraction could silently produce partial output.If any of the
grepcommands fail to match (e.g., different header format), you may end up with a partial or malformed version string. Consider adding validation before constructing the version.Suggested improvement
MAJ=$(grep "#define EC_VERSION_MAJ" "$ECMASTER_ROOT/SDK/INC/EcVersion.h" | awk '{print $3}') MIN=$(grep "#define EC_VERSION_MIN" "$ECMASTER_ROOT/SDK/INC/EcVersion.h" | awk '{print $3}') SP=$(grep "#define EC_VERSION_SERVICEPACK" "$ECMASTER_ROOT/SDK/INC/EcVersion.h" | awk '{print $3}') BUILD=$(grep "#define EC_VERSION_BUILD" "$ECMASTER_ROOT/SDK/INC/EcVersion.h" | awk '{print $3}') - if [ -n "$MAJ" ] && [ -n "$MIN" ] && [ -n "$SP" ] && [ -n "$BUILD" ]; then + if [ -n "$MAJ" ] && [ -n "$MIN" ] && [ -n "$SP" ] && [ -n "$BUILD" ] && \ + [[ "$MAJ" =~ ^[0-9]+$ ]] && [[ "$MIN" =~ ^[0-9]+$ ]]; then VERSION="$MAJ.$MIN.$SP.$BUILD"
167-181: Network interface listing could be simplified and made more robust.The current multi-pipe approach works but is fragile. Consider using
ip -o linkfor more reliable parsing.applications/holocat/cpp/hc_data_rx_op.cpp (2)
16-21: Consider making the input port condition consistent with upstream operators.In
holocat_op.cpp(line 147-152), ports usecondition(holoscan::ConditionType::kNone)to make them optional. Without this, the scheduler may block waiting for input that might not always arrive, depending on the data flow timing.Proposed fix
void HcDataRxOp::setup(holoscan::OperatorSpec& spec) { // Configure input port for receiving counter data - spec.input<int>("count_in"); + spec.input<int>("count_in") + .condition(holoscan::ConditionType::kNone); // Make input optional HOLOSCAN_LOG_INFO("HcDataRxOp: Setup complete - configured input port 'count_in'"); }
27-35: Error logging may be too aggressive for normal operation.Logging
HOLOSCAN_LOG_ERRORon every failed receive could flood logs during normal startup or when no data is available yet. Consider usingLOG_DEBUGorLOG_WARNwith rate limiting, similar to the pattern inholocat_op.cppwhich usesLOG_DEBUGfor optional input handling.Proposed fix
// Receive count value from ECat bus auto maybe_count = op_input.receive<int>("count_in"); if (!maybe_count) { - HOLOSCAN_LOG_ERROR("HcDataRxOp: Failed to receive count from ECat bus"); + HOLOSCAN_LOG_DEBUG("HcDataRxOp: No data received on count_in"); return; } last_count_ = maybe_count.value(); - HOLOSCAN_LOG_INFO("HcDataRxOp: Received count: {}", last_count_); + HOLOSCAN_LOG_DEBUG("HcDataRxOp: Received count: {}", last_count_); }applications/holocat/Dockerfile (1)
43-49: Fallback verification silently succeeds on partial SDK.The fallback check only verifies two files exist but doesn't ensure they're valid or that required link-layer libraries are present. This could lead to runtime failures that would have been caught by the full verification.
Consider adding more context to the fallback
RUN /opt/holocat/scripts/verify_ecmaster.sh || \ (echo "Verification failed - checking if SDK is minimally viable..." && \ test -f "/opt/acontis/ecmaster/SDK/INC/EcMaster.h" && \ test -f "/opt/acontis/ecmaster/Bin/Linux/x64/libEcMaster.so" && \ - echo "✓ Minimal SDK requirements met for build") + echo "⚠ Minimal SDK requirements met - full verification failed" && \ + echo " Runtime issues may occur. Check SDK installation.")applications/holocat/cpp/holocat_config.hpp (1)
17-31: Consider adding default member initializers to prevent undefined behavior.Struct members without default values will be uninitialized if a
HolocatConfigis default-constructed, potentially causing undefined behavior when accessed.Proposed fix
struct HolocatConfig { - std::string adapter_name; - std::string eni_file; - uint32_t rt_priority; - uint32_t job_thread_priority; - bool enable_rt; - uint32_t dio_out_offset; - uint32_t dio_in_offset; - uint32_t max_acyc_frames; - uint32_t job_thread_stack_size; - std::string log_level; - uint64_t cycle_time_us; + std::string adapter_name{}; + std::string eni_file{}; + uint32_t rt_priority{0}; + uint32_t job_thread_priority{0}; + bool enable_rt{false}; + uint32_t dio_out_offset{0}; + uint32_t dio_in_offset{0}; + uint32_t max_acyc_frames{0}; + uint32_t job_thread_stack_size{0}; + std::string log_level{"info"}; + uint64_t cycle_time_us{1000}; // For validation error reporting - std::string error_message; + std::string error_message{}; };applications/holocat/cpp/main.cpp (1)
107-121: Incomplete configuration printing.The comment
// ... print other fieldsat line 116 suggests this feature is incomplete. Consider printing all config fields or removing the comment if intentional.Complete the configuration output
std::cout << " Adapter: " << config.adapter_name << "\n"; std::cout << " ENI File: " << config.eni_file << "\n"; std::cout << " Cycle Time: " << config.cycle_time_us << " μs\n"; std::cout << " RT Priority: " << config.rt_priority << "\n"; - // ... print other fields + std::cout << " Job Thread Priority: " << config.job_thread_priority << "\n"; + std::cout << " Enable RT: " << (config.enable_rt ? "true" : "false") << "\n"; + std::cout << " DIO Out Offset: " << config.dio_out_offset << "\n"; + std::cout << " DIO In Offset: " << config.dio_in_offset << "\n"; + std::cout << " Log Level: " << config.log_level << "\n";applications/holocat/cpp/holocat_op.cpp (1)
314-315: Return value ofecatExecJobis explicitly unreferenced.The
EC_UNREFPARM(dwRes)pattern is used multiple times (lines 315, 341, 346, 350) to suppress warnings, but silently ignoring errors from EtherCAT job execution could mask issues in production.Consider at least logging errors in debug mode:
Proposed enhancement
dwRes = ecatExecJob(eUsrJob_ProcessAllRxFrames, EC_NULL); - EC_UNREFPARM(dwRes); + if (dwRes != EC_E_NOERROR) { + HOLOSCAN_LOG_DEBUG("ProcessAllRxFrames returned: {} (0x{:x})", ecatGetText(dwRes), dwRes); + }applications/holocat/cpp/holocat_app.cpp (2)
72-79: Silent exception swallowing in config extraction.The
try_extractlambda catches all exceptions and only logs a warning, leavingtargetpotentially uninitialized (default-initialized for the struct). This could lead to unexpected behavior if critical fields fail to load.Consider differentiating between "key not found" (acceptable) and "conversion failed" (may indicate config error):
Proposed enhancement
auto try_extract = [this](const std::string& key, auto& target) { try { target = from_config(key).as<std::decay_t<decltype(target)>>(); - } catch (...) { - // Key not found or conversion failed - target remains uninitialized - HOLOSCAN_LOG_WARN("Missing configuration key {}", key); + } catch (const std::exception& e) { + HOLOSCAN_LOG_WARN("Configuration key '{}': {}", key, e.what()); } };
42-43: Condition name collision with different periods.Both
ecat_bus_periodic_cond(line 34-35) andcounter_update_periodic_cond(line 42-43) use the same condition name"ethercat_cycle"but have different periods (cycle_time_usvs100ms). This may cause confusion or conflicts.auto counter_update_periodic_cond = make_condition<holoscan::PeriodicCondition>( - "ethercat_cycle", 100ms); + "counter_update_cycle", 100ms);applications/holocat/cpp/holocat_op.hpp (2)
10-11: Include guard naming could be more specific.The guard
INC_HOLOCAT_His generic. Consider using the full path-based naming convention for uniqueness:-#ifndef INC_HOLOCAT_H -#define INC_HOLOCAT_H 1 +#ifndef HOLOCAT_OP_HPP_ +#define HOLOCAT_OP_HPP_
115-115: Missing closing comment for namespace.Adding a namespace closing comment improves readability:
-} +} // namespace holocat `#endif` /* INC_HOLOCAT_H */
| add_holohub_application(vila_live) | ||
| add_holohub_application(holocat) | ||
| add_holohub_application(holochat) |
There was a problem hiding this comment.
Keep the application list alphabetized.
The new holocat entry is appended near the end, but this list is intended to be alphabetical. Please move holocat (and keep holochat adjacent) into the proper “holo*” section to maintain ordering.
🤖 Prompt for AI Agents
In `@applications/CMakeLists.txt` around lines 159 - 161, The application list in
the CMake file is out of alphabetical order: the
add_holohub_application(holocat) entry is appended at the end next to vila_live
and holochat; move the holocat entry into the proper "holo*" section so the
"holo*" entries remain alphabetized and keep add_holohub_application(holochat)
adjacent to it (i.e., reorder the add_holohub_application(...) lines so all
holo* entries are sorted alphabetically).
| #ifndef INC_HOLOCAT_CONFIG_H | ||
| #define INC_HOLOCAT_CONFIG_H 1 | ||
|
|
||
| #include <string> | ||
|
|
There was a problem hiding this comment.
Missing <cstdint> include for fixed-width integer types.
The struct uses uint32_t and uint64_t which require <cstdint> to be included.
Proposed fix
`#ifndef` INC_HOLOCAT_CONFIG_H
`#define` INC_HOLOCAT_CONFIG_H 1
+#include <cstdint>
`#include` <string>📝 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.
| #ifndef INC_HOLOCAT_CONFIG_H | |
| #define INC_HOLOCAT_CONFIG_H 1 | |
| #include <string> | |
| `#ifndef` INC_HOLOCAT_CONFIG_H | |
| `#define` INC_HOLOCAT_CONFIG_H 1 | |
| `#include` <cstdint> | |
| `#include` <string> |
🧰 Tools
🪛 Clang (14.0.6)
[error] 12-12: 'string' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In `@applications/holocat/cpp/holocat_config.hpp` around lines 9 - 13, The header
is missing the <cstdint> include required for fixed-width types used in this
file; add `#include` <cstdint> near the top (alongside `#include` <string>) so
usages of uint32_t and uint64_t in the structs or declarations compile correctly
(update in holocat_config.hpp where those types are referenced).
| struct timespec t_now; | ||
| clock_gettime(CLOCK_MONOTONIC, &t_now); | ||
| static struct timespec t_last{t_now.tv_sec, t_now.tv_nsec}; | ||
|
|
There was a problem hiding this comment.
Static local variable in compute() may cause issues with multiple operator instances.
The static struct timespec t_last retains state across all instances of HolocatOp. If multiple instances exist, they will share this timing state, causing incorrect elapsed time calculations.
Proposed fix - make it a member variable
In the header, add:
// Performance monitoring
struct timespec t_last_{0, 0};
bool first_compute_ = true;In compute():
// Performance monitoring and diagnostics
struct timespec t_now;
clock_gettime(CLOCK_MONOTONIC, &t_now);
- static struct timespec t_last{t_now.tv_sec, t_now.tv_nsec};
+ if (first_compute_) {
+ t_last_ = t_now;
+ first_compute_ = false;
+ }
// ... elapsed time calculation using t_last_ instead of t_last
- t_last = t_now;
+ t_last_ = t_now;🤖 Prompt for AI Agents
In `@applications/holocat/cpp/holocat_op.cpp` around lines 354 - 357, The static
timespec t_last in compute() causes shared state across HolocatOp instances;
make it an instance member (e.g., add struct timespec t_last_{0,0}; and bool
first_compute_ = true; to HolocatOp's class) and replace the static usage in
compute() with these members: on first call use first_compute_ to initialize
t_last_ from current time and set first_compute_ = false, otherwise compute
elapsed using t_last_ and then update t_last_ with the current time; update any
references to t_last in compute() accordingly.
| void EcMasterJobTask(); | ||
|
|
||
| // Static function to interface with OsCreateThread | ||
| static EC_T_VOID EcMasterJobTaskStatic(EC_T_VOID* pvAppContext); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for usage of EcMasterJobTask and EcMasterJobTaskStatic in the codebase
rg -n "EcMasterJobTask" --type cpp --type hppRepository: nvidia-holoscan/holohub
Length of output: 94
🏁 Script executed:
# Search for usage of EcMasterJobTask in the codebase (broader search)
rg -n "EcMasterJobTask" --glob "*.cpp" --glob "*.hpp"Repository: nvidia-holoscan/holohub
Length of output: 245
🏁 Script executed:
# Check the holocat_op.cpp file to see if these methods are defined
cat applications/holocat/cpp/holocat_op.cpp | wc -lRepository: nvidia-holoscan/holohub
Length of output: 70
🏁 Script executed:
# Search for definitions of these methods in holocat_op.cpp
rg -n "EcMasterJobTask|EcMasterJobTaskStatic" applications/holocat/cpp/holocat_op.cppRepository: nvidia-holoscan/holohub
Length of output: 49
🏁 Script executed:
# Check what files are in the holocat/cpp directory
ls -la applications/holocat/cpp/Repository: nvidia-holoscan/holohub
Length of output: 907
🏁 Script executed:
# View the declarations in the header file to confirm context
sed -n '60,75p' applications/holocat/cpp/holocat_op.hppRepository: nvidia-holoscan/holohub
Length of output: 729
Remove or document the unused method declarations EcMasterJobTask and EcMasterJobTaskStatic.
These methods are declared in holocat_op.hpp (lines 67-70) but are never defined or used anywhere in the codebase. Either remove them or add a TODO comment if they're planned for future implementation.
🤖 Prompt for AI Agents
In `@applications/holocat/cpp/holocat_op.hpp` around lines 67 - 70, The
declarations EcMasterJobTask() and EcMasterJobTaskStatic(EC_T_VOID*) in
holocat_op.hpp are unused; either delete both declarations to remove dead code
or keep them but add a clear TODO comment indicating they are intentionally
unimplemented and planned for future use (e.g., "TODO: implement EcMasterJobTask
and EcMasterJobTaskStatic" with expected behavior), and if kept ensure the
static signature matches the expected OsCreateThread callback convention; update
any related comments near the declarations accordingly.
| std::filesystem::path config_path; | ||
| if (!args.config_file.empty()) { | ||
| config_path = args.config_file; | ||
| } else { | ||
| config_path = std::filesystem::canonical(argv[0]).parent_path() / "holocat_config.yaml"; | ||
| } | ||
|
|
||
| if (!std::filesystem::exists(config_path)) { | ||
| throw std::runtime_error("Configuration file not found: " + config_path.string()); | ||
| } |
There was a problem hiding this comment.
std::filesystem::canonical() can throw before the existence check.
canonical() at line 98 will throw std::filesystem::filesystem_error if argv[0] doesn't resolve to an existing path. This exception occurs before your explicit exists() check at line 101, making the error message less helpful.
Proposed fix using weakly_canonical
if (!args.config_file.empty()) {
config_path = args.config_file;
} else {
- config_path = std::filesystem::canonical(argv[0]).parent_path() / "holocat_config.yaml";
+ // Use weakly_canonical to avoid throwing if intermediate path doesn't exist
+ auto exe_path = std::filesystem::weakly_canonical(argv[0]);
+ config_path = exe_path.parent_path() / "holocat_config.yaml";
}📝 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.
| std::filesystem::path config_path; | |
| if (!args.config_file.empty()) { | |
| config_path = args.config_file; | |
| } else { | |
| config_path = std::filesystem::canonical(argv[0]).parent_path() / "holocat_config.yaml"; | |
| } | |
| if (!std::filesystem::exists(config_path)) { | |
| throw std::runtime_error("Configuration file not found: " + config_path.string()); | |
| } | |
| std::filesystem::path config_path; | |
| if (!args.config_file.empty()) { | |
| config_path = args.config_file; | |
| } else { | |
| // Use weakly_canonical to avoid throwing if intermediate path doesn't exist | |
| auto exe_path = std::filesystem::weakly_canonical(argv[0]); | |
| config_path = exe_path.parent_path() / "holocat_config.yaml"; | |
| } | |
| if (!std::filesystem::exists(config_path)) { | |
| throw std::runtime_error("Configuration file not found: " + config_path.string()); | |
| } |
🤖 Prompt for AI Agents
In `@applications/holocat/cpp/main.cpp` around lines 94 - 103, Replace the use of
std::filesystem::canonical(argv[0]) (which can throw) with
std::filesystem::weakly_canonical or otherwise guard the call so it won’t throw
before your exists check: when args.config_file is empty, compute config_path
using std::filesystem::weakly_canonical(argv[0]).parent_path() /
"holocat_config.yaml" (or catch filesystem_error around canonical and fall back
to weakly_canonical) so that the subsequent std::filesystem::exists(config_path)
runs reliably and your thrown runtime_error contains the intended path
information (references: config_path, args.config_file, argv,
std::filesystem::canonical, std::filesystem::weakly_canonical,
std::filesystem::exists).
| # Set environment variables for EC-Master SDK | ||
| ENV ECMASTER_ROOT=/opt/acontis/ecmaster | ||
| ENV LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/opt/acontis/ecmaster/lib:/opt/acontis/ecmaster/Bin/Linux/x64 |
There was a problem hiding this comment.
LD_LIBRARY_PATH hardcodes x64 architecture.
The LD_LIBRARY_PATH includes /opt/acontis/ecmaster/Bin/Linux/x64 which won't work on arm64 systems. The verification script correctly detects architecture, but the Dockerfile doesn't account for this.
Proposed fix using build-time architecture detection
+# Detect architecture for library path
+ARG TARGETARCH
+
# Set environment variables for EC-Master SDK
ENV ECMASTER_ROOT=/opt/acontis/ecmaster
-ENV LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/opt/acontis/ecmaster/lib:/opt/acontis/ecmaster/Bin/Linux/x64
+# Note: LD_LIBRARY_PATH is set at runtime based on architecture
+# For x64: /opt/acontis/ecmaster/Bin/Linux/x64
+# For arm64: /opt/acontis/ecmaster/Bin/Linux/arm64Alternatively, set it dynamically in an entrypoint script or use a multi-arch approach.
🤖 Prompt for AI Agents
In `@applications/holocat/Dockerfile` around lines 24 - 26, The Dockerfile
currently hardcodes "/opt/acontis/ecmaster/Bin/Linux/x64" in ENV
LD_LIBRARY_PATH; change it to detect/set the proper arch at build or runtime:
add ARG TARGETARCH (or use an entrypoint script) and map TARGETARCH to the
vendor folder name, e.g. set ENV ECMASTER_ARCH after mapping ("amd64"→"x64",
"arm64" or "aarch64"→ appropriate folder), then set ENV
LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:${ECMASTER_ROOT}/lib:${ECMASTER_ROOT}/Bin/Linux/${ECMASTER_ARCH};
update references to ENV ECMASTER_ROOT and LD_LIBRARY_PATH accordingly so the
path is not hardcoded to x64.
2 participants
Add HoloCat application that integrates EtherCAT real-time communication with NVIDIA's Holoscan streaming framework.
Key Features:
Components:
This enables real-time industrial automation applications using EtherCAT fieldbus communication within the Holoscan ecosystem.
Summary by CodeRabbit
New Features
Tooling
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.