-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: internal state machine and test code #23
feat: internal state machine and test code #23
Conversation
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.
Ideally what this PR should contain is an actual working implementation of the Inner and Outer state machines. Right now it just has the implementation of the inner state machine, but no modifications to show how it will work in practice, or tests of that ongoing.
There's no need to "disregard changes in xxx". Implement the changes that are actually needed to affect the behavior desired. And write at minimum an acceptance test (in somewhere outside of src
) that will demonstrate the behavior.
That could be something that involves spinning up the containers and saying, "this simple program should produce this output in the logs". It is not quite a unit test, but it shows what you are trying to accomplish.
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.
This is looking good. Some minor edits and a question to address tomorrow to make sure we're on the same page.
Looks like black is newly failing CI/CD on python files not involved with this PR, so should not block. A maintenance PR will be needed to address either the CI/CD or the black failures.
current_state_ = State::HOME; | ||
gripper_present_ = node_->get_parameter("gripper_present").as_bool(); | ||
inner_state_machine_ = new InnerStateMachine(node_, State::HOME); |
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'm still a little unsure why the InnerStateMachine needs to know about the external state? Is this just for logging?
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.
In the earlier version before separating the internal state machine from the external one, I used the external state to control internal state logic. After the separation., right now it is only used for logging.
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.
InnerStateMachine only accepts node_ as its sole argument. the external state is removed. Respective changes were made to inner_state_machine.hpp and README.md
inner_state_machine_ = new InnerStateMachine(node_); |
erobs/src/pdf_beamtime/src/inner_state_machine.cpp
Lines 5 to 7 in e33193c
InnerStateMachine::InnerStateMachine( | |
const rclcpp::Node::SharedPtr node) | |
: node_(node) |
https://github.com/maffettone/erobs/blob/19470036fe498c0d3a4e50f68608ac4b79c722f0/src/pdf_beamtime/README.md
This PR implements the logic of the inner state machine for the pdf_beamtime.

Added internal state machine functions as follows:
Initial tests for the inner and outer state transitions for an uninterrupted pick-and-place task have been completed successfully.