-
Notifications
You must be signed in to change notification settings - Fork 3
Feat(state-manager): add StateManager #14
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
Conversation
Signed-off-by: johnaa <[email protected]>
Signed-off-by: John Abogado <[email protected]>
Signed-off-by: John Abogado <[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 PR! I have provided some initial review comments.
I feel there are some features still missing from the StateManager class,
- Getters and setters for the
action_statesvariable - Function to set the header. I would expect the users to directly serialize the
vda5050_types::Stateclass intonlohmann::jsonand in the current implementationget_statewould be missing the header fields. - Function to update the order progress. There is no way to update the
last_node_idandlast_node_sequence_idfields. In our current design, these fields will be read by theOrderManagerto increment the order and dispatch the next node/edge.
I also have a general comment on the API design. Most of the get functions return references to fields inside the internal robot_state_ object. This works only under the assumption that the StateManager outlives all consumers of those references and callers never store those references beyond the StateManager’s lifetime.
If the StateManager were ever destroyed earlier than expected, any stored references would become dangling and could lead to undefined behavior as it tightly couples the lifetime of external code to the lifetime of StateManager.
We should discuss whether returning values would make the API safer, especially if the StateManager is ever used without our execution engine. I would also like to include @Briancbn in this discussion for his thoughts!
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/CMakeLists.txt
Outdated
| $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}> | ||
| ) | ||
|
|
||
| add_library(state_manager src/vda5050_core/state_manager/state_manager.cpp) |
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.
Let's change the library name to client that includes all the state and order related features. The order related code will be added to the same library
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 think you missed this change
|
@Johnarman1398 you can ignore the CI failures for now. The failing tests are due to failure to connect to the online MQTT broker. I will try to investigate this issue. |
The issue seems to have solved itself XD |
Signed-off-by: johnaa <[email protected]>
Signed-off-by: johnaa <[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 changes. I think this PR is almost ready to be merged!
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
vda5050_core/include/vda5050_core/state_manager/state_manager.hpp
Outdated
Show resolved
Hide resolved
…unit tests Signed-off-by: johnaa <[email protected]>
Signed-off-by: johnaa <[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.
Looks good! I will merge it in once you make that remaining change
vda5050_core/CMakeLists.txt
Outdated
| $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}> | ||
| ) | ||
|
|
||
| add_library(state_manager src/vda5050_core/state_manager/state_manager.cpp) |
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 think you missed this change
Signed-off-by: johnaa <[email protected]>
Add StateManager implementation into vda5050_core
This PR implements tracking of VDA 5050-compliant state information.