-
Notifications
You must be signed in to change notification settings - Fork 3
refactor(orderGraphValidator): Isolate OrderGraphValidator and use vda5050_types
#13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
sauk2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work! I have provided my first phase of review comments.
I haven't read through the source file properly yet. These comments are mostly on the exposed API.
vda5050_core/include/vda5050_core/client/order/order_graph_validator.hpp
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/order_graph_validator/order_graph_validator.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/src/vda5050_core/order_graph_validator/order_graph_validator.cpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/order_graph_validator/order_graph_validator.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/src/vda5050_core/order_graph_validator/order_graph_validator.cpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/order_graph_validator/order_graph_validator.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/order_graph_validator/order_graph_validator.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/client/order/order_graph_validator.hpp
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/order_graph_validator/order_graph_validator.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
… namespace Signed-off-by: Shawn Chan <[email protected]>
…and refactor is_in_traversal_order logic for easier readability Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
…re them Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
…stead of FATAL Signed-off-by: Shawn Chan <[email protected]>
sauk2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments regarding the functional logic. I feel there are a few validation checks still missing,
- All node sequence IDs should be even and all edge sequence IDs should be odd.
This follows from the structure of a valid order - an order must contain at least one node (the AGV’s current position), which always starts at sequence ID0. From there, nodes and edges should alternate. - The validator should ensure that no two nodes or edges share the same sequence ID. Duplicate sequence IDs would break the ordering during execution.
- Sequence IDs should form a continuous chain with no gaps so there should be a check to search for any missing sequence IDs.
- There should be a clear boundary between base and horizon entities. Once the first unreleased node or edge is encountered, all following nodes and edges must also be unreleased.
vda5050_core/src/vda5050_core/client/order/order_graph_validator.cpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/client/order/order_graph_validator.hpp
Show resolved
Hide resolved
vda5050_core/test/unit/order_graph_validator/test_order_graph_validator.cpp
Outdated
Show resolved
Hide resolved
vda5050_core/CMakeLists.txt
Outdated
| ) | ||
| target_link_libraries(test_mqtt_client mqtt_client logger) | ||
|
|
||
| find_package(ament_cmake_gtest REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to bring in ament_cmake_gtest separately. The previous ament_cmake_gmock most probably does that
| ValidationResult OrderGraphValidator::is_valid_edges( | ||
| const vda5050_types::Order& order) | ||
| { | ||
| ValidationResult res{true, {}}; | ||
|
|
||
| std::string start_node_id = order.nodes.front().node_id; | ||
| std::string end_node_id = order.nodes.back().node_id; | ||
|
|
||
| for (vda5050_types::Edge e : order.edges) | ||
| { | ||
| if (e.start_node_id != start_node_id && e.end_node_id != end_node_id) | ||
| { | ||
| vda5050_types::Error invalid_edges_error{}; | ||
| invalid_edges_error.error_type = "Graph Validation Error"; | ||
| invalid_edges_error.error_references = { | ||
| vda5050_types::ErrorReference{"edgeId", e.edge_id}}; | ||
| invalid_edges_error.error_description = | ||
| "start_node_id and end_node_id of the edge does not match the node_ids " | ||
| "of the order's first and last node respectively."; | ||
| invalid_edges_error.error_level = vda5050_types::ErrorLevel::WARNING; | ||
|
|
||
| res.valid = false; | ||
| res.errors = {invalid_edges_error}; | ||
| return res; | ||
| } | ||
| } | ||
| return res; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current validation logic seems incorrect. Right now, the code compares every edge’s start_node_id only against the first node of the order and end_node_id only against the last node of the order. However, start_node_id and end_node_id on an edge describe the two nodes that the specific edge connects, not the global start and end of the entire order.
For a linear order, the correct relationship is sequence-based, i.e, the edge with sequence ID n should connect the node with sequence ID n-1 (as its start_node_id) and the node with sequence ID n+1 (as its end_node_id)
Because this validation depends entirely on the ordering of sequence IDs, it should not be done using only the first and last node. Instead, the check should be integrated into the part of the validation where you are already confirming that sequence IDs form a proper graph. That way you can directly verify that each edge connects the two adjacent nodes it is supposed to.
Signed-off-by: Shawn Chan <[email protected]>
…edges Signed-off-by: Shawn Chan <[email protected]>
…aph validation using graph_element struct Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
…pass CI" Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: Shawn Chan <[email protected]>
Signed-off-by: John Abogado <[email protected]>
|
@sauk2 For review, implements validation for VDA5050 order graphs to ensure basic order and sequencing rules are met before execution. Features:
|
Signed-off-by: John Abogado <[email protected]>
Signed-off-by: John Abogado <[email protected]>
Signed-off-by: John Abogado <[email protected]>
Summary
This PR addresses #8. This branch was created from the
developbranch.In this branch:
vda5050_types