Skip to content
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

Merged

Conversation

ChandimaFernando
Copy link
Collaborator

@ChandimaFernando ChandimaFernando commented Feb 8, 2024

This PR implements the logic of the inner state machine for the pdf_beamtime.
Added internal state machine functions as follows:
exclaidraw_pdf_beamtime_arch_inner_FSM

Initial tests for the inner and outer state transitions for an uninterrupted pick-and-place task have been completed successfully.

Copy link
Owner

@maffettone maffettone left a 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.

@ChandimaFernando ChandimaFernando marked this pull request as ready for review February 15, 2024 15:15
Copy link
Owner

@maffettone maffettone left a 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);
Copy link
Owner

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@ChandimaFernando ChandimaFernando Feb 16, 2024

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_);

InnerStateMachine::InnerStateMachine(
const rclcpp::Node::SharedPtr node)
: node_(node)

https://github.com/maffettone/erobs/blob/19470036fe498c0d3a4e50f68608ac4b79c722f0/src/pdf_beamtime/README.md

@maffettone maffettone merged commit d200dca into maffettone:humble Feb 16, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants