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

Add RWSClient::getIOSignals(). #63

Merged
merged 1 commit into from
Oct 31, 2019
Merged

Add RWSClient::getIOSignals(). #63

merged 1 commit into from
Oct 31, 2019

Conversation

de-vri-es
Copy link
Contributor

Getting a list of all signals is very useful if you're still getting to know the robot. I wanted this functionality for a ROS wrapper we've made at RoboHouse [1]. For now, it needs a forked abb_librws with this patch applied.

I was kinda waiting for the other PRs to be merged first, but I suppose I might as well make make this one already.

[1] https://gitlab.com/robohouse/abb_io

Copy link
Contributor

@jontje jontje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new function works as intended.

Do you also intend to expose the new function in the more user-friendly RWSInterface class? For example:

  • Implementing a struct for an IO-signal list item according to the documentation, which contains:

    • name: IO-signal name
    • type: Signal type {DO | DI | AO | AI | GI | GO}
    • category: Signals list item
    • lvalue: Signal value
    • lstate: Signals state {simulated | not simulated}
  • Parsing the XML document into a vector of such IO-signal list items, similar to RWSInterface::getRAPIDModulesInfo(...).

If not, can you open an issue for that?

Can you also update the PR to be in sync with the base branch?

@@ -357,6 +357,13 @@ class RWSClient : public POCOClient
*/
RWSResult getConfigurationInstances(const std::string topic, const std::string type);

/**
* \brief A method for retrieving the all available IO signals on the controller.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* \brief A method for retrieving the all available IO signals on the controller.
* \brief A method for retrieving all available IO signals on the controller.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@de-vri-es
Copy link
Contributor Author

I fixed the typo.

I actually switched from RWSInterface to RWSClient for abb_io because it allows for better error reporting. I might still get around to adding it to RWSInterface, but I'll make an issue to be sure it's not forgotten.

@de-vri-es
Copy link
Contributor Author

Issue opened as #66.

Copy link
Contributor

@jontje jontje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for processing my previous comments.

Great contribution! 👍

@jontje jontje merged commit c234a00 into ros-industrial:master Oct 31, 2019
@de-vri-es de-vri-es deleted the expose-get-io-signals branch October 31, 2019 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants