Type registry file #14
Conversation
Signed-off-by: Mohamed Abdelaziz <[email protected]>
|
Seems like yaml_cpp_vendor causes issues starting from Jazzy due to the changed expected usage (ros2/yaml_cpp_vendor#51). I will spin up a Jazzy/Kilted/Rolling container once I can and fix that issue. |
Signed-off-by: Mohamed Abdelaziz <[email protected]>
| @@ -0,0 +1,2 @@ | |||
| has_header: | |||
| - stereo_msgs/msg/DisparityImage No newline at end of file | |||
There was a problem hiding this comment.
This is good but actually I'd prefer to have DisparityImage in the default map. Do you want to add that to this PR? else I can do that as a separate PR.
There was a problem hiding this comment.
Sure thing! I only had this here as an example and for my local testing. How about PoseWithCovarianceStamped since you have PoseStamped already? In my experience at least, it is very common among both opensource and commerical SLAM systems.
There was a problem hiding this comment.
Both sound great, please add any common types you use to the default known_header_types.
|
|
||
| type_has_header_cache[type_name] = has_header; | ||
| // In case the type is not in the known list, attempt to read from type registry | ||
| if (it == known_header_types.end() && !type_registry_path_.empty()) { |
There was a problem hiding this comment.
How do you feel about using type_registry to update known_header_types and leaving the logic the same?
There was a problem hiding this comment.
This is what I initially had implemented, but this has the advantage of being able to add types to the yaml file while it is running. For instance, in case I had an object detection pipeline runnging (v4l2 -> Encoder -> TensorRT -> Decoder) and notice that the detections are coming at 15 Hz while the camera publishes at 30 Hz. It would be practical to be able to add isaac_ros_tensor_list_interfaces/msg/TensorList to the registry while the system is still running and monitoring the Encoder and TensorRT outputs to see where the slow down is happening.
There was a problem hiding this comment.
I'd prefer to keep this as simple as possible load the YAML file once during initialization and merge it directly into known_header_types. If the user needs to add new types just restart the node.
Signed-off-by: Mohamed Abdelaziz <[email protected]>
Signed-off-by: Mohamed Abdelaziz <[email protected]>
No big deal! it is definitely helpful to our team :) We have developed in ISAAC SDK for over 5 years now and an overview of latencies and frequencies in the system similar to Isaac Sight is something that was still missing.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR introduces a YAML-based type registry system (type_registry.yml) that allows users to declare custom message types with headers without modifying the C++ source code. When GreenwaveMonitor encounters a message type not in its hardcoded known_header_types map, it now falls back to checking the optional YAML registry file, caches the result, and uses it for subsequent latency calculations. The registry path is validated at node initialization and defaults to empty (no external registry loaded). The implementation adds yaml_cpp_vendor as a dependency, includes installation of the config/ directory, and provides a new has_header_from_type_registry() method for parsing the YAML on-demand.
PR Description Notes:
- The PR description states "the default value for the
type_registry_pathdefaults to this installed file," but the code actually defaults to an empty string (line54in greenwave_monitor.cpp), meaning no registry is loaded by default unless the user explicitly sets the parameter.
Critical Issues
1. Exception thrown when logging non-scalar YAML nodes (greenwave_monitor.cpp:308)
When encountering a non-scalar node in the has_header list, the code attempts to convert it to a string for logging:
RCLCPP_WARN(
this->get_logger(),
"Found a non-string value in the registry: %s. Skipping.",
type_node.as<std::string>().c_str());Calling .as<std::string>() on a non-scalar (e.g., a nested map or list) will throw a YAML::BadConversion exception, crashing the node instead of gracefully skipping the entry. This defeats the purpose of the error handling. Recommend removing the .as<std::string>() call entirely or replacing it with a generic message like "Found a non-scalar value in the registry. Skipping."
2. YAML file re-parsed on every unknown type lookup
has_header_from_type_registry() calls YAML::LoadFile() every time it is invoked. If multiple unknown types are encountered in a single session, the file is read and parsed repeatedly, which introduces unnecessary I/O and CPU overhead. Consider caching the parsed YAML node as a member variable (loaded once during initialization or on first access) and reusing it, or alternatively, load the registry once at startup and populate known_header_types directly.
3. Inconsistency between PR description and implementation for default registry path
The PR description and previous review comments suggest the default should load no config, but the PR description also states "the default value for the type_registry_path defaults to this installed file." The current implementation correctly defaults to an empty string (no file), which aligns with the review feedback. However, the discrepancy in the description may confuse users. The CMakeLists.txt installs the config/ directory, but users must explicitly set the type_registry_path parameter to use it.
4. Missing newline at end of type_registry.yml
The YAML file does not have a trailing newline, which can cause warnings or issues with certain text editors, diff tools, and POSIX compliance. This is a minor quality issue but should be fixed for consistency with best practices.
Confidence Score
2 / 5 – The PR implements a useful feature but contains a critical exception-throwing bug and a performance issue that should be addressed before merging. The default-path discrepancy also needs clarification.
5 files reviewed, 1 comment
| RCLCPP_WARN( | ||
| this->get_logger(), | ||
| "Found a non-string value in the registry: %s. Skipping.", | ||
| type_node.as<std::string>().c_str()); | ||
| } |
There was a problem hiding this comment.
logic: Attempting to convert a non-scalar node to string at line 308 will throw an exception. Remove the .as<std::string>() call or wrap in try-catch.
| RCLCPP_WARN( | |
| this->get_logger(), | |
| "Found a non-string value in the registry: %s. Skipping.", | |
| type_node.as<std::string>().c_str()); | |
| } | |
| } else { | |
| RCLCPP_WARN( | |
| this->get_logger(), | |
| "Found a non-string value in the registry. Skipping."); | |
| } |
|
Looks pretty good, please resolve the open comments, add a quick test (you can add to test_greenwave_monitor.py) and we should be good to merge. |
3 participants
As discussed in #10, this PR adds a
type_registry.yamlwhere the user can extend the type registry. The type registry will be installed by default to the share directory and the default value for thetype_registry_pathdefaults to this installed file. If a type is not found in the built in map, it will attempt to find it in the registry file (if set). If the type is found, it will be added to the cache, otherwise it will warn the user.