-
Notifications
You must be signed in to change notification settings - Fork 10
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
PLEXIL uses LightSetIntensity action #98
Conversation
First run I saw the following
Everything appeared to work, but the test went too fast for me to follow. So I'm trying it again now. |
I did not see that error the second time around. It may have resulted from me starting ow_plexil a little too soon after starting the simulation. The lander lights test performed as expected. I might recommend increasing the wait time between commands to make it easier to follow. ReferenceMission1 performed as expected. All tests pass 👍🏻 |
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.
No real comments on the code aside from a suggestion about how parameter checking could be simplified in the future.
Approved!
"should be in range [0.0 1.0]", intensity); | ||
valid_args = false; | ||
} | ||
if (valid_args) { | ||
unique_ptr<CommandRecord>& cr = new_command_record(cmd, intf); | ||
OwInterface::instance()->setLightIntensity (side, intensity, CommandId); | ||
OwInterface::instance()->lightSetIntensity (side, intensity, CommandId); | ||
acknowledge_command_sent(*cr); | ||
} |
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.
No change requests here, just something to consider for your refactoring of this code.
Verifying action input here in ow_plexil requires an awful lot of repetition of logic already performed by the action servers.
Could an alternative approach be to send arguments to the action server blindly, and let the action server perform value checks? Out of range values should result in an aborted action from the action server, which ow_plexil could look for (and perhaps already does). The user still gets the benefit of knowing why the command was aborted, only that message would come from the simulation terminal rather than the ow_plexil terminal.
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.
Good suggestion, though I'm not sure if that's better than the redundant check, which has its advantages (e.g. performance, customizing the message). It would apply to many actions. Will keep this in mind during future development.
This is an unrelated problem, and I agree with your guess that plexil may have been started too soon. I rarely if ever have seen this error. |
Summary:
Unrelated changes:
Test:
TestLanderLights
plan, e.g.$ roslaunch ow_plexil ow_exec.launch plan:=TestLanderLights.plx
Notes: