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.

Sign up for GitHub

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

Jump to bottom

Added check for cookie update and support for HTTP Basic authentication #18

Merged
merged 1 commit into from
Sep 6, 2018
Merged

Added check for cookie update and support for HTTP Basic authentication #18

merged 1 commit into from
Sep 6, 2018

Conversation

jontje
Copy link
Contributor

@jontje jontje commented Aug 21, 2018

As per title.

gavanderhoorn
gavanderhoorn reviewed Aug 26, 2018
View reviewed changes
Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Some general comments, mostly about the documentation.

Haven't tested this against actual hw, but I trust you have.

include/abb_librws/rws_client.h
@@ -272,21 +272,67 @@ class RWSClient : public POCOClient
*/
std::vector<SubscriptionResource> resources_;
};

/**
* \brief A constructor.
Copy link
Member

Choose a reason for hiding this comment

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

That is a tad too general. Do you think we could describe this better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are quite a few such general descriptions. I have opened an issue to improve this, see #25

include/abb_librws/rws_client.h
POCOClient(ip_address,
SystemConstants::General::DEFAULT_PORT_NUMBER,
username,
password)
Copy link
Member

Choose a reason for hiding this comment

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

If we start using C++11, we could use delegating constructors here.

(and for all the other ctor variants below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will keep that in mind.

include/abb_librws/rws_client.h
* \param port for the remote server's port.
* \param user for the remote server's authentication (assumed to be Digest).
* \param password for the remote server's authentication (assumed to be Digest).
* \param ip_address specifying the robot controller's IP address.
Copy link
Member

Choose a reason for hiding this comment

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

Is this the LAN address, or can it be any address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have only tested with LAN addresses, but I guess it could be any address as long as it is possible to establish a network connection.

The argument is passed down to a constructor for a POCO C++ HTTPClientSession.

include/abb_librws/rws_client.h
const Poco::UInt16 port = 80,
const std::string user = SystemConstants::General::DEFAULT_USERNAME,
const std::string password = SystemConstants::General::DEFAULT_PASSWORD)
const unsigned short port,
Copy link
Member

Choose a reason for hiding this comment

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

Why did this lose the Poco::UInt16 type spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... I don't remember... possibly a case of "copy-and-paste".

include/abb_librws/rws_interface.h
rws_client_(ip_address,
SystemConstants::General::DEFAULT_PORT_NUMBER,
username,
password)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about the delegating constructors here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will keep it in mind.

include/abb_librws/rws_poco_client.h
* \param user for the remote server's authentication (assumed to be Digest).
* \param password for the remote server's authentication (assumed to be Digest).
* \param username for the username to the remote server's authentication process.
* \param password for the password to the remote server's authentication process.
Copy link
Member

Choose a reason for hiding this comment

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

Should for be removed here?

Copy link
Member

Choose a reason for hiding this comment

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

Same comment for all other places that for is used in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have thought about improving the parameter descriptions, but I haven't had time yet. I have opened an issue to improve this, see #26

include/abb_librws/rws_poco_client.h
@@ -246,16 +247,16 @@ class POCOClient
*
* \param ip_address for the remote server's IP address.
* \param port for the remote server's port.
* \param user for the remote server's authentication (assumed to be Digest).
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer only Digest auth, or has the default changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the default is still Digest for RWS. However, the POCOClient implementation don't know that it is used for RWS at all, and since the HTTPCredentials can handle both it is more general this way (compared to the previous HTTPDigestCredentials).

include/abb_librws/rws_simple_state_machine_interface.h
SystemConstants::General::DEFAULT_PORT_NUMBER,
username,
password)
{}
Copy link
Member

Choose a reason for hiding this comment

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

Delegating constructors again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will keep it in mind.

src/rws_common.cpp
const std::string SystemConstants::General::MECHANICAL_UNIT_ROB_4 = "ROB_4";
const std::string SystemConstants::General::MECHANICAL_UNIT_ROB_L = "ROB_L";
const std::string SystemConstants::General::MECHANICAL_UNIT_ROB_R = "ROB_R";
const std::string SystemConstants::General::REMOTE = "remote";
Copy link
Member

Choose a reason for hiding this comment

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

We should probably change some of this to constexprs (but that would need C++11 again).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will keep it in mind.

@jontje
Copy link
Contributor Author

jontje commented Sep 6, 2018

@gavanderhoorn I have replied to your comments, and yes, I have verified this with real hardware.

@gavanderhoorn
Copy link
Member

Feel free to merge @jontje.

@jontje
Copy link
Contributor Author

jontje commented Sep 6, 2018

Great, thanks for the review @gavanderhoorn! :)

@jontje jontje merged commit 104f6a7 into ros-industrial:master Sep 6, 2018
@jontje jontje deleted the cookie_check_and_basic_authentication branch September 6, 2018 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavanderhoorn gavanderhoorn gavanderhoorn approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jontje @gavanderhoorn