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

Expose diagnostic error codes #225

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jessica-chen-ocado
Copy link

@jessica-chen-ocado jessica-chen-ocado commented Nov 14, 2024

PR to parse UR10 error codes from diagnostic logs that are streamed on the Primary Interface port 30001

@jessica-chen-ocado jessica-chen-ocado marked this pull request as ready for review November 14, 2024 18:53
std::string ErrorCodeMessage::toString() const
{
std::stringstream ss;
ss << "C" << message_code_ << "A" << message_argument_;
Copy link
Author

@jessica-chen-ocado jessica-chen-ocado Nov 14, 2024

Choose a reason for hiding this comment

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

I tried printing out data_type_, data_, and text_ here but the data type and data are 0 and text is empty.. Is this expected?

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily. That sounds like parsing might have gone wrong.

@urfeex
Copy link
Member

urfeex commented Nov 15, 2024

Thank you very much for your contribution! I've wanted to extract parts of #186 since a long time, since that one got too big at some point. I'll have a deeper look at this ASAP.

@jessica-chen-ocado
Copy link
Author

@urfeex Any updates on this?

Copy link
Member

@urfeex urfeex left a comment

Choose a reason for hiding this comment

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

Thank you again for tackling this. If we could restructure things a bit (and rename the ErrorCodeClient) so it could potentially be reused as a primary client with more features in future, I think we could get this merged.

Comment on lines +1 to +6
// Below code is copied from a branch on the Universal_Robot_Client_Library
// with some modification. Link to branch:
// https://github.com/UniversalRobots/Universal_Robots_Client_Library/tree/improve_primary_interface
//
// this is for emacs file handling -*- mode: c++; indent-tabs-mode: nil -*-

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Below code is copied from a branch on the Universal_Robot_Client_Library
// with some modification. Link to branch:
// https://github.com/UniversalRobots/Universal_Robots_Client_Library/tree/improve_primary_interface
//
// this is for emacs file handling -*- mode: c++; indent-tabs-mode: nil -*-

Comment on lines +1 to +4
// Below code is copied from a branch on the Universal_Robot_Client_Library
// with some modification. Link to branch:
// https://github.com/UniversalRobots/Universal_Robots_Client_Library/tree/improve_primary_interface
//
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Below code is copied from a branch on the Universal_Robot_Client_Library
// with some modification. Link to branch:
// https://github.com/UniversalRobots/Universal_Robots_Client_Library/tree/improve_primary_interface
//

std::string ErrorCodeMessage::toString() const
{
std::stringstream ss;
ss << "C" << message_code_ << "A" << message_argument_;
Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily. That sounds like parsing might have gone wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I think I would have rather gone for a "PrimaryClient" that could potentially do more than "just" consuming error codes. E.g. the first thing that I would like to implement is a sendScript function that sends a given URScript snippet to the robot and maybe registers a callback that gets called with a potential error.

You can see my attempt of that in https://github.com/UniversalRobots/Universal_Robots_Client_Library/blob/1780e1dfff3cc1cb09040e547fa73051f2058d28/include/ur_client_library/primary/primary_client.h

I'm not saying that you should use my implementation, but it would be nice if that structure could be kept in mind while implementing this.

Copy link
Author

Choose a reason for hiding this comment

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

I can re-use most of your implementation but may I suggest a few changes?

  1. I noticed that, in order to clean up the old implementation, you removed primary_stream_ from UrDriver and have PrimaryClient instantiate the stream instead. Would you be okay if I keep primary_stream_ in UrDriver to keep the scope of this PR small? This way, I wouldn’t need to modify other parts of UrDriver that depend on primary_stream_, such as UrDriver::checkCalibration. PrimaryClient could then reuse the existing primary stream, similar to how I instantiated ErrorCodeClient.
  2. It seems that in your implementation, you are only waiting for error feedback or any robot feedback when you send sendSecondaryScript or sendScript. However, what we would prefer is the option to continuously check for errors coming from the UR, directly from the UrDriver. To achieve this, I suggest that the errorMessageCallback push error codes to a queue (e.g., error_code_queue_) in PrimaryClient as they arrive. PrimaryClient could then provide a getErrorCodes() function that retrieves and clears whatever is in the queue.

The PrimaryClient class will look something like this

class PrimaryClient
{
public:
  PrimaryClient() = delete;
  PrimaryClient(comm::URStream<primary_interface::PrimaryPackage>& stream);
  virtual ~PrimaryClient() = default;

  void addPrimaryConsumer(std::shared_ptr<AbstractPrimaryConsumer> primary_consumer);
  void removePrimaryConsumer(std::shared_ptr<AbstractPrimaryConsumer> primary_consumer);
  :
  :
  std::deque<urcl::primary_interface::ErrorCode> getErrorCodes();

private:
  :
  void errorMessageCallback(ErrorCodeMessage& msg);

  std::deque<primary_interface::ErrorCode> error_code_queue_;
};

Copy link
Member

Choose a reason for hiding this comment

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

  1. I think for the scope of this PR this seems reasonable.
  2. yes, that seems fair

Let me know if I can be of any further assistance.

}

ASSERT_EQ(error_codes.size(), 1);
ASSERT_EQ(error_codes.at(0).message_code, 209);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe having error codes as an enum somewhere would be nice so once could understand error codes when reading the code :-)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe having error codes as an enum somewhere would be nice so once could understand error codes when reading the code :-)

I will suggest a link to the documentation of the error codes instead, to avoid maintain the list in this repo.
Currently all the manuals, including an error code description, can be accessed from https://myur.universal-robots.com/manuals

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.

3 participants